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
D5430: KDECompilerSettings: Pass -Wvla & -Wdate-time
mpyne accepted this revision. mpyne added a comment. This revision is now accepted and ready to land. I think these warning flags make sense as default warning flags. I don't agree that `__DATE__` and `__TIME__` should be avoided as a rule, but projects that find this valuable can disable the warning as appropriate (e.g. with a "developer mode" option) and make alternative provisions for reproducible builds (for packagers or those auditing the binaries generated by others). INLINE COMMENTS > KDECompilerSettings.cmake:376 > +endif() > +if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND NOT CMAKE_CXX_COMPILER_VERSION > VERSION_LESS 5.0 OR (CMAKE_CXX_COMPILER_ID MATCHES "Clang" AND NOT > CMAKE_CXX_COMPILER_VERSION VERSION_LESS 3.5)) > +# -Wdate-time: warn if we use __DATE__ or __TIME__ (we want to be able > to reproduce the exact same binary) The CMake docs seem to say that the operator precedence of OR vs AND will do what you mean here for the GCC version check. But still, if you're going to defensively parenthesize the Clang version check (and I agree with doing so), you should also do the same for the GCC version check. That way we don't have to check the CMake docs to make sure it's right. :) REPOSITORY R240 Extra CMake Modules BRANCH master REVISION DETAIL https://phabricator.kde.org/D5430 To: kfunk, mpyne Cc: mpyne, #frameworks, #build_system
D5456: KDoctools: build on Mac with docbook from homebrew
ltoscano added a comment.
opt/docbook-xsl/ ? O.o
Well, not up to me to discuss that.
Just a question, no need for ${CMAKE_INSTALL_DATAROOTDIR}? I understand that
it's a suffix, so not required, but if it was /opt, maybe it could be changed
to ${CMAKE_INSTALL_DATAROOTDIR}/docbook etc etc
REPOSITORY
R238 KDocTools
REVISION DETAIL
https://phabricator.kde.org/D5456
To: winterz, ltoscano
Cc: #frameworks, #documentation, skadinna
D5456: KDoctools: build on Mac with docbook from homebrew
winterz created this revision. Restricted Application added projects: Frameworks, Documentation. Restricted Application added subscribers: Documentation, Frameworks. REVISION SUMMARY On Mac, homebrew installs the docbook-xml and docbook-xls files under /usr/local/opt so add searchpaths accordingly TEST PLAN builds on Mac now after running brew install docbook docbook-xsl REPOSITORY R238 KDocTools REVISION DETAIL https://phabricator.kde.org/D5456 AFFECTED FILES cmake/FindDocBookXML4.cmake cmake/FindDocBookXSL.cmake To: winterz, ltoscano Cc: #frameworks, #documentation, skadinna
D5455: Extend Programmer's Guide with notes about influence of setlocale()
kossebau added a comment. Actually, I simplified/ignored how i18n calls internally make the gettext call chose a locale name for which a catalog has been found, by temporarily setting the LANGUAGE variable. What do you think, should this implementation detail be noted here as well, or would it only result in confused readers, who might already being challenged a lot by the complexity how to use the ki18n API? REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D5455 To: kossebau, #frameworks, ilic
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
D5455: Extend Programmer's Guide with notes about influence of setlocale()
kossebau added a comment. See thread around https://phabricator.kde.org/D5439#102005 for related discussion. REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D5455 To: kossebau, #frameworks, ilic
D5455: Extend Programmer's Guide with notes about influence of setlocale()
kossebau created this revision. Restricted Application added a project: Frameworks. REVISION SUMMARY Collect knowlegde found when investigating why i18n calls before line QApplication app; do not return translated strings. Perhaps needs more prominent place to warn about the issues. But having it documented somewhere will be a start. REPOSITORY R249 KI18n BRANCH addNoteAboutGettextLocaleIssuse REVISION DETAIL https://phabricator.kde.org/D5455 AFFECTED FILES docs/programmers-guide.md To: kossebau, #frameworks, ilic
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
D5443: fix duplicated symbols compilation error with mingw on Windows
apol added a reviewer: Windows. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D5443 To: mgallien, kfunk, #windows Cc: #frameworks
Re: Continuous Integration on Windows
Hello, On jeudi 13 avril 2017 19:31:37 CEST Kevin Funk wrote: > On Thursday, 13 April 2017 18:25:19 CEST Matthieu Gallien wrote: > > Hello, > > > > I am trying to use KFileMetaData on Windows with mingw-5.3 32 bits. I am > > getting compilation errors in the automatic tests and have an hard time > > getting it to works nominally. > > Paste the errors please, maybe we can fix. I have created a differential about the compilation error. The fact that I am not getting the extractors to run will need a bit more time on my side to understand the error. At least, the problem I am facing is that I seem to not be able to get any extractor to work. > > Is there public servers currently testing KDE Frameworks 5 including > > KFileMetaData ? > > Still no servers which compile KF5 master on a regular basis under Windows, > unfortunately. > > > Should I switch to Microsoft compilers ? > > Not necessarily, KF5 /should/ compile fine under both MinGW & MSVC. OK, thanks for the information. > Cheers, > Kevin > > > Thanks in advance for your help > > > > Best regards > > > > -- > > Matthieu Gallien
