Peer details: connection type follow-ups #180

pull jonatack wants to merge 3 commits into bitcoin-core:master from jonatack:peer-details-connection-type-followups changing 5 files +26 −13
  1. jonatack commented at 1:56 pm on January 10, 2021: contributor

    Three follow-ups to #163:

    • return relay type for inbound peers
    • improve markup handling in the tooltip to facilitate translations
    • update ConnectionType doxygen documentation

    Screenshot from 2021-01-11 08-37-44

  2. in src/qt/rpcconsole.cpp:475 in 09f4414509 outdated 
    471+                    "<li>Outbound Full Relay: default</li>"
472+                    "<li>Outbound Block Relay: does not relay transactions or addresses</li>"
473+                    "<li>Outbound Manual: added using RPC %1 or %2/%3 configuration options</li>"
474+                    "<li>Outbound Feeler: short-lived, for testing addresses</li>"
475+                    "<li>Outbound Address Fetch: short-lived, for soliciting addresses</li>"
476+                    "</ul>")
    jonatack commented at 2:00 pm on January 10, 2021:
    I’m not sure if the markup should be replaced by placeholders here or not. I see inline *.cpp markup like tr("<b>abc</b>") elsewhere in the GUI codebase.
    jonatack commented at 5:28 pm on January 10, 2021:
    resolved
  3. in src/qt/rpcconsole.cpp:478 in 09f4414509 outdated 
    474+                    "<li>Outbound Feeler: short-lived, for testing addresses</li>"
475+                    "<li>Outbound Address Fetch: short-lived, for soliciting addresses</li>"
476+                    "</ul>")
477+                     .arg("addnode")
478+                     .arg(QString(nonbreaking_hyphen) + "addnode")
479+                     .arg(QString(nonbreaking_hyphen) + "connect")));
    hebasto commented at 3:42 pm on January 10, 2021:

    How about

     0--- a/src/qt/rpcconsole.cpp
 1+++ b/src/qt/rpcconsole.cpp
 2@@ -463,19 +463,19 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty
 3     ui->dataDir->setToolTip(ui->dataDir->toolTip().arg(QString(nonbreaking_hyphen) + "datadir"));
 4     ui->blocksDir->setToolTip(ui->blocksDir->toolTip().arg(QString(nonbreaking_hyphen) + "blocksdir"));
 5     ui->openDebugLogfileButton->setToolTip(ui->openDebugLogfileButton->toolTip().arg(PACKAGE_NAME));
 6-    ui->peerConnectionTypeLabel->setToolTip(
 7-        ui->peerConnectionTypeLabel->toolTip()
 8-            .arg(tr("<ul>"
 9-                    "<li>Inbound Relay: initiated by peer</li>"
10-                    "<li>Outbound Full Relay: default</li>"
11-                    "<li>Outbound Block Relay: does not relay transactions or addresses</li>"
12-                    "<li>Outbound Manual: added using RPC %1 or %2/%3 configuration options</li>"
13-                    "<li>Outbound Feeler: short-lived, for testing addresses</li>"
14-                    "<li>Outbound Address Fetch: short-lived, for soliciting addresses</li>"
15-                    "</ul>")
16-                     .arg("addnode")
17-                     .arg(QString(nonbreaking_hyphen) + "addnode")
18-                     .arg(QString(nonbreaking_hyphen) + "connect")));
19+
20+    const std::vector<QString> list_items{
21+        tr("Inbound Relay: initiated by peer"),
22+        tr("Outbound Full Relay: default"),
23+        tr("Outbound Block Relay: does not relay transactions or addresses"),
24+        tr("Outbound Manual: added using RPC %1 or %2/%3 configuration options")
25+            .arg("addnode")
26+            .arg(QString(nonbreaking_hyphen) + "addnode")
27+            .arg(QString(nonbreaking_hyphen) + "connect"),
28+        tr("Outbound Feeler: short-lived, for testing addresses"),
29+        tr("Outbound Address Fetch: short-lived, for soliciting addresses")};
30+    const QString list = "<ul><li>" + Join(list_items, QString("</li><li>")) + "</li></ul>";
31+    ui->peerConnectionTypeLabel->setToolTip(ui->peerConnectionTypeLabel->toolTip().arg(list));
32 
33     if (platformStyle->getImagesOnButtons()) {
34         ui->openDebugLogfileButton->setIcon(platformStyle->SingleColorIcon(":/icons/export"));

    ?

    (and #include <string.h> <util/string.h> )

    jonatack commented at 5:01 pm on January 10, 2021:
    Nice, thanks! We were thinking in the same direction. Adapted it and added your name to the commit.
  4. jonatack force-pushed on Jan 10, 2021
  5. in src/qt/rpcconsole.cpp:48 in 3bb3ad2223 outdated 
    44@@ -45,6 +45,7 @@
45 #include <QTime>
46 #include <QTimer>
47 
48+#include <string>
    hebasto commented at 5:04 pm on January 10, 2021:
    My bad, I meant #include <util/string.h> for the Join template function.
    jonatack commented at 5:09 pm on January 10, 2021:
    Oops, fixed
  6. in src/qt/rpcconsole.cpp:464 in 3bb3ad2223 outdated 
    459@@ -459,11 +460,22 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty
460 
461     ui->splitter->restoreState(settings.value("PeersTabSplitterSizes").toByteArray());
462 
463-    QChar nonbreaking_hyphen(8209);
464+    const QChar nonbreaking_hyphen(8209);
465+    const std::vector<QString> CONNECTION_TYPE_DOC{
    jonatack commented at 5:08 pm on January 10, 2021:
    Named it CONNECTION_TYPE_DOC to show up in git grep with the constant of the same name in rpc/net.cpp and updated the net.h documentation
  7. jonatack force-pushed on Jan 10, 2021
  8. jonatack force-pushed on Jan 10, 2021
  9. in src/qt/guiutil.cpp:770 in 1a297a7ea0 outdated 
    763@@ -764,10 +764,10 @@ QString NetworkToQString(Network net)
764     assert(false);
765 }
766 
767-QString ConnectionTypeToQString(ConnectionType conn_type)
768+QString ConnectionTypeToQString(ConnectionType conn_type, bool relay_txes)
769 {
770     switch (conn_type) {
771-    case ConnectionType::INBOUND: return QObject::tr("Inbound");
772+    case ConnectionType::INBOUND: return QObject::tr(relay_txes ? "Inbound Full Relay" : "Inbound Block Relay");
    hebasto commented at 5:44 pm on January 10, 2021:

    df5fb7ed64f5e47cad66c2fe3582c66a7896f818, for correct work of translation mechanics

    0    case ConnectionType::INBOUND: return relay_txes ? QObject::tr("Inbound Full Relay") : QObject::tr("Inbound Block Relay");
    jonatack commented at 6:10 pm on January 10, 2021:
    It seems equivalent? Anyway, done–thank you for looking!
    hebasto commented at 6:56 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.

  10. hebasto changes_requested
  11. hebasto commented at 5:45 pm on January 10, 2021: member
    Approach ACK 1a297a7ea08c3ca5c591ba4942a5a12bc90a210d.
  12. jonatack force-pushed on Jan 10, 2021
  13. in src/net.h:115 in e711200759 outdated 
    110@@ -111,7 +111,8 @@ struct CSerializedNetMsg
111  * connection. Aside from INBOUND, all types are initiated by us.
112  *
113  * If adding or removing types, please update CONNECTION_TYPE_DOC in
114- * src/rpc/net.cpp. */
115+ * src/rpc/net.cpp and src/qt/rpcconsole.cpp, as well as the descriptions in
116+ * src/qt/guiutil.cpp and src/bitcoin-cli.cpp::NetinfoRequestHandler. */
    hebasto commented at 7:47 pm on January 10, 2021:
    nit: A compiler will care of all switch statements in src/qt/guiutil.cpp :)
    jonatack commented at 8:15 pm on January 10, 2021:
    True. Nevertheless, per element of least surprise, if we list the sites to update we may as well list all of them and save developer time.
  14. in src/qt/guiutil.cpp:770 in e711200759 outdated 
    763@@ -764,10 +764,10 @@ QString NetworkToQString(Network net)
764     assert(false);
765 }
766 
767-QString ConnectionTypeToQString(ConnectionType conn_type)
768+QString ConnectionTypeToQString(ConnectionType conn_type, bool relay_txes)
769 {
770     switch (conn_type) {
771-    case ConnectionType::INBOUND: return QObject::tr("Inbound");
772+    case ConnectionType::INBOUND: return relay_txes ? QObject::tr("Inbound Full Relay") : QObject::tr("Inbound Block Relay");
    hebasto commented at 7:49 pm on January 10, 2021:
    nit: Is a tooltip update required (see the screenshot)?
    jonatack commented at 8:35 pm on January 10, 2021:
    Updated to Inbound Full/Block Relay
    MarcoFalke commented at 7:10 am on January 28, 2021:
    SPV (light-wallet) peers are not “inbound block relay” peers
    jonatack commented at 7:46 am on January 28, 2021:
    good point, fixing
    jonatack commented at 11:36 pm on January 28, 2021:
    resolved with #201 or #203
  15. in src/qt/rpcconsole.cpp:448 in e711200759 outdated 
    444@@ -444,7 +445,7 @@ void RPCExecutor::request(const QString &command, const WalletModel* wallet_mode
445     }
446 }
447 
448-RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformStyle, QWidget *parent) :
449+RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle* _platformStyle, QWidget* parent) :
    hebasto commented at 7:49 pm on January 10, 2021:
    nit: This change is unrelated to this PR :)
    jonatack commented at 8:36 pm on January 10, 2021:
    removed (clang-format did it ;)
  16. in src/qt/rpcconsole.cpp:463 in e711200759 outdated 
    459@@ -459,11 +460,22 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty
460 
461     ui->splitter->restoreState(settings.value("PeersTabSplitterSizes").toByteArray());
462 
463-    QChar nonbreaking_hyphen(8209);
464+    const QChar nonbreaking_hyphen(8209);
    hebasto commented at 7:51 pm on January 10, 2021:

    nit:

    0    constexpr QChar nonbreaking_hyphen(8209);
    jonatack commented at 8:36 pm on January 10, 2021:
    done, thanks
  17. hebasto approved
  18. hebasto commented at 7:51 pm on January 10, 2021: member

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

    Screenshot from 2021-01-10 21-32-34

  19. gui: return inbound {full, block} relay type in peer details 4f09615733
  20. gui: improve markup handling of connection type tooltip 
    Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
    f3153dc08f
  21. doc: update ConnectionType Doxygen documentation 79a2576af1
  22. jonatack force-pushed on Jan 10, 2021
  23. jonatack commented at 8:42 pm on January 10, 2021: contributor

    Thanks @hebasto! Updated.

     0diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp
 1index 4fdc74c48..44addeeda 100644
 2--- a/src/qt/rpcconsole.cpp
 3+++ b/src/qt/rpcconsole.cpp
 4@@ -445,7 +445,7 @@ void RPCExecutor::request(const QString &command, const WalletModel* wallet_mode
 5     }
 6 }
 7 
 8-RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle* _platformStyle, QWidget* parent) :
 9+RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformStyle, QWidget *parent) :
10     QWidget(parent),
11     m_node(node),
12     ui(new Ui::RPCConsole),
13@@ -460,9 +460,9 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle* _platformSty
14 
15     ui->splitter->restoreState(settings.value("PeersTabSplitterSizes").toByteArray());
16 
17-    const QChar nonbreaking_hyphen(8209);
18+    constexpr QChar nonbreaking_hyphen(8209);
19     const std::vector<QString> CONNECTION_TYPE_DOC{
20-        tr("Inbound: initiated by peer"),
21+        tr("Inbound Full/Block Relay: initiated by peer"),
22         tr("Outbound Full Relay: default"),
23         tr("Outbound Block Relay: does not relay transactions or addresses"),
24         tr("Outbound Manual: added using RPC %1 or %2/%3 configuration options")

    Screenshot from 2021-01-11 08-37-44

  24. hebasto approved
  25. hebasto commented at 11:34 am on January 11, 2021: member
    re-ACK 79a2576af1e499102943aa4e1d98994ee8a9c6b5, only suggested changes since my previous review.
  26. in src/qt/forms/debugwindow.ui:1082 in f3153dc08f outdated 
    1078@@ -1079,7 +1079,7 @@
1079                <item row="1" column="0">
1080                 <widget class="QLabel" name="peerConnectionTypeLabel">
1081                  <property name="toolTip">
1082-                   <string>The type of peer connection:&lt;ul&gt;&lt;li&gt;Inbound Full/Block Relay: 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>
1083+                   <string>The type of peer connection: %1</string>
    promag commented at 1:50 pm on January 11, 2021:

    f3153dc08fa16aa3bbca52f67833e5c9fbe8e095

    Just move the whole tooltip? Why have a bit here?

    hebasto commented at 2:05 pm on January 11, 2021:

    Why have a bit here?

    In Qt Designer it makes obvious a tooltip presence.

    jonatack commented at 2:23 pm on January 11, 2021:

    Just move the whole tooltip? Why have a bit here?

    That’s what I started with in #163, see #163 (comment)

    There have been a few iterations on this tooltip 😄

    promag commented at 2:42 pm on January 11, 2021:

    I see, still not fond of the approach. Being a dynamic tooltip I think it’s fine to not be in the .ui file. If someone adds a tooltip in the designer then it will be obvious that it’s being replaced at runtime. Reading this code makes you check what is in the .ui, and you still have to check this code if you change what’s in the .ui. At the end anyone that wants to change this still has to check both places.

    I think that a good rule of thumb is to leave all static/constant stuff for .ui.

    Not saying that this is incorrect or that it doesn’t work, just my opinion. It’s fine if it’s merged as is.

    jonatack commented at 2:53 pm on January 11, 2021:
    I don’t have a strong opinion and saw other tooltips with html markup completely in the .cpp file, but I also don’t have Qt or Qt Designer experience. I’m happy to keep it as-is or move it, either way.
  27. promag commented at 1:53 pm on January 11, 2021: contributor
    Concept ACK.
  28. jarolrod commented at 8:12 pm on January 15, 2021: member

    ACK 79a2576af1e499102943aa4e1d98994ee8a9c6b5, tested on macOS 11.1 with Qt 5.15.2

  29. RandyMcMillan commented at 8:39 pm on January 26, 2021: contributor
    I don’t believe this much information should be displayed in a mouse over event. Attaching a click event to a UILabel (or a question mark image) would be a better approach (imo). The click event could trigger a modal view that gives a more detailed explanation of the information contained in the entire panel.
  30. jonatack commented at 8:49 pm on January 26, 2021: contributor

    This pull doesn’t add the tooltip. It makes these follow-ups to #163:

    • return relay type for inbound peers
    • improve markup handling in the tooltip to facilitate translations
    • update ConnectionType doxygen documentation

    (Feel free to open a pull to propose your approach.)

  31. jonatack commented at 8:50 pm on January 26, 2021: contributor
    This has 2 ACKs from @hebasto and @jarolrod and a Concept ACK from @promag. Anyone wish to ACK?
  32. laanwj approved
  33. laanwj commented at 7:05 pm on January 27, 2021: member
    Code review ACK 79a2576af1e499102943aa4e1d98994ee8a9c6b5
  34. laanwj merged this on Jan 27, 2021
  35. laanwj closed this on Jan 27, 2021
  36. jonatack deleted the branch on Jan 27, 2021
  37. jonatack commented at 7:54 pm on January 27, 2021: contributor
    Thanks!
  38. sidhujag referenced this in commit 450c6b029e on Jan 28, 2021
  39. bitcoin-core locked this on Aug 16, 2022


jonatack hebasto MarcoFalke promag jarolrod RandyMcMillan laanwj

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