Add `Type` column to peers window, update peer details name/tooltip #179

pull jonatack wants to merge 3 commits into bitcoin-core:master from jonatack:add-peers-dir-and-type-columns changing 6 files +28 −18
  1. jonatack commented at 9:27 PM on January 9, 2021: contributor

    This pull:

    • adds a sortable Type column to the GUI Peers tab window
    • updates the peer details row to Direction/Type, so the Type column without a direction makes sense (the tooltip is also updated)

    Screenshot from 2021-02-06 22-53-11

  2. jonatack renamed this:
    gui: add direction and type columns to peers window
    Add direction and type columns to peers window
    on Jan 9, 2021
  3. jarolrod commented at 1:51 AM on January 10, 2021: member

    ACK 09aa053dc5a83826265aa575ee61f99f0f76287a

    I love this! I was just thinking about how this would be a good feature today, you beat me to it. Code looks good to me. Tested on macOS Big Sur 11.1 with Qt 5.15.2

    <img width="1199" alt="Screen Shot 2021-01-09 at 8 47 42 PM" src="https://user-images.githubusercontent.com/23396902/104112401-5c409e00-52bc-11eb-8e82-292c4208e535.png">

  4. jonatack commented at 9:32 AM on January 10, 2021: contributor

    Thanks! Though I should drop the relay type from inbound or check if full or block (same in -netinfo).

  5. hebasto commented at 9:41 AM on January 10, 2021: member

    Concept ACK.

  6. DrahtBot added the label Needs rebase on Jan 10, 2021
  7. jonatack force-pushed on Jan 10, 2021
  8. jonatack force-pushed on Jan 10, 2021
  9. DrahtBot removed the label Needs rebase on Jan 10, 2021
  10. jonatack force-pushed on Jan 10, 2021
  11. jonatack commented at 12:06 PM on January 10, 2021: contributor

    Rebased after merge of #163, added relay type for inbounds, and inversed the sort for the Direction column to be inbound first.

  12. in src/qt/peertablemodel.cpp:33 in a657b0c4c1 outdated 
      28 | @@ -29,6 +29,10 @@ bool NodeLessThan::operator()(const CNodeCombinedStats &left, const CNodeCombine
  29 |          return pLeft->nodeid < pRight->nodeid;
  30 |      case PeerTableModel::Address:
  31 |          return pLeft->addrName.compare(pRight->addrName) < 0;
  32 | +    case PeerTableModel::Direction:
  33 | +        return pLeft->fInbound > pRight->fInbound;
  34 | +    case PeerTableModel::ConnectionType:
  35 | +        return pLeft->m_conn_type < pRight->m_conn_type;
    jonatack commented at 2:12 PM on January 10, 2021:

    Memo to myself, if we prefer to sort alphabetically by type name rather than by enum order:

    -        return pLeft->m_conn_type < pRight->m_conn_type;
+        return GUIUtil::ConnectionTypeToShortQString(pLeft->m_conn_type, pLeft->fRelayTxes) < GUIUtil::ConnectionTypeToShortQString(pRight->m_conn_type, pRight->fRelayTxes);
  13. in src/qt/peertablemodel.h:68 in ce52716d48 outdated 
      68 | +        Direction = 2,
  69 | +        Network = 3,
  70 | +        Ping = 4,
  71 | +        Sent = 5,
  72 | +        Received = 6,
  73 | +        Subversion = 7
    hebasto commented at 5:56 PM on January 10, 2021:

    To improve maintainability maybe explicitly assign a value to the first element only? It will make future insert/remove diffs smaller.

    jonatack commented at 6:22 PM on January 10, 2021:

    done

  14. in src/qt/guiutil.cpp:783 in 13e8676204 outdated 
     776 | @@ -777,6 +777,19 @@ QString ConnectionTypeToQString(ConnectionType conn_type)
 777 |      assert(false);
 778 |  }
 779 |  
 780 | +QString ConnectionTypeToShortQString(ConnectionType conn_type, bool relay_txes)
 781 | +{
 782 | +    switch (conn_type) {
 783 | +    case ConnectionType::INBOUND: return QObject::tr(relay_txes ? "Full Relay" : "Block Relay");
    hebasto commented at 6:01 PM on January 10, 2021: 
        case ConnectionType::INBOUND: return relay_txes ? QObject::tr("Full Relay") : QObject::tr("Block Relay");
    jonatack commented at 6:23 PM on January 10, 2021:

    This seems equivalent to me, but thanks for looking! done.

    hebasto commented at 6:54 PM on January 10, 2021:

    It is not equivalent for the lupdate tool:

    When Qt's translation tool, lupdate, is used to process a set of source files, the text wrapped in tr() calls is stored in a section of the translation file that corresponds to its translation context.

  15. jonatack force-pushed on Jan 10, 2021
  16. jonatack commented at 6:25 PM on January 10, 2021: contributor

    Thank you @hebasto for your feedback! Updated with your suggestions per git diff a657b0c 9f76ba6

  17. hebasto approved
  18. hebasto commented at 8:07 PM on January 10, 2021: member

    ACK 9f76ba6597cd5d984bac0fa5c076e6b123dba22c, tested on Linux Mint 20.1 (Qt 5.12.8):

    Screenshot from 2021-01-10 22-01-41

  19. jarolrod commented at 10:07 PM on January 10, 2021: member

    re-ACK 9f76ba6597cd5d984bac0fa5c076e6b123dba22c, tested on macOS Big Sur 11.1 (Qt 5.15.2)

  20. luke-jr commented at 7:10 PM on January 13, 2021: member

    Why are Outbound/Inbound strings better than arrows?

  21. jonatack commented at 7:38 PM on January 13, 2021: contributor

    Why are Outbound/Inbound strings better than arrows?

    Good question. I proposed words instead of arrows for these reasons:

    1. The peer details area uses words. When it displays, say, "Outbound Full Relay", it seems more coherent for the column data to also display "Outbound" and "Full Relay", than up-arrow and "Full Relay"
    2. -getinfo and -netinfo both also use words for the direction rather than arrows
  22. luke-jr commented at 10:58 PM on January 13, 2021: member

    Words use a lot more space than arrows... It's not unreasonable to have details show more .. detail :p

  23. hebasto commented at 11:06 PM on January 13, 2021: member

    Why are Outbound/Inbound strings better than arrows?

    Words use a lot more space than arrows... It's not unreasonable to have details show more .. detail :p

    Actually, words simplify the GUI for new-comers, as an "arrow up" does not imply "outbound" at all.

  24. jonatack commented at 7:32 AM on January 14, 2021: contributor

    Agree. And the GUI uses words for the direction in the main info tab.

  25. jonatack commented at 8:59 AM on January 14, 2021: contributor

    (If space is an issue we could use in/out instead of inbound/outbound, as that is the wording in the GUI info tab, -getinfo, and -netinfo)

  26. hebasto commented at 9:18 AM on January 14, 2021: member

    (If space is an issue we could use in/out instead of inbound/outbound, as that is the wording in the GUI info tab, -getinfo, and -netinfo)

    I was thinking about it from the beginning, but it won't work for some translations.

  27. jarolrod commented at 1:20 AM on January 17, 2021: member

    (If space is an issue we could use in/out instead of inbound/outbound, as that is the wording in the GUI info tab, -getinfo, and -netinfo)

    I think we should stick with inbound/outbound. Even if you changed to in/out wouldn't Direction keep the tab at about the same size

  28. luke-jr commented at 5:43 PM on January 17, 2021: member

    I was thinking something more like this:

    https://github.com/jonatack/gui/compare/add-peers-dir-and-type-columns...luke-jr:tmp_gui_peers_dir_arrows

  29. jarolrod commented at 8:51 PM on January 17, 2021: member

    I was thinking something more like this:

    jonatack/gui@add-peers-dir-and-type-columns...luke-jr:tmp_gui_peers_dir_arrows

    Below is a screenshot of what your branch looks like for me, font issue with the arrows. Anyways, my vote is still for inbound/outbound lukejr-arrows

  30. luke-jr commented at 10:24 PM on January 17, 2021: member

    I suppose we could also do icons with an arrow and tiny in/out text under it

  31. luke-jr commented at 5:10 PM on January 18, 2021: member

    There, how does this look? https://github.com/jonatack/gui/compare/add-peers-dir-and-type-columns...luke-jr:tmp_gui_peers_dir_arrows

  32. jarolrod commented at 5:48 PM on January 18, 2021: member

    Screenshot is taken from Luke-jr's branch

    I guess we should get some designer opinion on this: @Bosch-0 what do you think about this icon vs inbound/outbound text.

    I think this idea has a lot of potential, but as this iteration I would still vote for inbound/outbound text. I think it's more informative and fits in better with translations. Also I think Direction and Type should remain adjacent. <img width="740" alt="Screen Shot 2021-01-18 at 12 40 44 PM" src="https://user-images.githubusercontent.com/23396902/104948049-a562b300-598a-11eb-9610-19455e69a7a1.png">

  33. jonatack commented at 6:05 PM on January 18, 2021: contributor

    Thanks @luke-jr for the proposals and to @jarolrod for the screenshot renderings (a thought, no need to hide addresses if you test on signet or testnet, and signet starts up the fastest of all the chains.)

    • I agree the direction column should be next to the type and network (but no strong opinion on placement of the address column; maybe after the network column would be better)
    • The direction header could be shortened to "Dir", or "<->" like in -netinfo, or use icons/symbols to represent <->
    • The column header and column values are trivial to change and I suggest leaving that for a follow-up (not by me ;)

    This has 2 tested ACKs; @luke-jr if you could ACK as well this could go in and open the door for your proposed follow-up.

  34. luke-jr commented at 6:45 PM on January 18, 2021: member

    I don't like the text form... As-is, IIRC this creates horizontal scrolling because it doesn't all fit in the window.

  35. jonatack commented at 7:08 PM on January 18, 2021: contributor

    I think it depends on your window size state which since #165 is now stored and recalled, so (I believe) this is not an issue after the first resize to your preferred state.

  36. hebasto commented at 7:17 PM on January 18, 2021: member

    I suppose we could also do icons with an arrow and tiny in/out text under it

    Any text within icons causes translation issues, no?

  37. RandyMcMillan commented at 9:43 PM on January 18, 2021: contributor

    @jonatack @hebasto

    This worked for me...

    https://github.com/bitcoin-core/gui/pull/135/files#diff-e721b02fefce2a90ae95da0841adfdcc610ffccf93236ef236f81c1baead8e01R163

    Most of the issues you may run into - I already figured out...

    #135

  38. jonatack commented at 9:51 PM on January 18, 2021: contributor

    This current PR with 2 ACKs is my preferred version among the various proposals I have seen.

  39. jonatack commented at 9:54 PM on January 18, 2021: contributor

    I propose that further refinements be done in a follow-up (not by me ;).

  40. RandyMcMillan commented at 10:08 PM on January 18, 2021: contributor

    Something I didn't get to yet - was to separate the uacomment and give it its own column. This could be very useful for some other ideas Ive been thinking about.

  41. luke-jr commented at 11:33 PM on January 18, 2021: member

    https://github.com/jonatack/gui/compare/add-peers-dir-and-type-columns...luke-jr:tmp_gui_peers_dir_arrows is now translation-friendly...

    I'm still weak concept nack on inbound/outbound as strings in the table. I think it's worse than the status quo, so would rather this not be merged as-is.

  42. jonatack marked this as a draft on Jan 29, 2021
  43. jonatack commented at 3:49 PM on January 29, 2021: contributor

    Moving this back to Draft status until #201 is resolved.

  44. gui: allow ConnectionTypeToQString to prepend direction optionally 6fc72bd6f0
  45. gui: add "Type" column to Peers main window 151888383a
  46. gui: update to "Direction/Type" peer details name/tooltip be4cf4832f
  47. jonatack force-pushed on Feb 6, 2021
  48. jonatack marked this as ready for review on Feb 6, 2021
  49. jonatack commented at 10:15 PM on February 6, 2021: contributor

    Ready for review again. Three changes from the previous version:

    • It no longer replaces the arrows with a Direction column, which had 2 ACKs but also a light NACK and which can be decided later in another pull
    • For now, inbound peers do not display anything in the Type column, following the logic of merged #203
    • In the details area, the Connection Type field is renamed to Direction/Type, so the Type column without a direction makes sense. Its tooltip is also updated.

    I hope the translations work correctly in this latest push. @hebasto, @jarolrod, @luke-jr, @RandyMcMillan, would you mind having another look?

  50. jonatack commented at 10:17 PM on February 6, 2021: contributor

    Updated the PR description and screenshot with this one:

    Screenshot from 2021-02-06 22-53-11

  51. jonatack renamed this:
    Add direction and type columns to peers window
    Add `Type` column to peers window, update peer details name/tooltip
    on Feb 6, 2021
  52. jarolrod commented at 2:16 PM on February 17, 2021: member

    ACK be4cf4832f117ab5f97e1367f7bdcb5361de0dae

    Tested with Spanish translations. The text added in the tooltip and text associated with the Type column will need translations.

    <img width="1215" alt="pr179-spanishtest" src="https://user-images.githubusercontent.com/23396902/108216677-bbd26a80-7100-11eb-8309-b0152a143e0b.png">

  53. leonardojobim approved
  54. leonardojobim commented at 6:44 AM on February 21, 2021: none

    Tested ACK https://github.com/bitcoin-core/gui/commit/be4cf4832f117ab5f97e1367f7bdcb5361de0dae on Ubuntu 20.04 on VMWare.

    image

  55. RandyMcMillan commented at 12:27 AM on February 22, 2021: contributor

    Removing "Peer" from the "Peer Id" column would allow for more prudent use of space. Otherwise it should be translatable. A simple "ID" or "id" would be better. I think all caps "ID" is better imo.

    Maybe Type and Network should be left justified?

    NOTE: #202

    PR #202 is going to allow for more info/columns to be added - and more info displayed "at a glance". As more columns are added - the columns can be adjusted for a tighter fit and/or column stretching in a resize event.

    ACK 6fc72bd6f030c8b49e8b9f68188413766a239d73

    macOS 10.14.6 (18G103)

    Screen Shot 2021-02-21 at 7 22 01 PM

  56. MarcoFalke merged this on Feb 22, 2021
  57. MarcoFalke closed this on Feb 22, 2021
  58. jonatack deleted the branch on Feb 22, 2021
  59. sidhujag referenced this in commit ed9729095d on Feb 22, 2021
  60. gwillen referenced this in commit fe1e52f667 on Jun 28, 2022
  61. bitcoin-core locked this on Aug 16, 2022
Contributors
jonatackjarolrodhebastoluke-jrRandyMcMillanleonardojobim
Linked (view graph)
#289 Add "Direction" column to peers tab#372 Remove "Type" column from peers window
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: 2026-04-26 11:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me