Bugzilla – Full Text Bug Listing
| Summary: | [Compatibility] ICommandService/IExecutionListener not fired | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Brian de Alwis <bsd> | ||||
| Component: | UI | Assignee: | Paul Webster <pwebster> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | major | ||||||
| Priority: | P3 | CC: | carsten.hammer, Dimo.Stoilov, emoffatt, Lars.Vogel, loskutov, markus.tiede, ob1.eclipse, Oliver.Goetz, pwebster, remy.suen | ||||
| Version: | 4.2 | ||||||
| Target Milestone: | 4.2 M7 | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 350080, 376482 | ||||||
| Attachments: |
|
||||||
Could we enable this early in M6 then? PW This is a showstopper for some AnyEdit functionality on 4.2. I registered my handlers to act just before some commands are executed, via IExecutionListener.preExecute(). Unfortunately I see that this callback is never executed on 4.2 M6 (but works just fine on the 3.8 M6), so that my handler is never activated. I guess everyone relying on this interface is heavily hit by the change, as there are no workarounds possible. As this is a functionality which is hard to unit test (you have to trigger right keys on right selection etc) I guess many developers have even not realized yet that they code just never get called on 4.2. Please consider for M7. Example code:
String commandId = "org.eclipse.ui.file.save";
ICommandService service = (ICommandService) PlatformUI.getWorkbench().getService(
ICommandService.class);
Command command = service.getCommand(commandId);
command.addExecutionListener(new IExecutionListener(){...});
The org.eclipse.core.commands.CommandManager.firePreExecute() (and so IExecutionListener.preExecute()) is neither called on "Ctrl+S", nor on the global Menu "File->Save", nor on the global toolbar "Save" button.
Even worse, on right click-> "Save" in Java editor I see in the debugger, that CommandManager.firePreExecute() *tries* to call IExecutionListener.preExecute(), but for some obscure reasons the listener field is NULL! BTW I guess this are the same obscure reasons why the right click-> "Save" menu in Java editor does NOT show the Ctrl+S binding hint in 4.2 M6 (3.8 M6 has this).
So for me (IExecutionListener client) it looks like a total break of the compatibility story...
I'm not sure it will get fixed in M7 but it should definitely be considered. PW As this issue is not restricted to keybinding triggered command executions I adjusted the summary accordingly. We, the Jubula team, are also affected by this - see bug 376482 for further information. Paul, is this patch correct ? We should likely either apply it or defer... I'm hoping to look at it today or tomorrow and get it in, I'm stuck in emacs keybindings ATM PW (In reply to comment #3) > Example code: > > String commandId = "org.eclipse.ui.file.save"; > ICommandService service = (ICommandService) > PlatformUI.getWorkbench().getService( > ICommandService.class); > Command command = service.getCommand(commandId); > command.addExecutionListener(new IExecutionListener(){...}); The fix proposed will work for service.addExecutionListener(*) but not command.addExecutionListener(*). We'll have to consider what to do about the Command object. PW Added tests for notHandled, notEnabled, and postSuccess http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=d22b8ec3cf723d41bce0207139573c6f12eca5d3 PW Option 1 on branch pwebster/bug369159 http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=f8b001735b93dd47c7827b71d879df6a407b616b This disabled Command objects from firing their IExecutionLister* events. Then instruments various o.e.ui.workbench places and allows them to fire the command manager events: * LegacyHandlerService * E4HandlerProxy * HandlerServiceImpl so hs.executeCommand(*) and command.executeWithChecks(*) will fire events that show up to listeners added to ICommandService. There's no support for listeners added to Command objects. PW (In reply to comment #10) > Option 1 on branch pwebster/bug369159 I'll have to let it sit in my mind, and see if I can re-route all of the event firing through commands. It's not really a great solution as the commands aren't responsible for anything more than the programmatic representation of the abstraction. PW http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=75b49e68802d846d8ae53c49c4e343a369e567c8 OK, a bit of a mess but it routes firing the events through the Command object. I've opened bug 377293 as the division of responsibility here needs to be re-worked. PW With below in I20120430-1800 I don't get all the preExecute events, especially from keybindings.
ICommandService cs = (ICommandService) view.getSite().getService(
ICommandService.class);
cs.addExecutionListener(new IExecutionListenerWithChecks() {
@Override
public void preExecute(String commandId, ExecutionEvent event) {
System.out.println("preExecute " + commandId);
}
@Override
public void postExecuteSuccess(String commandId, Object returnValue) {
System.out.println("postExecuteSuccess " + commandId);
}
@Override
public void postExecuteFailure(String commandId,
ExecutionException exception) {
System.out.println("postExecuteFailure " + commandId);
}
@Override
public void notHandled(String commandId,
NotHandledException exception) {
System.out.println("notHandled " + commandId);
}
@Override
public void notEnabled(String commandId,
NotEnabledException exception) {
System.out.println("notEnabled " + commandId);
}
@Override
public void notDefined(String commandId,
NotDefinedException exception) {
System.out.println("notDefined " + commandId);
}
});
I've released a fix for window based MHandledItems. But I've encountered a problem. org.eclipse.ui.internal.BindingToModelProcessor runs are part of loading the model. It comes from the compat layer, and processes the 3.x extension points into binding tables and MKeyBindings. But we started saving the Binding object created in BTMP and now are merging that into the model. The problem is, that points to a completely disconnected command. Because we don't usually use the Command object in Eclipse 4 for state this hasn't been a problem. But with the execution of listeners it shows up because the keybindings are disconnected (no events fired) and arguably executing something from a Keybinding with State objects might not produce the desired results (as State are on the singleton Command objects). We'll need to go over this tomorrow. PW http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=aa3fabc57b242fcd960289450fa938b613eae295 I safed up the code that deals with the Command and Context manager directly, to make sure there is only one in the system at a time. PW In I20120503-1800 PW *** Bug 378789 has been marked as a duplicate of this bug. *** We, the Jubula team, still experience the same problem (bug 376482) with build "I20120503-1800". Are there any additional information we can provide to properly reproduce / resolve this? (In reply to comment #18) > We, the Jubula team, still experience the same problem (bug 376482) with build > "I20120503-1800". Are there any additional information we can provide to > properly reproduce / resolve this? So if you hit CTRL+S after a breakpoint shows the listener has been added, you don't get anything? PW Our listener does indeed get notified when pressing CTRL+S, but not when e.g. invoking the command via the tool- or menu-bar items. In comment 5 I (tried to) generalize the problem as it is not only restricted to keybinding triggered command executions. (In reply to comment #20) > Our listener does indeed get notified when pressing CTRL+S, but not when e.g. > invoking the command via the tool- or menu-bar items. The general problem with IExecutionListeners has been fixed. I have my IExecutionListener on my ICommandService notified for all calls, and the one on Command objects for command calls. You're now hitting bug 379257 ... in 4.2, Save on the toolbar or File menu are actions. Actions only notify the ICommandService registered IExecutionListeners, not any registered against Command objects. In general listeners against the ICommandService are preferred. There is on going work to convert the old-style actions into commands, but it is slow going. The other alternative is to switch to the command service listeners (works in 3.x and 4.2). Something similar to: IExecutionListener saveListener = new IExecutionListener() { /** {@inheritDoc} */ public void preExecute(String commandId, ExecutionEvent event) { // empty is ok } /** {@inheritDoc} */ public void postExecuteSuccess(String commandId, Object returnValue) { if (IWorkbenchCommandConstants.FILE_SAVE.equals(commandId) || IWorkbenchCommandConstants.FILE_SAVE_ALL .equals(commandId)) { completeProjectCheck(); } } /** {@inheritDoc} */ public void postExecuteFailure(String commandId, ExecutionException exception) { if (IWorkbenchCommandConstants.FILE_SAVE.equals(commandId) || IWorkbenchCommandConstants.FILE_SAVE_ALL .equals(commandId)) { completeProjectCheck(); } } /** {@inheritDoc} */ public void notHandled(String commandId, NotHandledException exception) { // empty is ok } }; commandService.addExecutionListener(saveListener); Thanks for the clarification - I did not know that those items are still realized via actions in 4.2! I will adjust our code accordingly to the alternative of registering against the ICommandService. (In reply to comment #22) > Thanks for the clarification - I did not know that those items are still > realized via actions in 4.2! I am sorry about bug 379257, there are a number of 3.7 changes that haven't been ported to 4.x yet. I had a preliminary look at merging this one in, but the code changes break File>Save and is too risky this late in the game. I've had to defer it to 4.2.1 PW I still see this code in org.eclipse.core.commands.Command that is part of org.eclipse.core.commands_3.9.200.v20180827-1727.jar:
...
public Object executeWithChecks(final ExecutionEvent event)
throws ExecutionException, NotDefinedException, NotEnabledException, NotHandledException {
if (shouldFireEvents) {
firePreExecute(event);
}
final IHandler handler = this.handler;
// workaround for the division of responsibilities to get
// bug 369159 working
if ((handler != null)
&& "org.eclipse.ui.internal.MakeHandlersGo".equals(handler.getClass() //$NON-NLS-1$
.getName())) {
return handler.execute(event);
}
...
Does this mean that this bug never has been fixed?
For me a cheatsheet call to org.eclipse.ui.file.refresh never succeeds because it does not find a matching handler (or it finds it but decides to not use it - don't understand that point).
(In reply to Carsten Hammer from comment #24) > I still see this code in org.eclipse.core.commands.Command that is part of > org.eclipse.core.commands_3.9.200.v20180827-1727.jar: > > ... > public Object executeWithChecks(final ExecutionEvent event) > throws ExecutionException, NotDefinedException, NotEnabledException, > NotHandledException { > if (shouldFireEvents) { > firePreExecute(event); > } > final IHandler handler = this.handler; > // workaround for the division of responsibilities to get > // bug 369159 working > if ((handler != null) > && "org.eclipse.ui.internal.MakeHandlersGo".equals(handler.getClass() > //$NON-NLS-1$ > .getName())) { > return handler.execute(event); > } > ... > > Does this mean that this bug never has been fixed? > For me a cheatsheet call to org.eclipse.ui.file.refresh never succeeds > because it does not find a matching handler (or it finds it but decides to > not use it - don't understand that point). See https://bugs.eclipse.org/bugs/show_bug.cgi?id=551399 |
Created attachment 209781 [details] Patch to HandlerServiceImpl to trigger appropriate callbacks Version: 4.2.0 Build id: I20120110-2200 ICommandService's execution listener events are not fired when commands are invoked through the EHandlerService (e.g., from keybindings). These events are fired in some situations, such as from menus or toolbars (e.g., the "Collapse" toolbar button in the Package Explorer): CommandManager.firePreExecute(String, ExecutionEvent) line: 1004 ExternalActionManager$CommandCallback.preExecute(IAction, Event) line: 434 ActionContributionItem.handleWidgetSelection(Event, boolean) line: 581 ActionContributionItem.access$2(ActionContributionItem, Event, boolean) line: 501 ActionContributionItem$5.handleEvent(Event) line: 411 EventTable.sendEvent(Event) line: 84 Display.sendEvent(EventTable, Event) line: 4135 ToolItem(Widget).sendEvent(Event) line: 1457 These command-like actions don't actually go through the EHandlerService. Since the CommandManager is available through the context, we could cause HandlerServiceImpl to trigger the various callbacks during execution (see the attached patch). A possible side-effect is that CommandActions (which if used in the UI will presumably be wrapped using an ActionContributionItem) may cause the execution listeners to be triggered twice, once through the ActionContributionItem (as above) and the other through the HandlerServiceImpl. Though I don't see any actual uses of CommandAction; menus and toolbars appear to use HandledContributionItems. I've attached a patch to HandlerServiceImpl to call out to the CommandManager to trigger the callbacks. In my observation, this seems to have the right effect: ICommandService listeners now receive notifications of commands via the EHandlerService, and there are no duplicate notifications.