Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 315098 - Support open-ended set of structural features in model components
Summary: Support open-ended set of structural features in model components
Status: RESOLVED FIXED
Alias: None
Product: e4
Classification: Eclipse Project
Component: UI (show other bugs)
Version: unspecified   Edit
Hardware: PC All
: P3 normal (vote)
Target Milestone: 1.0 RC0   Edit
Assignee: Thomas Schindl CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-31 11:24 EDT by Oleg Besedin CLA
Modified: 2010-06-16 09:07 EDT (History)
6 users (show)

See Also:


Attachments
Configure GenModel and Use EStructuralFeature (2.22 KB, patch)
2010-05-31 11:51 EDT, Thomas Schindl CLA
no flags Details | Diff
Workin progress (127.16 KB, patch)
2010-06-10 19:13 EDT, Thomas Schindl CLA
no flags Details | Diff
Started writing test suite (30.84 KB, patch)
2010-06-11 07:35 EDT, Thomas Schindl CLA
no flags Details | Diff
Added Testing of Processors (137.91 KB, patch)
2010-06-11 08:29 EDT, Thomas Schindl CLA
no flags Details | Diff
Added test for single valued feature (138.45 KB, patch)
2010-06-11 08:45 EDT, Thomas Schindl CLA
no flags Details | Diff
Added Test for StructuralFeatureContribution (143.31 KB, patch)
2010-06-11 09:00 EDT, Thomas Schindl CLA
no flags Details | Diff
XWT - Fallback (4.62 KB, patch)
2010-06-15 17:37 EDT, Thomas Schindl CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oleg Besedin CLA 2010-05-31 11:24:58 EDT
From bug 309867 comment 3 to comment 9 by Tom Schindl:

===
I think the current modelcomponent stuff is not
really useable because it lacks extensibility, whenever we add a new attribute
(with name xyz) we need to add one to the ModelComponent.

The attached patch adds 3 new classes who do the same ModelComponents and
ModelComponent are used for today but do it i a much more flexible way.

The 3 classes are:
ApplicationComponents:
 placeholders: List<ApplicationElement>
 components: List<ApplicationElementComponent>
 processors: List<ApplicationElementProcessor>

ApplicationElementComponent:
 feature: EStructuralFeature
 elements: List<EObject>
 positionInParent: String
 parentID: String

ApplicationElementProcessor:

===
Currently missing contributing possibilities in ModelComponent are:
* Part#menus
* Part#toolbar
* Window/Perspective#windows
* TimmedWindow#trimBars
* Window#mainMenu

If we stick to the ModelComponent stuff we need to add those attributes there!
===
Comment 1 Oleg Besedin CLA 2010-05-31 11:30:13 EDT
I could not get a reference to EStructuralFeature to propagate into the genmodel and then generate code.

I added to the ecore model:

    <eStructuralFeatures xsi:type="ecore:EReference" name="feature" eType="ecore:EClass Ecore.ecore#//EStructuralFeature"/>

And added to the genmodel:

      <genFeatures notify="false" createChild="false" propertySortChoices="true" ecoreFeature="ecore:EClass UIElements.ecore#//ModelComponent/feature"/>

and then can't open the genmodel editor:
- either I need to generate Ecore.ecore and then have reference to the "MEStructuralFeature" :-);
- using ecore:EReference or ecore:EDataType all fail for different reasons

Is there a way to generate genmodel from the updated ecore and then use it to generate code?
Comment 2 Oleg Besedin CLA 2010-05-31 11:38:30 EDT
From bug 309867 comment 5:
> if you are not comfortable with the "feature: EStructuralFeature" and it could
> be replace probably also with the feature name and represented as a String.

But that will take us another step from being able to edit e4 model files in a text editor. (Which might be OK and necessary, but needs to be discussed...)
Comment 3 Thomas Schindl CLA 2010-05-31 11:51:34 EDT
Created attachment 170545 [details]
Configure GenModel and Use EStructuralFeature
Comment 4 Thomas Schindl CLA 2010-05-31 11:55:28 EDT
A draw back when we use EStructuralFeature instead of simply the feature-name (EString) is that we present EMF-Types in our API on the other hand using EStructuralFeature would allow tooling to validate stuff based upon the meta information.

So probably:

--------8<--------
ApplicationElementComponent:
 feature: EString
 elements: List<EObject>
 positionInParent: String
 parentID: String
--------8<--------

is better.
Comment 5 Thomas Schindl CLA 2010-06-10 03:32:29 EDT
Any news on this? Is there someone working on this or do we postpone it after 4.1 (I ask because Paul had to introduce a new MenuContributionsContainer) and other parts like the Simple IDE suffer from the problem that they can't use the contribution system as it is today.

If nobody is working on it and we want it in 4.0, I can drive it and provide a first implementation.
Comment 6 Oleg Besedin CLA 2010-06-10 14:25:44 EDT
(In reply to comment #5)
> If nobody is working on it and we want it in 4.0, I can drive it and provide a
> first implementation.

That would be great. Long-term I am sure that we'll move away from the ecore sample reflective editor, so using strings sounds more future-proof.

Is there some natural place where we can have the list of model elements and corresponding strings?
Comment 7 Thomas Schindl CLA 2010-06-10 14:47:40 EDT
(In reply to comment #6)
> (In reply to comment #5)
> > If nobody is working on it and we want it in 4.0, I can drive it and provide a
> > first implementation.
> 
> That would be great. Long-term I am sure that we'll move away from the ecore
> sample reflective editor, so using strings sounds more future-proof.
> 

I'm not sure how using EStructureFeature or String relate to the reflective Editor. 

> Is there some natural place where we can have the list of model elements and
> corresponding strings?

I'm not sure I can follow. You can never ever have a complete list of feature names because one can extend our .ecore and then the set of strings is completely different.
Comment 8 Thomas Schindl CLA 2010-06-10 19:13:55 EDT
Created attachment 171680 [details]
Workin progress

This is a work in progress code ... 

Here to design I've taken:
* Add 2 new elements to our model-extension point
  * fragment: Allows you to contribute a fragement.e4xmi similar to snippets
  * processor: Allows you to contribute a processor (which is an POJO)
    the POJO gets created by DI and calls an annotated @Execute-Method
* The Fragment-Definition is not part of the UIElements-POJO but is found in an 
  Fragment.ecore. It consists of 4 Types
  * ModelFragments
    imports: List<MApplicationElement>
    fragments: List<MModelFragment>
  * ModelFragment:
    elements: List<MApplicationElement>
    merge(MApplication): List<MApplicationElement>
  * StringModelFragment:
    featurename: String
    parentElementId: String
    listPositionPath: String
  * StructuralModelFragment:
    feature: EStructuralFeature
    parentElementId: String
    listPositionPath: String

The main idea is that the mergeing is happening in the merge-method which can be implemented by clients (StringModelFragment/StructuralModelFragment) based on the information the EObject has. This gives us the freedome keep our core-API free from EMF (we could e.g. move StructuralModelFragment to a completely different bundle) but gives people who want to use EMF-Features for Tooling the possibility to do so, the same is true for listPositionPath, parentElementMatching, ... .

The attached code is feature complete but completely untested. Tomorrow I'll start writing a test suite.
Comment 9 Thomas Schindl CLA 2010-06-11 07:35:14 EDT
Created attachment 171707 [details]
Started writing test suite

fixed some minor bugs - the current state brings us fairly close to what ModelComponents can do today.
Comment 10 Thomas Schindl CLA 2010-06-11 08:29:10 EDT
Created attachment 171714 [details]
Added Testing of Processors
Comment 11 Thomas Schindl CLA 2010-06-11 08:45:10 EDT
Created attachment 171715 [details]
Added test for single valued feature
Comment 12 Thomas Schindl CLA 2010-06-11 09:00:17 EDT
Created attachment 171717 [details]
Added Test for StructuralFeatureContribution

Boris, Oleg - All tests passes and I think I'm ready for a review. There's one thing I'm not sure about because I simply copied it from the ModelExtensionProcessor.

It't the code which copies IDs from the old resource using E4XMIResource#getInternalId()/setInternalId but it only does it on the root top elements but not on those deeper in the graph.
Comment 13 Thomas Schindl CLA 2010-06-11 09:16:09 EDT
Just as a short note, the old functionality is still available and not modified yet until we ported everything to the new system.
Comment 14 Oleg Besedin CLA 2010-06-11 11:51:05 EDT
Thanks Tom, that was quick!

Few points:
====
In the patch you add:
- StringModelFragment (to specify destination feature as a string)
- StructuralModelFragment (to specify destination feature as a structural ref)

that's cool from the technical side, but... I strongly believe that we should not have two ways to do one thing. In this case, after trying both approaches, I strongly prefer string-based version:

When I tried to create a fragment based on StructuralModelFragment, the drop-down boxes in the Ecore sample editors stay empty. As such I needed to resort to either Notepad or a specialized editor to work with those fragments - which means that using structural features does not have any advantages over using strings. 

So, I'd go with the string-based descriptions. (We do need to add doc on how developers can determine those values.)

===
Personally, I'd prefer the current ModelComponents modified so that we can do changes in one step rather then go duplicate / deprecate approach. That's just my personal preference and whoever does the work picks the way so feel free to ignore it :-).

===
Names:
ModelProcessor - we have ModelComponent.processor; it gets confusing
listPositionPath - hmm... what? :-) 

I do like "model fragment" names, that's way better then the "old" "model component".

====
Couple specific code places:
- UIAllTests.launch - patch contains wrong change (setting Mac-specific entry)
- ResourceHandler - last block of changes - having "context" local variable is confusing as it overrides field named "context" and, besides, it does not seem to be needed

(In reply to comment #12)
> It't the code which copies IDs from the old resource using
> E4XMIResource#getInternalId()/setInternalId but it only does it on the root top
> elements but not on those deeper in the graph.

Hmm... I think this was fine before we got shared parts. If the fragment is only added to one place in the UI model, that should be fine, but I am not sure how it plays with shared parts present. 

====
Overall, this is a very nice change.
Comment 15 Thomas Schindl CLA 2010-06-11 12:01:49 EDT
(In reply to comment #14)
> Thanks Tom, that was quick!
> 
> Few points:
> ====
> In the patch you add:
> - StringModelFragment (to specify destination feature as a string)
> - StructuralModelFragment (to specify destination feature as a structural ref)
> 
> that's cool from the technical side, but... I strongly believe that we should
> not have two ways to do one thing. In this case, after trying both approaches,
> I strongly prefer string-based version:
> 

Yeah I'm fine with moving this out. The system is designed in a way that I can provide this as part of my tooling effort :-)

> When I tried to create a fragment based on StructuralModelFragment, the
> drop-down boxes in the Ecore sample editors stay empty. As such I needed to
> resort to either Notepad or a specialized editor to work with those fragments -
> which means that using structural features does not have any advantages over
> using strings. 

+1

> 
> So, I'd go with the string-based descriptions. (We do need to add doc on how
> developers can determine those values.)

+1

> 
> ===
> Personally, I'd prefer the current ModelComponents modified so that we can do
> changes in one step rather then go duplicate / deprecate approach. That's just
> my personal preference and whoever does the work picks the way so feel free to
> ignore it :-).
> 

Well I'll try to remove the old ModelComponent code ASAP once i have the new code in but we are very late for rc0 and I don't want to block people so this seemed to be an natural thing.

> ===
> Names:
> ModelProcessor - we have ModelComponent.processor; it gets confusing
> listPositionPath - hmm... what? :-) 
> 

Well it's positionInParent (I'll try to come up with a better name). ModelComponent.process will go. I'll try to come up with a better name.

> I do like "model fragment" names, that's way better then the "old" "model
> component".
> 
> ====
> Couple specific code places:
> - UIAllTests.launch - patch contains wrong change (setting Mac-specific entry)
> - ResourceHandler - last block of changes - having "context" local variable is
> confusing as it overrides field named "context" and, besides, it does not seem
> to be needed


ok, I think create this local context so that when old values are not left over.

> 
> (In reply to comment #12)
> > It't the code which copies IDs from the old resource using
> > E4XMIResource#getInternalId()/setInternalId but it only does it on the root top
> > elements but not on those deeper in the graph.
> 
> Hmm... I think this was fine before we got shared parts. If the fragment is
> only added to one place in the UI model, that should be fine, but I am not sure
> how it plays with shared parts present. 
> 
> ====
> Overall, this is a very nice change.

I'll make the changes and commit the changes this weekend.
Comment 16 Thomas Schindl CLA 2010-06-12 12:22:49 EDT
I've released changes to CVS and now start migrating our model components to the new system. 

I've also implemented the "positionInList" stuff which currently supports the following syntax:
* first
* before:my.element.id
* after:my.element.id
* index:0
Comment 17 Thomas Schindl CLA 2010-06-12 12:56:09 EDT
Yves, I've generated the images for the .edit stuff but I don't really know how this adapter stuff is going to work
Comment 18 Paul Webster CLA 2010-06-14 18:23:48 EDT
(In reply to comment #16)
> I've released changes to CVS and now start migrating our model components to
> the new system. 
> 
> I've also implemented the "positionInList" stuff which currently supports the
> following syntax:
> * first
> * before:my.element.id
> * after:my.element.id
> * index:0

I was just going to mention that the positionInParent format that we use is before=id, after=id, etc

I'd prefer to use this format here as well.

PW
Comment 19 Thomas Schindl CLA 2010-06-15 17:37:19 EDT
Created attachment 171998 [details]
XWT - Fallback

Yves, your code is referenceing. ModelComponents and the ModelExtensionProcessor who will be gone tomorrow. 

Can you try to update your code? The attached patch is simply commenting out the failing code - the replacement for ModelExtensionProcessor is ModelAssembler.
Comment 20 Thomas Schindl CLA 2010-06-16 09:07:38 EDT
The old system is gone now.

@Yves: I fixed your code as good as I could replacing ModelExtensionProcessor through ModelAssembler but I also added //FIXME Yves check ... so that you can do the final clean up your own.