Add peertablesortproxy module #18

pull hebasto wants to merge 4 commits into bitcoin-core:master from hebasto:200701-peer changing 10 files +93 −155
  1. hebasto commented at 5:59 pm on July 1, 2020: member

    The “Peers” table in the “Node” window does not hold multiple selection after sorting.

    This PR introduces a QSortFilterProxyModel subclass, that is a standard Qt practice for such cases.

    Now the sorting code is encapsulated into the dedicated Qt class, and we do not need to maintain it.

    Fixes #283 (additionally).

    On master (7ae86b3c6845873ca96650fc69beb4ae5285c801):

    • rows are sorted by “Ping”, and a selection is made Screenshot from 2020-11-28 22-53-11

    • rows are sorted by “NodeId”, and the previous selection is lost Screenshot from 2020-11-28 22-53-21

    With this PR:

    • rows are sorted by “Ping”, and a selection is made Screenshot from 2020-11-28 22-39-41

    • rows are sorted by “NodeId”, and the row are still selected Screenshot from 2020-11-28 22-39-53

  2. MarcoFalke commented at 8:31 pm on July 1, 2020: contributor
    Is this a refactor or feature or bugfix?
  3. hebasto commented at 4:58 am on July 2, 2020: member

    Is this a refactor or feature or bugfix?

    A bugfix, I think.

    The correct handling of multiple selections during sorting by a column could be observed in the transaction table.

  4. hebasto closed this on Jul 4, 2020
  5. hebasto reopened this on Jul 4, 2020
  6. DrahtBot added the label Needs rebase on Aug 3, 2020
  7. jonasschnelli commented at 9:20 am on October 23, 2020: contributor
    No sure if this is worth the effort. No strong opinion though. Please rebase If this should be dragged along.
  8. hebasto force-pushed on Oct 23, 2020
  9. hebasto commented at 11:07 am on October 23, 2020: member

    Please rebase If this should be dragged along.

    Rebased 0a03c82cefab82e273c5e76a3a3b93ae6a75df54 -> b119ae0c82c6f36afdf3eefd89b687ab78a9e34a (pr18.01 -> pr18.02).

  10. DrahtBot removed the label Needs rebase on Oct 23, 2020
  11. laanwj referenced this in commit 924a4ff7eb on Oct 29, 2020
  12. jarolrod commented at 6:05 am on November 12, 2020: member

    Tested ACK b119ae0 on Arch Linux and macOS 10.15.7. Code looks good and PR does what it says it will do.

    The behavior this PR introduces of showing nothing is a better behavior than the default functionality. I think an even better behavior would be to, as you select a group of peers, show the last peer that you selected in that selection group.

  13. hebasto commented at 8:10 am on November 12, 2020: member

    @jarolrod

    The behavior this PR introduces of showing nothing is a better behavior than the default functionality.

    Do you mean #13?

  14. jarolrod commented at 5:06 pm on November 12, 2020: member

    @jarolrod

    The behavior this PR introduces of showing nothing is a better behavior than the default functionality.

    Do you mean #13?

    Oh yes, sorry. Overlooked that this builds upon that PR.

  15. DrahtBot added the label Needs rebase on Nov 20, 2020
  16. hebasto force-pushed on Nov 28, 2020
  17. hebasto commented at 8:45 pm on November 28, 2020: member
    Rebased b119ae0c82c6f36afdf3eefd89b687ab78a9e34a -> ab070cf39a6756723a4efc17e6e98de9d47425d5 (pr18.02 -> pr18.03) due to the conflict #13.
  18. DrahtBot removed the label Needs rebase on Nov 28, 2020
  19. hebasto commented at 8:55 pm on November 28, 2020: member
    The OP updated with screenshots.
  20. hebasto force-pushed on Dec 22, 2020
  21. hebasto commented at 11:18 am on December 22, 2020: member
    Rebased ab070cf39a6756723a4efc17e6e98de9d47425d5 -> 5745711a8b2641fe727b98fa93425f4b0f4e4beb (pr18.03 -> pr18.04) on top of the recent changes in p2p logic and CI.
  22. hebasto commented at 11:32 am on December 22, 2020: member

    ~Converting to draft as a bug was discovered while testing in the light of #158.~

    That bug is not related to changes in this PR.

  23. hebasto marked this as a draft on Dec 22, 2020
  24. hebasto force-pushed on Dec 23, 2020
  25. hebasto marked this as ready for review on Dec 23, 2020
  26. hebasto marked this as a draft on Dec 24, 2020
  27. hebasto force-pushed on Dec 24, 2020
  28. hebasto marked this as ready for review on Dec 24, 2020
  29. hebasto commented at 12:52 pm on December 24, 2020: member

    Updated 6f1ccc74b260413ad6750a693e8e0bdc36881300 -> 6c273034da293f6d4ac691959c5c6435f67200be (pr18.05 -> pr18.06):

    • fixed a bug with the detailed widget update
    • rebased on top of #161

    Please review #161 first :)

  30. hebasto commented at 12:56 pm on December 24, 2020: member

    @jonasschnelli

    No sure if this is worth the effort.

    Benefits:

    • less lines of code (+109 / -175)
    • using a proper abstraction layer for sorting makes the code more readable and maintainable
    • using a proper abstraction layer fixes a buggy behavior when sorting is applied while multiple rows are selected

    Also this PR is required for #164 bugfix.

  31. DrahtBot added the label Needs rebase on Dec 28, 2020
  32. hebasto force-pushed on Dec 30, 2020
  33. hebasto commented at 7:49 pm on December 30, 2020: member
    Rebased 6c273034da293f6d4ac691959c5c6435f67200be -> 9bc2004db26a3a956256f8fd9b05e7a3e9e21aca (pr18.06 -> pr18.07) due to the conflict with #162.
  34. DrahtBot removed the label Needs rebase on Dec 30, 2020
  35. jonasschnelli referenced this in commit 616eace02a on Jan 11, 2021
  36. hebasto force-pushed on Jan 11, 2021
  37. hebasto commented at 9:48 am on January 11, 2021: member
    Rebased 9bc2004db26a3a956256f8fd9b05e7a3e9e21aca -> fb6901787d7eafec5bc17d5f0c271935ac1e1c17 (pr18.07 -> pr18.08) due to the conflict with #161.
  38. jarolrod commented at 1:25 am on January 19, 2021: member

    re-ack fb6901787d7eafec5bc17d5f0c271935ac1e1c17, tested on macOS 11.1 with Qt 5.15.2 @jonasschnelli in regards to comment: If I wanted to see how certain peers rank based on different metrics, it would be nice to have the peers I have in interest remain selected as I change the sorting. So, this PR does add value.

    Below are screenshots showing the behavior between master and this PR

    Master

    Master: Select three peers with default sorting (by Peer ID)

    Master: Change to sorting by Ping -> peers of interest are no longer selected

    PR

    PR: Select three peers with default sorting (by Peer ID)

    PR: Change to sorting by Ping -> Peers remain selected

  39. DrahtBot added the label Needs rebase on Jan 27, 2021
  40. hebasto force-pushed on Jan 27, 2021
  41. hebasto commented at 9:08 pm on January 27, 2021: member
    Rebased fb6901787d7eafec5bc17d5f0c271935ac1e1c17 -> 4a032f64e45c7d78a378057e878acc0b101467a6 (pr18.08 -> pr18.09) due to the conflict with #180.
  42. DrahtBot removed the label Needs rebase on Jan 27, 2021
  43. jarolrod commented at 6:16 pm on January 28, 2021: member
    re-ack 4a032f64e45c7d78a378057e878acc0b101467a6
  44. DrahtBot added the label Needs rebase on Feb 22, 2021
  45. hebasto force-pushed on Feb 22, 2021
  46. hebasto commented at 8:02 am on February 22, 2021: member
    Rebased 4a032f64e45c7d78a378057e878acc0b101467a6 -> 8d4a682791bedcc9cf471e9d77e77ec46d0bc314 (pr18.09 -> pr18.10) due to the conflict with #179.
  47. DrahtBot removed the label Needs rebase on Feb 22, 2021
  48. jarolrod commented at 4:28 pm on February 22, 2021: member
    re-ACK 8d4a682791bedcc9cf471e9d77e77ec46d0bc314, tested on macOS 11.1 Qt 5.15.2 after rebase
  49. DrahtBot added the label Needs rebase on Mar 4, 2021
  50. laanwj commented at 11:04 am on March 5, 2021: member
    Concept ACK, haven’t reviewed the code in detail but conceptually this is the proper Qt way of doing this.
  51. hebasto force-pushed on Mar 6, 2021
  52. hebasto commented at 6:36 pm on March 6, 2021: member
    Rebased 8d4a682791bedcc9cf471e9d77e77ec46d0bc314 -> 56e5f5aefce6df0e54fbbd556d7bcdca3f167d12 (pr18.10 -> pr18.11) due to the conflict with https://github.com/bitcoin/bitcoin/pull/21015.
  53. DrahtBot removed the label Needs rebase on Mar 6, 2021
  54. DrahtBot added the label Needs rebase on Mar 7, 2021
  55. qt: Add peertablesortproxy module df2d165ba9
  56. qt: Use PeerTableSortProxy for sorting peer table 778a64af20
  57. qt, refactor: Drop no longer used PeerTableModel::sort function 9a9f180df0
  58. qt, refactor: Drop no longer used PeerTableModel::getRowByNodeId func 5a4a15d2b4
  59. hebasto force-pushed on Mar 7, 2021
  60. hebasto commented at 3:07 pm on March 7, 2021: member
    Rebased 56e5f5aefce6df0e54fbbd556d7bcdca3f167d12 -> 5a4a15d2b4456272fd8aa080195f40a09576ae01 (pr18.11 -> pr18.12) due to the conflict with #166.
  61. DrahtBot removed the label Needs rebase on Mar 7, 2021
  62. jarolrod commented at 6:48 pm on March 7, 2021: member
    re-ACK 5a4a15d2b4456272fd8aa080195f40a09576ae01, tested on macOS 11.2 Qt 5.15.2 after rebase
  63. hebasto added the label Bug on Mar 22, 2021
  64. rebroad commented at 12:09 pm on March 29, 2021: contributor
    seems like a fair bit of code for functionality which I’d expect by default…. nice work though.
  65. in src/qt/clientmodel.cpp:192 in 778a64af20 outdated 
    188@@ -184,6 +189,11 @@ PeerTableModel *ClientModel::getPeerTableModel()
189     return peerTableModel;
190 }
191 
192+PeerTableSortProxy* ClientModel::peerTableSortProxy()
    promag commented at 6:06 pm on April 19, 2021:

    778a64af209e4fa692a3aca8376ba1bd5e1af881

    I don’t think this belongs here, should be a member of RPCConsole - usually, sort/filter models are instanced for each view unless you want to share the same sorting/filter result in multiple views (I know it’s only used in RPCConsole).

    hebasto commented at 8:59 am on April 28, 2021:

    From the ##bitcoin-core-gui IRC channel:

    <promag> #18 seems ready <promag> #18 (review) can be addressed later

  66. promag commented at 6:11 pm on April 19, 2021: contributor

    Concept ACK, I think this is the best approach to keep a model sorted and eventually filtered.

    Should setDynamicSortFilter(true)? nevermind, it’s true by default.

  67. promag commented at 9:50 pm on April 19, 2021: contributor

    Tested ACK 5a4a15d2b4456272fd8aa080195f40a09576ae01.

    #283 is fixed, selected cell is copied with copy shortcut.

  68. MarcoFalke referenced this in commit bce09da122 on Apr 28, 2021
  69. hebasto commented at 9:01 am on April 28, 2021: member

    @laanwj

    Concept ACK, haven’t reviewed the code in detail but conceptually this is the proper Qt way of doing this.

    Do you mind to have another look at this PR to promote your “Concept ACK”?

  70. hebasto merged this on Apr 28, 2021
  71. hebasto closed this on Apr 28, 2021
  72. hebasto deleted the branch on Apr 28, 2021
  73. sidhujag referenced this in commit ee3a634b39 on Apr 29, 2021
  74. MarcoFalke referenced this in commit eb9a1fe037 on May 7, 2021
  75. MarcoFalke referenced this in commit c857148636 on May 15, 2021
  76. fanquake referenced this in commit f9b522e50d on Feb 23, 2022
  77. gwillen referenced this in commit 4b9e6df7a5 on Jun 1, 2022
  78. bitcoin-core locked this on Aug 16, 2022


hebasto MarcoFalke jonasschnelli jarolrod laanwj rebroad promag

Labels
Bug

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 02:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me