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

meshcollider · 2019-05-06T11:44:54Z

Fixes the confusing behavior reported in #15952

promag · 2019-05-06T12:02:36Z

Mind attaching a screenshot?

meshcollider · 2019-05-06T12:14:38Z

@promag screenshot added

jonasschnelli · 2019-05-06T12:21:22Z

Concept ACK

promag

Concept ACK.

promag · 2019-05-06T12:41:25Z

src/qt/bitcoingui.cpp

Unnecessary QString and ...

flack · 2019-05-06T13:03:38Z

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

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.

ryanofsky · 2019-05-06T13:33:56Z

src/qt/bitcoingui.cpp

Could use const auto& path to avoid string copy.

DrahtBot · 2019-05-06T14:31:30Z

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 · 2019-05-07T09:15:54Z

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 · 2019-05-08T11:34:43Z

~~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

flack · 2019-05-08T11:39:19Z

or maybe just gray out the ones already loaded?

kristapsk

tACK c3ef63a

jonasschnelli · 2019-05-18T11:01:02Z

Tested ACK c3ef63a

…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

Github-Pull: bitcoin#15957 Rebased-From: c3ef63a

… 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

Github-Pull: bitcoin#15957 Rebased-From: c3ef63a

… of nothing Summary: bitcoin/bitcoin@c3ef63a --- This is a backport of Core [[bitcoin/bitcoin#15957 | PR15957]] Test Plan: ninja check ./src/qt/bitcoin-qt -regtest Focus the Open wallet menu item, see that [default wallet] appears, greyed out because it is loaded. Empty out the `std::vector<std::string> wallets` right before the for-loop that iterates through it, compile and run the above again, see that focusing the Open wallet menu item shows `No wallets available` Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6163

… 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

fa90fe6 refactor: Rename getWallets to getOpenWallets in WalletController (João Barbosa) 224eb95 gui: Sort wallets in open wallet menu (João Barbosa) Pull request description: Ensure wallets are sorted by name in the open wallet menu. This also improves the change done in bitcoin#15957, since it avoids a second call to `listWalletDir`. ACKs for top commit: laanwj: code review ACK fa90fe6 Tree-SHA512: 349ea195021e56760dea3551126d1b9dc4821faab44aaf2858229db4fddaf0ce0b5eb0a8fa5025f47c77134b003067a77e8c340f9655a99e1305d430ccecced8

… 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

fa90fe6 refactor: Rename getWallets to getOpenWallets in WalletController (João Barbosa) 224eb95 gui: Sort wallets in open wallet menu (João Barbosa) Pull request description: Ensure wallets are sorted by name in the open wallet menu. This also improves the change done in bitcoin#15957, since it avoids a second call to `listWalletDir`. ACKs for top commit: laanwj: code review ACK fa90fe6 Tree-SHA512: 349ea195021e56760dea3551126d1b9dc4821faab44aaf2858229db4fddaf0ce0b5eb0a8fa5025f47c77134b003067a77e8c340f9655a99e1305d430ccecced8

meshcollider added the GUI label May 6, 2019

fanquake added this to the 0.18.1 milestone May 6, 2019

promag reviewed May 6, 2019

View reviewed changes

src/qt/bitcoingui.cpp Outdated

Copy link

Contributor

promag May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary QString and ...

maflcko added the Needs backport label May 6, 2019

ryanofsky approved these changes May 6, 2019

View reviewed changes

src/qt/bitcoingui.cpp Outdated

Copy link

Contributor

ryanofsky May 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use const auto& path to avoid string copy.

practicalswift reacted with thumbs up emoji

Show loaded wallets as disabled in open menu instead of nothing

c3ef63a

kristapsk approved these changes May 14, 2019

View reviewed changes

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

maflcko removed the Needs backport label May 18, 2019

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request May 18, 2019

Show loaded wallets as disabled in open menu instead of nothing

3dbc7de

Github-Pull: bitcoin#15957 Rebased-From: c3ef63a

promag mentioned this pull request May 27, 2019

gui: Sort wallets in open wallet menu #16106

Merged

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Aug 23, 2019

Show loaded wallets as disabled in open menu instead of nothing

2f99bf0

Github-Pull: bitcoin#15957 Rebased-From: c3ef63a

Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Aug 24, 2019

Show loaded wallets as disabled in open menu instead of nothing

c59b524

Github-Pull: bitcoin#15957 Rebased-From: c3ef63a

meshcollider deleted the 201905_openwallet_empty branch September 5, 2019 11:56

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

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

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

Uh oh!

Conversation

meshcollider commented May 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented May 6, 2019

Uh oh!

meshcollider commented May 6, 2019

Uh oh!

jonasschnelli commented May 6, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag May 6, 2019

Choose a reason for hiding this comment

Uh oh!

flack commented May 6, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky May 6, 2019

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented May 6, 2019

Conflicts

Uh oh!

fanquake commented May 7, 2019

Uh oh!

meshcollider commented May 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flack commented May 8, 2019

Uh oh!

kristapsk left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented May 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

meshcollider commented May 6, 2019 •

edited

Loading

meshcollider commented May 8, 2019 •

edited

Loading