Fix i18n in "Save" section of configuration dialog
ClosedPublic

Authored by aspotashev on Mar 20 2019, 1:37 PM.

Diff Detail

Repository
R166 Spectacle
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
aspotashev created this revision.Mar 20 2019, 1:37 PM
Restricted Application added a project: Spectacle. · View Herald TranscriptMar 20 2019, 1:37 PM
Restricted Application added a subscriber: Spectacle. · View Herald Transcript
aspotashev requested review of this revision.Mar 20 2019, 1:37 PM

+1 Confirmed to work with Ukrainian translation.

Yuri, did you test .pot extraction? (I didn't since I'm too lazy, sorry for that.)

Yuri, did you test .pot extraction? (I didn't since I'm too lazy, sorry for that.)

Yes, I tested the spectacle.pot extraction and it works as expected (all the descriptions can be extracted with their appropriate comments).

Sorry, this was caused by a change from me. Could you please explain what I did wrong so I can avoid it in the future? Also why is the duplicate context in SaveOptionsPage needed? Does the context in ExportManager.cpp not suffice?

Sorry, this was caused by a change from me. Could you please explain what I did wrong so I can avoid it in the future? Also why is the duplicate context in SaveOptionsPage needed? Does the context in ExportManager.cpp not suffice?

Now, ExportManager::filenamePlaceholders does not actually translate the strings because option.value() cannot get the translation when loaded (it has no idea that it will be translated and when it is loaded it is too late for the translations).

Alexander tricks the translation system: I18N_NOOP2() just marks the string for translation (does not translate it) then i18nc() in SaveOptionsPage.cpp actually translates it.

For translations to work, it is necessary to keep the consistency of their context, so the same context as in ExportManager.cpp was added to the strings in SaveOptionsPage.cpp.

Thank you for the explanation!

ngraham accepted this revision.Mar 20 2019, 2:52 PM

Thanks I learned something too. Go ahead and land this in the stable branch and merge to master!

This revision is now accepted and ready to land.Mar 20 2019, 2:52 PM
This revision was automatically updated to reflect the committed changes.
aacid added a subscriber: aacid.Mar 20 2019, 8:28 PM

Using a KLocalizedString (i.e. ki18nc) seems like would be easier and less fragile since you wouldn't need to duplicate the context

davidre added a comment.EditedMar 21 2019, 8:56 AM

Using a KLocalizedString (i.e. ki18nc) seems like would be easier and less fragile since you wouldn't need to duplicate the context

I read the documentation of ki18n now. This means we would have a QMap<QString, KLocalizedString>{{"...", ki18nc(...)},..} and do arg(option.key(), option.value().toString()) now, right?

aacid added a comment.Mar 21 2019, 6:30 PM

Using a KLocalizedString (i.e. ki18nc) seems like would be easier and less fragile since you wouldn't need to duplicate the context

I read the documentation of ki18n now. This means we would have a QMap<QString, KLocalizedString>{{"...", ki18nc(...)},..} and do arg(option.key(), option.value().toString()) now, right?

Yes, that sounds about right

pino added a subscriber: pino.Mar 21 2019, 6:43 PM

Sorry, this was caused by a change from me. Could you please explain what I did wrong so I can avoid it in the future? Also why is the duplicate context in SaveOptionsPage needed? Does the context in ExportManager.cpp not suffice?

Now, ExportManager::filenamePlaceholders does not actually translate the strings because option.value() cannot get the translation when loaded (it has no idea that it will be translated and when it is loaded it is too late for the translations).

It is actually the order way round: it did not work because the strings were in a static container.
The static stuff is initialized at application startup, even before main() runs. So i18n() was called when the i18n machinery was not simply set up yet, and thus just returning untranslated strings.

The bottom of the line is: never have static i18n strings, either directly or part of another static container/struct.