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 #2646

Merged
merged 1 commit into from
Jun 14, 2022

Conversation

ann0see
Copy link
Member

@ann0see ann0see commented Jun 5, 2022

Short description of changes

Remove last concatenations which might benefit from being translatable. I couldn't find anything which is worth changing anymore.

CHANGELOG: Internal: Improved translator experience by using substitutions instead of concatenations
Context: Fixes an issue?

Fixes: #1114

Does this change need documentation? What needs to be documented and how?
No. It is already documented in the CONTRIBUTING file

Status of this Pull Request

Ready to be merged.

What is missing until this pull request can be merged?

Nothing. Just approvals

Checklist

  • I've verified that this Pull Request follows the general code principles
  • I tested my code and it does what I want
  • My code follows the style guide
  • I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
  • I've filled all the content above

@ann0see ann0see added the refactoring Non-behavioural changes, Code cleanup label Jun 5, 2022
@ann0see ann0see added this to the Release 3.9.0 milestone Jun 5, 2022
@ann0see ann0see requested review from hoffie and BLumia June 5, 2022 15:02
@@ -1183,7 +1183,7 @@ void CAudioMixerBoard::UpdateTitle()
QString strEscServerName = strServerName;
strEscServerName.replace ( "&", "&&" );

setTitle ( strTitlePrefix + tr ( "Personal Mix at: " ) + strEscServerName );
setTitle ( strTitlePrefix + tr ( "Personal Mix at: %1" ).arg ( strEscServerName ) );
Copy link
Collaborator

@pljones pljones Jun 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why isn't strTitlePrefix a substitution, too? And, indeed, why isn't line 1176 amended to use substitution?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would look a bit strange:
tr ( "%1Personal Mix at: %2") (note the missing space)

The RECORDING ACTIVE can be changed of course

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would look a bit strange:

That's not really much of an argument. And the change at 1176 also wasn't what I meant.

Maybe the title of the PR is wrong and this isn't about using substitutions instead of concatenations but something else?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give a suggestion on what should be done? This PR is to make translations easier – as noted in the linked issue. If a change is not worth it – or even the whole PR, we can close the issue and PR

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like you said:

Suggested change
setTitle ( strTitlePrefix + tr ( "Personal Mix at: %1" ).arg ( strEscServerName ) );
setTitle ( tr ( "%1Personal Mix at: %2" ).arg ( strTitlePrefix ).arg ( strEscServerName ) );

But only if using substitutions instead of concatenations is the goal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I don’t like this at all @BLumia @hoffie what do you think?

src/util.cpp Outdated Show resolved Hide resolved
src/audiomixerboard.cpp Outdated Show resolved Hide resolved
Fixes: jamulussoftware#1114
Co-Authored-By: Peter L Jones <pljones@users.noreply.github.com>
Co-Authored-By: Gary Wang <wzc782970009@gmail.com>
@ann0see
Copy link
Member Author

ann0see commented Jun 14, 2022

@pljones ready for merge?

@pljones
Copy link
Collaborator

pljones commented Jun 14, 2022

Looks like it's waiting on @hoffie - I think I've approved it already. (The GitHub UI can be really opaque...)

@ann0see ann0see merged commit 909ac4a into jamulussoftware:master Jun 14, 2022
@ann0see ann0see deleted the patch/useSubst branch June 14, 2022 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Non-behavioural changes, Code cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Translations: Use substitutions instead of concatenations
4 participants