bugfix: Call setWalletActionsEnabled(true) only for the first wallet #43

1. 
   hebasto commented at 9:44 am on August 3, 2020: member

   On master (a78742830aa35bf57bcb0a4730977a1e5a1876bc) there is a bug:

   • open an encrypted wallet; please note that the “Encrypt Wallet…” menu item is disabled that is expected:
   • then open any other wallet; note that the “Encrypt Wallet…” menu item gets enabled that is wrong:

   This PR fixes this bug.

2. gui: Call setWalletActionsEnabled(true) only for the first wallet 20c9e03554
3. 
   hebasto commented at 9:45 am on August 3, 2020: member

   The bug initially was reported by @Saibato in #42:

   • #42 (comment)
   • #42 (comment)
4. 
   hebasto commented at 9:49 am on August 3, 2020: member

   friendly ping @achow101 @meshcollider @promag @Sjors
5. MarcoFalke added the label Bug on Aug 3, 2020
6. in src/qt/bitcoingui.cpp:670 in 20c9e03554

   669     rpcConsole->addWallet(walletModel);
   670-    m_wallet_selector->addItem(display_name, QVariant::fromValue(walletModel));
   671-    if (m_wallet_selector->count() == 2) {
   672+    if (m_wallet_selector->count() == 0) {
   673+        setWalletActionsEnabled(true);
   674+    } else if (m_wallet_selector->count() == 1) {
   

   ---

   ---

   

   achow101 commented at 3:57 pm on August 3, 2020:

   Why does this change from 2 to 1?

   ---

   

   hebasto commented at 4:10 pm on August 3, 2020:

   Because m_wallet_selector->addItem() is called after this code now.

   ---

   

   promag commented at 11:30 pm on August 9, 2020:

   Why not just add the count == 1 case in the old code?

   Edit: if it’s due to some other signal/slot then I’d say to refactor to remove that dependency.

   ---

   

   hebasto commented at 6:36 am on August 15, 2020:

   Why not just add the count == 1 case in the old code?

   It would change wallet selector behavior, and it won’t fix a bug due to the unconditional setWalletActionsEnabled(true) call.

   Edit: if it’s due to some other signal/slot then I’d say to refactor to remove that dependency.

   Not sure what do you mean. FWIW, moving this logic into setCurrentWalletBySelectorIndex() or setCurrentWallet() won’t work as these methods do not know if a wallet has been added or removed.

7. 
   achow101 commented at 3:58 pm on August 3, 2020: member

   It’s not clear to me why this fixes the problem. Wouldn’t a better fix be to enable/disable that menu item depending on the wallet being selected?
8. 
   hebasto commented at 4:18 pm on August 3, 2020: member

   @achow101

   It’s not clear to me why this fixes the problem. Wouldn’t a better fix be to enable/disable that menu item depending on the wallet being selected?

   This is exactly how it works with this PR :) When a new wallet added in m_wallet_selector->addItem(), a signal calls BitcoinGUI::setCurrentWalletBySelectorIndex() slot that in turn calls BitcoinGUI::setCurrentWallet().

   Things are broken in master due to the unconditional call setWalletActionsEnabled(true).

9. 
   achow101 commented at 5:01 pm on August 3, 2020: member

   When a new wallet added in m_wallet_selector->addItem(), a signal calls BitcoinGUI::setCurrentWalletBySelectorIndex() slot that in turn calls BitcoinGUI::setCurrentWallet().

   Things are broken in master due to the unconditional call setWalletActionsEnabled(true).

   BitcoinGUI::setCurrentWallet() should be occurring after setWalletActionsEnabled(true), so in theory, it should be fine to do it unconditionally. So why doesn’t it work?

10. 
    hebasto commented at 5:17 pm on August 3, 2020: member

    @achow101 I was a bit wrong/incomplete describing adding a new wallet ((

    When no wallets are open, and a new first wallet is added, the currentIndexChanged(0) signal is emitted with parameter index=0. This signal calls setCurrentWalletBySelectorIndex(0) slot. Result: wallet menu items are set correctly.

    When any wallet is open already, and a non-first wallet is added, the currentIndexChanged() signal is not emitted. And setCurrentWalletBySelectorIndex() is not called. Result: wallet menu items are ignored.

    In the second case the setWalletActionsEnabled(true) call enables all wallet menu items unconditionally. That is wrong.

11. hebasto renamed this:
    Call setWalletActionsEnabled(true) only for the first wallet
    bugfix: Call setWalletActionsEnabled(true) only for the first wallet
    on Oct 16, 2020
12. jonasschnelli added this to the milestone 0.21.0 on Oct 22, 2020
13. 
    jonasschnelli commented at 9:31 am on October 23, 2020: contributor

    Tested ACK 20c9e035543892e322c7134e89eb33115678bb30 - I could reproduce the issue on master and have verify that this PR fixes it.
14. 
    achow101 commented at 4:28 pm on October 23, 2020: member

    ACK 20c9e035543892e322c7134e89eb33115678bb30
15. MarcoFalke merged this on Oct 24, 2020
16. MarcoFalke closed this on Oct 24, 2020

    ---

17. hebasto deleted the branch on Oct 24, 2020
18. sidhujag referenced this in commit c4ad8e831d on Oct 25, 2020
19. apoelstra referenced this in commit 6723d3ab5e on Dec 3, 2020
20. laanwj referenced this in commit 90c0f267bd on Dec 9, 2020
21. gwillen referenced this in commit c257b0d178 on Mar 23, 2021
22. Fabcien referenced this in commit 96b27196cc on Dec 4, 2021
23. bitcoin-core locked this on Feb 15, 2022

Contributors
hebasto achow101 promag jonasschnelli

Labels
Bug

Milestone
0.21.0