Peer details: connection type follow-ups #180

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.

resolved

How about

--- a/src/qt/rpcconsole.cpp
+++ b/src/qt/rpcconsole.cpp
@@ -463,19 +463,19 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty
     ui->dataDir->setToolTip(ui->dataDir->toolTip().arg(QString(nonbreaking_hyphen) + "datadir"));
     ui->blocksDir->setToolTip(ui->blocksDir->toolTip().arg(QString(nonbreaking_hyphen) + "blocksdir"));
     ui->openDebugLogfileButton->setToolTip(ui->openDebugLogfileButton->toolTip().arg(PACKAGE_NAME));
-    ui->peerConnectionTypeLabel->setToolTip(
-        ui->peerConnectionTypeLabel->toolTip()
-            .arg(tr("<ul>"
-                    "<li>Inbound Relay: initiated by peer</li>"
-                    "<li>Outbound Full Relay: default</li>"
-                    "<li>Outbound Block Relay: does not relay transactions or addresses</li>"
-                    "<li>Outbound Manual: added using RPC %1 or %2/%3 configuration options</li>"
-                    "<li>Outbound Feeler: short-lived, for testing addresses</li>"
-                    "<li>Outbound Address Fetch: short-lived, for soliciting addresses</li>"
-                    "</ul>")
-                     .arg("addnode")
-                     .arg(QString(nonbreaking_hyphen) + "addnode")
-                     .arg(QString(nonbreaking_hyphen) + "connect")));
+
+    const std::vector<QString> list_items{
+        tr("Inbound Relay: initiated by peer"),
+        tr("Outbound Full Relay: default"),
+        tr("Outbound Block Relay: does not relay transactions or addresses"),
+        tr("Outbound Manual: added using RPC %1 or %2/%3 configuration options")
+            .arg("addnode")
+            .arg(QString(nonbreaking_hyphen) + "addnode")
+            .arg(QString(nonbreaking_hyphen) + "connect"),
+        tr("Outbound Feeler: short-lived, for testing addresses"),
+        tr("Outbound Address Fetch: short-lived, for soliciting addresses")};
+    const QString list = "<ul><li>" + Join(list_items, QString("</li><li>")) + "</li></ul>";
+    ui->peerConnectionTypeLabel->setToolTip(ui->peerConnectionTypeLabel->toolTip().arg(list));
 
     if (platformStyle->getImagesOnButtons()) {
         ui->openDebugLogfileButton->setIcon(platformStyle->SingleColorIcon(":/icons/export"));

?

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

Nice, thanks! We were thinking in the same direction. Adapted it and added your name to the commit.

My bad, I meant #include <util/string.h> for the Join template function.

Oops, fixed

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

df5fb7ed64f5e47cad66c2fe3582c66a7896f818, for correct work of translation mechanics

    case ConnectionType::INBOUND: return relay_txes ? QObject::tr("Inbound Full Relay") : QObject::tr("Inbound Block Relay");

It seems equivalent? Anyway, done--thank you for looking!

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.

nit: A compiler will care of all switch statements in src/qt/guiutil.cpp :)

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.

nit: Is a tooltip update required (see the screenshot)?

Updated to Inbound Full/Block Relay

SPV (light-wallet) peers are not "inbound block relay" peers

good point, fixing

resolved with #201 or #203

nit: This change is unrelated to this PR :)

removed (clang-format did it ;)

nit:

    constexpr QChar nonbreaking_hyphen(8209);

done, thanks

f3153dc08fa16aa3bbca52f67833e5c9fbe8e095

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

Why have a bit here?

In Qt Designer it makes obvious a tooltip presence.

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 😄

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.

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.