Eclipse Integration E4

[N1k145]

This is a first shot for an E4 Implementation eclipse-glsp/glsp#1059

This is currently not fully functional , the context menu is currently not working.
And the Content of the E4 Part is a Hard-coded file

What I did is, I moved alot of the Logic from the GLSPDiagramEditor class into the GLSPDiagramComposite and added an E4 Part called GLSPDiagramPart.

I would like some feedback on what you think about this approach.

[N1k145]

@planger Can you take a look, when you have time, if this goes in a direction that you can agree on.

[planger]

@N1k145 Thank you very much for this proposal! On a first glance this looks pretty good. We'll look at it in more detail in the next few days and get back to you. Thanks again!

[planger]

Hi @N1k145 again, I had another look at the change and I think this goes into a good direction. My only concern is that this change causes API breaks and it is rather short before the next planned major release (end of August). So I assume it'll not be possible to finish this work in the next week to enable making this part of the next major release.

Do you think there is a chance to make the e4 compatibility just adding API but not breaking any? This way, we could put it into the next minor release as it doesn't break but just add API.

[N1k145]

@planger this depends a bit on how much code duplication you would like to have.
The main issue is that currently a lot of the implementation depends hard on the GLSPDiagramEditor class.
Also, all the Setup Logic is hard in that class and depends partly on the Editor Part.

We could define an Interface that is implemented by the GLSPDiagramEditor and the GLSPDiagramComposite that provides the functionality the implementations around the Editor depends on.

But moving the setup logic out of the Editor into the Composite is necessary, what we could do is duplicate the code, have it once in the Editor (without breaks) and the newer implementation in the Composite + E4 Part. That way we would add the two apis (Part and Composite) while leaving the Editor as is for now. And move that to the composite in the next major where breaking changes can be made.

[N1k145]

@planger I discussed this internally and I can spend more time on this in the next week.
Are there any concrete changes you would like to have, or just cleaning it up and making it round?

[planger]

Ok cool, thanks for the feedback!
No I don't have any concrete changes, looks good so far. So cleaning up, removing all hard-coded parts and making the examples work as before (including the project template) with all features (navigation, problem markers, shortcuts, etc.) is all that'd be required before we could merge it.

If we manage to get this done next week, we are happy to review it the week after. If it doesn't fit, I guess we could merge it after the release as an addition, keeping the editor class, but moving out code as much as possible into a class that is shared with the part implementation.

Thank you for your work! 👍

[N1k145]

@planger I did a bit of cleanup and created an Open Workflow File handler for the E4 Part.
I did a test of all the features I know of, for the E3 Editor all of them still work.

For the E4 Part the Problem Markers are not working with the Problems View, and I don't think they make sense in this case.
In my experience, when you are working with E4 you are not working on a File basis, as most of the Eclipse File Handling is not supported in E4. And the Problem Markers are requiring a File.
I also documented this in that way, so when you work with Files that you should use the E3 Editor.

Feel free to test it out, maybe I missed something.

I took a look at the project template, reworking the editor should not be necessary there, that is not using anything that will break, except the overridden createBrowser method, but that is commented with a note that this will be reverted with the release.

[planger]

Thanks again for this nice work! Most of what I tested seems to function nicely.

• For me on Ubuntu 22.04 the context menu didn't work though. I got the following exception:

java.lang.NullPointerException
	at org.eclipse.glsp.ide.editor.ui.BrowserContextMenuInstaller.requestContextMenu(BrowserContextMenuInstaller.java:45)
	at org.eclipse.glsp.ide.editor.ui.BrowserContextMenuInstaller$1.lambda$0(BrowserContextMenuInstaller.java:33)
	at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:40)
	at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:132)
	at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:5040)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4520)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1155)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1046)
	at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:155)
	at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:643)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:550)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:171)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:152)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:203)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:136)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:402)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:255)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:659)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:596)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1467)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1440)

• While undo/redo works, it isn't hooked up with the global undo/redo actions.
• The Diagram menu is not enabled with the e4 viewer
• Please also add this change to the CHANGELOG listing all breaking changes and how to migrate.

I also have some comments inline above and a few code-style suggestions which I added as a commit 816f80d. If you agree with those changes, please cherry-pick the commit onto your branch.

Thank you!

[planger]

Is there a reason why we remove findShell(clientId)? Wouldn't it be just as good to keep it and pass the clientId as before?

[N1k145]

I removed it as I did not really see the reason for it. And there is no EditorSite anymore. I added it back by getting the Shell from the Browser Composite

[N1k145]

Thank you for the review.
I fixed your comments and included your changes.

With the undo redo action there is an issue in the Eclipse implementation, the Buttons are not retargetable to E4 Actions because of the implementation in org.eclipse.ui.internal.ide.WorkbenchActionBuilder.createEditMenu()
I will open an Issue and PR in platform.ui as we also had to build a workaround for this already.

Edit:
Platform Issue: eclipse-platform/eclipse.platform.ui#977

[planger]

@N1k145 Just wanted to get back to you that we have your PR on our radar. We'll get back to it in the next couple of days. Thanks for your fixes regarding my earlier review!

[N1k145]

@planger thanks for the info, I'm on vacation next week and can't look into it during that time. So if something needs changes, I can have a look this week. Or after I'm back

[planger]

Thank you very much again for this contribution! Looks good to me!
I'll just let @tortmayr chime in if he has some further comments. Other than that, this is good to go. Thanks again!

[tortmayr]

Thank you very much for this contribution!
Nothing to add from my side. Code looks good to me and both the e3 and e4 implementations work as expected. 👍🏼

[N1k145]

@planger @tortmayr thank you for the reviews and merging this PR