Show "No wallets available" in open menu instead of nothing #15957

1. 
   meshcollider commented at 11:44 AM on May 6, 2019: contributor

   Fixes the confusing behavior reported in #15952

   

2. meshcollider added the label GUI on May 6, 2019
3. 
   promag commented at 12:02 PM on May 6, 2019: member

   Mind attaching a screenshot?

4. 
   meshcollider commented at 12:14 PM on May 6, 2019: contributor

   @promag screenshot added

5. fanquake added this to the milestone 0.18.1 on May 6, 2019
6. 
   jonasschnelli commented at 12:21 PM on May 6, 2019: contributor

   Concept ACK

7. in src/qt/bitcoingui.cpp:405 in febd959a6a outdated

    400 | @@ -400,6 +401,10 @@ void BitcoinGUI::createActions()
    401 |                      assert(invoked);
    402 |                  });
    403 |              }
    404 | +            if (available_wallets.empty()) {
    405 | +                QAction* action = m_open_wallet_action->menu()->addAction(QString(tr("No wallets available...")));
   

   ---

   ---

   

   promag commented at 12:41 PM on May 6, 2019:

   Unnecessary QString and ...

8. 
   promag commented at 12:51 PM on May 6, 2019: member

   Concept ACK.

9. MarcoFalke added the label Needs backport on May 6, 2019
10. 
    flack commented at 1:03 PM on May 6, 2019: contributor

    shouldn't it rather say something like "no additional wallets available"? IIUC wallets that are already open are not listed, so when you open the program with the default wallet and then it says "no wallets available", it might also be confusing

11. in src/qt/bitcoingui.cpp:374 in febd959a6a outdated

     369 | @@ -370,7 +370,8 @@ void BitcoinGUI::createActions()
     370 |          connect(openAction, &QAction::triggered, this, &BitcoinGUI::openClicked);
     371 |          connect(m_open_wallet_action->menu(), &QMenu::aboutToShow, [this] {
     372 |              m_open_wallet_action->menu()->clear();
     373 | -            for (std::string path : m_wallet_controller->getWalletsAvailableToOpen()) {
     374 | +            std::vector<std::string> available_wallets = m_wallet_controller->getWalletsAvailableToOpen();
     375 | +            for (std::string path : available_wallets) {
    

    ---

    ---

    

    ryanofsky commented at 1:33 PM on May 6, 2019:

    Could use const auto& path to avoid string copy.

12. ryanofsky approved
13. 
    ryanofsky commented at 2:02 PM on May 6, 2019: member

    utACK febd959a6aef9399d8a93b4476cf342217c35380. Code change looks good and this is less confusing than before. But it's still strange to have unloaded wallets listed in a menu on the left, and loaded wallets listed in a dropdown on the right, and no clear labeling of either list.

    I think an improved UI would make the menu wallet list and the dropdown wallet list identical, and either show all wallets in the lists with clear loaded/unloaded labeling, or only show loaded wallets and move the list of unloaded wallets to a separate "Load wallet..." dialog.

    Having the same wallets listed both in the file menu and dropdown would be redundant, but might be justified because the file menu is more discoverable and the dropdown menu is more convenient. We could drop one of the lists or move the list somewhere else like a top level Wallets menu.

14. 
    DrahtBot commented at 2:31 PM on May 6, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #15204 (gui: Add Open External Wallet action by promag)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

15. 
    fanquake commented at 9:15 AM on May 7, 2019: member

    Concept ACK

    I think an improved UI would make the menu wallet list and the dropdown wallet list identical, and either show all wallets in the lists with clear loaded/unloaded labeling

    👍

    shouldn't it rather say something like "no additional wallets available"?

    Agree, or just something like "All available wallets open."

16. 
    meshcollider commented at 11:34 AM on May 8, 2019: contributor

    ~Thoughts on instead using "checked" menu items, showing all wallets and leaving a check next to the ones already loaded (which would be a no-op when clicked)?~ I like the greyed out approach

    

17. 
    flack commented at 11:39 AM on May 8, 2019: contributor

    or maybe just gray out the ones already loaded?

18. Show loaded wallets as disabled in open menu instead of nothing c3ef63a52f
19. kristapsk approved
20. 
    kristapsk commented at 8:28 PM on May 14, 2019: contributor

    tACK c3ef63a52f304a600fff1f9c7caa5cb804d41d43

21. 
    jonasschnelli commented at 11:01 AM on May 18, 2019: contributor

    Tested ACK c3ef63a52f304a600fff1f9c7caa5cb804d41d43

22. jonasschnelli merged this on May 18, 2019
23. jonasschnelli closed this on May 18, 2019

    ---

24. jonasschnelli referenced this in commit 387eb5b343 on May 18, 2019
25. MarcoFalke removed the label Needs backport on May 18, 2019
26. MarcoFalke referenced this in commit 3dbc7def0f on May 18, 2019
27. sidhujag referenced this in commit 7af2053333 on May 18, 2019
28. pull[bot] referenced this in commit 2679bb8919 on Jul 8, 2019
29. HashUnlimited referenced this in commit 2f99bf0053 on Aug 23, 2019
30. Bushstar referenced this in commit c59b5240b7 on Aug 24, 2019
31. meshcollider deleted the branch on Sep 5, 2019
32. deadalnix referenced this in commit 6c61c44980 on May 20, 2020
33. PastaPastaPasta referenced this in commit ad1c40d722 on Oct 25, 2021
34. PastaPastaPasta referenced this in commit a63956ad32 on Oct 25, 2021
35. pravblockc referenced this in commit 1c6a9c4eff on Nov 18, 2021
36. pravblockc referenced this in commit 0d124aa356 on Nov 18, 2021
37. DrahtBot locked this on Dec 16, 2021

Milestone
0.18.1