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

[GUI] CoinControlDialog fix SelectAll / UnselectAll #1612

Merged

Conversation

random-zebra
Copy link

@random-zebra random-zebra commented May 9, 2020

Based on top of

Only last commit is new.
This updates the flow of the toggle button SelectAll/UnselectAll in the coin control dialog (pushButtonSelectAll).

  • use directly the checked state of the button, instead of caching it in a separate variable fSelectAllToggled.

  • when the button is in checked ("Unselect all") state, reset it to its initial state (unchecked, with "Select all" label) if all the entries are either manually deselected, or automatically cleared after a spend)

Closes point n.3 of #1609

Copy link

@CaveSpectre11 CaveSpectre11 left a comment

Choose a reason for hiding this comment

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

changed my opinion in the middle of writing; so just a "comment". Also, there's some other strange behavior when switching between the two coin controls, but i checked against 4.1 release and I see the same behavior; so it wasn't introduced with this or #1611. I'll write up separately; but if you want to play around, go into coin control in the send page, then out and into coin control in the delegation page, and you'll see different states (i.e. one will be highlighted like there's selections, the other won't, even though there is selected UTXOs).

More on that later. for now, just see below regarding [pure opinion] on the behavior of the quantity selected decision.

Aside of that; the root issue [select all when all coins have been spent being reset] is ACK.

src/qt/coincontroldialog.cpp Outdated Show resolved Hide resolved
@random-zebra
Copy link
Author

Rebased on master, now that #1611 has been merged and slightly modified the logic as per @CaveSpectre11's suggestion: when there's more selected utxos than unselected, the button is in "Unselect All" mode and, vice-versa, when there's more unselected then selected, the button switches to "Select All" mode.

@Fuzzbawls Fuzzbawls changed the title [GUI] CoinControlDialog: fix SelectAll / UnselectAll [GUI] CoinControlDialog fix SelectAll / UnselectAll Jun 14, 2020
furszy
furszy previously approved these changes Jun 15, 2020
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Code review ACK 28aef33

src/qt/coincontroldialog.cpp Outdated Show resolved Hide resolved
CaveSpectre11
CaveSpectre11 previously approved these changes Jun 15, 2020
Copy link

@CaveSpectre11 CaveSpectre11 left a comment

Choose a reason for hiding this comment

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

Looks great (with same nit). Will fire a build up tonight but code-wise utACK 28aef33

Thank you for considering (and running with) the more than/less than half idea.

- use directly the checked state of the button pushButtonSelectAll,
instead of caching it in fSelectAllToggled

- when the button is in checked ("Unselect all") state, reset to its
initial state (unchecked, with "Select all" label) if all the entries
are either manually deselected, or automatically cleared after a spend)
@random-zebra
Copy link
Author

Rebased and fixed nit.

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK 2ad58fd

@random-zebra random-zebra requested a review from furszy June 17, 2020 10:00
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

utACK 2ad58fd

@random-zebra
Copy link
Author

Merging...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants