qt, refactor: rpcconsole translatable string fixes and improvements #221

pull jonatack wants to merge 2 commits into bitcoin-core:master from jonatack:qt-rpcconsole-translation-improvements changing 2 files +26 −21
  1. jonatack commented at 5:28 PM on February 23, 2021: contributor
    • fixups from #206 review feedback (thanks!), see commit message for details
    • hoists repeatedly used translatable strings to the RPCConsole class for reuse
  2. in src/qt/rpcconsole.cpp:479 in e1a10ff218 outdated 
     473 | @@ -474,9 +474,9 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty
 474 |      const QString list{"<ul><li>" + Join(CONNECTION_TYPE_DOC, QString("</li><li>")) + "</li></ul>"};
 475 |      ui->peerConnectionTypeLabel->setToolTip(ui->peerConnectionTypeLabel->toolTip().arg(list));
 476 |      const QString hb_list{"<ul><li>\""
 477 | -        + tr("To") + "\" – " + tr("we selected the peer for high bandwidth relay") + "</li><li>\""
 478 | -        + tr("From") + "\" – " + tr("the peer selected us for high bandwidth relay") + "</li><li>\""
 479 | -        + tr("No") + "\" – " + tr("no high bandwidth relay selected") + "</li></ul>"};
 480 | +        + tr_to   + "\" – " + tr("we selected the peer for high bandwidth relay") + "</li><li>\""
 481 | +        + tr_from + "\" – " + tr("the peer selected us for high bandwidth relay") + "</li><li>\""
 482 | +        + tr_no   + "\" – " + tr("no high bandwidth relay selected")              + "</li></ul>"};
    hebasto commented at 5:47 PM on February 23, 2021:

    Does clang-format-diff.py agree with extra spaces?

    jonatack commented at 6:54 PM on February 23, 2021:

    Dropped (a bit of a shame though, seems less readable) and did the same for other touched lines.

  3. in src/qt/rpcconsole.cpp:1120 in e1a10ff218 outdated 
    1119 |      QString bip152_hb_settings;
1120 | -    if (stats->nodeStats.m_bip152_highbandwidth_to) bip152_hb_settings = tr("To");
1121 | -    if (stats->nodeStats.m_bip152_highbandwidth_from) bip152_hb_settings += (bip152_hb_settings.isEmpty() ? tr("From") : "/" + tr("From"));
1122 | -    if (bip152_hb_settings.isEmpty()) bip152_hb_settings = tr("No");
1123 | +    if (stats->nodeStats.m_bip152_highbandwidth_to) bip152_hb_settings = tr_to;
1124 | +    if (stats->nodeStats.m_bip152_highbandwidth_from) bip152_hb_settings += (bip152_hb_settings.isEmpty() ? tr_from : "/" + tr_from);
    hebasto commented at 5:49 PM on February 23, 2021:

    While here, why not s/"/"/QLatin1String("/")/ ?

    jonatack commented at 6:55 PM on February 23, 2021:

    Done, and also hoisted repeated translatables "never", "unknown" and "Ban for".

  4. hebasto commented at 5:52 PM on February 23, 2021: member

    Concept ACK.

  5. jonatack force-pushed on Feb 23, 2021
  6. in src/qt/rpcconsole.h:74 in 17edc9a9c8 outdated 
      69 | @@ -70,6 +70,8 @@ class RPCConsole: public QWidget
  70 |  
  71 |      QString tabTitle(TabTypes tab_type) const;
  72 |      QKeySequence tabShortcut(TabTypes tab_type) const;
  73 | +    QString tr_yes{tr("Yes")}, tr_no{tr("No")}, tr_to{tr("To")}, tr_from{tr("From")},
  74 | +        tr_never{tr("never")}, tr_unknown{tr("Unknown")}, tr_na{tr("N/A")}, tr_ban_for{tr("Ban for")};
    jonatack commented at 6:58 PM on February 23, 2021:

    maybe "never" should be capitalized like the others

            tr_never{tr("Never")}, tr_unknown{tr("Unknown")}, tr_na{tr("N/A")}, tr_ban_for{tr("Ban for")};
  7. in src/qt/rpcconsole.h:73 in 17edc9a9c8 outdated 
      69 | @@ -70,6 +70,8 @@ class RPCConsole: public QWidget
  70 |  
  71 |      QString tabTitle(TabTypes tab_type) const;
  72 |      QKeySequence tabShortcut(TabTypes tab_type) const;
  73 | +    QString tr_yes{tr("Yes")}, tr_no{tr("No")}, tr_to{tr("To")}, tr_from{tr("From")},
    hebasto commented at 7:09 PM on February 23, 2021:

    nit:

        const QString tr_yes{tr("Yes")}, tr_no{tr("No")}, tr_to{tr("To")}, tr_from{tr("From")},
    jonatack commented at 7:11 PM on February 23, 2021:

    Thanks, good idea. Updating s/never/Never/ too.

    jonatack commented at 7:16 PM on February 23, 2021:

    done

  8. hebasto approved
  9. hebasto commented at 7:09 PM on February 23, 2021: member

    ACK 17edc9a9c8ca90f13f81ec8dcb949a5dbb5b3d32, tested on Linux Mint 20.1 (Qt 5.12.8).

  10. jonatack force-pushed on Feb 23, 2021
  11. hebasto approved
  12. hebasto commented at 7:19 PM on February 23, 2021: member

    re-ACK 52d10ee3cf59371b54cd4c9455f6e5b68ce07682

  13. in src/qt/rpcconsole.h:74 in 52d10ee3cf outdated 
      69 | @@ -70,6 +70,8 @@ class RPCConsole: public QWidget
  70 |  
  71 |      QString tabTitle(TabTypes tab_type) const;
  72 |      QKeySequence tabShortcut(TabTypes tab_type) const;
  73 | +    const QString tr_yes{tr("Yes")}, tr_no{tr("No")}, tr_to{tr("To")}, tr_from{tr("From")},
  74 | +        tr_ban_for{tr("Ban for")}, tr_na{tr("N/A")}, tr_never{tr("Never")}, tr_unknown{tr("Unknown")};
    hebasto commented at 10:47 AM on February 24, 2021:

    Maybe introduce a namespace instead of tr_ prefix?

    jonatack commented at 1:14 PM on February 24, 2021:

    Sure. Would you provide a working example of what you're thinking of?

    hebasto commented at 2:09 PM on February 24, 2021:

    https://github.com/hebasto/gui/commit/9c7668050b872cbf2c755e3f945cc2a07d763b44

    Yeah, it is not a true namespace :)

    hebasto commented at 2:10 PM on February 24, 2021:

    If that idea is not that you like, could you, at least, make these variables private?

    jonatack commented at 2:39 PM on February 24, 2021:

    Thanks! I would not have guessed that version, so your branch saved time. LGTM, done, added co-credit.

  14. jonatack force-pushed on Feb 24, 2021
  15. hebasto approved
  16. hebasto commented at 2:43 PM on February 24, 2021: member

    re-ACK 2e2fe50ec1f3e3e4bf4b1b17d0a5351b9682601b

  17. jonatack commented at 2:50 PM on February 25, 2021: contributor

    Rebased following merge of #226, dropped "Never" from the struct as it is now only used once.

  18. jonatack force-pushed on Feb 25, 2021
  19. hebasto approved
  20. hebasto commented at 2:59 PM on February 25, 2021: member

    re-ACK 3d85e9229f074127c41eea43b4829c4d5bfc115a

  21. in src/qt/rpcconsole.cpp:1120 in 3d85e9229f outdated 
    1119 |      QString bip152_hb_settings;
1120 | -    if (stats->nodeStats.m_bip152_highbandwidth_to) bip152_hb_settings += "To";
1121 | -    if (stats->nodeStats.m_bip152_highbandwidth_from) bip152_hb_settings += (bip152_hb_settings == "" ? "From" : "/From");
1122 | -    if (bip152_hb_settings == "") bip152_hb_settings = "No";
1123 | +    if (stats->nodeStats.m_bip152_highbandwidth_to) bip152_hb_settings = ts.to;
1124 | +    if (stats->nodeStats.m_bip152_highbandwidth_from) bip152_hb_settings += (bip152_hb_settings.isEmpty() ? ts.from : QLatin1String("/") + ts.from);
    Talkless commented at 3:45 PM on February 28, 2021:

    nit: QLatin1String("/") could be QLatin1Char('/') instead. A bit simpler code path, no looping through source string (even if it's size 1) needed (peeking into Qt implementation).

    jonatack commented at 6:20 PM on February 28, 2021:

    Thanks @Talkless! Done.

  22. in src/qt/rpcconsole.h:139 in 3d85e9229f outdated 
     135 | @@ -136,6 +136,11 @@ public Q_SLOTS:
 136 |      void cmdRequest(const QString &command, const WalletModel* wallet_model);
 137 |  
 138 |  private:
 139 | +    struct TranslatedStrings {
    Talkless commented at 3:46 PM on February 28, 2021:

    Interesting technique. I guess translators will see TranslatedStrings as location, not rpcconsole.h? But I guess that doesn't matter, not much context needed?

    Talkless commented at 4:07 PM on February 28, 2021:

    I've tried with some small test project, and lupdate/linguist uses parent class as context, so no issues here.

    jonatack commented at 6:20 PM on February 28, 2021:

    @Talkless thank you for checking!

  23. Talkless commented at 3:47 PM on February 28, 2021: none

    Concept ACK

  24. RPCConsole::updateDetailWidget: convert strings to translated strings
    and in the touched lines:

- replace 2 occurrences of `== ""` with `isEmpty()`

- replace an unneeded `+=` with `=`
    0f035c12fb
  25. Hoist repeated translated strings to RPCConsole struct members
    and add missing braces to the touched conditionals.

Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
    6242beeb06
  26. jonatack force-pushed on Feb 28, 2021
  27. jonatack commented at 6:24 PM on February 28, 2021: contributor

    Updated with suggestion from @Talkless (thanks!) to replace QLatin1String("/") with QLatin1Char('/')

  28. hebasto approved
  29. hebasto commented at 6:30 PM on February 28, 2021: member

    re-ACK 6242beeb067139c01dd27c63ebcd24df5808cb15

  30. Talkless approved
  31. Talkless commented at 6:28 PM on March 1, 2021: none

    tACK 6242beeb067139c01dd27c63ebcd24df5808cb15, tested on Debian Sid with Qt 5.15.2. I see "Ban for.." translated to my native language as before, "To/From/Yes/No" are not but that's expected, as .ts files are not updated.

  32. jarolrod commented at 4:00 PM on March 7, 2021: member

    ACK 6242beeb067139c01dd27c63ebcd24df5808cb15

    Good idea! These strings are/will be super common. This allows them to be used in other places when appropriate.

  33. MarcoFalke merged this on Mar 7, 2021
  34. MarcoFalke closed this on Mar 7, 2021
  35. jonatack deleted the branch on Mar 7, 2021
  36. sidhujag referenced this in commit e23a0a1187 on Mar 8, 2021
  37. gwillen referenced this in commit d09330b758 on Jun 28, 2022
  38. bitcoin-core locked this on Aug 16, 2022
Contributors
jonatackhebastoTalklessjarolrod
Linked (view graph)
#206 Display fRelayTxes and bip152_highbandwidth_{to, from} in peer details
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