Skip to content

Introduce Coin Selection page #448

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

Merged
merged 11 commits into from
May 12, 2025
Merged

Conversation

johnny9
Copy link
Collaborator

@johnny9 johnny9 commented Mar 25, 2025

This is the first iteration of the Coin Selection menu

The option to open the Coin Selection menu is a QSetting that is toggled on/off in the ellipsis menu on the Send page. When enabled, the user can click on the Select inputs button and the Coin Selection menu will be push on the stack from below. The Coin Selection menu will list possible inputs and a checkbox is used to select the coins. Locked coins
are not selectable and instead of a check box, they have a locked icon.

Screencast.from.2025-03-25.00-49-26.mp4

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

first testing round

  • it always tells me I have 80 sats selected, while no coin is selected
qml-coinselection.webm
  • nit: vertical scrollbar at the coinlist when having many coins
  • when switching the Amount entry to sats, the coin amounts are still in BTC

@yashrajd
Copy link

Oh great to see this page being built!

Suggestion (I did not test build): the Total Selected and Over required amount list items should be sticky/Freezed and should always be on the top. This will help the user track these numbers while they're scrolling and selecting coins. Otherwise they have to scroll to the top to check...

@johnny9
Copy link
Collaborator Author

johnny9 commented Mar 25, 2025

Suggestion (I did not test build): the Total Selected and Over required amount list items should be sticky/Freezed and should always be on the top.

Yeah I agree on this. I'll move that component outside of the scroll view.

@johnny9
Copy link
Collaborator Author

johnny9 commented Mar 25, 2025

first testing round

* it always tells me I have 80 sats selected, while no coin is selected

Interesting, ill try to reproduce and fix this.

qml-coinselection.webm

* nit: vertical scrollbar at the coinlist when having many coins

I'll add the scroll bar to the list

* when switching the _Amount_ entry to sats, the coin amounts are still in BTC

All amount inputs and labels aren't going to look great right now. I need to create the components to display these values properly as well as the state to hold the preferences but i will be getting there soon.

@johnny9
Copy link
Collaborator Author

johnny9 commented Mar 28, 2025

Updates were made to address all comments outside of the issues with the amounts and the amount resetting after send. These are related issues and will be addressed when I circle back around to solve the BitcoinAmount formatting feature.

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

12aa84c
some other things I noticed while testing (maybe as notes for follow-up)

  • the over or remaining to select is not displaying the right value in case the review button doesn't work (this happens sometimes, was mentioned in previous PR)
Screencast.from.2025-03-31.16-55-52.webm
  • the coinlist shouldn't be open-enable in case the user has no coins?
  • sometimes when a re-index is needed it displays a random number (which I have not been able to repro using qt)

Screenshot from 2025-03-31 17-07-39

Screenshot from 2025-03-31 17-07-00

johnny9 added 9 commits May 1, 2025 00:04
The coins list model will be used to list out of the locked and
selectable coins in the coin selection form. A toggle method is
provided that will select or unselect the coin in the associated
WalletQmlModel.
The CoinSelection page is opened up from the Send page. The
button to push CoinSelection on the Send page stack is enabled
in the ellipsis menu. The toggle in the ellipsis menu can set a
QSetting for enabled/disabled. All of these components are added
to the DesktopWallets and Send page.
@johnny9
Copy link
Collaborator Author

johnny9 commented May 1, 2025

Update from aa78989 to a0ee146

  • fix year in copyright header for 2 new qml files

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

a0ee146

tested, basic functionality is ok

main issue that remains is that on empty wallets it always shows 80 sats are selected

Screencast.from.2025-05-05.13-39-37.webm

@johnny9
Copy link
Collaborator Author

johnny9 commented May 6, 2025

@MarnixCroes thank you for double checking. I thought I had pushed the fix already. The two commits are now a part of the PR.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK c9bf7ae, I have skimmed through the code and it looks OK.

Comment on lines +73 to +75
if (m_amount == "") {
return 0;
}
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

The Qt docs states that QString::toLongLong():

Returns 0 if the conversion fails.

@hebasto hebasto merged commit 320b3d4 into bitcoin-core:main May 12, 2025
8 of 9 checks passed
hebasto added a commit that referenced this pull request Jul 2, 2025
1769fa8 Fix typo in MultipleSendReview (Johnny)
5642319 qml: Change IconButton disabled color to neutral4 (johnny9)
0ed6bd9 qml: Disable multipleRecipients after clearing the recipients list (johnny9)
ed4ba2a qml: Add a recipient when multiple recipients are enabled (johnny9)
d9c3877 qml: Connect Recipient label to Note to self input (johnny9)
36aecee qml: New recipients use current's units (johnny9)
f673f7c qml: When multipleRecipients is disabled, clearToFront of the list (johnny9)
a583c6c qml: Split amount and display updating for Bitcoin amount input (johnny9)
5fd4441 qml: Restrict Recipients to 25 (johnny9)
c05e3bc qml: Handle removal of first recipient properly (johnny9)
3b15c2a qml: Clear Send form after sending transaciton (johnny9)
7543860 qml: Commit Recipient amount when active focus is lost (johnny9)
fe56371 qml: Add total calculation to SendRecipientsListModel (johnny9)
20f7189 qml: Cleanup BitcoinAmount (johnny9)
238ce8c qml: Replace NavButton with IconButton in Send (johnny9)
3acfb21 qml: Add plus big filled icon (johnny9)
0a08371 qml: Add MultipleSendReview page (johnny9)
e5e5d6a qml: Prepare transaction with recipients list (johnny9)
5dc3f33 qml: Add remove button to multiple recipients (johnny9)
fc6dd3a qml: Reduce size of recipient selectors (johnny9)
06e9586 qml: Add Multiple Recipients bar to Send form (johnny9)
059a7a2 qml: Add Multiple Recipients toggle to Send menu (johnny9)
20d6bcd qml: Introduce SendRecipientsListModel (johnny9)

Pull request description:

  This change introduces the multiple recipients controls to the Send form. It is enabled in the ellipses option menu by toggling on "Enable Multiple Recipients". This is stored as a QSetting for the user.

  This PR depends on #448 which contains the

  ![Screenshot from 2025-05-08 11-51-34](https://github.com/user-attachments/assets/c1756abd-b485-4cf1-b6a8-2c0fa91ff05a)
   first implementation of the Send options menu.
  ![Screenshot from 2025-05-08 11-51-26](https://github.com/user-attachments/assets/6ce9dd39-1b12-49dd-af50-2724fabbb8b9)

Top commit has no ACKs.

Tree-SHA512: 2f8f9e76eea107a898015966c6f838f303d7364852a26d0ab74dfb76bbf756563a9d29b0d6572c39b78ca23dc012c93af0fdcfa5d30f6bee693f8ac34c3b0c64
johnny9 pushed a commit to johnny9/gui-qml that referenced this pull request Jul 10, 2025
676db23 Fix typo in MultipleSendReview (Johnny)
69c8d52 qml: Change IconButton disabled color to neutral4 (johnny9)
01affb5 qml: Disable multipleRecipients after clearing the recipients list (johnny9)
2b1d39d qml: Add a recipient when multiple recipients are enabled (johnny9)
2093017 qml: Connect Recipient label to Note to self input (johnny9)
ee08381 qml: New recipients use current's units (johnny9)
1e4159a qml: When multipleRecipients is disabled, clearToFront of the list (johnny9)
5114425 qml: Split amount and display updating for Bitcoin amount input (johnny9)
e29622c qml: Restrict Recipients to 25 (johnny9)
4872600 qml: Handle removal of first recipient properly (johnny9)
4d040e9 qml: Clear Send form after sending transaciton (johnny9)
33502e4 qml: Commit Recipient amount when active focus is lost (johnny9)
8a1dbe0 qml: Add total calculation to SendRecipientsListModel (johnny9)
2de4f26 qml: Cleanup BitcoinAmount (johnny9)
b94f2bb qml: Replace NavButton with IconButton in Send (johnny9)
d7d1d45 qml: Add plus big filled icon (johnny9)
2552e3f qml: Add MultipleSendReview page (johnny9)
bd3fac2 qml: Prepare transaction with recipients list (johnny9)
fdbaf00 qml: Add remove button to multiple recipients (johnny9)
1a8b696 qml: Reduce size of recipient selectors (johnny9)
34706bb qml: Add Multiple Recipients bar to Send form (johnny9)
191edb7 qml: Add Multiple Recipients toggle to Send menu (johnny9)
0cb338c qml: Introduce SendRecipientsListModel (johnny9)

Pull request description:

  This change introduces the multiple recipients controls to the Send form. It is enabled in the ellipses option menu by toggling on "Enable Multiple Recipients". This is stored as a QSetting for the user.

  This PR depends on bitcoin-core#448 which contains the

  ![Screenshot from 2025-05-08 11-51-34](https://github.com/user-attachments/assets/c1756abd-b485-4cf1-b6a8-2c0fa91ff05a)
   first implementation of the Send options menu.
  ![Screenshot from 2025-05-08 11-51-26](https://github.com/user-attachments/assets/6ce9dd39-1b12-49dd-af50-2724fabbb8b9)

Top commit has no ACKs.

Tree-SHA512: 2f8f9e76eea107a898015966c6f838f303d7364852a26d0ab74dfb76bbf756563a9d29b0d6572c39b78ca23dc012c93af0fdcfa5d30f6bee693f8ac34c3b0c64
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants