Switch to the selected wallet after loading

w0xlt commented at 2:33 am on September 8, 2022: contributor

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.

gui: update the screen after loading wallet b8b59ff9fe

hebasto commented at 5:14 am on September 9, 2022: member

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

in src/qt/bitcoingui.cpp:413 in b8b59ff9fe

409@@ -410,7 +410,7 @@ void BitcoinGUI::createActions()
410 
411                 connect(action, &QAction::triggered, [this, path] {
412                     auto activity = new OpenWalletActivity(m_wallet_controller, this);
413-                    connect(activity, &OpenWalletActivity::opened, this, &BitcoinGUI::setCurrentWallet);
414+                    connect(activity, &OpenWalletActivity::opened, this, &BitcoinGUI::setCurrentWallet, Qt::QueuedConnection);

hebasto commented at 5:49 am on September 9, 2022:

Why this solution works?

w0xlt commented at 7:05 pm on September 9, 2022:

There’s a good explanation here: #471#pullrequestreview-989631968 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

luke-jr commented at 11:47 pm on September 10, 2022:

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

w0xlt commented at 3:21 pm on September 14, 2022:

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 added the label UX on Sep 9, 2022

w0xlt renamed this:
~~Update the screen after loading wallet~~
Switch to the selected wallet after loading
on Sep 9, 2022

jarolrod commented at 9:47 pm on September 13, 2022: member

ACK b8b59ff9fea69f4f25d98005e3ac9172a7a11c12

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 commented at 5:15 pm on September 21, 2022: contributor

Tested ACK on ubuntu 20.04.

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

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.

pablomartin4btc commented at 5:32 pm on September 21, 2022: contributor

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.

Sjors commented at 1:01 pm on October 6, 2022: member

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

hebasto approved

hebasto commented at 12:49 pm on October 27, 2022: member

ACK b8b59ff9fea69f4f25d98005e3ac9172a7a11c12, 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 merged this on Oct 27, 2022

hebasto closed this on Oct 27, 2022

sidhujag referenced this in commit 961c8ce93b on Oct 28, 2022

john-moffett cross-referenced this on Jan 10, 2023 from issue Switch RPCConsole wallet selection to the one most recently opened/restored/created by john-moffett

hebasto referenced this in commit c71a96c431 on Jul 4, 2023

bitcoin-core locked this on Oct 27, 2023

Switch to the selected wallet after loading #665