Qt: removes html tags from tr calls #13410

marcoagner commented at 12:25 PM on June 7, 2018: contributor

Far from glamorous but somewhat necessary, I think. :) I'm confident I got every offending tr call but let me know if you see something I've missed. Also, I preferred to keep important strings without divisions (e.g. tr("bla") + "" + tr("bla")) since dividing entire phrases may easily confuse translators and lose context in many languages; so take this into consideration for nits on style.

Obs.: The comments starting here is what brought me to it and I believe the final objective could be to ban html tags from translations as a whole (from just adding a check on update-translations.py to, who knows, add a test to fail if some tr() with html tag(s) is found in case there's a clean way to do it) but that's another thing for now.

fanquake added the label GUI on Jun 7, 2018

in src/qt/bitcoingui.cpp:1150 in 90f63c9fba outdated

1146 | @@ -1147,7 +1147,8 @@ void BitcoinGUI::updateProxyIcon()
1147 |          if (labelProxyIcon->pixmap() == 0) {
1148 |              QString ip_port_q = QString::fromStdString(ip_port);
1149 |              labelProxyIcon->setPixmap(platformStyle->SingleColorIcon(":/icons/proxy").pixmap(STATUSBAR_ICONSIZE, STATUSBAR_ICONSIZE));
1150 | -            labelProxyIcon->setToolTip(tr("Proxy is <b>enabled</b>: %1").arg(ip_port_q));
1151 | +            labelProxyIcon->setToolTip(tr("Proxy is %1enabled%2: %3")

MarcoFalke commented at 1:56 PM on June 7, 2018:

No need to translate %3?

in src/qt/askpassphrasedialog.cpp:48 in 90f63c9fba outdated

  42 | @@ -43,10 +43,16 @@ AskPassphraseDialog::AskPassphraseDialog(Mode _mode, QWidget *parent) :
  43 |      switch(mode)
  44 |      {
  45 |          case Encrypt: // Ask passphrase x2
  46 | -            ui->warningLabel->setText(tr("Enter the new passphrase to the wallet.<br/>Please use a passphrase of <b>ten or more random characters</b>, or <b>eight or more words</b>."));
  47 | +            {
  48 | +            QString text = tr("Enter the new passphrase to the wallet.");
  49 | +            text.append(tr("%1Please, use a passphrase of %2ten or more random characters%3, or %2eight or more words%3.")

MarcoFalke commented at 1:57 PM on June 7, 2018:

No need to translate %1

in src/qt/askpassphrasedialog.cpp:46 in 3494a8876b outdated

  42 | @@ -43,10 +43,15 @@ AskPassphraseDialog::AskPassphraseDialog(Mode _mode, QWidget *parent) :
  43 |      switch(mode)
  44 |      {
  45 |          case Encrypt: // Ask passphrase x2
  46 | -            ui->warningLabel->setText(tr("Enter the new passphrase to the wallet.<br/>Please use a passphrase of <b>ten or more random characters</b>, or <b>eight or more words</b>."));
  47 | +            {

practicalswift commented at 5:10 PM on June 7, 2018:

nit: Why the added block { … }?

marcoagner commented at 5:41 PM on June 7, 2018:

Recommenting so the edits do not clutter it (specially because I switched "declaration" by "initialization", oops). As you can see here: "Because transfer of control is not permitted to enter the scope of a variable, if a declaration statement is encountered inside the statement, it has to be scoped in its own compound statement:". So the alternatives I know are this or declaring the variable before the case and I don't have a preference here; so if this is ugly/confusing, it may be better to always have it declared before the case label.

Empact commented at 5:30 AM on June 8, 2018:

nit: Could do this inline, ala line 120

marcoagner commented at 8:21 AM on June 8, 2018:

I agree but the line was very convoluted like tr("Enter the new passphrase.") + + tr("Please, use......").arg("", ""); in case of avoiding leave the unnecessary  's placeholder (%1) for translators. Tried this as a middle ground.

marcoagner commented at 8:35 AM on June 8, 2018:

Meh... thinking it through, there's not enough reason to force this extra block pattern. Much better just breaking the line and using + operator, I think. Thx!

in src/qt/askpassphrasedialog.cpp:48 in 3494a8876b outdated

  42 | @@ -43,10 +43,15 @@ AskPassphraseDialog::AskPassphraseDialog(Mode _mode, QWidget *parent) :
  43 |      switch(mode)
  44 |      {
  45 |          case Encrypt: // Ask passphrase x2
  46 | -            ui->warningLabel->setText(tr("Enter the new passphrase to the wallet.<br/>Please use a passphrase of <b>ten or more random characters</b>, or <b>eight or more words</b>."));
  47 | +            {
  48 | +            QString text = tr("Enter the new passphrase to the wallet.");
  49 | +            text.append("<br/>" + tr("Please, use a passphrase of %2ten or more random characters%3, or %2eight or more words%3.")

Empact commented at 5:29 AM on June 8, 2018:

nit: %1, %2 would be more intuitive given there is no %1

Empact commented at 5:33 AM on June 8, 2018: member

Could you include an explanation as to why the html is being excluded in your commit message, for the historical record?

marcoagner commented at 8:15 AM on June 8, 2018: contributor

Thank you, @Empact. Updated.

qt: removes html tags from tr() calls

Removing HTML tags from tr() serves the purpose to facilitate
translators work by making them use placeholders like qt's "%1" instead
of possibly more convoluted HTML tags (e.g. a span tag with styles) and,
more importantly, to avoid translators misbehaviors like using HTML tags
and styles to pass verifications on update-address.py in order to render
Bitcoin addresses and whatnot in the GUI.

875847b366

jonasschnelli commented at 10:00 AM on June 8, 2018: contributor

I tend to NACK here since %1word seems less understandable for translators then word.

marcoagner commented at 10:12 AM on June 8, 2018: contributor

Yes, that makes sense, since I'm not removing any tag like  on this. I suppose this PR only makes sense if the priority is to ban HTML tags from tr() calls. And, unfortunately, as far as I know Qt has no better-looking built-in way to do this without splitting phrases for every style tag; which would probably make translations even trickier for many languages.

laanwj commented at 12:04 PM on June 8, 2018: member

NACK. I agree with the premise that using HTML in translation messages should be avoided if possible, However I disagree with this specific solution: %1...%2 is not clearer than using HTML, and more error-prone.

promag commented at 12:12 PM on June 8, 2018: member

@marcoagner are you aware of any project where your approach is used? or if there is a discussion of the pros and cons elsewhere?

I agree with @jonasschnelli though so NACK..

marcoagner commented at 12:45 PM on June 8, 2018: contributor

Given your points, I don't think I have a strong case for now with this specific approach. Thank you all for the time and thoughtful inputs.

marcoagner closed this on Jun 8, 2018

marcoagner deleted the branch on Jun 8, 2018

MarcoFalke commented at 1:54 PM on June 8, 2018: member

@marcoagner You could probably whitelist  if you wanted to reject html tags?

MarcoFalke locked this on Sep 8, 2021