Community
Participate
Working Groups
Currently some of our elements are not inheriting from ApplicationElement but I think it would be benefitial if every element in our model can be savely casted to an ApplicationElement here are the types (including abstract ones) who violate this contract: * BindingTableContainer * Bindings * HandlerContainer * KeySequence * Context * Dirtyable * UILabel * Expression * MenuContributions * ToolbarContributions * TrimContributions * PartDescriptorContainer
Created attachment 185031 [details] patch
(In reply to comment #0) > Currently some of our elements are not inheriting from ApplicationElement but I > think it would be benefitial if every element in our model can be savely casted > to an ApplicationElement here are the types (including abstract ones) who > violate this contract: > * BindingTableContainer > * HandlerContainer > * KeySequence > * Context > * MenuContributions > * ToolbarContributions > * Expression I can't speak to the other items, but it doesn't make sense for the above to inherit from ApplicationElement. They're not identifiable individually. Some are "mixin" interfaces, meant to be added to application elements. Some are simply typed objects where the value provides identity. PW
(In reply to comment #2) > (In reply to comment #0) > > Currently some of our elements are not inheriting from ApplicationElement but I > > think it would be benefitial if every element in our model can be savely casted > > to an ApplicationElement here are the types (including abstract ones) who > > violate this contract: > > > * BindingTableContainer > > * HandlerContainer > > * KeySequence > > * Context > > * MenuContributions > > * ToolbarContributions > > * Expression > > I can't speak to the other items, but it doesn't make sense for the above to > inherit from ApplicationElement. They're not identifiable individually. Some > are "mixin" interfaces, meant to be added to application elements. Some are > simply typed objects where the value provides identity. > > PW The problem is that if you don't take good care those mixins are in the end e.g. KeySequence showing up in a real type which does not inherit from ApplicationElement and we have various positions in our code where we blindly cast to MApplicationElement element. It's going to be curcucial that we can identify all ModelElements to their contributing bundle see #331010 and if we could ensure that every element has at its root MApplicationElement and hence ApplicationElementImpl we have one position in our code. If we don't inherit from MApplicationElement (which to me is similar to java.lang.Object) I think we definately need a super class which acts as it because of #331010. Beside that you say you don't want people to contribute to e.g. MenuContributions, ToolbarContributions I ask because you can only contribute to MApplicationElements (at least currently until I made my XPath stuff work).
Any objections to my last comment - I'd like to proceed with this ASAP because it blocks my work on the translation story
(In reply to comment #4) > Any objections to my last comment - I'd like to proceed with this ASAP because > it blocks my work on the translation story I'm still not comfortable with making these identifiable when they're not. PW
I'm not sure what you mean with identifiable when they are not. I'm open to suggestions but we really have to track a contributor to every element we have in our model instance, so there has to be a super class which has a contributor field. I don't mind creating
let me elborate why I thing the *ApplicationElementImpl* should be the base class for every of our model element. Think about the following, I'll have to add a contributor-field to ApplicationElement and to make it at runtime as memory effecient as possible I'll have to specialize the setContributor()-method to make String.intern() so that we don't waste a lot of String instance. Now in the current setup I'll have to implement this method in: * KeyBindingImpl * PartDescriptorImpl * ApplicationElementImpl and the likelyness that other have to make this too e.g. when inheriting from e.g. UILabel (which I agree is a typical mixin)
(In reply to comment #6) > I'm not sure what you mean with identifiable when they are not. > > I'm open to suggestions but we really have to track a contributor to every > element we have in our model instance, so there has to be a super class which > has a contributor field. The current mixin interfaces make it simply too easy to omit this requirement as you can e.g. see with CoreExpression
Created attachment 185185 [details] patch This is the minimal change I'd like to get in: a) Reorders inheritance in KeyBinding so that it first inherits from ApplicationElement b) Adds ApplicationElement to CoreExpression So that I can implement the contribution tracking because every instance in our application model implements MApplicationElement
I think I am with Tom on this one. - The elements that don't make sense on their own: every model object potentially can be altered programmatically, and, hence, need to have a unique ID and be individually identifiable. - The elements that are used only in mix-ins: there are no drawbacks in making them inherit from ApplicationElement, assuming that they are used to create model elements. For instance, BindingTableContainer is used [only] by Application which already has ApplicationElement inheritance via UIElement. The alternative is to somehow enforce that every constructed model element inherits from ApplicationElement and that won't work well with our current EMF factories.
Created attachment 185234 [details] junit-test This is a JUnit-Test which checks that all our none abstract, none interface types are MApplicationElement instances
While discussing with Eric I came to the following conclusion: a) Everything we expect to be a pure mixin has to be abstract=true, interface=true b) Everything else - also abstract classes - have to inherit from ApplicationElement
Created attachment 185247 [details] patch This patch does what I'd suggest in the comment above: a) Makes mixin classes interface=true * Context * Dirtyable * Input * Bindings * UILabel * MenuContributions * ToolBarContributions * TrimContributions * PartDescriptorContainer b) Makes abstract types inherit from ApplicationElement * Expression c) Reorders, Code Gen Optimizations * Part inherit additonally from UIElement (one might think this is a duplication and from an API PoV this correct but from a code gen PoV => this way we inherit many fields from base classes => ~500 lines) * PartDescriptor make ApplicationElement the first inheritance => First element should always be not be an interface => ~200 lines * KeySequence make ApplicationElement first super class * PlaceHolder add ApplicationElement (looks like a duplication which it is not) The Unit-test now has a first though quite dumb check that our model code is from code gen PoV optimal - there's still room for more optimizations :-)
oh forgot the patch is from an xmi/delta PoV backward compatible - bundles who have subclassed us (e.g. simpleide) might got broken because some concrete types vanished.
We talked about this a bit on the call. Can we get this in and then work on the problem Eric is having with multiple UIElement implementations? I think I can improve my JUnit checks to take things like the UIElement into account.
Erics problem that when we start to specialize methods to ensure consistency and the problem that we have to implement this could maybe solved also by roleing our own EStore and use Feature Delegation. I'll add Ed maybe he can give insights on this.
Oleg, Eric - I'd like to check this model changes in. Paul is ok with them and naturally I'm as well :-)
(In reply to comment #17) > Oleg, Eric - I'd like to check this model changes in. Paul is ok with them and > naturally I'm as well :-) Looks good to me.
Eric, now only there's only one UIElement implementation
I'd like to point you all to bug 332396 which will save us some more bytes in memory
