Bug 469595 - When using org.eclipse.ui.contexts.window binding context E4 application fails to open
Summary: When using org.eclipse.ui.contexts.window binding context E4 application fail...
Status: NEW
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.5   Edit
Hardware: PC Mac OS X
: P3 normal with 2 votes (vote)
Target Milestone: ---   Edit
Assignee: Platform-UI-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 562497
  Show dependency tree
 
Reported: 2015-06-08 03:48 EDT by Alex Blewitt CLA
Modified: 2020-05-03 12:05 EDT (History)
9 users (show)

See Also:


Attachments
Associated fragment that generates the error (3.19 KB, application/octet-stream)
2015-06-08 03:48 EDT, Alex Blewitt CLA
no flags Details
Contexts and binding tables in legacy applications have same ID's causing resolution troubles (83.68 KB, image/png)
2017-12-12 03:03 EST, Vasili Gulevich CLA
no flags Details
A plugin with a falwed model fragment (6.30 KB, application/zip)
2017-12-13 12:59 EST, Vasili Gulevich CLA
no flags Details
Regression (2.64 KB, image/jpeg)
2017-12-15 05:45 EST, Noopur Gupta CLA
no flags Details
Example or RCP plugin that fails to add a custom command handler to PartDescriptor (6.61 KB, application/zip)
2017-12-21 06:02 EST, Vasili Gulevich CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Blewitt CLA 2015-06-08 03:48:27 EDT
Created attachment 254172 [details]
Associated fragment that generates the error

When using a fragment that imports a BindingContext

org.eclipse.ui.contexts.window

and then uses it with a binding table, running the Eclipse application results in the below exception.

Using the following binding contexts works as expected:

org.eclipse.ui.contexts.dialogAndWindow
org.eclipse.ui.contexts.dialog

---

java.lang.ClassCastException: org.eclipse.e4.ui.model.application.commands.impl.BindingTableImpl cannot be cast to org.eclipse.e4.ui.model.application.commands.MBindingContext
	at org.eclipse.e4.ui.model.application.commands.impl.BindingTableImpl.eSet(BindingTableImpl.java:178)
	at org.eclipse.emf.ecore.impl.BasicEObjectImpl.eSet(BasicEObjectImpl.java:1071)
	at org.eclipse.e4.ui.internal.workbench.ModelAssembler$1.run(ModelAssembler.java:319)
	at org.eclipse.e4.ui.internal.workbench.ModelAssembler.resolveImports(ModelAssembler.java:328)
	at org.eclipse.e4.ui.internal.workbench.ModelAssembler.processModel(ModelAssembler.java:87)
	at org.eclipse.e4.ui.internal.workbench.ResourceHandler.loadMostRecentModel(ResourceHandler.java:259)
	at org.eclipse.e4.ui.internal.workbench.swt.E4Application.loadApplicationModel(E4Application.java:397)
	at org.eclipse.e4.ui.internal.workbench.swt.E4Application.createE4Workbench(E4Application.java:254)
	at org.eclipse.ui.internal.Workbench$5.run(Workbench.java:620)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:337)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:598)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:150)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:139)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:134)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:380)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:235)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:483)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:669)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:608)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1515)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1488)
Comment 1 Simon Scholz CLA 2016-06-23 10:19:09 EDT
I get the same error, but only when I start the application using SWTBot. When I start the product normally everything is fine. Just for JUnit plug-in tests it fails.
Comment 2 Vasili Gulevich CLA 2017-12-11 07:34:17 EST
I get this error every time I try to launch an Eclipse IDE with a fragment importing any binding contexts.

This may be realated to the fact, that compatibility layer provides BindingContexts and BindingTables of same IDs.
Comment 3 Vasili Gulevich CLA 2017-12-12 03:03:05 EST
Created attachment 271858 [details]
Contexts and binding tables in legacy applications have same ID's causing resolution troubles

Attached is a screenshot from Model Spy[1] illustrating that BindingTable references a context of same ID. I suspect this is in a violation of uniqueness constraint in EMF application model.

[1]: http://www.vogella.com/tutorials/Eclipse4LiveModelEditor/article.html
Comment 4 Vasili Gulevich CLA 2017-12-12 10:15:47 EST
In general I find a statement on https://wiki.eclipse.org/Eclipse4/RCP/Modeled_UI/Contributing_to_the_Model misleading:

-----It is important that any model elements that you intend to reference have a unique elementId (e.g., the MApplication).
-----

This does not seem to imply that an attempt to dereference a contribution with non-unique ID causes fatal startup failure.

I see two possible ways to solve the problem:

1. Relax a constraint on model element to have an unique element id to only require an pair "element type + element id" to be unique. This way binding contexts won't conflict with their own binding tables and views won't be shadowed by their own contexts. However this still leaves an open problem of inconsistent resolution of model elements with non-unique ids if they are of same type.

2. Change legacy model contributions to provide unique ids for important elements like commands, contexts, parts.

I've started implementing a patch for second approach, but have second thoughts now. The solution is quite intrusive and is peppered all over legacy code.

I'll complete it nevertheless, create a Gerrit change and will attempt first approach.
Comment 5 Lars Vogel CLA 2017-12-12 10:23:35 EST
Vasili, I assume the ID is not causing the issue. Unique ids is usually not required. Can you please create a mnimal RCP application which demonstrate the issue?
Comment 6 Eclipse Genie CLA 2017-12-12 14:38:52 EST
New Gerrit change created: https://git.eclipse.org/r/113250
Comment 7 Vasili Gulevich CLA 2017-12-13 12:59:24 EST
Created attachment 271894 [details]
A plugin with a falwed model fragment

(In reply to Lars Vogel from comment #5)
> Vasili, I assume the ID is not causing the issue. Unique ids is usually not
> required. Can you please create a mnimal RCP application which demonstrate
> the issue?

Please find a demonstration of exception from original report in attachment and  on

https://github.com/basilevs/bug469595

Exception is addressable by a trivial fix in ModelAssembler (we just need a null check).

My final goal however is to contribute a key binding and a command handler to a legacy application, and this is impossible when context or command have no unique IDs. Contributions are either silently ignored or cause warnings.
I now work on another sample RCP which would demonstrate my trouble.
Comment 9 Thomas Schindl CLA 2017-12-14 16:46:25 EST
The problem is the fragment merger who has to restrict himself to the correct types
Comment 10 Thomas Schindl CLA 2017-12-14 16:51:23 EST
I generally agree that the IDs chosen in the Addon are not good but:
a) changing IDs is a breaking change because someone might already depend on this invalid id
b) we should also fix the real problem which is found in the fragment merging where the wrong element is looked up it needs to constraint the search not just by the ID but also by type is searches for
Comment 11 Eclipse Genie CLA 2017-12-15 05:42:08 EST
New Gerrit change created: https://git.eclipse.org/r/113463
Comment 13 Noopur Gupta CLA 2017-12-15 05:45:27 EST
Created attachment 271923 [details]
Regression

(In reply to Eclipse Genie from comment #8)
> Gerrit change https://git.eclipse.org/r/113250 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=03aaf5d2ff623dec5f790a09d9538a6ce2b1ac85
> 
Reverted this with comment #12. It causes test failures in JDT UI (see https://ci.eclipse.org/jdt/job/eclipse.jdt.ui-Gerrit/89/) due to:

java.lang.IllegalArgumentException: can't find IDselectAll
	at org.eclipse.jface.action.ContributionManager.insertAfter(ContributionManager.java:324)
	at org.eclipse.jface.action.SubContributionManager.insertAfter(SubContributionManager.java:137)
	at org.eclipse.jdt.internal.ui.javaeditor.BasicJavaEditorActionContributor.contributeToMenu(BasicJavaEditorActionContributor.java:147) 
...

Also, the editor doesn't open in the UI. See attached screenshot.
Comment 14 Dani Megert CLA 2017-12-15 09:09:23 EST
(In reply to Eclipse Genie from comment #8)
> Gerrit change https://git.eclipse.org/r/113250 was merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=03aaf5d2ff623dec5f790a09d9538a6ce2b1ac85
> 

This completely destroyed the build.
Comment 15 Eclipse Genie CLA 2017-12-19 05:55:04 EST
New Gerrit change created: https://git.eclipse.org/r/114371
Comment 16 Vasili Gulevich CLA 2017-12-19 06:05:11 EST
(In reply to Lars Vogel from comment #5)
> Vasili, I assume the ID is not causing the issue. Unique ids is usually not
> required. 

Type-base ID conflict resolution is prototyped in https://git.eclipse.org/r/#/c/114371/1

No type information is available in 
 - org.eclipse.e4.ui.internal.workbench.ModelAssembler.runProcessor(IConfigurationElement)
 - org.eclipse.e4.ui.model.fragment.impl.StringModelFragmentImpl.mergeIdList(MApplication, List<MApplicationElement>, String)

Lars, do you have any ideas on how that was supposed to work for non-unique identifiers?

I also did not modify org.eclipse.e4.ui.internal.workbench.ModelFragmentComparator.compare(ModelFragmentWrapper, ModelFragmentWrapper) which may benefit from proper identification.
Comment 17 Vasili Gulevich CLA 2017-12-21 06:02:48 EST
Created attachment 272004 [details]
Example or RCP plugin that fails to add a custom command handler to PartDescriptor

A sample plugin that fails to add a custom command handler to PartDescriptor is attached.
Comment 18 Rolf Theunissen CLA 2019-05-30 12:29:47 EDT
(In reply to Eclipse Genie from comment #6)
> New Gerrit change created: https://git.eclipse.org/r/113250

Most likely the root cause of the regression of this change is: In CoolBarToTrimManager the ToolItem elementId was changed as well, however this element should shadow the ID of the IContributionItem which is not a model-element.

So still two options are open to resolve this bug:
1. Relax the constraint, i.e. unique ID's per element type.
2. Change (legacy) model elements to be unique.

I think option 2 is easier to implement. Also, if the elementId would be marked as 'id' in the EMF model, there would be support for validation of uniqueness, among others.
W.r.t. extensions that already reference ElementId's that will be changed. I think that those extensions are quite brittle as it is now, they might change any time some ordering in the model changes.