Vincent Massol wrote:
ok found some time to review this email at last. Sorry
for the delay.
On Oct 15, 2008, at 3:47 AM, Sergiu Dumitriu wrote:
Hi devs,
The new localization component is committed. I think it is working
pretty well, so it's time to decide what to do next (estimate
deadlines
and responsibles after each item):
0. Outside review - me and Marta refactored it about 5 times
already, a
third opinion would be nice.
ASAP, let's say by the end of the week - volunteers; this is a good
exercise, as it shows how to write and use components
* I see you've used non threadsafe components for bundles. I don't
think we should do that.
WDYM? Where exactly should those be used?
I found some unsafe code in Pulled*Bundle, the list holding the pulled
bundles wasn't synchronize (I thought I already synchronized it, maybe I
accidentally removed the sync block in one of the refactorings).
* I'd prefer to use getMessage() instead of get()
which is too cryptic
for a LocalizationManager class IMO.
OK, but for backwards compatibility we should have get() also. Should it
be deprecated and planned for a future removal?
* The Bundle implementations are actually
BundleManager I think, each
managing a different type of resources (ResourceManager which might be
a better name)
I don't think Resource is better, since it seems generic. We're dealing
with localization resources, which are called (AFAIK) bundles.
* I think there's an important bug. The
LocalizationManager is
supposed to be threadsafe but the use() method (I'm still unsure what
it does and how it's used) doesn't look threadsafe
It is, since each bundle type uses a synchronized block. The
synchronization is on the execution context, and unless someone copies
some entries between two contexts, each execution context should hold
its unique list of pulled bundles. The method from
DefaultLocalizationManager doesn't need to be synchronized, since it
merely forwards the call to a threadsafe method.
* I'm surprised there are not tests. We must
really write the tests
either before the code or at the very least at the same time as
otherwise they're loosing half of their power (which is to improve
design)
Agreed. I'll start doing proper TDD from now on.
* WikiInformation doesn't seem to belong to this
module.
Partially agree. Some things should be here (the language ones), but I
had to include some things that belong to other modules (the wiki
constants, getCurrentWikiName). What do you propose?
* I'm really missing tests to understand how the
localization module
is used.
Hm... I thought the javadoc should be descriptive enough. I'll rework
the javadoc to make it more clear.
* Is it possible for an application to specify a
single resource
document and not use the other resource documents located in other
applications in the wiki? I think either a namespace at the level of
LocalizationManager (for example: getResourceCollection(String
namespace)) or at the level of getMessage(String name, String
namespace) (I prefer at the level of LocalizationManager) might be
better. This also means introducing the concept of ResourceCollection
as we have in the configuration module for configurations
(ConfigurationCollection).
I thought of that, too, but I thought it is too complicated for the
moment, adding little benefit. This could be improved in the future, if
we find it necessary.
I don't think that this separation is needed, since applications should
use prefixed key names, and it might be hard to mix applications
otherwise. What I mean is that once we use Interface Extensions to
interweave applications, several collections should be used in parallel.
* Is there a design document listing all the use cases
we want to have
somewhere?
No. My fault. We should write it collaboratively.
1. Add it in
the build cycle (include it in the core pom as a
submodule)
ASAP - me
After we agree on 0, sure.
2. Deprecate the XWikiMessageTool class and
methods. Should be
completely phased out by 1.9 according to our deprecation strategy.
ASAP - me
+1 but it's possible we'll need the new velocity bridge to make the
adaptation to the new component. I'd really like that we don't model
the new component against the old message tool (like get() methods for
ex).
But this will make upgrades hard, unless we have an optional backwards
compatible behavior.
3. Use it in
the xwiki-core, replacing the messagetool
Full replacement until 1.7M2 - me and other volunteers
+0, I think this is not a priority for 1.7 (it's not even defined in
our roadmap). I'd much much prefer you finish the Blog application
overhaul which is planned and people are waiting for it. And finish
the velocity module refactoring for uberspectors + ability to support
macro reloading (not sure what's the status on that one).
4. Propose a standard for writing keys
Next week - me + discussion on devs
ok
5. Start properly translating applications in
their own documents
Until 1.7 final - everybody, especially application owners
Shouldn't we start extracting applications from XE into their own
modules first?
We've been planning to do this for some time, with little progress.
6. Split
ApplicationResources into smaller files, moving everything to
its proper place: .properties files in plugins, wiki documents for
applications, CoreResources.properties (the clean version of
ApplicationResources).
ok
Until 1.7 final - everybody
7. Improve the localization component:
7a. Replace hashmaps with caches
Until 1.7M2 - me or volunteers
7b. Write tests
Until 1.7M2 - me or volunteers
Tests are a priority for validating the design.
A note for items 2 and 3: The localization
component interface is
compatible with the message tool, as it provides the get(string) and
get(string, list) methods. However, old code casts the retrieved
object
to XWikiMessageTool, so replacing the "msg" context variable is not
advisable yet.
And I really don't like using get(). Again I'm 100% sure we shouldn't
consider the old message tool to model our new components.
Again, this is in case we remove the MessageTool (which we should) and
clients want to upgrade without manually replacing all the instances of
$msg.get they have in their wiki.
I used
'l10n' as the variable name for the new tool, as
it reflects better what it does. And, as Vincent suggested, this is a
workaround until the velocity bridge is in place, when we shouldn't
use
direct context variables, but request access to services through this
bridge.
Yes and at that time we should be able to map the old msg.get() to the
new code.
From me, +1 to all of these.
WDYT? Any volunteers where needed?
--
Sergiu Dumitriu
http://purl.org/net/sergiu/