GeezerVison

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

To avoid the clang-tidy warnings we need to disable all checks. See at the top of gui/CMakeFiles.txt (that should be moved into a shared function since it already used in another place).

Thanks for you explanation. I don't get which folder needs the additional clang-tidy configuration file. My change does not add generated files in a new directory, does it?

I see no reason to hold this back. Please consider merging.

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

I don't know much about cmake and asked AI to review this. In my experience AI reviews are often mostly wrong - take this AI-review below with a mountain of salt:

The deprecation claim checks out. Qt's own docs for qt_create_translation state it's deprecated in favor of the target-based qt6_add_lupdate/qt6_add_translations (Qt docs
(https://doc.qt.io/qt-6/qtlinguist-cmake-qt-create-translation.html)). (Note: the PR title/body says QT_CREATE_TRANSLATION, not QT_TRANSLATE_TRANSLATION — I assume that's what you meant.)
qt_add_translations needs Qt ≥6.2, and this repo already requires Qt6 everywhere in CI (including the "oldest supported" ubuntu-22.04 job, which already has
qt6-l10n-tools/qt6-tools-dev-tools installed), so there's no version-compatibility issue.

But the patch drops something it doesn't replace. The old code did:
QT_CREATE_TRANSLATION(qms ${CMAKE_CURRENT_SOURCE_DIR} ${tss})
...
install(FILES ${qms} DESTINATION ${CMAKE_INSTALL_BINDIR} COMPONENT applications)
The PR removes the QT_CREATE_TRANSLATION(qms ...) call but leaves install(FILES ${qms} ...) untouched (gui/CMakeLists.txt:75 in the PR). Nothing sets qms anymore, so that install line now
installs an empty file list — the built package ships with zero .qm translation files.

This isn't cosmetic: gui/translationhandler.cpp loads translations from disk at runtime (QFile::exists(datadir/lang/...), mTranslator->load(...)), not from a Qt resource.
qt_add_translations by default embeds generated .qm files into a compiled-in Qt resource (prefix /i18n) instead of leaving them as loose files — getting standalone .qm paths back requires
explicitly passing QM_FILES_OUTPUT_VARIABLE, which the PR doesn't do.

There's also direct corroborating evidence in the repo: .github/workflows/release-windows.yml does
move build\gui*.qm win_installer\files\translations
expecting loose .qm files under build/gui/, matching the old mechanism. That workflow only runs on release tags / nightly schedule / manual dispatch — not on this PR's own CI — so this
breakage wouldn't be caught before a release.

Minor secondary note: the PR deletes the comment # TODO: passing "-no-obsolete" here breaks the translations and then passes LUPDATE_OPTIONS "-no-obsolete" anyway. That's probably fine in
practice since qt_add_translations only runs lupdate via an opt-in update_translations target (not part of the default build, unlike the old always-on macro) — but the reasoning that
flag was avoided for is no longer documented anywhere, so it could resurface if someone runs that target to refresh translations later.

Verdict: moving off QT_CREATE_TRANSLATION is the right direction, but this PR isn't a correct/complete fix — it needs to wire qms/QM_FILES_OUTPUT_VARIABLE back into the install() call and
verify where qt_add_translations actually places .qm files relative to what release-windows.yml expects, before merging.

AI review (second opinion on the earlier AI review in #8062 (comment))

I built 5d0e4c5 with -DBUILD_GUI=ON -DBUILD_TESTING=ON and Qt 6.10.2 and ran ctest: all 91 tests pass, including test-translationhandler.

The earlier review, point by point

• qms not set, so install() installs no .qm files: fixed by the latest commit (QM_FILES_OUTPUT_VARIABLE qms). cmake --install installs all 14 .qm files.
• qt_add_translations embeds the .qm files as a resource: correct in general, but QM_FILES_OUTPUT_VARIABLE turns that off, so the .qm files stay separate files in build/gui/.
• release-windows.yml expects build\gui\*.qm: the location is fine, but there is another problem, see below.
• -no-obsolete: fine now. lupdate no longer runs in a normal build, only when the update_translations target is built.

Problem: the release-windows build gets no .qm files

release-windows.yml builds only --target cppcheck-gui and then does move build\gui\*.qm .... With Qt >= 6.7 (the release uses 6.10.0), qt_add_translations creates the .qm files in a separate <project>_lrelease target. That target is part of all, but cppcheck-gui does not depend on it. Before, the .qm files were sources of cppcheck-gui.

Tested with Qt 6.10.2, after deleting build/gui/*.qm:

cmake --build build --target cppcheck-gui

• main: 14 .qm files
• this PR: 0 .qm files

This could be the same problem that made #8047 be reverted.

Adding this after the qt_add_translations(...) call fixed it for me (14 .qm files, and configuring with -Werror=dev --warn-uninitialized as in the release workflow gives no new warnings):

add_dependencies(cppcheck-gui release_translations)

I only tested this with Qt 6.10, so CI should confirm that it works with the oldest Qt too.

Other notes

• Nice side effect: on main a normal build rewrites all gui/*.ts files in the source tree (trac #14169). With this PR it does not. Maybe mention the ticket in the title.
• Minor: without SOURCE_TARGETS cppcheck-gui, Qt >= 6.7 makes update_translations scan all the targets of the project (lib, cli, test, generated files). Running it added no new strings, but the 342 .ui locations in each .ts file became paths like ../build/gui/ui_mainwindow.h, and the 31 obsolete/vanished entries were removed (expected with -no-obsolete). This only matters if someone uses that target instead of lupdate gui.pro.

Thanks for the review!

1. Should we really add -no-obsolete? Qt leaves it on purpose, to give translators a chance to use the obsolete translation for slightly changed strings. Thus, I removed the option.
2. I added add_dependencies as you suggested. Older versions of Qt did not require this.
3. I updated the readme to clarify that the update step (lupdate) has to be performed manually.

Should we really add -no-obsolete? Qt leaves it on purpose, to give translators a chance to use the obsolete translation for slightly changed strings. Thus, I removed the option.

okay I am not against removing it. I don't feel like an expert on this and you do it on purpose...

It seems to me this is finished now, I merge it.