Introduce Wallet Select Dropdown #401

johnny9 · 2024-05-19T03:59:38Z

WalletSelect is a Popup that appears after clicking the main
WalletBadge in the DesktopNavigation bar. It contains a ListView
that allows the user to select one of the wallets listed in the
wallet directory.

This PR uses the arrow icon that is shared with the Tooltip so it is based off of that commit.

MarnixCroes

two nits

wallet list should be closed when clicking outside of it. i.e. navigate to settings -> wallet list is still shown
small alignment

src/qml/models/walletlistmodel.cpp

johnny9 · 2024-05-27T12:31:33Z

Update from 4ef3149 to 5d191d1

Updated include guard on wallet list model
Added copyright header to walletlistmodel.cpp
Adjusted padding on Add Wallet icon
Changed closePolicy on WalletSelect Popup to CloseOnPressOutside
Rebased with main

pablomartin4btc

tACK 4ef3149

A few observations (cc @GBKS)

Sample wallet ("Singlesig Wallet") is still shown, is this going to be removed in the next iteration/ follow-up?
Should the wallets in the listview be ordered (e.g. in current desktop QT they are by naming as you can see in the following pict)?
In Figma, the wallet lists have an "X" on the right side of each item, is this to close/ unload the wallet? I see in src/qml/models/walletlistmodel.cpp the list is built looking at the wallet dirs (so there's no distinction in previous model/ current QT between "load" and "select" wallet functionality, now will be done all in once), so what's that "X" for? It needs to be added to this PR (as it's missing at the moment)?

I agree with @MarnixCroes, his observation regarding clicking outside the wallet menu (listview) the menu should be closed I think.

GBKS · 2024-06-07T11:36:37Z

so what's that "X" for?

The idea is that you can close the wallet that is currently open (result is that there would be no open wallet). And if you press "x" on a close wallet, you would remove it from the application altogether. We don't have those anywhere else in the UI yet on mobile.

Are there other options you may want to directly take on the wallet from here? Like changing the name? If we have more, we could instead use an ellipsis ... to access a sub-menu.

D33r-Gee

tACK on WSL Ubuntu 22.04

Looks like it behaves the way it's intended (see screenshot below)

screenshot

hebasto · 2024-06-08T08:15:03Z

@pablomartin4btc

tACK 4ef3149

Want to re-ACK?

hebasto · 2024-06-08T08:17:47Z

@D33r-Gee

tACK on WSL Ubuntu 22.04

To ensure that someone's ACK is related to a certain version of the branch, it is wise to mention the top commit hash. Besides, the merge script needs a top commit hash to count an ACK.

pablomartin4btc · 2024-06-09T11:25:21Z

so what's that "X" for?

The idea is that you can close the wallet that is currently open (result is that there would be no open wallet). And if you press "x" on a close wallet, you would remove it from the application altogether. We don't have those anywhere else in the UI yet on mobile.

Mmm... then we need to indicate somehow when the wallet is open and the "x" would close it and when it's closed and the "x" will remove it from the UI... and how do you get it back when has been removed? It's not very intuitive at the moment.

Also, I'm not sure if listing all wallets available in the datadir would be practical... perhaps this can be configured as an option and disabled as default or if there are more then 5/ 10 wallets asking the user and persist that into the config?

Are there other options you may want to directly take on the wallet from here? Like changing the name? If we have more, we could instead use an ellipsis ... to access a sub-menu.

Yeah, there's no "renaming" feature in QT currently, it could be useful and we can put the "remove" from the app or "close" the wallet... still, how do you added back once removed?

pablomartin4btc · 2024-06-09T11:27:16Z

@pablomartin4btc

tACK 4ef3149

Want to re-ACK?

Checking... somehow I missed 5d191d1.

pablomartin4btc

tACK 5d191d1

I still have these concerns.

GBKS · 2024-06-20T07:21:42Z

Pablo and I spoke about the wallet selector and I did another design iteration based on my current understanding. Here's a video that covers it. Still have some minor questions in there, please take a look and let me know how it looks.

pablomartin4btc

Here's a video that covers it.

These are my obs:

ordering the wallet list on last opened criteria (last 5? then alphabetical?);
scroll solution ok (but we need to solve the bar that gets on top of the remove/ close wallet icons);
icon for remove wallet looks fine but for closing looks a bit simple (what about using the wallet icon at the top when there are no wallets at all?)... also, what if we put both icons? you dont need to close (unload) the wallet first and then remove it, no?
once we remove a wallet, perhaps after the user clicks on the "Add +" we show these options: create a new or "add previously removed walet" or "link a wallet dir"?
we still have the issue when there are many wallets, what about a warning? "you have more than 15 wallets in your datadir, do you want to list them all? (and something saying this will be asked once and saved the configuration into the settings?);

These are open questions to the other reviewers as well.

GBKS · 2024-06-21T09:55:56Z

Thanks for the feedback.

ordering the wallet list on last opened criteria (last 5? then alphabetical?);

Why not always use the last open date?

scroll solution

I will mock this up

icon for remove wallet looks fine but for closing looks a bit simple...

I'll do another revision of this design as we discussed yesterday, where the wallet options are in a small overlay menu

once we remove a wallet, perhaps after the user clicks on the "Add +" we show these options: create a new or "add previously removed walet" or "link a wallet dir"?

The current design is that in the "Add wallet" screen, you can import. From there you select the wallet file you want to add. Does that work?

we still have the issue when there are many wallets

Why is this an issue?

pablomartin4btc · 2024-06-27T14:58:21Z

ordering the wallet list on last opened criteria (last 5? then alphabetical?);

Why not always use the last open date?

Depends on the amount of wallets we are talking about, that's what I call "issue", perhaps on the mobile version doesn't make sense to have as many, not sure. I'll ask other devs.

once we remove a wallet, perhaps after the user clicks on the "Add +" we show these options: create a new or "add previously removed walet" or "link a wallet dir"?

The current design is that in the "Add wallet" screen, you can import. From there you select the wallet file you want to add. Does that work?

Ok, maybe that's the use case. Is it possible for you to do a prototype as you did in the previous video to see how it would work? From removing the wallet from the list to add it back (if it's doable with the tool you are using).

we still have the issue when there are many wallets

Why is this an issue?

Performance will be impacted loading multiple wallets but what I'm not convinced about is building the list of wallets in the menu with whatever amount of wallets there are in the data directory. I'll ask around, perhaps there's nothing wrong with it and I'm just overly worried.

D33r-Gee · 2024-07-03T15:22:45Z

re-tACK 5d191d1 on WSL Ubuntu 22.04 works as expected...

Does it need rebase?

MarnixCroes

5d191d1
some possible improvements:

when having many wallets there should be some scrollbar, also to be able to select wallets displayed at the bottom

the wallet name should not break out of the width of the wallet select pop up

MarnixCroes · 2024-07-04T09:00:14Z

src/qml/pages/wallet/DesktopWallets.qml

+            WalletSelect {
+                id: walletSelect
+                model: walletListModel
+                closePolicy: Popup.CloseOnPressOutside


close when press outside.
I guess it should also close when clicking on it again, additionally when clicking outside?

I want to add this as an issue. It turns out its more difficult than I thought to fix this.

D33r-Gee · 2024-07-25T18:21:23Z

src/qml/models/walletlistmodel.cpp

+    beginInsertRows(QModelIndex(), rowCount(), rowCount());
+    m_items.append(item);
+    endInsertRows();
+}


Does it need a blank line at the end?

src/qml/models/walletlistmodel.h

D33r-Gee · 2024-07-25T21:04:05Z

tACK 411a08a on WSL Ubuntu 22.04

Works as expected...

Ubuntu 22.04 Screenshot

MarnixCroes

tACK 833fc38
previous points #401 (review) can be done as follow up

D33r-Gee · 2024-08-08T17:30:16Z

src/qml/pages/wallet/WalletBadge.qml

+            PropertyChanges { target: balanceText; color: root.textHoverColor }
+        }
+    ]
+}


does it need a blank line at the bottom?

D33r-Gee · 2024-08-08T17:37:35Z

tACK 833fc38 on WSL Ubuntu 22.04

Works as expected... Only comment is about a missing ending line in WalletBadge.qml

Screenshot from Ubuntu

johnny9 · 2024-08-08T21:11:06Z

from 833fc38 to 366c605

New line at the end of WalletBadge.qml
Constrained width of wallet names as suggested by Marnix
Constrained height of wallet list as suggested by Marnix and they are now scrollable
Fixed colors to match latest figma

previous (833fc38):

update (366c605)

Will address other significant changes as new PRs as to not make this one too large for now. For instance, load/delete buttons in the wallet list I plan to have as a follow up as well as the logic for ordering the list based on recent use.

MarnixCroes

tACK 366c605

some nits, can be follow up:

a visible scrollbar for the wallet list
when a wallet name exceeds the width and is thus cut off -> would be good display the full wallet name in a tooltip on hover

src/qml/pages/wallet/WalletBadge.qml

The WalletBadge is an Item that shows a wallet's name, type and balance. The WalletBadge balance shows all of the decimal places for a whole bitcoin and highlights the satoshis available. The component can be configured to not show the Icon or the Balance.

WalletSelect is a Popup that appears after clicking the main WalletBadge in the DesktopNavigation bar. It contains a ListView that allows the user to select one of the wallets listed in the wallet directory.

johnny9 · 2024-08-09T17:12:44Z

Update from 366c605 to 0939f2b

Fixed typo in comment in WalletBadge.qml

D33r-Gee · 2024-08-09T19:35:01Z

tACK 0939f2b on WSL Ubuntu 22.04

Works as expected and looks great

Ubuntu Screenshot

MarnixCroes

re-ack 0939f2b

hebasto · 2024-08-12T10:04:38Z

re-ack 0939f2b

FWIW, "ack" is not counted by the merge script. The "ACK" is expected.

ba98b83 qml: add scrollbar to wallet select list (Marnix) Pull request description: This adds a scrollbar to the wallet selection list. Addresses #401 (review) --- ### Images scrollbar visible on hover/when scrolling when having more wallets than that are initially visible ![image](https://github.com/user-attachments/assets/6cecd990-5e20-48d7-95eb-fcf79cd368d6) no scrollbar when not needed: ![image](https://github.com/user-attachments/assets/1d9d7418-858a-4576-a1fc-c63ead211919)     ACKs for top commit: pablomartin4btc: utACK ba98b83 jarolrod: ACK ba98b83 Tree-SHA512: 1013230070913dcb2be6674b9c25acc51c49b8e603b079ca6e6802b3d6b6ce7fabca44a25bef1f641c133b75051d132200573e3d5846c59beb14bcff5edbd34c

MarnixCroes reviewed May 20, 2024

View reviewed changes

src/qml/models/walletlistmodel.cpp Show resolved Hide resolved

johnny9 force-pushed the wallet-select-pr branch 2 times, most recently from 6670781 to 5d191d1 Compare May 27, 2024 12:28

pablomartin4btc reviewed May 27, 2024

View reviewed changes

D33r-Gee reviewed Jun 7, 2024

View reviewed changes

pablomartin4btc reviewed Jun 9, 2024

View reviewed changes

pablomartin4btc reviewed Jun 20, 2024

View reviewed changes

MarnixCroes reviewed Jul 4, 2024

View reviewed changes

johnny9 force-pushed the wallet-select-pr branch from 5d191d1 to 411a08a Compare July 25, 2024 03:30

D33r-Gee reviewed Jul 25, 2024

View reviewed changes

src/qml/models/walletlistmodel.h Show resolved Hide resolved

johnny9 mentioned this pull request Aug 1, 2024

Constrain items in WalletSelector on Desktop johnny9/BitcoinCoreAppDevelopment#1

Closed

johnny9 force-pushed the wallet-select-pr branch from 411a08a to 833fc38 Compare August 7, 2024 19:51

MarnixCroes approved these changes Aug 8, 2024

View reviewed changes

D33r-Gee reviewed Aug 8, 2024

View reviewed changes

johnny9 force-pushed the wallet-select-pr branch 2 times, most recently from 59d59b5 to 366c605 Compare August 8, 2024 21:02

MarnixCroes approved these changes Aug 9, 2024

View reviewed changes

src/qml/pages/wallet/WalletBadge.qml Outdated Show resolved Hide resolved

johnny9 added 2 commits August 9, 2024 12:50

qml: Introduce the WalletSelect component

0939f2b

WalletSelect is a Popup that appears after clicking the main WalletBadge in the DesktopNavigation bar. It contains a ListView that allows the user to select one of the wallets listed in the wallet directory.

johnny9 force-pushed the wallet-select-pr branch from 366c605 to 0939f2b Compare August 9, 2024 17:08

johnny9 mentioned this pull request Aug 9, 2024

When a wallet is selected, load the actual balance into the badge johnny9/BitcoinCoreAppDevelopment#9

Open

MarnixCroes approved these changes Aug 12, 2024

View reviewed changes

hebasto merged commit 8c19122 into bitcoin-core:main Aug 12, 2024
9 checks passed

MarnixCroes mentioned this pull request Aug 12, 2024

add scrollbar to wallet select list #416

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Wallet Select Dropdown #401

Introduce Wallet Select Dropdown #401

johnny9 commented May 19, 2024 •

edited

Loading

MarnixCroes left a comment

johnny9 commented May 27, 2024

pablomartin4btc left a comment

GBKS commented Jun 7, 2024

D33r-Gee left a comment

hebasto commented Jun 8, 2024

hebasto commented Jun 8, 2024

pablomartin4btc commented Jun 9, 2024

pablomartin4btc commented Jun 9, 2024

pablomartin4btc left a comment

GBKS commented Jun 20, 2024

pablomartin4btc left a comment

GBKS commented Jun 21, 2024

pablomartin4btc commented Jun 27, 2024

D33r-Gee commented Jul 3, 2024

MarnixCroes left a comment •

edited

Loading

MarnixCroes Jul 4, 2024

johnny9 Aug 8, 2024

D33r-Gee Jul 25, 2024

D33r-Gee commented Jul 25, 2024

MarnixCroes left a comment

D33r-Gee Aug 8, 2024

D33r-Gee commented Aug 8, 2024

johnny9 commented Aug 8, 2024 •

edited

Loading

MarnixCroes left a comment

johnny9 commented Aug 9, 2024

D33r-Gee commented Aug 9, 2024

MarnixCroes left a comment

hebasto commented Aug 12, 2024

Introduce Wallet Select Dropdown #401

Introduce Wallet Select Dropdown #401

Conversation

johnny9 commented May 19, 2024 • edited Loading

MarnixCroes left a comment

Choose a reason for hiding this comment

johnny9 commented May 27, 2024

pablomartin4btc left a comment

Choose a reason for hiding this comment

GBKS commented Jun 7, 2024

D33r-Gee left a comment

Choose a reason for hiding this comment

hebasto commented Jun 8, 2024

hebasto commented Jun 8, 2024

pablomartin4btc commented Jun 9, 2024

pablomartin4btc commented Jun 9, 2024

pablomartin4btc left a comment

Choose a reason for hiding this comment

GBKS commented Jun 20, 2024

pablomartin4btc left a comment

Choose a reason for hiding this comment

GBKS commented Jun 21, 2024

pablomartin4btc commented Jun 27, 2024

D33r-Gee commented Jul 3, 2024

MarnixCroes left a comment • edited Loading

Choose a reason for hiding this comment

MarnixCroes Jul 4, 2024

Choose a reason for hiding this comment

johnny9 Aug 8, 2024

Choose a reason for hiding this comment

D33r-Gee Jul 25, 2024

Choose a reason for hiding this comment

D33r-Gee commented Jul 25, 2024

MarnixCroes left a comment

Choose a reason for hiding this comment

D33r-Gee Aug 8, 2024

Choose a reason for hiding this comment

D33r-Gee commented Aug 8, 2024

johnny9 commented Aug 8, 2024 • edited Loading

MarnixCroes left a comment

Choose a reason for hiding this comment

johnny9 commented Aug 9, 2024

D33r-Gee commented Aug 9, 2024

MarnixCroes left a comment

Choose a reason for hiding this comment

hebasto commented Aug 12, 2024

johnny9 commented May 19, 2024 •

edited

Loading

MarnixCroes left a comment •

edited

Loading

johnny9 commented Aug 8, 2024 •

edited

Loading