Bug 344948

Comment 1 Ed Willink 2011-05-25 05:31:38 EDT

Created attachment 196521 [details]
Fix to make action work if Ecore prototype available

Create Dynamic Instance is used in the OCLinEcore tutorial, so it's a bit embarassing if it doesn't work.

Attached makes it work proved the required EClass was in the input Ecore file; if not a save and re-open message appears.

All changes are in ...oclinecore.ui so impact should be limited to OCLinEcore editor.

a) some logic to actually find the EClass: new action code no risk
b) use the same TypeManager for OCLinEcore edit
c) leave the loaded Ecore resource in the TypeManager's externalResourceSet

b) and c) risk a fight between original and changed content, however since the original content is still not adequately findable from the new; the code in a) has to search hard to find it, it seem unlikely to interact.

Realistic tradeoff:

no-patch => no Create Dynamic Instance => tutorial change
patch => might be an obscure issue after an Ecore to OCLinEcore persistence mode change

The main use case of edit and save should not be at risk.

Comment 2 Ed Willink 2011-05-27 01:36:26 EDT

Created attachment 196724 [details]
Fix for erroneous pivot URI

Simpler patch fixing the root cause.

I used to create the pivot URI of e.g. http://xxx.ecore as http://xxx.ecore.pivot and changed to pivot:://http/xxx.ecore so that URI suffixes were unaffected, but missed one application of the ".pivot" suffix.

With the fix, the mapping to the Ecore source doesn't get lost, so there is no need to search for it.

Comment 3 Axel Uhl 2011-05-27 04:46:34 EDT

(In reply to comment #2)
Ed,

I'm trying to understand the code enough to provide some sort of a review, although there is probably no way I can quickly work my way into the larger context of the Pivot implementation for now.

In CreateDynamicInstanceHandler.execute, should the error message be externalized into the plugin.properties?

Unfortunately, I can't get my workspace to build, not talking even about run because I can't seem to find the com.google.collect in a <1.0.0 version required by the Xtext stuff, so Xtext doesn't build and transitively nothing using Xtext builds.

Generally, the way I see the createResource(URI, String) usage, you're now creating resources that have the same URI as the original Ecore resources, only with a different content type. We discussed this already at EclipseCon. I don't think it's a good idea to have resources with distinct content having the same URI, even though you argue that the Pivot elements are "logically" the "same" as their underlying Ecore counterparts, but you know I disagree with this position.

+1 to commit if you say that the tests are still all ok (which I for said reasons currently can't easily validate). In the long run, when it comes to promoting the Pivot stuff to plugins, I'd like to have this discussion again about element identity and the co-existence of Pivot and Ecore elements which really worries me.

Comment 4 Ed Willink 2011-05-27 05:20:55 EDT

(In reply to comment #3)
> (In reply to comment #2)
> I'm trying to understand the code enough to provide some sort of a review,
> although there is probably no way I can quickly work my way into the larger
> context of the Pivot implementation for now.

No problem. I'm in Zurich at Tools Sunday to Saturday, so we can discuss and plan anything you want. Perhaps you could preepare an agenda of what to look at. 
> 
> In CreateDynamicInstanceHandler.execute, should the error message be
> externalized into the plugin.properties?

The examples plugins are only partially internationalized. This must be resolved when promting from examples.
> 
> Unfortunately, I can't get my workspace to build, not talking even about run
> because I can't seem to find the com.google.collect in a <1.0.0 version
> required by the Xtext stuff, so Xtext doesn't build and transitively nothing
> using Xtext builds.

I'm using Xtext RC1 at present. It could be that RC2 has the Orbit/Xtext conflict that broke our build 523. Try Xtext from the Hudson job latest successful build.
> 
> Generally, the way I see the createResource(URI, String) usage, you're now
> creating resources that have the same URI as the original Ecore resources, only
> with a different content type. We discussed this already at EclipseCon. I don't
> think it's a good idea to have resources with distinct content having the same
> URI, even though you argue that the Pivot elements are "logically" the "same"
> as their underlying Ecore counterparts, but you know I disagree with this
> position.

Yes and No. The related elements have the same fragment URI but different resource URIs, so fragments can be cross-resolved, but full URIs are unique.
> 
> +1 to commit if you say that the tests are still all ok (which I for said
> reasons currently can't easily validate). In the long run, when it comes to
> promoting the Pivot stuff to plugins, I'd like to have this discussion again
> about element identity and the co-existence of Pivot and Ecore elements which
> really worries me.

Thanks,

Comment 5 Ed Willink 2011-05-27 05:35:29 EDT

Committed to HEAD for RC3.

Comment 6 Ed Willink 2012-05-29 13:23:38 EDT

Closing all bugs resolved in Indigo.