Bugzilla – Full Text Bug Listing
| Summary: | org.eclipse.e4.ui.workbench has a dependency on SWT | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] e4 | Reporter: | Thomas Schindl <tom.schindl> | ||||||||||||||||||||||||||
| Component: | UI | Assignee: | Project Inbox <e4.ui-inbox> | ||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||||||||||||||
| Severity: | major | ||||||||||||||||||||||||||||
| Priority: | P3 | CC: | bokowski, hsoliwal, john.arthorne, ob1.eclipse, pwebster, remy.suen, tjwatson | ||||||||||||||||||||||||||
| Version: | 0.9 | ||||||||||||||||||||||||||||
| Target Milestone: | 4.1 M6 | ||||||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||||||
| OS: | Mac OS X - Carbon (unsup.) | ||||||||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||||||||
| Bug Depends on: | 321995, 322226, 338559 | ||||||||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||
|
Description
Thomas Schindl
Created attachment 174142 [details]
patch
*** Bug 319721 has been marked as a duplicate of this bug. *** Created attachment 174146 [details]
Patch
This address the complete problem of getting SWT dependencies in ui.workbench through dependencies
and the saga continues with org.eclipse.e4.ui.workbench having a direct dependency on "org.eclipse.jface.bindings" and "org.eclipse.e4.ui.bindings"(=> depending once more on SWT) Updateting title! org.eclipse.e4.ui.workbench should be free of dependencies on SWT this has been one of the core design decision we've taken when designing e4 and it looks we are violating it all over the place It looks the only thing needed is to move E4CommandProcessor to ui.workbench.swt and we are done This is probably not something you can undo in RC2 PW I tend to agree (the risk and time to change it is probably too big) and I'm really sorry that I had not discovered such a major (it's probably not the right term but I don't know another one) design defect. For me this is really one of the worst things when looking at e4 from the none Eclipse 4.0 SDK developers point of view because it is simply violating the e4 design as I understood and promoted it the last 2 years. Created attachment 174161 [details]
Patch
This patch simply moves the E4CommandProcessor to SWT (this is not 100% ideal because one would have to reimplement the logic) but now ui.workbench once more has 0 dependencies on SWT/JFace
Created attachment 174177 [details]
Patch
This patch follows another route for DI and Services because there the dependency on SWT is only there to sync with the Display-Loop but there's a much better possibility to do this using the Realm provided by Eclipse Databinding.
The command processor change is the same than before - I can only say that the workbench runs and behaves as expected, the ui-test pass with the realm fix and rcp applications like Contact-Demo are still working.
Paul do you still think this is too risky? Anyone else? I can only once more say I'd really like to see this fixed I'm +1 on the goal (but haven't looked at the patch in detail). Created attachment 174183 [details]
Patch including XWT
updated to HEAD and included change for XWT
(In reply to comment #10) > Created an attachment (id=174177) [details] > Patch > This patch follows another route for DI and Services because there the > dependency on SWT is only there to sync with the Display-Loop but there's a > much better possibility to do this using the Realm provided by Eclipse > Databinding. I would be careful with that - there is a little piece of code in there trying to ensure that we pick up proper value for the Display. (For instance, that we don't try to create a new Display on shutdown.) The Realm code will use something like SWTObservables.getRealm(Display) which might or might not perform similar checks. Bottom line, it all sounds good and makes sense, but I feel that at this point (2 weeks before the release) we should limit code changes to what is clearly necessary for the release. If we, say, had a demo running e4 in a non-SWT environment, then this change could be justified, but as it is... Myself, I'd leave it for after release. Created attachment 174194 [details]
patch
Paul said the previous patch breaks event ordering now it is done 100% at the same time id had been before.
(In reply to comment #14) > (In reply to comment #10) > > Created an attachment (id=174177) [details] [details] > > Patch > > This patch follows another route for DI and Services because there the > > dependency on SWT is only there to sync with the Display-Loop but there's a > > much better possibility to do this using the Realm provided by Eclipse > > Databinding. > > I would be careful with that - there is a little piece of code in there trying > to ensure that we pick up proper value for the Display. (For instance, that we > don't try to create a new Display on shutdown.) > I saw the code but Realm is not creating displays because it has one or not which is set on start up. It simply wraps the syncExec/asyncExec. > The Realm code will use something like SWTObservables.getRealm(Display) which > might or might not perform similar checks. The realm is created on start of the system with the display used by the application so there's no need for this Display checks found in the original code. If realm would broken all the renderes would be as well. > > Bottom line, it all sounds good and makes sense, but I feel that at this point > (2 weeks before the release) we should limit code changes to what is clearly > necessary for the release. If we, say, had a demo running e4 in a non-SWT > environment, then this change could be justified, but as it is... Myself, I'd > leave it for after release. I know and give up at this point - I have to confess that I'm a bit frustrate about myself (mostly) not having found out and us introducing such a fundamental design error. Paul - can we together look at the E4CommandProcessor because this look like a bit tricky to get right. (In reply to comment #17) > Paul - can we together look at the E4CommandProcessor because this look like a > bit tricky to get right. Yes, over the next couple of weeks we need to go over the architecture, both high level and in specific places. ex: commands should be UI free. The bindings need to be processed at the UI level, and the implementation (EBindingService) will be tied to SWT/JFace. PW (In reply to comment #18) > (In reply to comment #17) > > Paul - can we together look at the E4CommandProcessor because this look like a > > bit tricky to get right. > > Yes, over the next couple of weeks we need to go over the architecture, both > high level and in specific places. > > ex: commands should be UI free. The bindings need to be processed at the UI > level, and the implementation (EBindingService) will be tied to SWT/JFace. > I've been start to experiment and making EBindingService free from JFace but showing interfaces in the API. Ideally we could have the EBindingService free and only the implementation is specific to the toolkit. Would this make sense? Created attachment 176992 [details]
work in progress
splitting ui.bindings in 2 pieces
Created attachment 176993 [details]
extracted SWT-Specific stuff
What about the idea of splitting up jface into 2 pieces: a) jface.core: Generic stuff which does not need SWT (e.g. many of the binding base classes, IStructuredSelection, ...) b) jface: SWT-Specific classes (viewers, ....), requires and reexports jface.core Looks like i'm too dumb to split JFace into 2 parts so that ui.bindings would only dependend on the none-swt specific core which holds: * Binding * Trigger * TriggerSequence when doing this my inner Eclipse dies somewhere in SWT :-( (In reply to comment #22) > What about the idea of splitting up jface into 2 pieces: > a) jface.core: Generic stuff which does not need SWT (e.g. many of the binding > base classes, IStructuredSelection, ...) > > b) jface: SWT-Specific classes (viewers, ....), requires and reexports > jface.core I am supportive of this, and ideally we'd be looking for other opportunities to split JFace. For example, the viewers package should be relatively easy to split off (but I haven't tried it). Created attachment 177078 [details]
jface with removed binding stuff
This patch removes Binding, Trigger, TriggerSequence from JFace
Created attachment 177079 [details]
Bundle with jface.core classes
The classes from jface in their own bundle
This works fine for e4-Applications like the Contacts-demo (I verified keybindings are still working) but when launching an inner Eclipse I get a core dump on OS-X somewhere inside SWT and I have no idea at all what wrong Created attachment 177080 [details]
Stacktrace of dieing SWT
(In reply to comment #24) > (In reply to comment #22) > > What about the idea of splitting up jface into 2 pieces: > > a) jface.core: Generic stuff which does not need SWT (e.g. many of the binding > > base classes, IStructuredSelection, ...) > > > > b) jface: SWT-Specific classes (viewers, ....), requires and reexports > > jface.core > > I am supportive of this, and ideally we'd be looking for other opportunities to > split JFace. For example, the viewers package should be relatively easy to > split off (but I haven't tried it). Ok. I'll file an extra bug for this. Should we try to do it directly in 3.x or start create a 4.x branch and backport to 3.x when we are confident that we don't break things? Ok. I found the reason (doing this in 3.x gives me a real exception). The problem has something to do with OSGi/Equinox, the package split and package scoped methods ----------8<---------- !ENTRY org.eclipse.ui.workbench 4 2 2010-08-20 13:20:25.654 !MESSAGE Problems occurred when invoking code from plug-in: "org.eclipse.ui.workbench". !STACK 0 java.lang.IllegalAccessError: tried to access method org.eclipse.jface.bindings.Binding.deletes(Lorg/eclipse/jface/bindings/Binding;)Z from class org.eclipse.jface.bindings.BindingManager at org.eclipse.jface.bindings.BindingManager.removeDeletions(BindingManager.java:1951) at org.eclipse.jface.bindings.BindingManager.computeBindings(BindingManager.java:520) at org.eclipse.jface.bindings.BindingManager.recomputeBindings(BindingManager.java:1757) at org.eclipse.jface.bindings.BindingManager.getActiveBindingsByParameterizedCommand(BindingManager.java:945) at org.eclipse.jface.bindings.BindingManager.getActiveBindingsFor(BindingManager.java:1160) at org.eclipse.jface.bindings.BindingManager.getActiveBindingsFor1(BindingManager.java:1209) at org.eclipse.jface.bindings.BindingManager.getBestActiveBindingFor(BindingManager.java:1274) at org.eclipse.jface.bindings.BindingManager.getBestActiveBindingFor(BindingManager.java:1264) at org.eclipse.jface.action.ExternalActionManager$CommandCallback.getAcceleratorText(ExternalActionManager.java:315) at org.eclipse.jface.action.ActionContributionItem.update(ActionContributionItem.java:798) at org.eclipse.jface.action.ActionContributionItem.fill(ActionContributionItem.java:342) at org.eclipse.jface.action.ToolBarManager.update(ToolBarManager.java:353) at org.eclipse.jface.action.ToolBarManager.createControl(ToolBarManager.java:111) at org.eclipse.jface.action.ToolBarContributionItem.fill(ToolBarContributionItem.java:192) at org.eclipse.jface.action.CoolBarManager.update(CoolBarManager.java:930) at org.eclipse.jface.action.CoolBarManager.createControl(CoolBarManager.java:247) at org.eclipse.jface.internal.provisional.action.CoolBarManager2.createControl2(CoolBarManager2.java:76) at org.eclipse.jface.window.ApplicationWindow.createCoolBarControl(ApplicationWindow.java:523) at org.eclipse.ui.internal.WorkbenchWindow.createDefaultContents(WorkbenchWindow.java:1063) at org.eclipse.ui.internal.WorkbenchWindowConfigurer.createDefaultContents(WorkbenchWindowConfigurer.java:623) at org.eclipse.ui.application.WorkbenchWindowAdvisor.createWindowContents(WorkbenchWindowAdvisor.java:268) at org.eclipse.ui.internal.WorkbenchWindow.createContents(WorkbenchWindow.java:1010) at org.eclipse.jface.window.Window.create(Window.java:431) at org.eclipse.ui.internal.Workbench$63.runWithException(Workbench.java:3589) at org.eclipse.ui.internal.StartupThreading$StartupRunnable.run(StartupThreading.java:31) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:134) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3586) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3279) at org.eclipse.ui.application.WorkbenchAdvisor.openWindows(WorkbenchAdvisor.java:803) at org.eclipse.ui.internal.Workbench$31.runWithException(Workbench.java:1566) at org.eclipse.ui.internal.StartupThreading$StartupRunnable.run(StartupThreading.java:31) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:134) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3586) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3279) at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2537) at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2427) at org.eclipse.ui.internal.Workbench$7.run(Workbench.java:670) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:332) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:663) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149) at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:115) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:369) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:179) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:619) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:574) at org.eclipse.equinox.launcher.Main.run(Main.java:1407) at org.eclipse.equinox.launcher.Main.main(Main.java:1383) ----------8<---------- What we see here is a that a class in org.eclipse.jface tries to access a method (delete) in Binding (org.eclipse.jface.core) which is package-scoped and fails. I'm not sure whether this is by design or there's a bug in equinox? Adding Tom Watson maybe he can clarify the situation from the equinox. Making "final boolean deletes(final Binding binding)" public makes things work but I'm not sure that's what we want. Paul - beside the pending problem of splitting JFace. Could you take a look at: * patch in comment 20 * extracted new bundle in comment 21 So that e4 code is prepared to take advantage of the JFace split once we managed it? (In reply to comment #30) > Adding Tom Watson maybe he can clarify the situation from the equinox. This is not a bug in equinox, this is how split packages work. A properly "designed" split package (I'm not sure there is such a thing) has no package-level dependencies between the various parts of the split package. When you split a package out into multiple parts each part is packaged in a different bundle. In OSGi each bundle has its own class loader, the class loader is what the module layer is based on, so now each part is loaded with a different class loader. Two classes do not have package level access to each other unless they are in the same package name AND they are loaded by the same class loader. So you have to consider doing something else: 1) create some internal "API" which is x-friended to the other "part" so it can call into the non-API things it needs to 2) create new real API that can be used by the other "part" 3) use fragments to split your optional functionality out. fragments use the same class loader as there host and will get package-level access. I'm not sure this is a good idea though because there is no good way to declare a dependency on the fragment right now. (In reply to comment #33) > (In reply to comment #30) > > Adding Tom Watson maybe he can clarify the situation from the equinox. > > This is not a bug in equinox, this is how split packages work. A properly > "designed" split package (I'm not sure there is such a thing) has no > package-level dependencies between the various parts of the split package. > When you split a package out into multiple parts each part is packaged in a > different bundle. In OSGi each bundle has its own class loader, the class > loader is what the module layer is based on, so now each part is loaded with a > different class loader. Two classes do not have package level access to each > other unless they are in the same package name AND they are loaded by the same > class loader. > Ok thanks for confirming what I already expected. > So you have to consider doing something else: > > 1) create some internal "API" which is x-friended to the other "part" so it can > call into the non-API things it needs to > 2) create new real API that can be used by the other "part" I think we can go with declaring the method in question as API by making it public. Created attachment 177092 [details]
updated jface.core bundle
(In reply to comment #32) > Paul - beside the pending problem of splitting JFace. Could you take a look at: > * patch in comment 20 > * extracted new bundle in comment 21 > > So that e4 code is prepared to take advantage of the JFace split once we > managed it? -1 at this time. It's not as simple as taking a couple of classes and just moving them. Their are other parts of the binding system that would need to have their classes split into abstract and SWT specific parts (I believe) while maintaining their API-ness. I think it's do-able, but it will require some design and test, and we have other priorities in the framework at the moment. PW (In reply to comment #36) > (In reply to comment #32) > > Paul - beside the pending problem of splitting JFace. Could you take a look at: > > * patch in comment 20 > > * extracted new bundle in comment 21 > > > > So that e4 code is prepared to take advantage of the JFace split once we > > managed it? > > -1 at this time. It's not as simple as taking a couple of classes and just > moving them. Their are other parts of the binding system that would need to > have their classes split into abstract and SWT specific parts (I believe) while > maintaining their API-ness. I think it's do-able, but it will require some > design and test, and we have other priorities in the framework at the moment. > > PW So what's your take on fixing this design issue? I welcome everything that removes the dependency on SWT in workbench.ui. Tom, I agree that SWT has leaked into too many places in the code. The original intent of the PartRenderingEngine was to be 'agnostic' (i.e. not SWT specific) for example. Could you build a list of where/why SWT is being accessed outside the rendering code ? We can go over this and perhaps come up with a slightly different packaging arrangement(I had a few cases where I couldn't set up my addons dependencies correctly precisely because of the .workbench dependency you mention). (In reply to comment #38) > Tom, I agree that SWT has leaked into too many places in the code. The original > intent of the PartRenderingEngine was to be 'agnostic' (i.e. not SWT specific) > for example. > I'm not concerned about the PartRenderingEngine because if someone wants to render in a different technology he/she simple has to invest into writing his/her own. > Could you build a list of where/why SWT is being accessed outside the rendering > code ? We can go over this and perhaps come up with a slightly different > packaging arrangement(I had a few cases where I couldn't set up my addons > dependencies correctly precisely because of the .workbench dependency you > mention). This is the only place left with an inappropriate dependency - which makes e4 only useable inconjunction with SWT. The other problems I identified are already fixed. one more thing I'm not sure of is do we really expect client code to directly using EBindingService. Should they not modify our Application-Model instead? Paul - I think we can close this now that the stuff was added to the Addons and the bundle is free from SWT-Dependencies right? this if fixed now |