D5208: Allow loading i18n catalogs from arbitrary locations

[Chusslove Illich]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170328/5dccf1b8/attachment-0001.html>