gui: Sort wallets in open wallet menu #16106

promag commented at 6:15 PM on May 27, 2019: member

Ensure wallets are sorted by name in the open wallet menu. This also improves the change done in #15957, since it avoids a second call to listWalletDir.

fanquake added the label GUI on May 27, 2019

DrahtBot commented at 7:28 PM on May 27, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16273 (refactor: Remove unused includes by practicalswift)
#16261 (gui: Refactor OpenWalletActivity by promag)
#15450 ([GUI] Create wallet menu option by achow101)

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.

kristapsk approved

kristapsk commented at 8:29 PM on May 27, 2019: contributor

ACK 1547ad29abc2b6b3ec08d3758e66d038dafeac66

in src/qt/walletcontroller.cpp:49 in 1547ad29ab outdated

  45 | @@ -46,13 +46,15 @@ std::vector<WalletModel*> WalletController::getWallets() const
  46 |      return m_wallets;
  47 |  }
  48 |  
  49 | -std::vector<std::string> WalletController::getWalletsAvailableToOpen() const
  50 | +std::map<std::string, bool> WalletController::getAllWallets() const

Empact commented at 8:41 PM on May 27, 2019:

nit: could be more self-documenting - something like getWalletsNamesAndLoaded

promag commented at 9:16 PM on May 27, 2019:

Renamed these methods and added comments, hope it's clear now.

Empact commented at 8:41 PM on May 27, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/16106/commits/1547ad29abc2b6b3ec08d3758e66d038dafeac66

promag force-pushed on May 27, 2019

Empact commented at 9:22 PM on May 27, 2019: member

re-utACK https://github.com/bitcoin/bitcoin/pull/16106/commits/4aec893d08b912d38993d73630323be307b43e90

pox commented at 9:48 AM on May 28, 2019: contributor

Looks correct. Would benefit from test coverage for the change in expected behavior (i.e. assert wallets are displayed sorted alphabetically) to prevent future regressions (e.g. if someone decides to modify the impl to use an unordered_map).

in src/qt/bitcoingui.cpp:373 in 4aec893d08 outdated

 369 | @@ -370,13 +370,12 @@ 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 | -            std::vector<std::string> available_wallets = m_wallet_controller->getWalletsAvailableToOpen();
 374 | -            std::vector<std::string> wallets = m_node.listWalletDir();
 375 | -            for (const auto& path : wallets) {
 376 | +            for (const std::pair<std::string, bool>& i : m_wallet_controller->listWalletDir()) {

fanquake commented at 2:21 PM on May 28, 2019:

qt/bitcoingui.cpp:373:54: warning: loop variable 'i' has type 'const std::pair<std::string, bool> &' (aka 'const pair<basic_string<char, char_traits<char>, allocator<char> >, bool> &') but is initialized with type 'std::__1::__map_iterator<std::__1::__tree_iterator<std::__1::__value_type<std::__1::basic_string<char>, bool>, std::__1::__tree_node<std::__1::__value_type<std::__1::basic_string<char>, bool>, void *> *, long> >::value_type' (aka 'pair<const std::__1::basic_string<char>, bool>') resulting in a copy [-Wrange-loop-analysis]
            for (const std::pair<std::string, bool>& i : m_wallet_controller->listWalletDir()) {
                                                     ^
qt/bitcoingui.cpp:373:18: note: use non-reference type 'std::pair<std::string, bool>' (aka 'pair<basic_string<char, char_traits<char>, allocator<char> >, bool>') to keep the copy or type 'const std::__1::__map_iterator<std::__1::__tree_iterator<std::__1::__value_type<std::__1::basic_string<char>, bool>, std::__1::__tree_node<std::__1::__value_type<std::__1::basic_string<char>, bool>, void *> *, long> >::value_type &' (aka 'const pair<const std::__1::basic_string<char>, bool> &') to prevent copying
            for (const std::pair<std::string, bool>& i : m_wallet_controller->listWalletDir()) {
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

promag commented at 2:37 PM on May 28, 2019:

Thanks, fixed.

fanquake commented at 2:24 PM on May 28, 2019: member

Concept ACK

master 76e2cded477bc483ec610212bdadf21fe35292d4: wallets-master

This PR https://github.com/bitcoin/bitcoin/pull/16106/commits/4aec893d08b912d38993d73630323be307b43e90: wallets-16106

promag force-pushed on May 28, 2019

jonasschnelli commented at 7:05 AM on May 29, 2019: contributor

utACK ffa113085f9fdbc6ab065a54ceec17df119540dc

in src/qt/bitcoingui.cpp:373 in ffa113085f outdated

 369 | @@ -370,13 +370,12 @@ 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 | -            std::vector<std::string> available_wallets = m_wallet_controller->getWalletsAvailableToOpen();
 374 | -            std::vector<std::string> wallets = m_node.listWalletDir();
 375 | -            for (const auto& path : wallets) {
 376 | +            for (auto i : m_wallet_controller->listWalletDir()) {

Empact commented at 10:01 PM on May 29, 2019:

I think you still want to capture by reference here, const auto& or const std::pair<const std::string, bool>&.

promag commented at 9:59 PM on May 30, 2019:

Done.

promag force-pushed on May 30, 2019

Empact commented at 7:19 PM on May 31, 2019: member

re-utACK https://github.com/bitcoin/bitcoin/pull/16106/commits/ddad3a642d50251a067c91932b372cb60f48e1a7

DrahtBot added the label Needs rebase on Jun 23, 2019

promag force-pushed on Jun 30, 2019

DrahtBot removed the label Needs rebase on Jun 30, 2019

in src/qt/walletcontroller.cpp:57 in 7a549fcc17 outdated

  56 | +        wallets[name] = false;
  57 | +    }
  58 |      for (WalletModel* wallet_model : m_wallets) {
  59 | -        auto it = std::remove(wallets.begin(), wallets.end(), wallet_model->wallet().getWalletName());
  60 | -        if (it != wallets.end()) wallets.erase(it);
  61 | +        wallets[wallet_model->wallet().getWalletName()] = true;

laanwj commented at 11:11 AM on July 8, 2019:

I think this needs to check if wallet_model->wallet().getWalletName() is in the map in the first place before setting it to true, in case there's a wallet loaded (for whatever reason) that's not in the wallet dir, it shouldn't suddenly appear in this list

promag commented at 11:39 AM on July 8, 2019:

Good catch.

promag commented at 1:24 PM on July 8, 2019:

for whatever reason

One reason is external wallets.

promag commented at 2:12 PM on July 8, 2019:

Fixed.

fanquake added the label Waiting for author on Jul 8, 2019

promag force-pushed on Jul 8, 2019

fanquake removed the label Waiting for author on Jul 8, 2019

gui: Sort wallets in open wallet menu 224eb9534a

promag force-pushed on Jul 8, 2019

refactor: Rename getWallets to getOpenWallets in WalletController fa90fe6440

promag force-pushed on Jul 8, 2019

laanwj commented at 2:31 PM on July 8, 2019: member

code review ACK fa90fe6440301edba71a6f0b1ba3ef1c78d5eae4

laanwj merged this on Jul 8, 2019

laanwj closed this on Jul 8, 2019

pull[bot] referenced this in commit 2679bb8919 on Jul 8, 2019

promag deleted the branch on Jul 8, 2019

deadalnix referenced this in commit 40eb5c46bf on May 21, 2020

deadalnix referenced this in commit ae5f71c818 on May 21, 2020

PastaPastaPasta referenced this in commit a63956ad32 on Oct 25, 2021

pravblockc referenced this in commit 0d124aa356 on Nov 18, 2021

MarcoFalke locked this on Dec 16, 2021