Connection Type Translator Comments #345

jarolrod commented at 12:18 AM on May 27, 2021: member

This PR introduces Qt translator comments for Connection Type strings in guiutil.cpp as well as rpcconsole.cpp.

This is an alternate implementation of the idea presented in the last three commits of #289. It is especially inspired by commit 842f4e834dfe5fd2786a5092f78ea28da1b36e4f.

Per Qt Dev Notes, it is better to not break up strings when not necessary. This way we preserve the full context for translators.

jarolrod renamed this:
~~qt: connection type translator comments~~
Connection Type Translator Comments
on May 27, 2021

jarolrod force-pushed on May 27, 2021

jarolrod commented at 1:00 AM on May 27, 2021: member

updated from 0819d50 -> f5c2377 (pr345.01 -> pr345.02, diff)

Changes:

Add translator comments for some missed connection type strings.

hebasto added the label Translations on May 29, 2021

in src/qt/rpcconsole.cpp:481 in f5c23778e5 outdated

 476 | +        /*: Explanatory text for an Outbound peer connection which
 477 | +            relays all network information. This is the default behavior for
 478 | +            Outbound connections. */
 479 |          tr("Outbound Full Relay: default"),
 480 | +        /*: Explanatory text for an Outbound peer connection which
 481 | +            only relays network information about Blocks. */

hebasto commented at 11:43 AM on May 29, 2021:

For translators it is not obvious the information relayed through the network is related to blocks, transactions, addresses, and service needs (handshaking, ping etc). "only relays network information about Blocks" is not the same as "does not relay transactions or addresses" to them. Btw, the latter seems more correct as BLOCK_RELAY peers handle service network commands well.

jonatack commented at 4:17 PM on July 23, 2021:

Yes.

(It's not clear to me what value these descriptions add, as they are next to descriptions that already exist.)

hebasto commented at 7:26 PM on July 25, 2021:

It's not clear to me what value these descriptions add, as they are next to descriptions that already exist.

If understand your concern correctly, the point is that in Transifex.com on-line editor a translator can see only a translatable string and an attached comment, without surrounding code.

jonatack commented at 7:46 PM on July 25, 2021:

Thanks for the explanation. That makes sense.

hebasto commented at 11:46 AM on May 29, 2021: member

Approach ACK f5c23778e54b74cdd4f8fe8afd507d0a0e045d37.

hebasto requested review from jonatack on May 29, 2021

in src/qt/rpcconsole.cpp:495 in f5c23778e5 outdated

 490 |              .arg(QString(nonbreaking_hyphen) + "connect"),
 491 | +        /*: Explanatory text for a temporary Outbound peer connection which
 492 | +            is used to test the validity of known addresses. */
 493 |          tr("Outbound Feeler: short-lived, for testing addresses"),
 494 | +        /*: Explanatory text for an Outbound peer connection which is used
 495 | +            to get addresses from a peer. */

jonatack commented at 4:18 PM on July 23, 2021:

s/get/request/

in src/qt/rpcconsole.cpp:476 in f5c23778e5 outdated

 470 | @@ -471,14 +471,28 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty
 471 |  
 472 |      constexpr QChar nonbreaking_hyphen(8209);
 473 |      const std::vector<QString> CONNECTION_TYPE_DOC{
 474 | +        //: Explanatory text for an Inbound peer connection.
 475 |          tr("Inbound: initiated by peer"),
 476 | +        /*: Explanatory text for an Outbound peer connection which

jonatack commented at 4:19 PM on July 23, 2021:

s/Inbound/inbound/ and s/Outbound/outbound/ in these descriptions when used as an adjective and not as a proper noun, which seems to be the case throughout.

in src/qt/guiutil.cpp:677 in f5c23778e5 outdated

 670 | @@ -671,14 +671,23 @@ QString ConnectionTypeToQString(ConnectionType conn_type, bool prepend_direction
 671 |  {
 672 |      QString prefix;
 673 |      if (prepend_direction) {
 674 | -        prefix = (conn_type == ConnectionType::INBOUND) ? QObject::tr("Inbound") : QObject::tr("Outbound") + " ";
 675 | +        prefix = (conn_type == ConnectionType::INBOUND) ?
 676 | +                     //: An Inbound Connection from a Peer.
 677 | +                     QObject::tr("Inbound") :
 678 | +                     //: An Outbound Connection to a Peer.

jonatack commented at 4:21 PM on July 23, 2021:

here and above, s/Peer/peer/

in src/qt/rpcconsole.cpp:491 in f5c23778e5 outdated

 486 | +            manual connections. */
 487 |          tr("Outbound Manual: added using RPC %1 or %2/%3 configuration options")
 488 |              .arg("addnode")
 489 |              .arg(QString(nonbreaking_hyphen) + "addnode")
 490 |              .arg(QString(nonbreaking_hyphen) + "connect"),
 491 | +        /*: Explanatory text for a temporary Outbound peer connection which

jonatack commented at 4:22 PM on July 23, 2021:

throughout: either s/ which/, which/ or s/which/that/

in src/qt/guiutil.cpp:688 in f5c23778e5 outdated

 684 |      case ConnectionType::OUTBOUND_FULL_RELAY: return prefix + QObject::tr("Full Relay");
 685 | +    //: Peer connection type which only relays Block network information.
 686 |      case ConnectionType::BLOCK_RELAY: return prefix + QObject::tr("Block Relay");
 687 | +    //: Peer connection type established manually through one of several methods.
 688 |      case ConnectionType::MANUAL: return prefix + QObject::tr("Manual");
 689 | +    //: Peer connection type which feels or tests known addresses for validity.

jonatack commented at 4:23 PM on July 23, 2021:

would remove "feels" and maybe use "verifies", as "tests" can be either a noun or a verb and so is more ambiguous

in src/qt/guiutil.cpp:684 in f5c23778e5 outdated

 680 |      }
 681 |      switch (conn_type) {
 682 |      case ConnectionType::INBOUND: return prefix;
 683 | +    //: Peer connection type which relays all network information.
 684 |      case ConnectionType::OUTBOUND_FULL_RELAY: return prefix + QObject::tr("Full Relay");
 685 | +    //: Peer connection type which only relays Block network information.

jonatack commented at 4:24 PM on July 23, 2021:

throughout: s/Block/block/ as it is not a proper noun, unless part of one like "Outbound Block Relay", e.g. a connection type

jonatack commented at 4:25 PM on July 23, 2021: contributor

Thanks @jarolrod for reminding me to look at this.

shaavan commented at 4:01 PM on July 27, 2021: contributor

Concept ACK

I like the changes that are done here. They would very much help a translator to translate the strings with proper context correctly. However, I want to make some suggestions here:

In the Guiutil.cpp file in lines 675 to 677: It would help a translator if the meaning of inbound and outbound was briefly explained there.
In the rpconsole.cpp file at line number 494: It should be mentioned that the Outbound peer connection is temporary, as mentioned in line 491

jarolrod force-pushed on Aug 10, 2021

jarolrod commented at 5:18 PM on August 10, 2021: member

Updated from f5c2377 -> 87b8129 (pr345.02 -> pr345.03)

Thanks @hebasto @jonatack @ShaMan239 for the reviews. I have implemented all of the review suggestions.

jarolrod force-pushed on Aug 10, 2021

jarolrod commented at 5:21 PM on August 10, 2021: member

updated from 87b8129 -> 07adcbe (pr345.03 -> pr345.04)

Changes: small cleanup with capitalized Inbound Connection

Talkless approved

Talkless commented at 1:37 PM on August 11, 2021: none

Code review ACK, comments looks good.

hebasto commented at 1:38 PM on August 11, 2021: member

@Talkless

Code review ACK, comments looks good.

Please insert a top commit hash :)

in src/qt/guiutil.cpp:676 in 07adcbe6dc outdated

 670 | @@ -671,14 +671,26 @@ QString ConnectionTypeToQString(ConnectionType conn_type, bool prepend_direction
 671 |  {
 672 |      QString prefix;
 673 |      if (prepend_direction) {
 674 | -        prefix = (conn_type == ConnectionType::INBOUND) ? QObject::tr("Inbound") : QObject::tr("Outbound") + " ";
 675 | +        prefix = (conn_type == ConnectionType::INBOUND) ?
 676 | +                     /*: An inbound connection from a peer. An inbound connection
 677 | +                         is a connection initated by a peer. */

hebasto commented at 1:44 PM on August 11, 2021:

typo: initated ==> initiated

Talkless commented at 2:06 PM on August 11, 2021:

Good catch. I should start piping diffs into vim and use :set spell... ...

hebasto commented at 2:07 PM on August 11, 2021:

Good catch. I should start piping diffs into vim and use :set spell... ...

You also could run test/lint/lint-spelling.sh :)

Talkless commented at 1:06 PM on August 14, 2021:

Cool, thanks!

jarolrod commented at 8:56 PM on September 5, 2021:

Thanks updated

in src/qt/rpcconsole.cpp:480 in 07adcbe6dc outdated

 475 |          tr("Inbound: initiated by peer"),
 476 | +        /*: Explanatory text for an outbound peer connection which
 477 | +            relays all network information. This is the default behavior for
 478 | +            Outbound connections. */
 479 |          tr("Outbound Full Relay: default"),
 480 | +        /*: Explanatory text for an outbound peer connection which relays

jonatack commented at 1:53 PM on August 11, 2021:

throughout: either precede "which" with a comma and space, or use "that", e.g.

"connection, which relays"

"connection that relays"

jarolrod commented at 8:56 PM on September 5, 2021:

thanks, updated

jonatack commented at 2:31 PM on August 11, 2021: contributor

(Maybe update the translations in merged #317 per the writing style here, if relevant.)

jarolrod force-pushed on Sep 5, 2021

jarolrod commented at 8:57 PM on September 5, 2021: member

updated from 07adcbe -> 8c13340 (pr345.04 - > pr345.05)

Changes: address @hebasto and @jonatack comments, thanks for the review! @jonatack

(Maybe update the translations in merged #317 per the writing style here, if relevant.)

I will leave for follow-ups that improve translator comments throughout the GUI

in src/qt/guiutil.cpp:686 in 8c1334060c outdated

 682 |      }
 683 |      switch (conn_type) {
 684 |      case ConnectionType::INBOUND: return prefix;
 685 | +    //: Peer connection type that relays all network information.
 686 |      case ConnectionType::OUTBOUND_FULL_RELAY: return prefix + QObject::tr("Full Relay");
 687 | +    /*: Peer connection type that relays relays network information about

shaavan commented at 6:04 PM on September 13, 2021:

The updated translated comment looks good. But I think there is some scope for correction. This might be an intentional decision by OP. But I still better mention it.

    /*: Peer connection type that relays relay network information about

or

    /*: Peer connection type that relays network information about

jarolrod commented at 6:16 PM on September 13, 2021:

was not intentional :D, thanks for catching this. Will update

jarolrod commented at 5:34 AM on September 21, 2021:

thanks, addressed in 371e2b9

jarolrod force-pushed on Sep 21, 2021

jarolrod commented at 5:34 AM on September 21, 2021: member

Updated from 8c13340 -> 371e2b9 (pr345.05 -> pr345.06, diff)

Addressed @shaavan comments and removed a redundant word; thanks for the review!

shaavan approved

shaavan commented at 1:14 PM on September 21, 2021: contributor

ACK 371e2b901bcbb1ab39f14431060f412030813757

The translator's comments are looking great! Kudos to @jarolrod!

in src/qt/rpcconsole.cpp:478 in 371e2b901b outdated

 470 | @@ -471,14 +471,28 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty
 471 |  
 472 |      constexpr QChar nonbreaking_hyphen(8209);
 473 |      const std::vector<QString> CONNECTION_TYPE_DOC{
 474 | +        //: Explanatory text for an inbound peer connection.
 475 |          tr("Inbound: initiated by peer"),
 476 | +        /*: Explanatory text for an outbound peer connection that
 477 | +            relays all network information. This is the default behavior for
 478 | +            Outbound connections. */

jonatack commented at 7:06 PM on September 21, 2021:

            outbound connections. */

in src/qt/guiutil.cpp:691 in 371e2b901b outdated

 687 | +    /*: Peer connection type that relays network information about
 688 | +        blocks and not transactions or addresses. */
 689 |      case ConnectionType::BLOCK_RELAY: return prefix + QObject::tr("Block Relay");
 690 | +    //: Peer connection type established manually through one of several methods.
 691 |      case ConnectionType::MANUAL: return prefix + QObject::tr("Manual");
 692 | +    //: Peer connection type that verifies or tests known addresses for validity.

jonatack commented at 7:06 PM on September 21, 2021:

would choose between "verifies" and "tests". also, see the definition in net.h#L160

    //: Short-lived peer connection type that tests the aliveness of known addresses.

in src/qt/guiutil.cpp:693 in 371e2b901b outdated

 689 |      case ConnectionType::BLOCK_RELAY: return prefix + QObject::tr("Block Relay");
 690 | +    //: Peer connection type established manually through one of several methods.
 691 |      case ConnectionType::MANUAL: return prefix + QObject::tr("Manual");
 692 | +    //: Peer connection type that verifies or tests known addresses for validity.
 693 |      case ConnectionType::FEELER: return prefix + QObject::tr("Feeler");
 694 | +    //: Peer connection type that solicits known addresses from a peer.

jonatack commented at 7:13 PM on September 21, 2021:

    //: Short-lived peer connection type that solicits known addresses from a peer.

in src/qt/rpcconsole.cpp:491 in 371e2b901b outdated

 486 | +            manual connections. */
 487 |          tr("Outbound Manual: added using RPC %1 or %2/%3 configuration options")
 488 |              .arg("addnode")
 489 |              .arg(QString(nonbreaking_hyphen) + "addnode")
 490 |              .arg(QString(nonbreaking_hyphen) + "connect"),
 491 | +        /*: Explanatory text for a temporary outbound peer connection that

jonatack commented at 7:14 PM on September 21, 2021:

        /*: Explanatory text for a short-lived outbound peer connection that

in src/qt/rpcconsole.cpp:492 in 371e2b901b outdated

 487 |          tr("Outbound Manual: added using RPC %1 or %2/%3 configuration options")
 488 |              .arg("addnode")
 489 |              .arg(QString(nonbreaking_hyphen) + "addnode")
 490 |              .arg(QString(nonbreaking_hyphen) + "connect"),
 491 | +        /*: Explanatory text for a temporary outbound peer connection that
 492 | +            is used to test the validity of known addresses. */

jonatack commented at 7:14 PM on September 21, 2021:

"temporary" seems a bit vague; could be true for most connections

            is used to test the aliveness of known addresses. */

jarolrod commented at 3:35 AM on September 22, 2021:

I like this word here, addressed in 4832737

in src/qt/rpcconsole.cpp:494 in 371e2b901b outdated

 489 |              .arg(QString(nonbreaking_hyphen) + "addnode")
 490 |              .arg(QString(nonbreaking_hyphen) + "connect"),
 491 | +        /*: Explanatory text for a temporary outbound peer connection that
 492 | +            is used to test the validity of known addresses. */
 493 |          tr("Outbound Feeler: short-lived, for testing addresses"),
 494 | +        /*: Explanatory text for a temporary outbound peer connection that is used

jonatack commented at 7:15 PM on September 21, 2021:

        /*: Explanatory text for a short-lived outbound peer connection that is used

jonatack commented at 7:16 PM on September 21, 2021: contributor

ACK 371e2b901bcbb

modulo some comments below, thanks for improving this!

qt: connection type translator comments

Introduce Qt translator comments for connection types.

4832737c7d

jarolrod force-pushed on Sep 22, 2021

jarolrod commented at 3:37 AM on September 22, 2021: member

updated from 371e2b9 -> 4832737 (pr345.06 -> pr345.07, diff)

Changes: addressed all of @jonatack comments, thanks for the review!

jonatack commented at 6:52 PM on September 22, 2021: contributor

Code review re-ACK 4832737c7dcc87afea5e1e88945ec311417aa876 per git diff 371e2b9 4832737, changes are translator comment edits since my review yesterday (thank you for updating)

hebasto approved

hebasto commented at 8:51 AM on September 29, 2021: member

ACK 4832737c7dcc87afea5e1e88945ec311417aa876

hebasto merged this on Sep 29, 2021

hebasto closed this on Sep 29, 2021

sidhujag referenced this in commit 5303b8156f on Sep 29, 2021

bitcoin-core locked this on Sep 29, 2022