Adds an example for setting the metadata of an application using KAboutData
together with KI18n and KDBusAddons.
Extends and improves notes about when and why to set the organizationDomain
and the desktopFileName.
Details
- Reviewers
aacid mpyne ltoscano stikonas - Group Reviewers
Frameworks - Commits
- R244:f01e8b41f622: API dox: more info about KAboutData's orgDomain/desktopFileName properties
Diff Detail
- Repository
- R244 KCoreAddons
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
src/lib/kaboutdata.cpp | ||
---|---|---|
689 | I would add something like | |
src/lib/kaboutdata.h | ||
314 | The QApplication instance should happen before this line, not just before setApplicationData (that's what Albert was trying to explain). |
src/lib/kaboutdata.h | ||
---|---|---|
314 | Why should it happen before? (sorry for being stubborn and curious here, I want to make sure to understand all code, to then also write proper warnings in the dox when needed, so people know they should not create it before). The thing I understood Albert to point to yesterday was KMainWindow auto-filling the translator info in the application metadata (see KMainWindowPrivate::init(...), https://cgit.kde.org/kxmlgui.git/tree/src/kmainwindow.cpp#n239). But we also found that this actually has nothing to do with the Q*App instance vs. KAboutData instance (besides KMainWindow expecting the QApp instance to exist and KApplicationData::setAppData() having been called). |
src/lib/kaboutdata.h | ||
---|---|---|
314 | To put it straight: I don't know (and I can't investigate properly now). But if you create the KAboutData instance before the call to QApplication, part of the "About <foo>" dialog is untranslated. |
src/lib/kaboutdata.h | ||
---|---|---|
314 | Bah, forgot to write: That TODO would only be really needed for the KAboutData::setApplicationData(aboutData); as that is something that changed compared to kdelibs4, where setting the appdata before the qapp creation was fine. But in KF5 it now longer is. But the actual KAboutData itself should be fine without. |
src/lib/kaboutdata.h | ||
---|---|---|
314 | Update from irc: both @ltoscano and me could reproduce by patching kpartloader or okteta that with some current KF (e.g. 5.32) the i18n calls done as part of the KAboutData constructor call seem to fail when done before the QApplication instance is created (yes, KLocalizedString::setApplicationDomain() always called before the first i18n() call). Given that the requirement of an qapp instance is nowhere documented in https://api.kde.org/frameworks/ki18n/html/prg_guide.html the question now is if that is a regression or simply never has worked. More on that another day though. |
src/lib/kaboutdata.h | ||
---|---|---|
314 | Aha! i18n() calls before the creation of QApplication instance currently fail (or rather: only return the untranslated string) for this reason: That is why before QApplication app(c, v); any calls to i18n will return the untranslated version, because internally "C" is set as value for LC_MESSAGES category, ignoring whatever the respective environment variables contain. Only after qapp sets "" as locale name for the LC_MESSAGES category via the mentioned call, gettext() will fall back to get the locale name from the env var, with the "LANGUAGE" var being the first taken into account, which is also the one ki18n uses to pass the locale name for which a catalog has been found. See also No instant proposal myself how this could/should be fixed best. Possibly it should be simply only documented, that code either has to delay any i18n calls until QApplication instance is created. And if one really needs to use i18n() calls without a QApp instance, to call setlocale(LC_ALL, ""); one itself, and then restore any previous value perhaps even, just to stay clean. So tasks seem to be:
First though: device-less leisure time :) |
- update example to not do i18n calls before qapplication instance
- improve KF6 TODO comments
src/lib/kaboutdata.cpp | ||
---|---|---|
689 | Why would you also add a comment here? Would you think the comments at the places which should be changed are not enough? But I propose to leave the actual implementation more to whoever changes it at KF6 times, as it is hard to predict what other requirements/views will be at that time :) |
src/lib/kaboutdata.cpp | ||
---|---|---|
689 | I think that any extra unneeded step is only prone to produce issues. |
src/lib/kaboutdata.cpp | ||
---|---|---|
689 | Isn't the KF6 comment added in KAboutData::desktopFileName() not already doing that, more or less? Not sure what you exactly ask for. |
src/lib/kaboutdata.cpp | ||
---|---|---|
689 | Nothing, I'm just stupid and I should not comment when I'm tired. Sorry. |
src/lib/kaboutdata.h | ||
---|---|---|
314 | For proposed patch to improve ki18n docu see https://phabricator.kde.org/D5455 |
Is there any reason why we cannot just do the needed setlocale(LC_ALL, "") within ki18n itself (e.g. in the KLocalizedStringPrivateStatics ctor which is used to support translation of i18n calls? QCoreApplication is going to do this anyways so making it happen earlier at runtime seems like what we want.
Thought about this as well before, reasons against it that came into my mind though:
- ki18n is a util library, not so nice in general by libraries to decide on central settings which affect the complete process and also at random times (whenever the first i18n call would be made), ideally is explicitely asked for by from the main program
- first point even more an issue as ki18n is also pulled in into plain Qt applications by the Plasma QPA plugin
- apps using the switch-language feature from the KHelpMenu need any i18n calls only done after the qapp instance is creatd, as that feature works by registering a hook-up with Q_COREAPP_STARTUP_FUNCTION, to then set the custom selected language by setting the LANGUAGE environment variable (see https://cgit.kde.org/kxmlgui.git/tree/src/kswitchlanguagedialog_p.cpp).
src/lib/kaboutdata.h | ||
---|---|---|
314 | The TODO in the porting script means : after running this script, perform this change by hand. This isn't unfinished code, it's just a reminder for the reader. The rules given to us by the Qt developers are simple: do not use any parts of Qt that depend on locales before QCoreApplication is created. So, do not create KAboutData before QCoreApplication is created. This isn't KDE4 anymore. |
Ping. Is there anything wrong that would need to be improved in the current patch, or can this go in as is?
Adding an official note to the constructor to not create a KAboutData instance before the QApp instance is done should be a separate patch, perhaps even also get a runtime warning.
This patch here is about orgDomain & desktopFileName, let's do one thing after the other.
src/lib/kaboutdata.h | ||
---|---|---|
314 |
Do you by any chance have an URL to some quotable text for that statement handy? Looks better to point to something official here, which I sadly have not yet come across in the Qt docs so far (the code seen though would motivate such a rule). |
I think the API dox fixes are fine to go in. We can add i18n-related warnings in a separate patch as you suggest.