[UI][E] Menu Migration from Eclipse3 to Eclipse4 #1119
Conversation
Migration of the Main-Menu, as well as all Commands and Handlers to the E4 Platform. Explanation: Since 2012 Saros/E uses the compatibility layer provided by Eclipse for the UI to function. To drop the need for the compatibility layer all UI Elements have to be migrated and all references to org.eclipse.ui have to be removed.
| @@ -187,7 +187,12 @@ public static void runSafeSWTSync(final Logger log, final Runnable runnable) { | |||
| * @return the display of the current workbench | |||
| */ | |||
| public static Display getDisplay() { | |||
| return PlatformUI.getWorkbench().getDisplay(); | |||
| // return PlatformUI.getWorkbench().getDisplay(); | |||
There was a problem hiding this comment.
There is a reason we do not use Display#getDefault.
When Eclipse shuts down it disposes the display. With this change you will creating a new one and this is something we do not want to happen.
There was a problem hiding this comment.
So after looking at the issue I think this is not the correct fix.
Either we ensure that the Platform is available when our context is started or we modify the affected classes.
There was a problem hiding this comment.
I pushed a new Commit that contains a fix by keeping track of the display after its first creation.
But it should be seen as a temporary fix that we chose because of time constraints.
The better solution would probably be to change all affected classes since there doesnt seem to be an equivalent to PlatformUI.getWorkbench().getDisplay() in E4.
There was a problem hiding this comment.
Looking at the issue, well I do not know. I could modify the affected class. Regarding your solution and looking at the current call stack I guess it would be sufficient just to call getCurrent (assuming the Display was already created and Eclipse call us on the creator thread). Of course your solution works (if the display was created, even if there is no workbench) but it requires good knowledge on SWT / Eclipse Startup.
eclipse/src/saros/ui/command_handlers/SessionAddSelectedResourcesHandler.java
Show resolved
Hide resolved
| public class SessionAddSelectedResourcesHandler { | ||
| @Inject private ConnectionHandler connectionHandler; | ||
| @Inject private ISarosSessionManager sessionManager; | ||
| @Inject private ResourcePropertyTester resourcePropertyTester; |
There was a problem hiding this comment.
If I am not mistaken it is always null and not even used.
eclipse/src/saros/ui/command_handlers/SessionAddSelectedResourcesHandler.java
Show resolved
Hide resolved
| <children xsi:type="menu:MenuSeparator" xmi:id="_RkmK0DYVEeuIF8EFjFT7JQ" elementId="saros.eclipse.menuseparator.RosterEnd" accessibilityPhrase="RosterEnd"/> | ||
| <children xsi:type="menu:MenuSeparator" xmi:id="_2St4kDYVEeuIF8EFjFT7JQ" elementId="saros.eclipse.menuseparator.CollaborationStart" accessibilityPhrase="CollaborationStart"/> | ||
| <children xsi:type="menu:HandledMenuItem" xmi:id="_6984oDYVEeuIF8EFjFT7JQ" elementId="saros.eclipse.handledmenuitem.5" iconURI="platform:/plugin/saros.eclipse/icons/elcl16/session_tsk.png" command="_pZa5oDYPEeuIF8EFjFT7JQ"> | ||
| <visibleWhen xsi:type="ui:CoreExpression" xmi:id="_N9R-gDYYEeuIF8EFjFT7JQ" coreExpressionId="saros.ui.definitions.notIsSarosSessionRunning"/> |
There was a problem hiding this comment.
Isn't there a way to negate the coreExpressionId reference via xml ?
| public void applicationStarted( | ||
| @UIEventTopic(UIEvents.UILifeCycle.APP_STARTUP_COMPLETE) Event event, Saros saros) { | ||
|
|
||
| saros.getWorkbench().addWorkbenchListener(saros.workbenchShutdownListener); |
There was a problem hiding this comment.
I do not understand the benefit ?
There was a problem hiding this comment.
This Listener was previously added in Line 133.
But with the Addition of Eclipse 4 Fragments Saros is initialized before the Workbench and the Listener has to be added seperately after the Startup is complete and the Workbench exists.
There was a problem hiding this comment.
The listener is still removed in the stop method. This seems pretty confusing. Isn't there no new method that should be added ?
There was a problem hiding this comment.
For us it seemed that the addition of E4 components moved the startup of saros up, before the initialization of a workbench.
Thats why we moved the addition of the workbench listener it to this extra function.
But the time whene the stop method is called didnt change so there was no need to move the remove call to an extra function.
We realized that there is some redundancy though, since the workbench listener was and still is called before the stop method, which both call stopLifeCycle() and we saw no change when removing the listener completely.
But we guessed that the listener serves a purpose, probably when the workbench isnt shutdown by ordinary means which would not stop all plugins
There was a problem hiding this comment.
There was a problem hiding this comment.
I subscribed to UIEvents.UILifeCycle.APP_SHUTDOWN_STARTED which seems to be the counterpart. The only issue I found is that it is called twice.
eclipse/src/saros/ui/command_handlers/SessionAddSelectedContactsHandler.java
Outdated
Show resolved
Hide resolved
| WizardUtils.openStartSessionWizard( | ||
| SelectionRetrieverFactory.getSelectionRetriever(IResource.class).getSelection()); | ||
| return null; | ||
| } | ||
|
|
||
| @CanExecute | ||
| public boolean canExecute() { |
There was a problem hiding this comment.
I actually find this canExecute method really confusing. The visibleWhen checks are still in the plugin.xml file. It seems this is the enabledWhen check. However this method is different from SessionAddSelectedResourcesHandler class. So why is there no check if the resource is already shared ? And last but not least looking at the SessionAddSelectedResourcesHandler class, how does the method know how it has to convert the selections ?
There was a problem hiding this comment.
The plugin.xml still contains the popup menus which have not yet been migrated, thats where the visibleWhen expression are from atm.
ShareResourceHandler and SessionAddSelectedResourcesHandler are fundamentally different, one starts a session the other one adds to an existing session.
I think youre right regarding the SessionAddSelectedResourcesHandler @CanExecute Method that it cant convert the Selection and the Button in the Popup Menu is not yet working properly.
There was a problem hiding this comment.
I just looked at the old plugin.xml code and it seems this handler is a bit "misused" through the magic that happens on line 53 which makes it really hard to understand.
So is the design of e4 where the canExecute method gets the selection data but when the framework calls the execute method you have to query the selection service on your own ?
Fix session null checks Fix ActiveSelection Resource Conversion Remove unecessary injected Fields
This is a temporary workaround for PlatformUI.getWorkbench().getDisplay() to ensure that the functionality stays the same, by manually keeping track of the Display after it is first instantiated.
|
Are there any plans by you to work further on the whole migration ? I do not really feel very comfortable pushing a partial migration patch into the current master ? |
Migration of the Main-Menu, as well as all Commands and Handlers to the E4 Platform.
Explanation:
Since 2012 Saros/E uses the compatibility layer provided by Eclipse for the UI to function.
To drop the need for the compatibility layer all UI Elements have to be migrated and all references to org.eclipse.ui have to be removed.
Also described here.