D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
This revision was automatically updated to reflect the committed changes. Closed by commit R244:f01e8b41f622: API dox: more info about KAboutData's orgDomain/desktopFileName properties (authored by kossebau). REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5439?vs=13439=13779 REVISION DETAIL https://phabricator.kde.org/D5439 AFFECTED FILES src/lib/kaboutdata.cpp src/lib/kaboutdata.h To: kossebau, #frameworks, aacid, mpyne, ltoscano, stikonas Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
stikonas accepted this revision. REPOSITORY R244 KCoreAddons BRANCH improveKAboutDataAPIDox REVISION DETAIL https://phabricator.kde.org/D5439 To: kossebau, #frameworks, aacid, mpyne, ltoscano, stikonas Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
ltoscano accepted this revision. This revision is now accepted and ready to land. REPOSITORY R244 KCoreAddons BRANCH improveKAboutDataAPIDox REVISION DETAIL https://phabricator.kde.org/D5439 To: kossebau, #frameworks, aacid, mpyne, ltoscano, stikonas Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
mpyne accepted this revision. mpyne added a comment. I think the API dox fixes are fine to go in. We can add i18n-related warnings in a separate patch as you suggest. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D5439 To: kossebau, #frameworks, aacid, mpyne, ltoscano, stikonas Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
kossebau added a comment. This revision now requires changes to proceed. 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. INLINE COMMENTS > dfaure wrote in 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. > 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. 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). REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D5439 To: kossebau, #frameworks, aacid, mpyne, ltoscano, stikonas Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
dfaure added inline comments. INLINE COMMENTS > kossebau wrote in 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). > The other mention was the TODO added by kf5/convert-kcmdlineargs.pl, > Sadly the commit which added that TODO does it only as side-effect and > without any reasoning: > https://cgit.kde.org/kde-dev-scripts.git/commit/kf5/convert-kcmdlineargs.pl?id=ac74bc07de810ef5a51a3030bfa96980f72ad8b1 > And from all code path I have seen there is nothing in the KAboutData > constructor which requires a QApp instance. And the related i18n calls also > are said nowhere said to require a QApp instance. > @dfaure The above being your commit, do you remember (he, only 2 years ago ;) > ) your motivation for that TODO? 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. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D5439 To: kossebau, #frameworks, stikonas, aacid, ltoscano, mpyne Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
kossebau added a comment. In https://phabricator.kde.org/D5439#102241, @mpyne wrote: > 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: 1. 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 2. first point even more an issue as ki18n is also pulled in into plain Qt applications by the Plasma QPA plugin 3. 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). REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D5439 To: kossebau, #frameworks, stikonas, aacid, ltoscano, mpyne Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
mpyne added a comment. 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. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D5439 To: kossebau, #frameworks, stikonas, aacid, ltoscano, mpyne Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
kossebau added inline comments. INLINE COMMENTS > kossebau wrote in kaboutdata.h:314 > Aha! i18n() calls before the creation of QApplication instance currently fail > (or rather: only return the untranslated string) for this reason: > gettext(), as used internally by ki18n, looks at the locale names set for the > different locale categories (LC_*). By default at program start, those are > all fixed to "C". Only if they are set to "" (empty string) then the > environment variables are looked at, otherwise those are ignored for the > respective category. > To unlock all categories for being set via the environment variables, the > quickest way is to call `setlocale(LC_ALL, "");`. And this is what is done > during the Q*Application instance creation, on Unix. > > 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 > http://man7.org/linux/man-pages/man3/setlocale.3.html > https://code.woboq.org/qt5/qtbase/src/corelib/kernel/qcoreapplication.cpp.html#_ZN23QCoreApplicationPrivate10initLocaleEv > http://stackoverflow.com/questions/25661295/why-does-qcoreapplication-call-setlocalelc-all-by-default-on-unix-linux > > 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. > In any case: no i18n() in static construction code, that is racy and should > be simply considered to not work. > > So tasks seem to be: > > 1. change the example code to have needed order > 2. write patch for ki18n docu > 3. make some blog post to raise awareness > > First though: device-less leisure time :) For proposed patch to improve ki18n docu see https://phabricator.kde.org/D5455 REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D5439 To: kossebau, #frameworks, stikonas, mpyne, aacid, ltoscano Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
ltoscano added inline comments. INLINE COMMENTS > kossebau wrote in 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. Nothing, I'm just stupid and I should not comment when I'm tired. Sorry. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D5439 To: kossebau, #frameworks, stikonas, mpyne, aacid, ltoscano Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
kossebau added inline comments. INLINE COMMENTS > ltoscano wrote in kaboutdata.cpp:689 > I think that any extra unneeded step is only prone to produce issues. > In 90% of the cases (and probably more, like basically all stuff on kde.org), > the desktop file name is org domain + component. No need to do it again and > again. A flag is a minimal price to pay, if needed. Maybe it's not even > needed, because you can compute it on the fly (if not set, return org domain > + component, if both set). > So yes, I'd like a reminder here. > At least > // KF6: evaluate if desktopFileName can return org domain + component if not > explicitly set. Isn't the KF6 comment added in KAboutData::desktopFileName() not already doing that, more or less? Not sure what you exactly ask for. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D5439 To: kossebau, #frameworks, stikonas, mpyne, aacid, ltoscano Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
ltoscano added inline comments. INLINE COMMENTS > kossebau wrote in 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? > The KF6 TODO proposes to change the behaviour to follow the idea of > https://phabricator.kde.org/D5405. > "set it to domain + component name" sounds as you propose to store a > auto-generated value in the desktopFileName variable. But that would need an > extra flag to know if the value is auto-generated and thus needs to be > updated if the sources for the value change or if the value has been > explicitely set by the user, so should not be auto-updated. > > 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 :) I think that any extra unneeded step is only prone to produce issues. In 90% of the cases (and probably more, like basically all stuff on kde.org), the desktop file name is org domain + component. No need to do it again and again. A flag is a minimal price to pay, if needed. Maybe it's not even needed, because you can compute it on the fly (if not set, return org domain + component, if both set). So yes, I'd like a reminder here. At least // KF6: evaluate if desktopFileName can return org domain + component if not explicitly set. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D5439 To: kossebau, #frameworks, stikonas, mpyne, aacid, ltoscano Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
kossebau added inline comments. INLINE COMMENTS > ltoscano wrote in kaboutdata.cpp:689 > I would add something like > // KF6: if desktopFileName has not been explicitly set, set it to domain + > component name Why would you also add a comment here? Would you think the comments at the places which should be changed are not enough? The KF6 TODO proposes to change the behaviour to follow the idea of https://phabricator.kde.org/D5405. "set it to domain + component name" sounds as you propose to store a auto-generated value in the desktopFileName variable. But that would need an extra flag to know if the value is auto-generated and thus needs to be updated if the sources for the value change or if the value has been explicitely set by the user, so should not be auto-updated. 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 :) REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D5439 To: kossebau, #frameworks, stikonas, mpyne, aacid, ltoscano Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
kossebau updated this revision to Diff 13439. kossebau added a comment. - update example to not do i18n calls before qapplication instance - improve KF6 TODO comments REPOSITORY R244 KCoreAddons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5439?vs=13413=13439 BRANCH improveKAboutDataAPIDox REVISION DETAIL https://phabricator.kde.org/D5439 AFFECTED FILES src/lib/kaboutdata.cpp src/lib/kaboutdata.h To: kossebau, #frameworks, stikonas, mpyne, aacid, ltoscano Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
kossebau added inline comments. INLINE COMMENTS > kossebau wrote in 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. Aha! i18n() calls before the creation of QApplication instance currently fail (or rather: only return the untranslated string) for this reason: gettext(), as used internally by ki18n, looks at the locale names set for the different locale categories (LC_*). By default at program start, those are all fixed to "C". Only if they are set to "" (empty string) then the environment variables are looked at, otherwise those are ignored for the respective category. To unlock all categories for being set via the environment variables, the quickest way is to call `setlocale(LC_ALL, "");`. And this is what is done during the Q*Application instance creation, on Unix. 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 http://man7.org/linux/man-pages/man3/setlocale.3.html https://code.woboq.org/qt5/qtbase/src/corelib/kernel/qcoreapplication.cpp.html#_ZN23QCoreApplicationPrivate10initLocaleEv http://stackoverflow.com/questions/25661295/why-does-qcoreapplication-call-setlocalelc-all-by-default-on-unix-linux 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. In any case: no i18n() in static construction code, that is racy and should be simply considered to not work. So tasks seem to be: 1. change the example code to have needed order 2. write patch for ki18n docu 3. make some blog post to raise awareness First though: device-less leisure time :) REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D5439 To: kossebau, #frameworks, stikonas, mpyne, aacid, ltoscano Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
kossebau added inline comments. INLINE COMMENTS > ltoscano wrote in 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 " dialog is untranslated. 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. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D5439 To: kossebau, #frameworks, stikonas, mpyne, aacid, ltoscano Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
kossebau added inline comments. INLINE COMMENTS > kossebau wrote in 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). > The other mention was the TODO added by kf5/convert-kcmdlineargs.pl, > Sadly the commit which added that TODO does it only as side-effect and > without any reasoning: > https://cgit.kde.org/kde-dev-scripts.git/commit/kf5/convert-kcmdlineargs.pl?id=ac74bc07de810ef5a51a3030bfa96980f72ad8b1 > And from all code path I have seen there is nothing in the KAboutData > constructor which requires a QApp instance. And the related i18n calls also > are said nowhere said to require a QApp instance. > @dfaure The above being your commit, do you remember (he, only 2 years ago ;) > ) your motivation for that TODO? 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. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D5439 To: kossebau, #frameworks, stikonas, mpyne, aacid, ltoscano Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
ltoscano added inline comments. INLINE COMMENTS > kossebau wrote in 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). > The other mention was the TODO added by kf5/convert-kcmdlineargs.pl, > Sadly the commit which added that TODO does it only as side-effect and > without any reasoning: > https://cgit.kde.org/kde-dev-scripts.git/commit/kf5/convert-kcmdlineargs.pl?id=ac74bc07de810ef5a51a3030bfa96980f72ad8b1 > And from all code path I have seen there is nothing in the KAboutData > constructor which requires a QApp instance. And the related i18n calls also > are said nowhere said to require a QApp instance. > @dfaure The above being your commit, do you remember (he, only 2 years ago ;) > ) your motivation for that TODO? 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 " dialog is untranslated. REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D5439 To: kossebau, #frameworks, stikonas, mpyne, aacid, ltoscano Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
kossebau added a subscriber: dfaure. kossebau added inline comments. INLINE COMMENTS > ltoscano wrote in kaboutdata.h:314 > The QApplication instance should happen before this line, not just before > setApplicationData (that's what Albert was trying to explain). 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). The other mention was the TODO added by kf5/convert-kcmdlineargs.pl, Sadly the commit which added that TODO does it only as side-effect and without any reasoning: https://cgit.kde.org/kde-dev-scripts.git/commit/kf5/convert-kcmdlineargs.pl?id=ac74bc07de810ef5a51a3030bfa96980f72ad8b1 And from all code path I have seen there is nothing in the KAboutData constructor which requires a QApp instance. And the related i18n calls also are said nowhere said to require a QApp instance. @dfaure The above being your commit, do you remember (he, only 2 years ago ;) ) your motivation for that TODO? REPOSITORY R244 KCoreAddons REVISION DETAIL https://phabricator.kde.org/D5439 To: kossebau, #frameworks, stikonas, mpyne, aacid, ltoscano Cc: dfaure
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
ltoscano requested changes to this revision.
ltoscano added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kaboutdata.cpp:689
>
> KAboutData ::setOrganizationDomain(const QByteArray )
> {
I would add something like
// KF6: if desktopFileName has not been explicitly set, set it to domain +
component name
> kaboutdata.h:314
> + * // create the aboutData instance
> + * KAboutData aboutData("foo", i18n("Foo"), "0.1",
> + * i18n("To Foo or not To Foo"),
The QApplication instance should happen before this line, not just before
setApplicationData (that's what Albert was trying to explain).
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D5439
To: kossebau, #frameworks, stikonas, mpyne, aacid, ltoscano
D5439: API dox: more info about KAboutData's orgDomain/desktopFileName properties
kossebau created this revision. Restricted Application added a project: Frameworks. REVISION SUMMARY 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. REPOSITORY R244 KCoreAddons BRANCH improveKAboutDataAPIDox REVISION DETAIL https://phabricator.kde.org/D5439 AFFECTED FILES src/lib/kaboutdata.cpp src/lib/kaboutdata.h To: kossebau, #frameworks, stikonas, mpyne, aacid, ltoscano
