Bugzilla – Full Text Bug Listing
| Summary: | Support open-ended set of structural features in model components | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] e4 | Reporter: | Oleg Besedin <ob1.eclipse> | ||||||||||||||||
| Component: | UI | Assignee: | Thomas Schindl <tom.schindl> | ||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||||
| Severity: | normal | ||||||||||||||||||
| Priority: | P3 | CC: | bokowski, ob1.eclipse, pwebster, remy.suen, tom.schindl, yves.yang | ||||||||||||||||
| Version: | unspecified | ||||||||||||||||||
| Target Milestone: | 1.0 RC0 | ||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||
| OS: | All | ||||||||||||||||||
| Whiteboard: | |||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Oleg Besedin
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?
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...) Created attachment 170545 [details]
Configure GenModel and Use EStructuralFeature
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. 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. (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? (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. 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.
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.
Created attachment 171714 [details]
Added Testing of Processors
Created attachment 171715 [details]
Added test for single valued feature
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.
Just as a short note, the old functionality is still available and not modified yet until we ported everything to the new system. 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. (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. 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 Yves, I've generated the images for the .edit stuff but I don't really know how this adapter stuff is going to work (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 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.
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. |