Differential D5208

Allow loading i18n catalogs from arbitrary locations
ClosedPublic
Actions

Authored by davidedmundson on Mar 28 2017, 3:23 AM.

Details

Reviewers

ilic

Group Reviewers

Frameworks

Commits

R249:4c95ac8bb47f: Allow loading i18n catalogs from arbitrary locations

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

Diff Detail

Repository

R249 KI18n

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

davidedmundson created this revision.Mar 28 2017, 3:23 AM

Restricted Application added a project: Frameworks. · View Herald TranscriptMar 28 2017, 3:23 AM

Missed a file

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.

autotests/klocalizedstringtest.cpp
59 ↗	(On Diff #12892)	I think we're not allowed to use initializer lists yet, due to MSVC11 support.
523 ↗	(On Diff #12892)	setApplicationDomain should not be called twice, use i18nd method instead, like i18nd("ki18n-test2", "Cheese").
src/kcatalog.cpp
124 ↗	(On Diff #12892)	Only read access is needed here, user QHash::value instead of QHash::operator[].
126 ↗	(On Diff #12892)	Maybe I'm seeing something wrongly here, but... customLocaleDir + relpath may miss path separator? Really !QFileInfo::exists and not QFileInfo::exists?
151 ↗	(On Diff #12892)	Also only read access needed.

This revision now requires changes to proceed.Mar 28 2017, 10:59 AM

ilic added inline comments.Mar 28 2017, 11:08 AM

src/klocalizedstring.h
558 ↗	(On Diff #12892)	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.

Thanks.

Issues addressed

davidedmundson marked 3 inline comments as done.Jun 27 2017, 1:51 PM

davidedmundson added inline comments.

autotests/klocalizedstringtest.cpp
59 ↗	(On Diff #12892)	Pretty sure it's fine. kactivities and kjs use them with no issue.

This revision now requires changes to proceed.Jun 27 2017, 1:51 PM

ilic added inline comments.Jun 28 2017, 7:11 AM

autotests/klocalizedstringtest.cpp
59 ↗	(On Diff #12892)	Well, since no-one else complained either, guess it's ok. Easy to fix later if someone does complain.
src/kcatalog.cpp
124 ↗	(On Diff #12892)	Also the lock should be removed, now that only read access is used.
151 ↗	(On Diff #12892)	Also the lock should be removed.

davidedmundson marked an inline comment as done.Jul 4 2017, 6:48 AM

davidedmundson added inline comments.

src/kcatalog.cpp
124 ↗	(On Diff #12892)	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.

ilic accepted this revision.Jul 4 2017, 7:49 AM

ilic added inline comments.

src/kcatalog.cpp
124 ↗	(On Diff #12892)	Hm, right... That makes me wonder where else there should be a lock and it's missing :) But that's for another story.

This revision is now accepted and ready to land.Jul 4 2017, 7:49 AM

Closed by commit R249:4c95ac8bb47f: Allow loading i18n catalogs from arbitrary locations (authored by davidedmundson). · Explain WhyJul 11 2017, 12:18 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

		Path
M		autotests/klocalizedstringtest.h (3 lines)
M		autotests/klocalizedstringtest.cpp (21 lines)
A	M	autotests/po/fr/ki18n-test2.po (15 lines)
M		src/kcatalog.cpp (26 lines)
M		src/kcatalog_p.h (2 lines)
M		src/klocalizedstring.h (10 lines)
M		src/klocalizedstring.cpp (4 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	12890	9d81fac		Mar 28 2017, 3:23 AM	★	★
Diff 2	12892	9d81fac	Missed a file	Mar 28 2017, 3:26 AM	★	★
Diff 3	12962	9d81fac	Thanks.	Mar 29 2017, 1:02 PM	★	★
Diff 4	16509	97ce753	R249:4c95ac8bb47f64b61c7088b8b1112d191ba0ce2b	Jul 11 2017, 12:18 PM	★	★