will send another review with the documentation

A couple of things I am not sure about

• defaults to stable translations, I'd say it should default to trunk. e.g. playground and kdereview don't even have stable
• considering one has to enable this feature in the cmakecache I think it should be made a dependency of target all. having to manually enable the feature and then also manually run fetch-translations seems a bit meh, at least on first run, on subsequent runs with $build/po/ present I'd agree with it not fetching everything again.

Lastly, you need to retrigger cmake after the fetchpo.rb script was run. ki18n_install uses a GLOB to find all languages, but that GLOB is only refreshed on cmake runs, so the first time it GLOBS there is nothing and once fetch-translations was run the user needs to manually run cmake again to make the GLOB actually find something. So what happens is:

git clone kde:ksysguard
mkdir ksysguard/build
cd ksysguard/build
cmake .. -DKDE_L10N...etc
make fetch-translations
cmake .. # excess cmake call necessary to actually be able to build the now fetched translations
make
make install

It's also pretty bad because ki18n doesn't actually tell you about the problem but simply doesn't build the translations and then you have people like me going ❓❓😱❓❓

See https://phabricator.kde.org/D5167, with this all mentioned issues should be fixed.

See the workaround I had to introduce in 287. Before doing any changes, I'd like some thoughts on how should we deal with projects that use Qt translations system.

why find_package(KF5I18n) ?

In D5143#97840, @aacid wrote:

why find_package(KF5I18n) ?

Because ki18n_install(po)

That creates a dependency for ki18n for frameworks that only use .ts files, no?

In D5143#97847, @aacid wrote:

That creates a dependency for ki18n for frameworks that only use .ts files, no?

It's not really a dependency. It just won't do much over there. AFAIK.

FWIW, I thought this for KI18n framework initially, maybe there's a way to better support this for the ts case, I can give it some thought.

The way I understand it the use case here is during development, so I am not sure the ki18n circular dep is particularly concerning. Avoiding it seems fairly difficult as find_package needs to happen at configure-time, fetch-translations at build-time, and so we only actually know whether or not it is ts-only at build-time (when we have downloaded the translations).

That being said appropximations that come to mind:

• grep all Messages.sh to check if there are any pots that do not end in _qt.pot. as a matter of policy we seem to use that for all qt-only pots
• grep all Messages.sh to check if there are lines without $EXTRACT_TR_STRINGS (the extractor for qt-only)

But really, I am not sure this is worth the effort. What might be more useful/time efficient is introducing a cmake variable for qt-only translated projects in the main CMakeLists.txt. So you can opt-out of ki18n on a per-project basis.

In D5143#97847, @aacid wrote:

That creates a dependency for ki18n for frameworks that only use .ts files, no?

One thing we can do is remove the ki18n_install step from here and move it to each project like this, much like we're doing in KF now:
https://phabricator.kde.org/D5197

If the project uses ts instead of po, then we'll have to do something similar there on the project side. I wanted to be able to save modifying all projects, but maybe it's what we want.

In D5143#97985, @apol wrote:

In D5143#97847, @aacid wrote:

That creates a dependency for ki18n for frameworks that only use .ts files, no?

One thing we can do is remove the ki18n_install step from here and move it to each project like this, much like we're doing in KF now:
https://phabricator.kde.org/D5197

That'd be a reasonable way to resolve it indeed.

Oh! OTOH, given fetch-translations pulls it into bin_dir while normally we'd have it in src_dir this may well be awkward in implementation. e.g. https://phabricator.kde.org/D5197 effectively ought to be

ki18n_install("${CMAKE_SOURCE_DIR}/po")
ki18n_install("${CMAKE_BINARY_DIR}/po")

this gets funnier with frameworks as they have a conditional wrapping the src_dir version

if (IS_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/po")
    ki18n_install(po)
    kdoctools_install(po)
endif()
ki18n_install(${CMAKE_BINARY_DIR}/po)

While the manual call is more explicit and thus obvious it also makes a reader who doesn't know about fetch-translations go "WHAT?!?" so I'd actually argue that the bin_dir call needs a comment explaining why double-ki18n_install calls are a thing... in every single project that gets the ki18n_install call.
Maybe we can live it, but personally I find it a bit egh. I'd find it nicer if ki18n_install simply finds the right po dir. If src_dir has one, use that, if bin_dir has one, use that, if neither has one give up (specifying an absolute path would bypass this of course).

Food for thought.

This only works if you used kde:okular but not git://anongit.kde.org/okular.git, no?

Addressed on the next iteration.

Ping?

needs a version bump in the docs now 👅

beyond that LGTM

Fixed.
Can I please have a ship it? 😿 we can maybe polish it over time