gui
gui copied to clipboard
bitcoin-core
Improve 'Requested Payments History' Multiselect
The "Requested payments history" list has somewhat broken functionality. You can only select contiguous rows. Sorting the list unexpectedly modifies any selection you made:
Initially sorted by date and multiple items selected:
Now sort by label ->
The context menu appears when multiple rows are selected despite the actions only affecting the first in the list:
These issues are all corrected.
First, a QSortFilterProxyModel is inserted to take care of sorting instead of sorting the model manually. (This is the same approach taken for all other sortable lists in the GUI.) This also preserves any selections the user had made prior to sorting. Second, the selection model is changed to ExtendedSelection to allow for non-contiguous multi-select. ~~Third, the context menu is disabled when multiple rows are selected, as none of the context menu options operate on multiple selected items.~~ Update: now the context menu operates on multiple rows. It will copy the data to the clipboard separated by newlines.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviews
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | hebasto, pablomartin4btc |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
No conflicts as of last run.
First, a
QSortFilterProxyModelis inserted to take care of sorting instead of sorting the model manually.
Concept ACK on it.
Second, the selection model is changed to
ExtendedSelectionto allow for non-contiguous multi-select.
Why? There are no use cases for multi-select, aren't there?
Third, the context menu is disabled when multiple rows are selected, as none of the context menu options operate on multiple selected items.
Not sure if such a behavior is a user-friendly one.
Why? There are no use cases for multi-select, aren't there?
The "Show" and "Remove" buttons act on multiple selections. If you want to select multiple entries to delete, for instance, non-contiguous multi-select could be useful. Right now you can only delete single or contiguous items.
Not sure if such a behavior is a user-friendly one.
I was unsure whether to disable the context menu completely or show it but have everything disabled. I chose the former option, but the latter is easily possible. I think the current behavior (only acting on the first in the selection) is arguably the worst.
It's also possible to change it to work with multiple items. For instance, "Copy address" could change to "Copy addresses" upon multiple selection and copy a comma-separated list of addresses that you've selected to the clipboard.
Happy for feedback on this point!
Updated to now show the context menu with multiple selections. The data is copied to the clipboard separated by newlines.
Example:
Results in the following copied to the clipboard:
bitcoin:TB1QETGUVSJCM80365PQ2GFT8NASVYYPMCG8FEWN3W?message=Message%20with%20no%20label%20or%20amount
bitcoin:mt2nX2QuRkWkRd2ewbE1mTZ1zaKUeBDEZs?amount=10.00000000&label=Legacy%20Address&message=For%20More%20Testing
bitcoin:2N1Qu4pSpTbCud8ToT9NJWW2PiDm7V97TSq?label=P2SH%20Segwit%20Address&message=For%20testing
The individual context actions are enabled only if all selected items have data for that category. For example, if only two of the three selected items have a "label", then the "Copy labels" context menu action is disabled. The "Copy URIs" and "Copy addresses" actions will always be active.
Propose these changes: https://github.com/john-moffett/bitcoin-gui/compare/2022_12_FixReceiveCoinsMultiselect...luke-jr:bitcoin-core-gui:qt_reqs_multiselect_pr684
🤔 There hasn't been much activity lately and the CI seems to be failing.
If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.
I could take this to move it forward @john-moffett, if you are not able to, please let me know.
🤔 There hasn't been much activity lately and the CI seems to be failing.
If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.