Fix Step infobrowser and examples localization
ClosedPublic

Authored by yurchor on Nov 19 2019, 3:36 PM.

Details

Summary

Currently (version 19.12), all object info and examples localizations are useless. This patch fixes current way of extracting and using Step translations.

Test Plan
  1. Extraction works. However, I'm uncertain if it is worth to have separate catalogs for object infos and examples (please, give an advice).
  1. Translation (when put into the common /po dir) compiles and works. Translations are installed into the objinfo/<locale> dir to avoid conflicts with current lag packs. It is tested that Step can display the compiled localized HTML files.

Screenshot with translated tutorial and infobrowser (Ukrainian, clean install):

Diff Detail

Repository
R341 Step
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18955
Build 18973: arc lint + arc unit
yurchor created this revision.Nov 19 2019, 3:36 PM
Restricted Application added a project: KDE Edu. · View Herald TranscriptNov 19 2019, 3:36 PM
Restricted Application added a subscriber: kde-edu. · View Herald Transcript
yurchor requested review of this revision.Nov 19 2019, 3:36 PM
cordlandwehr added inline comments.
step/infobrowser.cc
158

In my opinion, I would prefer to stick with the resource-based location for the un-translated HTML files. This makes the development setup (slightly) simpler because no XDG_DATA_DIRS variable must be set and the default handling is easier to break. Actually, the default HTML files would be a fallback even if the environment variables are set incorrectly.

yurchor marked an inline comment as done.Nov 19 2019, 5:59 PM

Ok. Thanks for your comment.

yurchor updated this revision to Diff 70010.Nov 19 2019, 6:01 PM

Use the resource-based location for the untranslated HTML files

yurchor updated this revision to Diff 70015.Nov 19 2019, 7:51 PM

Remove remnants of objinfo HTML installation for English (not needed as we use internal resources)

cordlandwehr accepted this revision.Nov 19 2019, 8:44 PM
This revision is now accepted and ready to land.Nov 19 2019, 8:44 PM
aacid added a subscriber: aacid.Nov 19 2019, 9:22 PM
aacid added inline comments.
step/infobrowser.cc
156–159

I think using KLocalizedString::languages() here makes more sense, i think QLocale uses a bit of a different naming system for "complex" stuff like pt_BR

aacid added a comment.Nov 19 2019, 9:26 PM

How does the STEP_BUILD_TRANSLATIONS thing work? Does one have to enable it manually?

I see some code like

if(CURRENT_LANG)
    set(STEP_BUILD_TRANSLATIONS 1)
endif(CURRENT_LANG)

Does it mean that you can only build one language at a time?

yurchor marked an inline comment as done.Nov 19 2019, 9:44 PM

Ok. Thanks.

yurchor updated this revision to Diff 70023.Nov 19 2019, 9:45 PM

Use KLocalizedString::languages()

How does the STEP_BUILD_TRANSLATIONS thing work? Does one have to enable it manually?

Yes. That's written in the Phabricator review request description. You should explicitly add -DSTEP_BUILD_TRANSLATIONS=true to the cmake command to process the translations. I'm not a big CMake expert, but it should also work without these "if"s because of "foreach".

I see some code like

if(CURRENT_LANG)
    set(STEP_BUILD_TRANSLATIONS 1)
endif(CURRENT_LANG)

Does it mean that you can only build one language at a time?

No. I tested it to build for all the languages that have corresponding files in /po (e.g. uk and de) . The code you've mentioned has actually been *removed" from CMakeLists.txt.

yurchor updated this revision to Diff 70032.Nov 19 2019, 10:39 PM

Fix tutorials and example iterations

yurchor updated this revision to Diff 70034.Nov 19 2019, 10:45 PM

Remove extra variable

yurchor updated this revision to Diff 70037.Nov 20 2019, 6:48 AM

Fix CMake target names to build the full set of files

yurchor edited the test plan for this revision. (Show Details)Nov 20 2019, 6:58 AM

Does this work cmake wise if there's no po/ folder?

step/infobrowser.cc
156–159

can i convince you to iterate over KLocalizedString::languages instead of using the first one only?

This way we will try all the langauges you may have selected and then only fallback to english if none was found

pino added a subscriber: pino.Nov 20 2019, 10:48 PM
pino added inline comments.
step/infobrowser.cc
156–159

can i convince you to iterate over KLocalizedString::languages instead of using the first one only?

or even better, use KLocalizedString::localizedFilePath() to locate the translated html file?
note that it requires to slightly rearragne the installation directory of the localized resources: instead of objinfo/<lang>/, it must be objinfo/l10n/<lang>/

Does this work cmake wise if there's no po/ folder?

Yes (tested).

yurchor marked an inline comment as done.Nov 21 2019, 7:20 AM

Implemented.

I do not know how to implement Pino's proposal. Can I beg you for an example and just a few words on why it is better? Thanks in advance.

yurchor updated this revision to Diff 70087.Nov 21 2019, 7:21 AM

Use iteration for languages

So far, the only usage of KLocalizedString::localizedFilePath() I could find is KMyMoney.

@pino Is it acceptable not to use this rare API here?

It's rare because people don't use it.

QString KLocalizedString::localizedFilePath ( const QString & filePath )
static

Find a path to the localized file for the given original path.

It means you don't need to iterate at all: you just say "give me the localized file path", one line instead of the cycle on lines 156-168.

But as it was said, the structure of the translated files needs to be changed (objinfo/<lang> -> objinfo/l10n/<lang>), which means that the cmake file should be adapted to follow that structure.

yurchor updated this revision to Diff 70272.Nov 24 2019, 8:14 PM

Use KLocalizedString::localizedFilePath(). As it is incompatible with internal resources, install English default HTML files and remove resource embedding again (sorry, Andreas).

yurchor marked an inline comment as done.Nov 24 2019, 8:52 PM

@ltoscano BTW, should extractxml script be ported to Python 3 in this Phabricator review to work with our translation system? Can it be done later?

No need to change it here - we have definitely more work to do to switch every script to Python 3.

Sorry for disturbing, but

  1. Will there be further reviews?
  2. Can this be pushed to 19.12 branch to enable translations there?

Thanks in advance for your answers.

Hi, the patch looks fine for me. Thanks for fixing this!

The release-team@ list is the place where to raise blockers.

This revision was automatically updated to reflect the committed changes.