Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to the selected wallet after loading #665

Merged
merged 1 commit into from
Oct 27, 2022

Conversation

w0xlt
Copy link
Contributor

@w0xlt w0xlt commented Sep 8, 2022

Currently, the user loads a wallet and the screen does not switch to the selected wallet after loading (File -> Open Wallet -> wallet name).

This PR changes that by making the OpenWalletActivity::opened signal connection a Qt::QueuedConnection type.

@hebasto
Copy link
Member

hebasto commented Sep 9, 2022

Maybe amend a PR title/commit message to mention "switch to the selected wallet after loading"?

@@ -410,7 +410,7 @@ void BitcoinGUI::createActions()

connect(action, &QAction::triggered, [this, path] {
auto activity = new OpenWalletActivity(m_wallet_controller, this);
connect(activity, &OpenWalletActivity::opened, this, &BitcoinGUI::setCurrentWallet);
connect(activity, &OpenWalletActivity::opened, this, &BitcoinGUI::setCurrentWallet, Qt::QueuedConnection);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this solution works?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a good explanation here: #471 (review) as this is the same case found in Restore Wallet.

As the default value for this parameter is Qt::AutoConnection, it appears to be using Qt::DirectConnection, in which the slot is invoked immediately.

https://doc.qt.io/qt-6/qt.html#ConnectionType-enum

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like a race that would be better being explicitly handled in the correct order?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Qt::DirectConnection guarantees the correct order in this case, as BitcoinGUI::setCurrentWallet will only run after control returns to the event loop of the receiver's thread.

@hebasto hebasto added the UX All about "how to get things done" label Sep 9, 2022
@w0xlt w0xlt changed the title Update the screen after loading wallet Switch to the selected wallet after loading Sep 9, 2022
Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b8b59ff

Can confirm that this fixes the issue. It appears safe to explicitly specify what kind of ConnectionType we want, there isn't an issue with the parameters given to the meta system and queuing this. We probably still want to continue to flesh this out and determine if we can refactor to remove explicitly specifying the ConnectionType

@pablomartin4btc
Copy link
Contributor

Tested ACK on ubuntu 20.04.

Loading a wallet now sets it on the "Wallet:" combobox at the top right hand corner.

image

So far I couldn't find if there's an alternative to avoid specifying the ConnectionType on the params. I see the different types will depend on whether the sender and receiver belong to the same thread or not.

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a kind of similar issue when there's a selected wallet on the main window and opening the rpc console sets a different one. I didn't check yet (rpcconsole.cpp?) if there's a call to select a wallet and it's not working properly.

image

@Sjors
Copy link
Member

Sjors commented Oct 6, 2022

Concept ACK. It'd be nice if Console switches too; wouldn't be the first time I import descriptors into the wrong wallet :-)

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK b8b59ff, tested on Ubuntu 22.04.

Feels like a race that would be better being explicitly handled in the correct order?

Yes, a PR with a better solution is welcome.

@hebasto hebasto merged commit 39710f5 into bitcoin-core:master Oct 27, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 28, 2022
hebasto added a commit that referenced this pull request Jul 4, 2023
…ly opened/restored/created

99c0eb9 Fix RPCConsole wallet selection (John Moffett)

Pull request description:

  If a user opens multiple wallets in the GUI from the menu bar, the last one opened is the active one in the main window. However, For the RPC Console window, the  _first_ one opened is active. This can be confusing, as wallet RPC commands may be sent to a wallet the user didn't intend.

  This PR makes the RPC Console switch to the wallet just opened / restored / created from the menu bar, which is how the main GUI now works.

  Similar to #665 and specifically requested [in a comment](#665 (comment)).

ACKs for top commit:
  luke-jr:
    utACK 99c0eb9
  hebasto:
    ACK 99c0eb9, tested on Ubuntu 23.04.

Tree-SHA512: d5e5acdaa114130ad4d27fd3f25393bc8d02d92b5001cd39352601d04283cdad3bd62c4da6d369c69764e3b188e9cd3e83152c00b09bd42966082ad09037c328
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Oct 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
UX All about "how to get things done"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants