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

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

Merged
merged 1 commit into from May 18, 2019

Conversation

Projects
None yet
9 participants
@meshcollider
Copy link
Member

commented May 6, 2019

Fixes the confusing behavior reported in #15952

image

@meshcollider meshcollider added the GUI label May 6, 2019

@promag

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Mind attaching a screenshot?

@meshcollider

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

@promag screenshot added

@fanquake fanquake added this to the 0.18.1 milestone May 6, 2019

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Concept ACK

@promag
Copy link
Member

left a comment

Concept ACK.

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

This comment has been minimized.

Copy link
@promag

promag May 6, 2019

Member

Unnecessary QString and ...

@flack

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

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

@ryanofsky
Copy link
Contributor

left a comment

utACK febd959. 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.

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 6, 2019

Contributor

Could use const auto& path to avoid string copy.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

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

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.

@fanquake

This comment has been minimized.

Copy link
Member

commented May 7, 2019

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."

@meshcollider

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

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

image

@flack

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

or maybe just gray out the ones already loaded?

@meshcollider meshcollider force-pushed the meshcollider:201905_openwallet_empty branch from febd959 to c3ef63a May 8, 2019

@kristapsk
Copy link
Contributor

left a comment

tACK c3ef63a

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented May 18, 2019

Tested ACK c3ef63a

@jonasschnelli jonasschnelli merged commit c3ef63a into bitcoin:master May 18, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonasschnelli added a commit that referenced this pull request May 18, 2019

Merge #15957: Show "No wallets available" in open menu instead of not…
…hing

c3ef63a Show loaded wallets as disabled in open menu instead of nothing (MeshCollider)

Pull request description:

  Fixes the confusing behavior reported in #15952

  ![image](https://user-images.githubusercontent.com/3211283/57224284-0e8e7f80-705d-11e9-9554-2450cc3dbb8e.png)

ACKs for commit c3ef63:
  jonasschnelli:
    Tested ACK c3ef63a
  kristapsk:
    tACK c3ef63a

Tree-SHA512: fc2b94936ca32b89e8146c65e3629785883d78660afc8838818df652a4df9185ddca6b36ebf140a7159ab42b0fa5aa72867558d4572a009be06f0831fa813d1f

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request May 18, 2019

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 18, 2019

Merge bitcoin#15957: Show "No wallets available" in open menu instead…
… of nothing

c3ef63a Show loaded wallets as disabled in open menu instead of nothing (MeshCollider)

Pull request description:

  Fixes the confusing behavior reported in bitcoin#15952

  ![image](https://user-images.githubusercontent.com/3211283/57224284-0e8e7f80-705d-11e9-9554-2450cc3dbb8e.png)

ACKs for commit c3ef63:
  jonasschnelli:
    Tested ACK c3ef63a
  kristapsk:
    tACK c3ef63a

Tree-SHA512: fc2b94936ca32b89e8146c65e3629785883d78660afc8838818df652a4df9185ddca6b36ebf140a7159ab42b0fa5aa72867558d4572a009be06f0831fa813d1f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.