Bug 298333 - TranslationSupport cleanup
Summary: TranslationSupport cleanup
Status: RESOLVED WONTFIX
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: p2 (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: P2 Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: api
Depends on:
Blocks:
 
Reported: 2009-12-21 13:47 EST by John Arthorne CLA
Modified: 2020-02-20 02:22 EST (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description John Arthorne CLA 2009-12-21 13:47:21 EST
I'd like to take a stab at an API replacement for TranslationSupport. I think having translated properties/licenses/etc in a different API from the raw untranslated values will be confusing for API clients. Many will not find or notice TranslationSupport and will incorrectly use untranslated values where they should instead be using translated values.

Also, the current API involves clients creating a new instance each time, which means the caching is duplicated and there is no single point of control for configuring the translation source or locale.

I'd like to attempt integrating the methods directly into new methods on IInstallableUnit. We would use the LocaleProvider service for obtaining the "current" locale, and another service for obtaining the translation source. This will allow a single point of configurability, and allow decoupling dependencies (for example metadata package won't need to know about IProfile which is the default translation source).
Comment 1 Susan McCourt CLA 2009-12-21 15:00:47 EST
this all sounds good to me.
When this work is done we should:
- move the test out of the ui tests suite and move into the main test suite
- I can move the IServiceUI back into the UI and get rid of the p2.common bundle
Comment 2 Thomas Hallgren CLA 2009-12-21 15:01:35 EST
This may relate to bug 296531 as well.
Comment 3 John Arthorne CLA 2009-12-22 12:11:53 EST
Released in HEAD. TranslationSupport is moved into an internal package in the metadata bundle. The "translation source" is currently manually converted by the profile registry when it starts up, but that could later be changed to a more complicated service lookup if needed. There are now API methods on IInstallableUnit for obtaining translated properties/licenses/copyright of any IU.
Comment 4 Thomas Hallgren CLA 2009-12-22 16:44:31 EST
I have problems with this fix. There's nothing connecting an IU as such with the IQueryable that is used for the translation support. The API gives the impression that an IU can provide translated properties for a certain locale without access to the meta-data that provides it.

What if I'm currently looking at an IU that isn't represented in the 'self' profile? Perhaps I'm using two different agents, each with its own locale (my task is perhaps to work with translations). Surely, using a static variable for this must be too limiting.
Comment 5 Thomas Hallgren CLA 2009-12-22 18:18:37 EST
This fix also effectively breaks the p2QL support for translations. In p2QL I introduced an interface ITranslationSupport. The default implementation was using the TranslationSupport class but the intention was to be able to use other types of support as well (backed by database etc.).

The now private constructor on TranslationSupport makes this impossible.
Comment 6 John Arthorne CLA 2009-12-24 10:25:24 EST
Ok, I'll need to revisit this after holidays. In most cases the client wanting to get the translation doesn't know/care where the translations come from, so the API on IInstallableUnit is easier for them. All of the client code I changed went from something like "new TranslationSupport().getIUProperty(iu, key) to iu.getProperty(key, locale) which is roughly equivalent but simpler.

I realize we do need a back end mechanism to allow someone to control/specify where translations come from, but since there weren't any users for this that I knew of, I thought that problem could be addressed later. 

Do you think it's sufficient to allow a client to specify the IQueryable translation source, or are there cases where even the lookup/caching mechanism would be different?
Comment 7 Thomas Hallgren CLA 2009-12-26 10:35:06 EST
(In reply to comment #6)
> Do you think it's sufficient to allow a client to specify the IQueryable
> translation source, or are there cases where even the lookup/caching mechanism
> would be different?

I think it is important that the translation is done in some context but I'm not sure if the IQueryable itself is the best fit. If you, for instance, query a composite (or a profile), then location of the cache is probably more related to the top level queryable then to each individual repository. But then again, a cache that isn't directly tied to an IQueryable will need some kind of listener in order to invalidate itself when conditions change (things are added to a profile for instance). The same thing is true if the repository as such is mutable.

On the other hand, given the fairly static nature of translations, perhaps it's enough to let an Agent provide the support together with an easy way of clearing it? The responsibility of clearing would then be on the user.

Another approach would be to avoid caches altogether and instead trust that an efficient index is provided (see bug 298266) that would allow quick access of translation fragments. A special IQuery could then make use of the index to resolve the translations (the p2QL already has this).

The important thing in my opinion is that we avoid static defaults that redirects things to the 'self' profile. Any code written to make use of that will become useless in multi agent scenarios. I do understand the motivation behind the simplified access method on the IU but I'm concerned that it will lead to less reusable code. I'd rather see that an agent (or some kind of query context) provided the support.
Comment 8 John Arthorne CLA 2010-03-05 16:08:41 EST
I have been reviewing this but not planning to do anything more for 3.6. Currently TranslationSupport has a public constructor so a client wanting to do specialized translation lookup can use that if they want. The translation source can be configured globally by accessing and modifying the (non-API) TranslationSupport instance. Otherwise we'll have the same limitation that we had in the past two releases - by default translations will only be searched for in the self profile.  The fact that nobody has complained about this gives me the sneaking suspicion that nobody out there is actually translating repositories today. If anyone has an interest in this they're welcome to take it on.
Comment 9 Thomas Hallgren CLA 2010-03-05 18:55:22 EST
The important thing is that whoever wants to do translations in a different way can do so and as things stand now, they can.

My own use-case is for the aggregator where we allow browsing of the sources of the aggregation. All of that takes place using an agent that is not in any way connected to the self profile. All is well though. We just have to avoid the methods on the IInstallableUnit and everything is fine.

One minor concern is that the current javadoc on the IInstallableUnit methods says very little about the fact that the translation that takes place is hard wired to the self profile and the alternatives at hand.
Comment 10 Eclipse Webmaster CLA 2019-09-06 16:14:36 EDT
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.
Comment 11 Ed Merks CLA 2020-02-20 02:22:16 EST
I assume this issue has died.