Improve ‘Requested Payments History’ Multiselect #684

pull john-moffett wants to merge 1 commits into bitcoin-core:master from john-moffett:2022_12_FixReceiveCoinsMultiselect changing 4 files +179 −96
  1. john-moffett commented at 2:30 pm on December 5, 2022: contributor

    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.

  2. DrahtBot commented at 2:30 pm on December 5, 2022: contributor

    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.

  3. john-moffett force-pushed on Dec 5, 2022
  4. hebasto added the label UX on Dec 6, 2022
  5. hebasto commented at 5:38 pm on December 6, 2022: member

    First, a QSortFilterProxyModel is inserted to take care of sorting instead of sorting the model manually.

    Concept ACK on it.

    Second, the selection model is changed to ExtendedSelection to 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.

  6. john-moffett commented at 5:58 pm on December 6, 2022: contributor

    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!

  7. john-moffett force-pushed on Dec 15, 2022
  8. john-moffett commented at 7:45 pm on December 15, 2022: contributor

    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:

    0bitcoin:TB1QETGUVSJCM80365PQ2GFT8NASVYYPMCG8FEWN3W?message=Message%20with%20no%20label%20or%20amount
1bitcoin:mt2nX2QuRkWkRd2ewbE1mTZ1zaKUeBDEZs?amount=10.00000000&label=Legacy%20Address&message=For%20More%20Testing
2bitcoin: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.

  9. DrahtBot added the label Needs rebase on Jan 17, 2023
  10. Fix Requested Payments History Multiselect 
    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.
    a6f567590b
  11. john-moffett force-pushed on Jan 17, 2023
  12. DrahtBot removed the label Needs rebase on Jan 17, 2023
  13. luke-jr commented at 11:13 pm on June 29, 2023: member
    Would prefer the refactoring split into a different commit
  14. DrahtBot added the label CI failed on Aug 17, 2023
  15. DrahtBot removed the label CI failed on Aug 22, 2023
  16. DrahtBot added the label CI failed on Aug 31, 2023
  17. DrahtBot removed the label CI failed on Sep 4, 2023
  19. pablomartin4btc commented at 3:13 am on September 20, 2023: contributor

    Concept ACK and tACK

    image

    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: image

    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.

  20. DrahtBot added the label CI failed on Oct 21, 2023
  21. DrahtBot commented at 2:28 pm on February 5, 2024: contributor

    🤔 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.

  22. hebasto commented at 10:07 pm on February 11, 2024: member
    @john-moffett Are you still working on this PR?
  23. pablomartin4btc commented at 6:37 pm on April 12, 2024: contributor
    I could take this to move it forward @john-moffett, if you are not able to, please let me know.
  24. DrahtBot commented at 1:56 am on July 11, 2024: contributor

    🤔 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.

  25. hebasto commented at 3:56 pm on July 30, 2024: member
    Closing due to author’s inactivity. Labeled “Up for grabs”.
  26. hebasto closed this on Jul 30, 2024
  27. hebasto added the label Up for grabs on Jul 30, 2024


john-moffett DrahtBot hebasto luke-jr pablomartin4btc

Labels
Up for grabs UX CI failed

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-23 00:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me