Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Translations: Use substitutions instead of concatenations #1114

Closed
hoffie opened this issue Feb 25, 2021 · 13 comments · Fixed by #2646
Closed

Translations: Use substitutions instead of concatenations #1114

hoffie opened this issue Feb 25, 2021 · 13 comments · Fixed by #2646

Comments

@hoffie
Copy link
Member

hoffie commented Feb 25, 2021

Describe the improvement

Some strings currently use concatenations. Example:

QString strSndCardDriverSetupTT = tr ( "Opens the driver settings. Note: " ) + APP_NAME +

To Reproduce

Could probably use something like grep -Po 'tr\s*\(.*\).*\+.*' src/* to find other places (this will still yield false positives such as HTML concatenation, which should be OK I think).

Expected behavior

This was suggested by @pljones in #1097 (comment):

Would it not be easier for a translator to have the full text with substitutions (so, QString ( tr ( "Hello, %1" ) ).arg( getName() )..., giving the context)? Here there are three pieces, one starting with a measurement unit.

Additional context
Must only be changed after 3.7.0 is completed.

@hoffie hoffie self-assigned this Feb 25, 2021
@hoffie
Copy link
Member Author

hoffie commented Feb 25, 2021

Hmm, even HTML concatenations may lead to inconsistent spacing:

"<li>" + tr ( "Enabling " ) + "<b>" + tr ( "Stereo" ) + "</b> " + tr ( " mode "

In this case, it's a double space and will most like not matter in practice as I assume it will be condensed to one as it would be in ordinary HTML. But still...

@pljones
Copy link
Collaborator

pljones commented Feb 25, 2021

Which would have to become something like...

QString ( tr ( "%1Enabling %2Stereo%3 mode%4" ) ).arg ( "<li>" ).arg ( "<b>" ).arg ( "</b>" ).arg ( "</li>" );

... I think... And the the translator sees "%1Enabling %2Stereo%3 mode%4" without the args... does that help? Hm.

Would providing tr( "Enabling <b>Stereo</b> mode" ) be easier? If more complex translations (this seems like it's not one...) are needed, then having the flexibility to adjust where the formatting occurs might be useful. But does the XML support embedded HTML?

@hoffie
Copy link
Member Author

hoffie commented Mar 28, 2021

And the the translator sees "%1Enabling %2Stereo%3 mode%4" without the args... does that help? Hm.

I don't think so. :(

Would providing tr( "Enabling <b>Stereo</b> mode" ) be easier? If more complex translations (this seems like it's not one...) are needed, then having the flexibility to adjust where the formatting occurs might be useful.

As long as we stick to simple HTML (I guess bold formatting and line breaks are the most common reason for using it?), this would be way more readable (although we would have to check translation PRs thoroughly for them not to break HTML).

But does the XML support embedded HTML?

Don't know / haven't tried.

I'm not currently working on this and I'm therefore removing myself as the assignee.

@hoffie hoffie removed their assignment Mar 28, 2021
@ann0see
Copy link
Member

ann0see commented Feb 25, 2022

@BLumia since you we’re the last person doing a full translation of the software. How many places do need to be updated in order not to have split out tr()?

I think this issue should also be documented in the style guide in CONTRIBUTING.md

@pljones
Copy link
Collaborator

pljones commented Feb 25, 2022

I think this issue should also be documented in the style guide in CONTRIBUTING.md

I think that is probably the best idea in this issue :).

@BLumia
Copy link
Contributor

BLumia commented Feb 26, 2022

this will still yield false positives such as HTML concatenation, which should be OK I think

"<li>" + tr ( "Enabling " ) + "<b>" + tr ( "Stereo" ) + "</b> " + tr ( " mode "

This should also be avoided. If using HTML markup is a must, just also include HTML tags in the source strings. Letting translators see these markup tags is not a bad thing.

e.g. tr("Enabling <b>Stereo</b> mode") is better than tr("Enabling %1 mode").args("<b>"+tr("Stereo")+"</b>") IMO. Translator will know the word Stereo is emphasized and all they need to know is keep the HTML tag in their translated string. Programmer also won't need to write a lot of args() only because of there are HTML tags in the source string.

How many places do need to be updated in order not to have split out tr()?

I already updated those I can find that have this issue, there might still be some strings that need to be corrected but I'm not sure about the amount.

I think this issue should also be documented in the style guide in CONTRIBUTING.md

Totally agree since there are some strings with this issue introduced in new commits, contributors should be aware this can be an issue when they want to add new strings to the program.

@BLumia
Copy link
Contributor

BLumia commented Feb 26, 2022

Additionally, if the place we need to display rich text is a QLabel or QMessageBox, and we are on Qt version >= 5.14, we can also use markdown :)

QLabel* label = new QLabel("test *markdown*");
label->setTextFormat(Qt::MarkdownText);
label->show();

image

@hoffie
Copy link
Member Author

hoffie commented Feb 26, 2022

Additionally, if the place we need to display rich text is a QLabel or QMessageBox, and we are on Qt version >= 5.14, we can also use markdown :)

That's certainly good to know. Currently we still support older Qt versions though.

@softins
Copy link
Member

softins commented Feb 28, 2022

this will still yield false positives such as HTML concatenation, which should be OK I think

"<li>" + tr ( "Enabling " ) + "<b>" + tr ( "Stereo" ) + "</b> " + tr ( " mode "

This should also be avoided. If using HTML markup is a must, just also include HTML tags in the source strings. Letting translators see these markup tags is not a bad thing.

e.g. tr("Enabling <b>Stereo</b> mode") is better than tr("Enabling %1 mode").args("<b>"+tr("Stereo")+"</b>") IMO. Translator will know the word Stereo is emphasized and all they need to know is keep the HTML tag in their translated string. Programmer also won't need to write a lot of args() only because of there are HTML tags in the source string.

Yes, agree 100%, and Qt Linguist fully supports this too. It recognises and highlights embedded HTML:

image

Although it doesn't appear to warn the user if the translated string differs in the HTML markup.

It might be quite an undertaking to change this project-wide though (although probably worthwhile). It would need to be done while other activity is fairly quiet and then included without delay, or rebasing could be very arduous!

@pljones
Copy link
Collaborator

pljones commented Feb 28, 2022

Although it doesn't appear to warn the user if the translated string differs in the HTML markup.

That's not necessarily bad -- if, say, there was a German translation of the qJackCtl documentation, changing the href of the link might be appropriate.

@ann0see ann0see added this to the Release 3.9.0 milestone Apr 11, 2022
@ann0see
Copy link
Member

ann0see commented Apr 11, 2022

Ok: See #2561 which added a style note related to this issue.

@ann0see
Copy link
Member

ann0see commented May 7, 2022

Since I think we have it documented, can we close this? I think

Could probably use something like grep -Po 'tr\s*(.).+.' src/ to find other places (this will still yield false positives such as HTML concatenation, which should be OK I think).

Could be run by someone.

@gilgongo gilgongo removed this from the Release 3.9.0 milestone Jun 3, 2022
@ann0see
Copy link
Member

ann0see commented Jun 5, 2022

Files to be checked:

  • Audiomixerboard.cpp: Personal Mix at:
  • clientsettingsdlg: L - x (investigate)
  • util.cpp Version:
  • sound/asio/sound.cpp No ASIO audio device driver found (investigate)

ann0see added a commit to ann0see/jamulus that referenced this issue Jun 5, 2022
ann0see added a commit to ann0see/jamulus that referenced this issue Jun 8, 2022
Fixes: jamulussoftware#1114
Co-Authored-By: Peter L Jones <pljones@users.noreply.github.com>
Co-Authored-By: Gary Wang <wzc782970009@gmail.com>
ann0see added a commit to ann0see/jamulus that referenced this issue Jun 8, 2022
Fixes: jamulussoftware#1114
Co-Authored-By: Peter L Jones <pljones@users.noreply.github.com>
Co-Authored-By: Gary Wang <wzc782970009@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants