Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 363763

Summary:	command callback parameters and command service cleanup
Product:	[ECD] Orion	Reporter:	Susan McCourt <susan>
Component:	Client	Assignee:	Susan McCourt <susan>
Status:	RESOLVED FIXED	QA Contact:
Severity:	normal
Priority:	P3	CC:	libingw
Version:	0.3
Target Milestone:	0.4 M1
Hardware:	PC
OS:	Windows 7
Whiteboard:

Description Susan McCourt

2011-11-14 19:24:58 EST

Right now there's pretty poor separation between CommandService and Command.  We started out with the Command invoking the proper callbacks, providing the info.  But in practice, most of this info gets passed from the command service anyway, the command just has to know it to hook the callback to the DOM event.

With key bindings, and now URL bindings, the command service invokes callbacks directly.

Up to now, I've hacked knowledge in both places but it needs to get cleaned up.

1) the command service (at render time) is really the one that knows most of the important information for invoking a callback.  It should probably set this up after a command generates a DOM element.
2) we have over 5 callback parameters now, and in some places even store them as an array to share them around different methods.  There is some ugly code like "domId = callbackParameters[2];"
Instead, we need to define an object to represent the callback parameters so we can access them by name, pass them around easily, and add parameters without going back and changing the signature on clients.

#1 can be done internal to the command service.
#2 will require changing all the command clients, but will make them more flexible in the future

Comment 1 Susan McCourt

2011-11-15 10:22:05 EST

Another thing to cleanup.
Right now I save contextual information (enough to invoke a callback) on any command that has a key binding or URL binding.  We may want to save this kind of information on all commands so that any command can be executed programmatically by its id vs. a binding.

Comment 2 Susan McCourt

2011-11-16 14:42:52 EST

The way parameters are specified is very hacky.  Kind of an associative array, but I use two predefined slots (options, optionsTriggered) and treat the remaining slots as parameters.  Instead we should have a ParameterDescription object which includes an array of CommandParameters rather than inferring command parameters via the remaining slots.

ParameterDescription has options, optionsTriggered, array of CommandParameter

CommandParameter should have name, label, value, etc.

Comment 3 Susan McCourt

2011-11-22 20:52:30 EST

Fixed.  (multiple commits).
- There are now real objects for defining parameters (ParametersDescription) and the parameters themselves (CommandParameter)
- Got rid of many unnecessary parameters in the renderCommand methods and fixed the order so that callers don't have to include a bunch of "null, null, null" in order to specify more common things like "textOnly"
- Defined a CommandInvocation data structure which lets the command service keep one record of a command's context.  This is sent as calldata for a callback, so clients can now use
  data.items
  data.domNode
etc. instead of including a bunch of parameters in the callback that kept changing.

This address comment #0 and comment #2.
I did not really address comment #1 as I'm not sure I want to store a CommandInvocation for every item-level command.  There are some hacks right now in the command framework where we assume that bindings are only used for "global" and "dom" scope.  Until we have a real need for invocation/context information for other commands, I'm going to wait on this.