D5208: Allow loading i18n catalogs from arbitrary locations
This revision was automatically updated to reflect the committed changes. Closed by commit R249:4c95ac8bb47f: Allow loading i18n catalogs from arbitrary locations (authored by davidedmundson). REPOSITORY R249 KI18n CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5208?vs=12962=16509 REVISION DETAIL https://phabricator.kde.org/D5208 AFFECTED FILES autotests/klocalizedstringtest.cpp autotests/klocalizedstringtest.h autotests/po/fr/ki18n-test2.po src/kcatalog.cpp src/kcatalog_p.h src/klocalizedstring.cpp src/klocalizedstring.h To: davidedmundson, #frameworks, ilic Cc: ilic
D5208: Allow loading i18n catalogs from arbitrary locations
ilic accepted this revision. ilic added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > davidedmundson wrote in kcatalog.cpp:124 > I think I still need it. > > QHash::value() isn't marked as being atomic so we still need to make sure > we're not reading at the same time as someone else is modifying the hash > table. Hm, right... That makes me wonder where else there should be a lock and it's missing :) But that's for another story. REPOSITORY R249 KI18n BRANCH master REVISION DETAIL https://phabricator.kde.org/D5208 To: davidedmundson, #frameworks, ilic Cc: ilic
D5208: Allow loading i18n catalogs from arbitrary locations
davidedmundson marked an inline comment as done. davidedmundson added inline comments. INLINE COMMENTS > ilic wrote in kcatalog.cpp:124 > Also the lock should be removed, now that only read access is used. I think I still need it. QHash::value() isn't marked as being atomic so we still need to make sure we're not reading at the same time as someone else is modifying the hash table. REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D5208 To: davidedmundson, #frameworks, ilic Cc: ilic
D5208: Allow loading i18n catalogs from arbitrary locations
ilic added inline comments. INLINE COMMENTS > davidedmundson wrote in klocalizedstringtest.cpp:59 > Pretty sure it's fine. kactivities and kjs use them with no issue. Well, since no-one else complained either, guess it's ok. Easy to fix later if someone does complain. > ilic wrote in kcatalog.cpp:124 > Only read access is needed here, user QHash::value instead of > QHash::operator[]. Also the lock should be removed, now that only read access is used. > ilic wrote in kcatalog.cpp:151 > Also only read access needed. Also the lock should be removed. REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D5208 To: davidedmundson, #frameworks, ilic Cc: ilic
D5208: Allow loading i18n catalogs from arbitrary locations
davidedmundson marked 3 inline comments as done. davidedmundson added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > ilic wrote in klocalizedstringtest.cpp:59 > I think we're not allowed to use initializer lists yet, due to MSVC11 support. Pretty sure it's fine. kactivities and kjs use them with no issue. REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D5208 To: davidedmundson, #frameworks, ilic Cc: ilic
D5208: Allow loading i18n catalogs from arbitrary locations
davidedmundson updated this revision to Diff 12962. davidedmundson marked 3 inline comments as done. davidedmundson added a comment. Thanks. Issues addressed REPOSITORY R249 KI18n CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5208?vs=12892=12962 BRANCH master REVISION DETAIL https://phabricator.kde.org/D5208 AFFECTED FILES autotests/klocalizedstringtest.cpp autotests/klocalizedstringtest.h autotests/po/fr/ki18n-test2.po src/kcatalog.cpp src/kcatalog_p.h src/klocalizedstring.cpp src/klocalizedstring.h To: davidedmundson, #frameworks, ilic Cc: ilic
D5208: Allow loading i18n catalogs from arbitrary locations
ilic added inline comments. INLINE COMMENTS > klocalizedstring.h:558 > > +static void addDomainLocaleDir(const QByteArray , const QString > ); > + Misses docstring. It should be made clear that this method is for "special purposes" (e.g. plugins and whatnot), and that normal programs should not force locale directories but rely on standard data directory lookup hierarchy. REPOSITORY R249 KI18n REVISION DETAIL https://phabricator.kde.org/D5208 To: davidedmundson, #frameworks, ilic Cc: ilic
D5208: Allow loading i18n catalogs from arbitrary locations
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(>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(>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
D5208: Allow loading i18n catalogs from arbitrary locations
davidedmundson updated this revision to Diff 12892. davidedmundson added a comment. Missed a file REPOSITORY R249 KI18n CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D5208?vs=12890=12892 BRANCH master REVISION DETAIL https://phabricator.kde.org/D5208 AFFECTED FILES autotests/klocalizedstringtest.cpp autotests/klocalizedstringtest.h autotests/po/fr/ki18n-test2.po src/kcatalog.cpp src/kcatalog_p.h src/klocalizedstring.cpp src/klocalizedstring.h To: davidedmundson, #frameworks
D5208: Allow loading i18n catalogs from arbitrary locations
davidedmundson created this revision.
Restricted Application added a project: Frameworks.
REVISION SUMMARY
In KDE4, Plasma used to be able to load bundled translations with any
plasmoid downloaded from kde-look.
It used added a KGlobal::dirs()->addResourceDir("locale") at runtime
when we load a plasmoid.
Rather than just constantly expanding the list of locale search paths,
this patch allows for adding a custom path for a specific translation
domain which should be a lot faster and also prevent any collisions.
A static method is added to KCatalog, then proxied via KLocalizedString
as that header is exported.
TEST PLAN
Added unit test
Also patched plasma-framework and debug showed it loaded .po files
from the correct directory
REPOSITORY
R249 KI18n
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D5208
AFFECTED FILES
autotests/klocalizedstringtest.cpp
autotests/klocalizedstringtest.h
src/kcatalog.cpp
src/kcatalog_p.h
src/klocalizedstring.cpp
src/klocalizedstring.h
To: davidedmundson, #frameworks
