Community
Participate
Working Groups
Looks like we've currently only added support to externalize label and tooltip strings and forgot about mnemonics
I've renamed the bug. To me it looks like the following attributes have missing NLS support: * MenuElement#mnemonics * Command#commandName * Command#description (* Category#name) * Catgory#description * Part#description * PartDescriptor#description
Mnemonics are already specified in labels? I don't think we should have separate mnemonics, certainly not for menus / command. It is possible to make a case for parts - there are couple long-standing issues in 3.x with mnemonics related to part names (such as no mnemonics on Show View -> Other), but it can be more naturally resolved by specifying mnemonics in the labels "&View" and then filtering them for CTabs. As for description fields - would it make sense to use UILabel instead? Or, maybe, we can make a base class UIText and then reuse it for UILabel and descriptions.
I have no idea why we have mnemonics in the Menu-Model, Paul/Eric do you know why we have them. If they are not needed we'll have to remove them
(In reply to comment #2) > Mnemonics are already specified in labels? I don't think we should have > separate mnemonics, certainly not for menus / command. mnemonics are separate from command names, especially as included in Menus (this is already the case in 3.x). The problem: Show In is the command name, but when included in Window it should be S&how In and when included in Help it should be Show &In. So the Mnemonic is added separately. PW
[...] > As for description fields - would it make sense to use UILabel instead? Or, > maybe, we can make a base class UIText and then reuse it for UILabel and > descriptions. So you want another mixin for descriptions? I don't think it is a good idea to make UILabel derive from UIText because descrition is not making sense from most of the elements that mixin UILabel (e.g. MenuElement, MWindow,...)
I'm not sure about Category#name. Any opinions on this?
(In reply to comment #4) > mnemonics are separate from command names, especially as included in Menus > (this is already the case in 3.x). > > The problem: Show In is the command name, but when included in Window it should > be S&how In and when included in Help it should be Show &In. So the Mnemonic > is added separately. I never used it myself so I am probably missing something. I see "mnemonics" under "location" attribute for org.eclipse.ui.menus, but that element says it has been deprecated and replaced by "locationURI", presumably under "menuContribution", and that does not have mnemonics. The "menu" element has mnemonics, but it has the label as well, and I assume that you'd have to specify 2 different menu elements in the "Show In" example with 2 different labels and 2 corresponding mnemonics? At any rate, the question is if it is a good design to have separate mnemonics. Assuming it will be specified in a way that does not duplicate information already on the label, I am not sure how would I approach translation of separate mnemonics.
(In reply to comment #5) > [...] > > > As for description fields - would it make sense to use UILabel instead? Or, > > maybe, we can make a base class UIText and then reuse it for UILabel and > > descriptions. > > So you want another mixin for descriptions? > If we are going to change this area, I'd rather use aggregation. Instead of class blah implements UILabel how about class blah { UILabel myLabel; UIText myDescription; } By the way, I really like idea of having UIText as we could potentially tie it into bi-directional processing.
[...] > If we are going to change this area, I'd rather use aggregation. Instead of > > class blah implements UILabel > > how about > class blah { > UILabel myLabel; > UIText myDescription; > } > Ok. I see where you are heading to and then UILabel would be: UILabel { UIText label; UIText tooltip; } The problem with this is that it makes using the model more unnatural. Think about creating an new element ----------8<---------- MPart p = MBasicFactory.INSTANCE.createPart(); MUIText t = MBasicFactory.INSTANCE.createUIText(); t.setText("BlaBla"); p.setLabel(t); ----------8<---------- or accessing like this: ----------8<---------- String label = p.getLabel().getText(); ----------8<---------- The domain element is nothing more than a String-Holder but we'll get a lot more object instances.
[...] > or accessing like this: > ----------8<---------- > String label = p.getLabel().getText(); > ----------8<---------- The access has be even worse because you need to add a NULL-Check because you can't be sure getLabel() is not returning a NULL! Tom
(In reply to comment #9) > MPart p = MBasicFactory.INSTANCE.createPart(); > MUIText t = MBasicFactory.INSTANCE.createUIText(); > t.setText("BlaBla"); > p.setLabel(t); Hmm... You certainly have a point here. How about: p.setLabel(new MUILabel("BlahBlah")), or p.setLabel(new MUILabel("%key", IContributor contributor, URI iconURI)) > The domain element is nothing more than a String-Holder but we'll get a lot > more object instances. Yes, that is something that needs to be considered more carefully. Having a String wrapper would potentially benefit Bidi and translation, including multi-language support. But there is a cost associated with it, and the cost is put on all consumers, regardless of there they use those features. I guess to avoid sidetracking this bug I'll open a separate entry for this and leave this bug to concentrate on the changes needed for the upcoming release.
(In reply to comment #11) > I guess to avoid sidetracking this bug I'll open a separate entry for this and > leave this bug to concentrate on the changes needed for the upcoming release. Opened bug 337336 .
So i think we need to add getLocalize... method to the mentionned classes!
Created attachment 190670 [details] Patch * adds support for finding context of an MApplicationElement * fixes PartDescriptorImpl to find localized strings * adds generic method to translate any feature text
(In reply to comment #11) > (In reply to comment #9) > > MPart p = MBasicFactory.INSTANCE.createPart(); > > MUIText t = MBasicFactory.INSTANCE.createUIText(); > > t.setText("BlaBla"); > > p.setLabel(t); > > Hmm... You certainly have a point here. How about: > > p.setLabel(new MUILabel("BlahBlah")), or > > p.setLabel(new MUILabel("%key", IContributor contributor, URI iconURI)) > This would mean that you'll need a special datatype and to represent the String value this would really complicate tooling and naturally the default EMF-Tooling would not really work with this kind of new datatype (this is an assumption). So the only possibility IMHO is to model it and then it looks like I've shown in my other reply. I really don't see any benefit beside the fact that we need to take care that we implement all getLocal* methods. On a side note maybe we should better model them as volatile = true, derived = true, changeable = false features.
Created attachment 190680 [details] example as volatile, transient, not changeable feature This is an example how it would be as a feature instead of a method - what's not yet done is fire notify events but this essentially but I've not invested into it before we've not decided if this is a good idea.
... adding Ed to get in some feedback from other modeling experts
(In reply to comment #14) > Created attachment 190670 [details] > Patch > > * adds support for finding context of an MApplicationElement > * fixes PartDescriptorImpl to find localized strings > * adds generic method to translate any feature text Nice catch, looks good to me. Couple small things to consider: ===> ModelUtils#getContainingContext(): The line: curParent = (MUIElement) ((EObject) element).eContainer(); Is container always going to be an MUIElement? In the cycle below you have: curParent = (MApplicationElement) ((EObject) curParent).eContainer(); ===> LocalizationHelper#getLocalizedFeature(): String key = (String) ((EObject)element).eGet(feature); The LocalizationHelper is probably going to be an API and therefore can be called with all sorts of arguments. Does it make sense to add a check that eGet(feature) actually returns a String?
(In reply to comment #18) > (In reply to comment #14) > > Created attachment 190670 [details] [details] > > Patch > > > > * adds support for finding context of an MApplicationElement > > * fixes PartDescriptorImpl to find localized strings > > * adds generic method to translate any feature text > > Nice catch, looks good to me. Couple small things to consider: > > ===> ModelUtils#getContainingContext(): The line: > curParent = (MUIElement) ((EObject) element).eContainer(); > Is container always going to be an MUIElement? In the cycle below you have: > curParent = (MApplicationElement) ((EObject) curParent).eContainer(); > No it is NOT. In case of PartDescriptor the parent is going to be MApplication which is MContext but NOT MUIElement! > ===> LocalizationHelper#getLocalizedFeature(): > String key = (String) ((EObject)element).eGet(feature); > The LocalizationHelper is probably going to be an API and therefore can be > called with all sorts of arguments. Does it make sense to add a check that > eGet(feature) actually returns a String? Ok. Will do.
(In reply to comment #19) > ... In case of PartDescriptor the parent is going to be MApplication > which is MContext but NOT MUIElement! Hmm... that might not be a perfect example :-): MApplication extends MElementContainer<MWindow> MElementContainer<T extends MUIElement> extends MUIElement I just bugged Eric and it seems that in general: - a model element does not have to be an MUIElement - the model element's eContainer()is going to be an MUIElement Might still be worth adding some "instanceof" checks for the time being.
(In reply to comment #20) > (In reply to comment #19) > > ... In case of PartDescriptor the parent is going to be MApplication > > which is MContext but NOT MUIElement! > > Hmm... that might not be a perfect example :-): > MApplication extends MElementContainer<MWindow> > MElementContainer<T extends MUIElement> extends MUIElement > Ha you are right. The only guarantee we have is that every element is an MApplicationElement! > I just bugged Eric and it seems that in general: > - a model element does not have to be an MUIElement > - the model element's eContainer()is going to be an MUIElement > > Might still be worth adding some "instanceof" checks for the time being. see above MApplicationElement is a save assumption for every element in our domain model!
To restate: MApplicationElement is similar to Object in normal Java. There's no element in our model that does NOT inherit from it.
The correct code is this: public static IEclipseContext getContainingContext(MApplicationElement element) { MApplicationElement curParent = null; if ( element instanceof MUIElement && ((MUIElement)element).getCurSharedRef() != null) curParent = ((MUIElement)element).getCurSharedRef().getParent(); else curParent = (MApplicationElement) ((EObject) element).eContainer(); while (curParent != null) { if (curParent instanceof MContext) { return ((MContext) curParent).getContext(); } if ( (curParent instanceof MUIElement) && ((MUIElement)curParent).getCurSharedRef() != null) curParent = ((MUIElement)curParent).getCurSharedRef().getParent(); else curParent = (MApplicationElement) ((EObject) curParent).eContainer(); } return null; }
I've released the patch with the adoption of MApplicationElement instead of MUIElement which is guaranteed to be the case for any element in our model
ping what are we doing with: * MenuElement#mnemonics * Command#commandName * Command#description (* Category#name) * Catgory#description * Part#description * PartDescriptor#description Paul/Eric is Category#name also subject to translation?
(In reply to comment #25) > ping what are we doing with: > * MenuElement#mnemonics > * Command#commandName > * Command#description > (* Category#name) > * Catgory#description > * Part#description > * PartDescriptor#description > > Paul/Eric is Category#name also subject to translation? Command name and description, category name and description, and mnemonics are all translatable in 3.x PW
Created attachment 193370 [details] Patch I noone objects to those model changes i'm going to release them later today. Please note that you'll have to update your code to use getLocalized!
Created attachment 193371 [details] Patch ups forgot the MenuItem#mnemonics
Created attachment 193377 [details] Test-Patch ... and the missing test model change
I release the 2 patches to HEAD. I think we should open new bugs to make use of the new API
