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
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
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
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,
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
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
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`
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
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
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.
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
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
>
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
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
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
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
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
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
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?
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
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
21 matches
Mail list logo
