Peer details: replace Direction with Connection Type

[jonatack]

Closes #159.

[maflcko]

Nice. Concept ACK

[hebasto]

Concept ACK.

[hebasto]

Concept ACK.

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

[jonatack]

Thanks for the excellent feedback @hebasto! Updated.

[hebasto]

I have reviewed the code (8e7f82a), 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
  

[jonatack]

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

[hebasto]

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

Looks great:

[jonatack]

It was suggested to re-open here.

[maflcko]

Concept re-ACK. Thanks for re-opening.

[jonatack]

Rebased.

[hebasto]

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

[jonatack]

Rebased git range-diff ae8f797 aef9693 7098509

[hebasto]

re-ACK 7098509, only rebased since my previous review.

[jonatack]

Rebased after the merge of bitcoin/bitcoin#20786.

[hebasto]

re-ACK 20fa975, only rebased since my previous review due to the conflict with bitcoin/bitcoin#20786, verified with

$ git range-diff master be8294ff428da7d13043b3f9a27d7ace1c2dadf4 20fa97560b2d27f94f556a570f3556356877b79f

[jarolrod]

ACK 20fa975

Tested on macOS Big Sur 11.1 with qt 5.15.2. Looks great!

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,

[jonatack]

Better? If yes, bold or no?

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

[jarolrod]

@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)

[jonatack]

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

[jarolrod]

@jonatack looks awesome 👍

[jonatack]

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

[hebasto]

@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.

[jonatack]

@hebasto thanks--done.

[hebasto]

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

[jarolrod]

re-ACK 06ba9b3

[maflcko]

translation nit: How are translators supposed to translate :<ul><li>? 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]

@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]

translation nit: How are translators supposed to translate :<ul><li>?

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 :<ul><li> 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]

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]

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]

SGTM too.

[jonatack]

Done in #180