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

Fix note name translations in status bar #22427

Conversation

bluebear94
Copy link
Contributor

The note name translations belong to the EditPitchBase context, not the engraving context. Fix the context argument to muse::mtrc call accordingly.

Resolves: (Discord message)

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable) – is this applicable?

The note name translations belong to the `EditPitchBase` context,
not the `engraving` context. Fix the context argument to
`muse::mtrc` call accordingly.
@Jojo-Schmitz
Copy link
Contributor

Should make it into 4.3.0 too

@JLWALTENER
Copy link

@bluebear94 ,
Artefact tested

Note names in status bar 2

The Note name is fixed, that nice,
Unfortunately the Octave number is not translated, for reference the Octave number follow this

NumerotationOctaves

The Octave number is translated in French in the same file you have updated they are in the Octave 9 to -1

Thanks for your effort

@Jojo-Schmitz
Copy link
Contributor

That's not a new issue though, been that way ever since, as far as I can tell

@JLWALTENER
Copy link

@Jojo-Schmitz ,
Exactely, I remember discussion on this since 0.9.3 with Nicolas...
It would be nice to be solved.

@Jojo-Schmitz
Copy link
Contributor

Yes, it would. See also #19154

@JLWALTENER
Copy link

@Jojo-Schmitz ,
Excellent, I have not find this issue, as the Strings label is not positionned.
So your issue and mine is strictely the same, regarding language.

@cbjeukendrup
Copy link
Contributor

cbjeukendrup commented Apr 17, 2024

This is not a good solution, for two reasons:

  • The engraving module should not depend on things from other modules, and especially not on random UI classes. And thus also not on translation contexts that belong to other modules.
  • (Passing "computed" things to the translation functions is perhaps not the safest idea, unless it's really certain that the computation will always produce something for which we have a suitable translatable string.) Putting this between parentheses because apparently it was already like this.

The better solution would be to move these strings to the engraving module and make the translation context engraving/notenames, and then reuse these strings in EditPitchBase, instead of basically the other way around. The notation module is allowed to depend on the engraving module.

(If you wonder, why would we do so difficult about this: in general, the purpose of "architecture" is making it easier to introduce features and making it less easy to introduce bugs. Small things like this contribute to that.)

@bluebear94
Copy link
Contributor Author

bluebear94 commented Apr 17, 2024

Now that I’m looking at the translation files again, it turns out that I should have used the global context. This is probably more appropriate to use than the EditPitchBase context, though we might want to move these strings to an engraving/notenames context as @cbjeukendrup mentioned. Currently, these strings are used only in notenames.qml and AmbitusSettings.qml.

`global` was the correct context to use, not `EditPitchBase`.
Copy link
Contributor

@cbjeukendrup cbjeukendrup left a comment

Choose a reason for hiding this comment

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

I'm happy with this version for now!

@cbjeukendrup
Copy link
Contributor

Resolves: (Discord message)

Dropping a screenshot of that message here for quick access:
Scherm­afbeelding 2024-04-17 om 04 47 16
(Note that for this PR the expected behaviour is "La4" for French, instead of "La3"; see above: #22427 (comment))

@JLWALTENER
Copy link

@cbjeukendrup ,
This is the reverse, the correct numbering is La3 for French instead of A4 for English.

@JLWALTENER
Copy link

Tested the new artefact, the result is the same as the first one.
So there is 2 bugs

  1. Note names in the status bar was not translated, Fixed by this PR
  2. Octave numbering in the status bar not translated, bug still exist

The correct translation of octave number could be found in Transifex, see the screenshot below

Octave numbers in Transifex

Could it be possible to retrieve the translated numbers from those strings ?

@Jojo-Schmitz
Copy link
Contributor

That is a different issue, see further up and can't get solved with this PR but requires some code logic change.

@JLWALTENER
Copy link

@Jojo-Schmitz , Understood, but these 2 bugs are related, we cannot hve in the status bar le note name translated and the octave numbering not translated, otherwise we will have the same situation as MS v2 and 3.
I see no inconvenient to have 2 PRs, one to fix the note name translation, this one, and a second to fix the octave numbering, but merged at the same time.

@Jojo-Schmitz
Copy link
Contributor

This PR and issue is about a recent regression (4.2.1 vs 4.3.0), the other is about an improvement, or maybe a bug, but a rather old one, existing sine Mu2. Yes. that should get fixed, eventually (and yes, rather sooner than later), but it hasn't got anything to do with the PR/issue at hand here.

@JLWALTENER
Copy link

For reference, the discussion in the French forum
https://musescore.org/fr/node/362519

@bluebear94
Copy link
Contributor Author

If we want to localize octave indications, then we’ll need a translation for each pitch class in each octave. This is what we’re doing with the EditPitchBase translations, except that that doesn’t cover double-sharp and double-flat notes or enharmonic equivalents.

Eventually, we might want to unify note name translations in the EditPitchBase context with a future engraving/notenames context.

@JLWALTENER
Copy link

@bluebear94 ,
The double sharp and flat are correctely displayed in the status bar.
Curiously, the strings tuning are correctely translated, see below

Strings tuning Fr

@Jojo-Schmitz
Copy link
Contributor

They come from an entirely different place. In the status bar they are constructed by concatenating a (translatable) notename and a calculated (and untranslatable) octave number

@JLWALTENER
Copy link

It's that I have understand.
It Is necessary I open a Feature request for this long standing need ?

@Jojo-Schmitz
Copy link
Contributor

I believe #19154 covers that, doesn't it?

@JLWALTENER
Copy link

Ok, that's fine to me.

@zacjansheski
Copy link
Contributor

Tested on MacOS 14, Windows 11, Ubuntu 22.04.3. Approved

@RomanPudashkin RomanPudashkin merged commit 1b4dc75 into musescore:master Apr 19, 2024
11 checks passed
@Jojo-Schmitz
Copy link
Contributor

This one needs to go to 4.3.0 too

@Jojo-Schmitz
Copy link
Contributor

See #22481

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

Successfully merging this pull request may close these issues.

None yet

6 participants