Peer details: replace Direction with Connection Type #163

pull jonatack wants to merge 4 commits into bitcoin-core:master from jonatack:display-peer-conn-types changing 5 files +29 −8
  1. jonatack commented at 12:27 PM on December 24, 2020: contributor

    Screenshot from 2021-01-09 11-23-17

    Closes #159.

  2. MarcoFalke commented at 12:34 PM on December 24, 2020: contributor

    Nice. Concept ACK

  3. in src/qt/guiutil.cpp:763 in c093dedbc1 outdated 
     758 | +    if (conn_type == "feeler") return "Outbound feeler";
 759 | +    if (conn_type == "addr-fetch") return "Outbound address fetch";
 760 | +    // Inbound peer connections
 761 | +    if (conn_type == "inbound") return "Inbound full relay";
 762 | +    if (conn_type == "inbound-block-relay") return "Inbound block relay";
 763 | +    return "";
    MarcoFalke commented at 12:37 PM on December 24, 2020:

    nit: Would be nice if this default fallback was replaced with an assert(false) and the above ifs with an enum-switch-case

    jonatack commented at 12:38 PM on December 24, 2020:

    Agree, but ISTM that only works for integer types.

    MarcoFalke commented at 12:40 PM on December 24, 2020:

    ConnectionType is an enum class (implementation defined integer)

    jonatack commented at 3:23 PM on December 24, 2020:

    Right, I wanted to do that but the ConnectionType enum wasn't exposed in CNodeStats. Ok, done.

  4. jonatack force-pushed on Dec 24, 2020
  5. jonatack force-pushed on Dec 24, 2020
  6. hebasto commented at 10:58 AM on December 25, 2020: member

    Concept ACK.

  7. in src/qt/guiutil.h:228 in bf8135db2a outdated 
     224 | @@ -224,6 +225,9 @@ namespace GUIUtil
 225 |      /* Convert OS specific boost path to QString through UTF-8 */
 226 |      QString boostPathToQString(const fs::path &path);
 227 |  
 228 | +    /* Convert ConnectionType enum to QString */
    hebasto commented at 1:29 PM on December 25, 2020:

    571b0469fa952be98874d1afd772a7fa708dd83f

    This comment is not Doxygen-compatible.

    jonatack commented at 5:10 PM on December 25, 2020:

    thanks! fixed

  8. in src/qt/guiutil.cpp:779 in bf8135db2a outdated 
     759 | +    case ConnectionType::FEELER: return "Outbound feeler";
 760 | +    case ConnectionType::ADDR_FETCH: return "Outbound address fetch";
 761 | +    } // no default case, so the compiler can warn about missing cases
 762 | +    assert(false);
 763 | +}
 764 | +
    hebasto commented at 1:33 PM on December 25, 2020:

    571b0469fa952be98874d1afd772a7fa708dd83f

    The returned values should be translated.

    UX note: I have a personal preference do not capitalize the first letter of returned values. Feel free to ignore it.

    jonatack commented at 2:20 PM on December 25, 2020:

    I also prefer all lowercase (e.g. rpc/cli) but the GUI UI uses capitalization (I hesitated to use titlecase here).

    hebasto commented at 2:44 PM on December 25, 2020:

    I perceive it like "Connection type: Outbound full relay". That's why I prefer the lowercased variant.

    If the capitalized variant will be required in some other places in the GUI, it is easy to provide it with a generic function.

    I'm not a design expert, then I'm fine with any variant.

    jonatack commented at 5:11 PM on December 25, 2020:

    Thanks--translated and changed to titlecase for coherence with the general GUI UI.

  9. in src/qt/guiutil.cpp:755 in bf8135db2a outdated 
     748 | @@ -749,6 +749,19 @@ QString boostPathToQString(const fs::path &path)
 749 |      return QString::fromStdString(path.string());
 750 |  }
 751 |  
 752 | +QString ConnectionTypeToQString(ConnectionType conn_type)
 753 | +{
 754 | +    switch (conn_type) {
 755 | +    case ConnectionType::INBOUND: return "Inbound full relay";
    hebasto commented at 1:35 PM on December 25, 2020:

    571b0469fa952be98874d1afd772a7fa708dd83f

    It seems not always true :)

    IIUC, you recent changes in bitcoin-cli distinguish "block relay" for inbound connections as well.

    jonatack commented at 2:37 PM on December 25, 2020:

    IIUC, you recent changes in bitcoin-cli distinguish "block relay" for inbound connections as well.

    Yes, I'm anticipating inbound full relay and inbound block relay connection types (https://github.com/bitcoin/bitcoin/pull/20726 and https://github.com/bitcoin/bitcoin/pull/20729) but I guess it's better to revert to just inbound for now and update it later.

    jonatack commented at 5:11 PM on December 25, 2020:

    Now "Inbound"

  10. in src/qt/forms/debugwindow.ui:1083 in bf8135db2a outdated 
    1076 | @@ -1077,14 +1077,17 @@
1077 |                  </widget>
1078 |                 </item>
1079 |                 <item row="1" column="0">
1080 | -                <widget class="QLabel" name="label_23">
1081 | +                <widget class="QLabel" name="peerConnectionTypeLabel">
1082 | +                 <property name="toolTip">
1083 | +                  <string>The connection type of this peer.</string>
1084 | +                 </property>
    hebasto commented at 1:43 PM on December 25, 2020:

    bf8135db2a658aed3129ec82e237f8030a8ec7dd

    I've noted your comment, but here the tooltip provides no additional info. I don't think that adding such a tooltip for the sake itself is a good approach for the GUI:

    jonatack commented at 2:31 PM on December 25, 2020:

    True. Will improve it with more useful info. Perhaps it should link to more information about the connection types (like we have in the codebase), maybe in a markdown file in /doc.

    jonatack commented at 2:39 PM on December 25, 2020:

    Maybe I should add more useful info, like in the RPC help (getpeerinfo)

    hebasto commented at 2:50 PM on December 25, 2020:

    Anyway, please keep it useful and succinct, or else do not add it at all :)

    jonatack commented at 5:12 PM on December 25, 2020:

    Added info similar to the getpeerinfo help, see updated screenshot in the PR description.

  11. hebasto changes_requested
  12. hebasto commented at 1:43 PM on December 25, 2020: member

    Concept ACK.

    I have reviewed the code (bf8135db2a658aed3129ec82e237f8030a8ec7dd), and tested it.

  13. jonatack force-pushed on Dec 25, 2020
  14. jonatack commented at 5:14 PM on December 25, 2020: contributor

    Thanks for the excellent feedback @hebasto! Updated.

  15. hebasto commented at 9:29 PM on December 25, 2020: member

    I have reviewed the code (8e7f82a6a84c7b2553f88767dd356f480d1ae4b7), and tested it.

    The following patch:

    --- a/src/qt/forms/debugwindow.ui
+++ b/src/qt/forms/debugwindow.ui
@@ -1079,7 +1079,7 @@
                <item row="1" column="0">
                 <widget class="QLabel" name="peerConnectionTypeLabel">
                  <property name="toolTip">
-                  <string>The type of peer connection: Outbound Full Relay (default), Outbound Block Relay (does not relay transactions or addresses), Inbound (initiated by peer), Outbound Manual (added using RPC addnode or -addnode/-connect config options), Outbound Feeler (short-lived, for testing addresses), or Outbound Address Fetch (short-lived, for soliciting addresses).</string>
+                  <string>The type of peer connection: Outbound Full Relay (default), Outbound Block Relay (does not relay transactions or addresses), Inbound (initiated by peer), Outbound Manual (added using RPC %1 or %2/%3 config options), Outbound Feeler (short-lived, for testing addresses), or Outbound Address Fetch (short-lived, for soliciting addresses).</string>
                  </property>
                  <property name="text">
                   <string>Connection Type</string>
--- a/src/qt/rpcconsole.cpp
+++ b/src/qt/rpcconsole.cpp
@@ -496,6 +496,8 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty
     clear();
 
     GUIUtil::handleCloseWindowShortcut(this);
+
+    ui->peerConnectionTypeLabel->setToolTip(ui->peerConnectionTypeLabel->toolTip().arg("addnode").arg(QString(nonbreaking_hyphen) + "addnode").arg(QString(nonbreaking_hyphen) + "connect"));
 }
 
 RPCConsole::~RPCConsole()
    • removes RPC and option names from the translatable string
    • prevents effects like this DeepinScreenshot_select-area_20201225232751
  16. jonatack commented at 9:46 PM on December 25, 2020: contributor

    Thanks @hebasto, tested and added your suggestion, updated the screenshot in the PR description.

  17. hebasto approved
  18. hebasto commented at 9:55 PM on December 25, 2020: member

    ACK 6cd242c50f4a8d3f3c509937d80962fe58096ead, tested on Linux Mint 20 (x86_64) + Qt 5.12.8.

    Looks great: Screenshot from 2020-12-25 23-52-27

  19. in src/net.h:729 in a60f67b2fb outdated 
     724 | @@ -725,6 +725,7 @@ class CNodeStats
 725 |      // Name of the network the peer connected through
 726 |      std::string m_network;
 727 |      uint32_t m_mapped_as;
 728 | +    ConnectionType m_conn_type;
    promag commented at 6:55 PM on December 26, 2020:

    a60f67b2fb0eede205db8cce11ad0e6f42dbc90e

    This was suggested here https://github.com/bitcoin/bitcoin/pull/19725#discussion_r473361238 and now makes m_conn_type_string redundant, doesn't makes sense to keep both TBH. Since m_conn_type_string is only used in getpeerinfo then drop it and refactor ConnectionTypeAsString to take conn_type instead?

    MarcoFalke commented at 8:45 PM on December 26, 2020:

    Concept ACK, but RPC changes (refactoring) can only be made in the main repo

    jonatack commented at 9:19 PM on December 26, 2020:

    Done, I think. Should I close this PR and re-open in the main repo?

    hebasto commented at 9:24 PM on December 26, 2020:

    Maybe just open a pr in the main repo with the first and last commits?

    promag commented at 9:26 PM on December 26, 2020:

    IMO this isn't a RPC change, it's just a member refactor in a net struct, and the refactor is motivated by this GUI change. Otherwise this can go forward in 2 ways I think, either leave this as it was, and then in main repo drop the redundant member, or 1st refactor and make this dependant of that PR.

    jonatack commented at 10:29 PM on December 26, 2020:

    Opened the same pull in https://github.com/bitcoin/bitcoin/pull/20778 and will let the maintainers decide, closing this one for now to not maintain multiple pulls.

    promag commented at 12:24 PM on December 28, 2020:

    Just noting that redundancy is not a blocker, but could be avoided.

  20. jonatack force-pushed on Dec 26, 2020
  21. jonatack closed this on Dec 28, 2020
  22. jonatack commented at 9:45 PM on December 28, 2020: contributor

    It was suggested to re-open here.

  23. jonatack reopened this on Dec 28, 2020
  24. MarcoFalke commented at 9:50 PM on December 28, 2020: contributor

    Concept re-ACK. Thanks for re-opening.

  25. DrahtBot added the label Needs rebase on Dec 28, 2020
  26. jonatack force-pushed on Dec 28, 2020
  27. jonatack commented at 11:43 PM on December 28, 2020: contributor

    Rebased.

  28. DrahtBot removed the label Needs rebase on Dec 29, 2020
  29. hebasto approved
  30. hebasto commented at 4:57 PM on December 30, 2020: member

    re-ACK aef969363a7685fc805833a3ec6863c27500d02f, since my previous review only rebased and the "p2p, rpc, refactor: remove CNodeStats::m_conn_type_string" commit added.

  31. DrahtBot added the label Needs rebase on Jan 2, 2021
  32. jonatack force-pushed on Jan 2, 2021
  33. jonatack commented at 9:14 AM on January 2, 2021: contributor

    Rebased git range-diff ae8f797 aef9693 7098509

  34. DrahtBot removed the label Needs rebase on Jan 2, 2021
  35. in src/rpc/net.cpp:252 in 70985097c0 outdated 
     248 | @@ -249,7 +249,7 @@ static RPCHelpMan getpeerinfo()
 249 |                  recvPerMsgCmd.pushKV(i.first, i.second);
 250 |          }
 251 |          obj.pushKV("bytesrecv_per_msg", recvPerMsgCmd);
 252 | -        obj.pushKV("connection_type", stats.m_conn_type_string);
 253 | +        obj.pushKV("connection_type", ConnectionTypeAsString(stats.m_conn_type));
    hebasto commented at 9:35 AM on January 2, 2021:

    70985097c00827de94a7922620b63e1ee10dd53d

    Mind fixing https://github.com/bitcoin/bitcoin/pull/19725/commits/395acfa83a5436790c1a722a5609ac9d48df235f (https://github.com/bitcoin/bitcoin/pull/19725) by aligning the order of the "connection_type" field both in RPC help and in RPC output?

    jonatack commented at 9:39 PM on January 2, 2021:

    thanks; done

  36. hebasto approved
  37. hebasto commented at 9:35 AM on January 2, 2021: member

    re-ACK 70985097c00827de94a7922620b63e1ee10dd53d, only rebased since my previous review.

  38. jonatack force-pushed on Jan 2, 2021
  39. jonatack commented at 9:43 PM on January 2, 2021: contributor

    Thanks @hebasto, updated the last commit to move the getpeerinfo connection_type help to the correct place per review feedback in #163 (review) and updated the commit message.

  40. hebasto approved
  41. hebasto commented at 8:30 PM on January 3, 2021: member

    re-ACK be8294ff428da7d13043b3f9a27d7ace1c2dadf4

  42. DrahtBot added the label Needs rebase on Jan 7, 2021
  43. laanwj commented at 3:05 PM on January 7, 2021: member

    Concept ACK

  44. gui: create GUIUtil::ConnectionTypeToQString utility function 7e2beab2d2
  45. jonatack force-pushed on Jan 8, 2021
  46. jonatack commented at 2:39 PM on January 8, 2021: contributor

    Rebased after the merge of https://github.com/bitcoin/bitcoin/pull/20786.

  47. hebasto approved
  48. hebasto commented at 3:20 PM on January 8, 2021: member

    re-ACK 20fa97560b2d27f94f556a570f3556356877b79f, only rebased since my previous review due to the conflict with https://github.com/bitcoin/bitcoin/pull/20786, verified with

    $ git range-diff master be8294ff428da7d13043b3f9a27d7ace1c2dadf4 20fa97560b2d27f94f556a570f3556356877b79f
  49. DrahtBot removed the label Needs rebase on Jan 8, 2021
  50. jarolrod commented at 4:30 PM on January 8, 2021: member

    ACK 20fa97560b2d27f94f556a570f3556356877b79f

    Tested on macOS Big Sur 11.1 with qt 5.15.2. Looks great! <img width="497" alt="Screen Shot 2021-01-08 at 11 01 46 AM" src="https://user-images.githubusercontent.com/23396902/104039378-4c359b00-51a4-11eb-827e-8b5f75dc2f20.png">

    Small NIT: There's a lot of text in the tooltip. It would be nice if all the possible values were put into a list instead of separated by commas. It would look something like this, which to me would be an improvement.

    See: Supported HTML Subset,

  51. jonatack commented at 11:07 PM on January 8, 2021: contributor

    Better? If yes, bold or no?

    Also, any opinions on Inbound being first in the list vs where it is currently.

    Screenshot from 2021-01-09 00-04-04 Screenshot from 2021-01-08 23-57-49

  52. jarolrod commented at 11:49 PM on January 8, 2021: member

    @jonatack the list looks great! I think not-bold is better. I think that inbound can stay where it is. If i may, I would suggest one more thing:

    Write the descriptions like this: Outbound Block Relay: does not relay transactions or addresses

    instead of: Outbound Block Relay (does not relay transactions or addresses)

  53. jonatack commented at 12:13 AM on January 9, 2021: contributor

    @jarolrod test with inbound first and your colon suggestion (agree, it's more readable)

    Screenshot from 2021-01-09 01-10-03

  54. jarolrod commented at 12:18 AM on January 9, 2021: member

    @jonatack looks awesome 👍

  55. jonatack force-pushed on Jan 9, 2021
  56. jonatack commented at 10:34 AM on January 9, 2021: contributor

    Thanks @jarolrod. Updated in latest push, if you'd like to review/test. @hebasto, can you verify the changed tooltip code?

  57. hebasto commented at 12:09 PM on January 9, 2021: member

    @hebasto, can you verify the changed tooltip code?

    The used HTML code is valid.

    But I'd prefer to keep the tooltip content in the form *.ui file. It makes usage of design tools (e.g., Qt Designer) much easier. In *.cpp file should be text substitution code only.

  58. jonatack force-pushed on Jan 9, 2021
  59. jonatack commented at 12:47 PM on January 9, 2021: contributor

    @hebasto thanks--done.

  60. gui: replace Direction with Connection Type in peer details 2c19ba2e1d
  61. gui: improve connection type tooltip
    - remove RPC and option names from the translatable string
- use non-breaking hyphens
    c95fe6e38f
  62. rpc: move getpeerinfo connection_type help to correct place
    per review feedback
    06ba9b3008
  63. jonatack force-pushed on Jan 9, 2021
  64. hebasto approved
  65. hebasto commented at 12:59 PM on January 9, 2021: member

    re-ACK 06ba9b300866f33e21512af9d7d2891ee1501bf4, the tooltip content is organized as unordered list.

  66. jarolrod commented at 3:36 PM on January 9, 2021: member

    re-ACK 06ba9b300866f33e21512af9d7d2891ee1501bf4

  67. MarcoFalke merged this on Jan 10, 2021
  68. MarcoFalke closed this on Jan 10, 2021
  69. in src/qt/forms/debugwindow.ui:1082 in 06ba9b3008 
    1076 | @@ -1077,14 +1077,17 @@
1077 |                  </widget>
1078 |                 </item>
1079 |                 <item row="1" column="0">
1080 | -                <widget class="QLabel" name="label_23">
1081 | +                <widget class="QLabel" name="peerConnectionTypeLabel">
1082 | +                 <property name="toolTip">
1083 | +                   <string>The type of peer connection:&lt;ul&gt;&lt;li&gt;Inbound: initiated by peer&lt;/li&gt;&lt;li&gt;Outbound Full Relay: default&lt;/li&gt;&lt;li&gt;Outbound Block Relay: does not relay transactions or addresses&lt;/li&gt;&lt;li&gt;Outbound Manual: added using RPC %1 or %2/%3 configuration options&lt;/li&gt;&lt;li&gt;Outbound Feeler: short-lived, for testing addresses&lt;/li&gt;&lt;li&gt;Outbound Address Fetch: short-lived, for soliciting addresses&lt;/li&gt;&lt;/ul&gt;</string>
    MarcoFalke commented at 9:45 AM on January 10, 2021:

    translation nit: How are translators supposed to translate :&lt;ul&gt;&lt;li&gt;? It might be better to say "Can be one of %s" and then use Join(translated_strings, ", ") or simply "Refer to the getpeerinfo RPC help for details on the connection types"?

    jonatack commented at 10:08 AM on January 10, 2021:

    @hebasto, if we replace the escaped markup with placeholders (as done here for the nonbreaking hyphen), is that still QT Designer-friendly? Can do it in #179.

    hebasto commented at 10:22 AM on January 10, 2021:

    translation nit: How are translators supposed to translate :&lt;ul&gt;&lt;li&gt;?

    There was a discussion in private IRC with @jonatack:

    <hebasto> 14:24:52 but I expect the translators' burden :) 14:25:39 hope that translators are aware of escaping :)

    I'm not an expert in translation but I believe that :&lt;ul&gt;&lt;li&gt; is not a subject to translate as %1 or %2.

    It might be better to say "Can be one of %s" and then use Join(translated_strings, ", ") or simply "Refer to the getpeerinfo RPC help for details on the connection types"?

    Either of the suggested variants LGTM.

    if we replace the escaped markup with placeholders (as done here for the nonbreaking hyphen), is that still QT Designer-friendly?

    I think it is. But, tbh, I'd prefer more succinct @MarcoFalke's suggestions.

    jonatack commented at 10:39 AM on January 10, 2021:

    Thanks. Helpfulness and readability for users were the reasons for the iterations above and it seems best to not regress on those improvements with referring GUI users to the command line or dropping the readable list layout. I'll propose replacing the escaped markup with placeholders.

    hebasto commented at 10:47 AM on January 10, 2021:

    Why not a single placeholder for the whole unordered list? We still have a (shorter) tooltip in the form file, and make translators' life easier :)

    jonatack commented at 11:01 AM on January 10, 2021:

    SGTM too.

    jonatack commented at 1:56 PM on January 10, 2021:

    Done in #180

  70. jonatack deleted the branch on Jan 10, 2021
  71. sidhujag referenced this in commit dc579d1895 on Jan 10, 2021
  72. laanwj referenced this in commit d1ddead09a on Jan 27, 2021
  73. jonatack referenced this in commit ff8afeb222 on Jan 30, 2021
  74. bitcoin-core locked this on Aug 16, 2022
Contributors
jonatackMarcoFalkehebastopromaglaanwjjarolrod
Linked (view graph)
#159 Replace "Direction" with "Connection Type" in Peer Tab#180 Peer details: connection type follow-ups
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-14 15:20 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me