ilic requested changes to this revision.
ilic added a comment.
This revision now requires changes to proceed.
I think it's fine to add this capability, especially given that Gettext
(which Ki18n is an extension of) not only has it, but uses it exclusively (it
does not search through any environment-derived directories).
However, there are a few questions of semantics.
So far it was always possible for a user to override the locale directory
through environment variables, e.g. to test translation without building the
full program. Should we still enable this, i.e. provide environment overriding
also in case program sets the locale directory? But I guess we can postpone
this question, since as you mention in KDE 4 Plasma the locale directory was
being forced by the program anyway by other means.
On the other side, if the locale directory for the domain is set, should (as
in currently proposed implementation) environment-derived directories still be
looked through if catalog is not found in the set directory?
I think better name for the method is setDomainLocaleDir, because if it is
called again for the same domain, it will override the previous setting and not
add another directory for it.
INLINE COMMENTS
> klocalizedstringtest.cpp:59
> if (m_hasFrench) {
> - m_hasFrench = compileCatalogs(dataDir);
> + m_hasFrench = compileCatalogs({QFINDTESTDATA("po/fr/ki18n-test.po"),
> QFINDTESTDATA("po/fr/ki18n-test-qt.po")}, dataDir);
> }
I think we're not allowed to use initializer lists yet, due to MSVC11 support.
> klocalizedstringtest.cpp:523
> + compileCatalogs({QFINDTESTDATA("po/fr/ki18n-test2.po")}, dir.path());
> + KLocalizedString::setApplicationDomain("ki18n-test2");
> + KLocalizedString::addDomainLocaleDir("ki18n-test2", dir.path() +
> "/locale");
setApplicationDomain should not be called twice, use i18nd method instead, like
i18nd("ki18n-test2", "Cheese").
> kcatalog.cpp:124
> + {
> + QMutexLocker lock(&catalogStaticData->mutex);
> + const QString customLocaleDir =
> catalogStaticData->customCatalogDirs[domain];
Only read access is needed here, user QHash::value instead of QHash::operator[].
> kcatalog.cpp:126
> + const QString customLocaleDir =
> catalogStaticData->customCatalogDirs[domain];
> + if (!customLocaleDir.isEmpty() && !QFileInfo::exists(customLocaleDir
> + relpath)) {
> + return customLocaleDir;
Maybe I'm seeing something wrongly here, but... customLocaleDir + relpath may
miss path separator? Really !QFileInfo::exists and not QFileInfo::exists?
> kcatalog.cpp:151
> + {
> + QMutexLocker lock(&catalogStaticData->mutex);
> + if (catalogStaticData->customCatalogDirs.contains(domain_)) {
Also only read access needed.
REPOSITORY
R249 KI18n
REVISION DETAIL
https://phabricator.kde.org/D5208
To: davidedmundson, #frameworks, ilic
Cc: ilic
