This Bugzilla instance is deprecated, and most Eclipse projects now use GitHub or Eclipse GitLab. Please see the deprecation plan for details.
Bug 417237 - [Commands] The ExecutionEvent in IExecutionListener.preExecute no longer includes trigger information
Summary: [Commands] The ExecutionEvent in IExecutionListener.preExecute no longer incl...
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.3   Edit
Hardware: Macintosh Mac OS X
: P3 normal (vote)
Target Milestone: 4.4 M3   Edit
Assignee: Paul Elder CLA
QA Contact: Paul Elder CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-13 19:09 EDT by Mark Feber CLA
Modified: 2013-10-29 13:27 EDT (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Feber CLA 2013-09-13 19:09:15 EDT
Starting with Kepler, the ExecutionEvent in IExecutionListener.preExecute no longer includes trigger information.

A sample stack trace from a command executed via a keybinding:

CommandManager$ExecutionListener.preExecute(String, ExecutionEvent) line: 126	
Command.firePreExecute(ExecutionEvent) line: 693	
Command.executeWithChecks(ExecutionEvent) line: 465	
ParameterizedCommand.executeWithChecks(Object, Object) line: 508	
HandlerServiceImpl.executeHandler(ParameterizedCommand, IEclipseContext) line: 213	
KeyBindingDispatcher.executeCommand(ParameterizedCommand, Event) line: 285	
KeyBindingDispatcher.press(List<KeyStroke>, Event) line: 504	
KeyBindingDispatcher.processKeyEvent(List<KeyStroke>, Event) line: 555	
KeyBindingDispatcher.filterKeySequenceBindings(Event) line: 376	
KeyBindingDispatcher.access$0(KeyBindingDispatcher, Event) line: 322	
KeyBindingDispatcher$KeyDownFilter.handleEvent(Event) line: 84	
....


The code in the HandlerServiceImpl explicity does not get/include the trigger in the information passed that creates the ExecutionEvent

public Object executeHandler(ParameterizedCommand command, IEclipseContext staticContext) 
 {
  final IEclipseContext executionContext = getExecutionContext();
  addParms(command, staticContext);
  // executionContext.set(STATIC_CONTEXT, staticContext);
  push(executionContext, staticContext);
  try {
	// Command cmd = command.getCommand();
	return command.executeWithChecks(null, peek());
	} catch (ExecutionException e) {...

As a result the ExecutionEvent.getTrigger will always return null making it completely useless in this context and breaks code that relied on the information contained therein...

Why the change?  It would be useful to Emacs+ to support the previous behavior
Comment 1 Paul Webster CLA 2013-09-17 11:40:58 EDT
Might be a dup of bug 412681

PW
Comment 2 Mark Feber CLA 2013-09-17 13:58:03 EDT
I do not think so.  The description is different and looking at the diff associated with 412681, it still shows null being explicitly set as the trigger:

@@ -170,7 +171,7 @@ public class HandlerServiceImpl implements EHandlerService {
 		push(executionContext, staticContext);
 		try {
 			Command cmd = command.getCommand();
-			cmd.setEnabled(peek());
+			cmd.setEnabled(new ExpressionContext(peek().context));
 			return cmd.isEnabled();
 		} finally {
 			pop();
@@ -210,7 +211,7 @@ public class HandlerServiceImpl implements EHandlerService {
 		push(executionContext, staticContext);
 		try {
 			// Command cmd = command.getCommand();
-			return command.executeWithChecks(null, peek());
+			return command.executeWithChecks(null, new ExpressionContext(peek().context));
 		} catch (ExecutionException e) {
 			staticContext.set(HANDLER_EXCEPTION, e);
 		} catch (NotDefinedException e) {

(In reply to Paul Webster from comment #1)
> Might be a dup of bug 412681
> 
> PW
Comment 3 Paul Elder CLA 2013-09-19 12:38:31 EDT
(In reply to comment #1)
> Might be a dup of bug 412681
> 
> PW
Definitely not a dup. The trigger is being stored in the static context (under Event.class), but then not being recovered in HandlerServiceImpl, as comment 2 shows.
Comment 4 Paul Elder CLA 2013-10-03 15:41:30 EDT
Pushed patch to Gerrit for review:

https://git.eclipse.org/r/17011
Comment 6 Paul Elder CLA 2013-10-24 13:51:26 EDT
Fixed in master per comment 5.
Comment 7 Eric Moffatt CLA 2013-10-29 13:27:30 EDT
Verified in 4.4.0.I20131028-2000. Did a Ctrl-M and traced the execution code which showed that the 'trigger' parameter was now an set.Event.