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

Library menu: change "Engine DJ Prime" to "Engine DJ" #14248

Open
wants to merge 1 commit into
base: 2.5
Choose a base branch
from

Conversation

audministrator
Copy link

Changed Engine Prime to Endgine DJ

@github-actions github-actions bot added the ui label Jan 27, 2025
@ronso0 ronso0 changed the title Changed comment "Engine DJ Prime" to "Engine DJ" Library menu: change "Engine DJ Prime" to "Engine DJ" Jan 27, 2025
Comment on lines +159 to +160
QString exportTitle = tr("E&xport Library to Engine DJ");
QString exportText = tr("Export the library to the Engine DJ format");
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a comment for translators that 'Engine DJ' is a brand / name and should not be translated.
The syntax is //: hint text and that goes one the line above the tr strings, so here it'd be
//: "Engine DJ" must not be translated

@ronso0
Copy link
Member

ronso0 commented Jan 27, 2025

If this change is okay (@mr-smidge ?), there are other user-visible strings that need to be changed for consistency (and maybe also code comments?).
See https://github.com/search?q=repo%3Amixxxdj%2Fmixxx%20%22engine+prime%22&type=code

@audministrator
Copy link
Author

Corected labels for concistency

@audministrator audministrator marked this pull request as ready for review January 27, 2025 20:49
@ronso0
Copy link
Member

ronso0 commented Jan 27, 2025

Hmmno.. did you forget to push?

@mr-smidge
Copy link
Contributor

This change looks good 👍 - Denon now use "Engine DJ" as the all-encompassing term, covering things like Engine OS (the firmware that runs on hardware players) and Engine Desktop (the software preparation application).

@audministrator
Copy link
Author

Hi Guys,

To make it not that confusing .... I did not use ENGINE DJ but eventually changed it to ENGINE OS ?
Based on the input from @Ronso 's instruction on how to get started in GitHub

image

But now that I know how to make a PR in GitHub.
I can rename it again to ENGINE DJ if you guys feel this is a better description ...
No problem on my side to do it just let me know ...

@ronso0
Copy link
Member

ronso0 commented Jan 28, 2025

You make it confusing : )
The current changes(Engine Prime -> Engine DJ) have been approved by @mr-smidge.

All that should be done now is add the translator hint, and update the other occurences of "Engine Prime", too.
I.e. only the tr strings in src/ directory and maybe the comments. But NOT CMakeLists.txt or changelog.md !

@audministrator
Copy link
Author

Don't mind how it turns out at the end ...
Whether it is ENGINE DJ or ENGINE OS, as long as it is not ENGINE PRIME ;-)

@mr-smidge
Copy link
Contributor

Don't mind how it turns out at the end ... Whether it is ENGINE DJ or ENGINE OS, as long as it is not ENGINE PRIME ;-)

My suggestion would be to stick with "Engine DJ" 😄.

Agree with @ronso0 to do the translator hint.

@ronso0
Copy link
Member

ronso0 commented Jan 28, 2025

Okay, so what about the other changes #14248 (comment) @audministrator ?
Currently only WMainMenubar has been changed. You mentioned you "Corected labels for concistency", do you have any local changes you didn't push, yet?

@audministrator
Copy link
Author

@mr-smidge "My suggestion would be to stick with "Engine DJ"

Sure no problem to modify it all again to Engine DJ. It will be a good practivce for me to actively use GitHuib :-)

@Ronso Can you tell me if I can make the change in the branch again ? Or should I start all over creating a new branch ?
Next this will push all changes made so far...

@audministrator
Copy link
Author

audministrator commented Jan 29, 2025

@Ronso

Changed all labels and comments in the code from "Engine OS" to "Engine DJ" as preferred by mr-smidge.

How can I apply the changes in current branch ? should I do something or are the changes automatically available in the current PR ?

image

@ronso0
Copy link
Member

ronso0 commented Jan 29, 2025

This PR conatins only changes in WMainMenubar, that's why I asked if you have local uncommited changes.
If you expect more changes, something went wrong.

The merge was not necessary btw, we don't need up-to-date 2.5 for this tr changes.

@ronso0 ronso0 marked this pull request as draft January 30, 2025 22:21
@ronso0
Copy link
Member

ronso0 commented Jan 30, 2025

Looks good, thank you!

Please also add the translator hint everywhere you touched a tr string, see https://github.com/mixxxdj/mixxx/pull/14248/files#r1931116776

When you're done I'll squash all commits.

@audministrator
Copy link
Author

Added //: "Engine DJ" must not be translated in all files and commit is done

@ronso0
Copy link
Member

ronso0 commented Jan 31, 2025

Thank you, but as I wrote: the translator hint only needs to be added where tr strings are touched (tr("some text")).
qDebug/qWarning and code comments are not tranlsated.

I'll fixup, squash all comits and force-push later on.

@audministrator
Copy link
Author

Sorry learning every day ;-)
Now I see what you meant with 'tr' string ...

Thanks

#ifdef __ENGINEPRIME__
QString exportTitle = tr("E&xport Library to Engine Prime");
QString exportText = tr("Export the library to the Engine Prime format");
QString exportTitle = tr("E&xport Library to Engine DJ");
Copy link
Member

Choose a reason for hiding this comment

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

Is the & expected there?

Copy link
Member

Choose a reason for hiding this comment

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

& marks the letter that is used for the mnemonic / keyboard shortcut.
It's been there before, so why remove it?

Though we may add a translator hint for those, too.
Will do that once I clean up this branch in the next days.

Copy link
Member

Choose a reason for hiding this comment

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

Na, I think I won't. let's add that when we notice issues with that (and we haven't seen any until now AFAIR)

@ronso0 ronso0 marked this pull request as ready for review February 6, 2025 12:58
@ronso0
Copy link
Member

ronso0 commented Feb 6, 2025

Squashed and cleaned up.
Should be ready, please check!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improvement libdjinterop : Change menu option name (more generic / less confusing)
4 participants