Bug 332395

Comment 1 Thomas Schindl 2010-12-12 12:21:15 EST

Created attachment 185031 [details]
patch

Comment 2 Paul Webster 2010-12-13 10:25:27 EST

(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

Comment 3 Thomas Schindl 2010-12-13 10:41:42 EST

(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).

Comment 4 Thomas Schindl 2010-12-14 16:22:56 EST

Any objections to my last comment - I'd like to proceed with this ASAP because it blocks my work on the translation story

Comment 5 Paul Webster 2010-12-14 16:36:34 EST

(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

Comment 6 Thomas Schindl 2010-12-14 17:05:09 EST

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

Comment 7 Thomas Schindl 2010-12-14 17:20:55 EST

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)

Comment 8 Thomas Schindl 2010-12-14 17:27:05 EST

(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

Comment 9 Thomas Schindl 2010-12-14 17:46:32 EST

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

Comment 10 Oleg Besedin 2010-12-15 10:03:52 EST

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.

Comment 11 Thomas Schindl 2010-12-15 10:55:44 EST

Created attachment 185234 [details]
junit-test

This is a JUnit-Test which checks that all our none abstract, none interface types are MApplicationElement instances

Comment 12 Thomas Schindl 2010-12-15 11:26:19 EST

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

Comment 13 Thomas Schindl 2010-12-15 12:41:49 EST

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 :-)

Comment 14 Thomas Schindl 2010-12-15 12:46:58 EST

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.

Comment 15 Thomas Schindl 2010-12-17 04:21:28 EST

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.

Comment 16 Thomas Schindl 2010-12-17 04:30:05 EST

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.

Comment 17 Thomas Schindl 2010-12-17 13:51:43 EST

Oleg, Eric - I'd like to check this model changes in. Paul is ok with them and naturally I'm as well :-)

Comment 18 Oleg Besedin 2010-12-17 14:08:38 EST

(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.

Comment 19 Thomas Schindl 2010-12-17 16:24:04 EST

Eric, now only there's only one UIElement implementation

Comment 20 Thomas Schindl 2010-12-17 16:33:38 EST

I'd like to point you all to bug 332396 which will save us some more bytes in memory