Improve 'Requested Payments History' Multiselect #684
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
john-moffett
force-pushed
the
2022_12_FixReceiveCoinsMultiselect
branch
from
49bc102
to
5ae5293
Compare
Concept ACK on it.
Why? There are no use cases for multi-select, aren't there?
Not sure if such a behavior is a user-friendly one. |
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.
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! |
john-moffett
force-pushed
the
2022_12_FixReceiveCoinsMultiselect
branch
from
5ae5293
to
0705836
Compare
|
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: 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. |
The recent payment requests list has somewhat broken functionality. You can only select contiguous rows. Sorting the list loses the selection data. 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 also preserves selection data when sorting. Second, the selection model is changed to ExtendedSelection to allow for non-contiguous multi- select. Third, the context menu now operates on multiple selections properly. It will copy newline-separated rows of data if appropriate.
john-moffett
force-pushed
the
2022_12_FixReceiveCoinsMultiselect
branch
from
0705836
to
a6f5675
Compare
|
Would prefer the refactoring split into a different commit |
There was a problem hiding this comment.
Concept ACK and tACK
I agree with the improvement/ fix of non-contiguous multi-select behavior, it's something that a user have in most apps.
The grayed-out of the context menu items behavior is something that's already in master and it's not part of this PR, just to avoid any confusions.
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.
For this specific case, I'd add another improvement/ fix which would be a tool-tip indicating why the action is disabled:
Same for the following statement, it's currently in master, since those fields are not optional when a user creates a payment/ new address.
The "Copy URIs" and "Copy addresses" actions will always be active.
I agree with @luke-jr in the split of the code into x commits, I took a quick look at his proposal. I like more the fact that the text of the action changes to its plural form when multi-select is performed as the author of the PR propose here.
I'd also add another feature, if possible, or maybe on a follow-up. that would be to add another context menu action to "Select All", and maybe just "Copy" with a tool-tip indicating that "All fields of the selected row will be copied". But as @hebasto mentioned above, perhaps there's no real use case for what I'm proposing here.
|
🤔 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. |
|
@john-moffett Are you still working on this PR? |
|
I could take this to move it forward @john-moffett, if you are not able to, please let me know. |
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
QSortFilterProxyModelis 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 toExtendedSelectionto 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.