Community
Participate
Working Groups
Provide common command infrastructure. Eclipse currently lacks a common infrastructure for tracking command invocation, and for managing undo/redo. Several components, including certain non-UI components, require such an infrastructure. Eclipse should provide a common command infrastructure that can be shared between components. [Platform UI, Platform Core]
*** Bug 39034 has been marked as a duplicate of this bug. ***
Randy has provided a patch in bug 39034, but see the comments there.
This is critical for clients of both GEF and EMF
*** Bug 29939 has been marked as a duplicate of this bug. ***
I believe that the EMF command framework can be this common command framework , because it's general purpose command framework as it does not depened on the EMF technology (does not depened on the EMF models). All we have to do is to remove it from the EMF common plugin
The patch provided in bug 39034 is similar in API to the EMF command implementation. The difference are: Command#getAffectedObjects() is not a generic and is often misused to mean anything the client wants. If the puspose of a Command framework is to combine disparate technologies, then the API must be the used the same in practice. This method is deleted in the proposed framework. It is *NOT* needed for its intended purpose, which is to select items after the command has executed or been undone. The same function can be acheived without API on the command class. I've seen WSAD I/E do a similar thing with Pages in a MultiPageEditorPart. On undo/redo, they would flip to the page in the editor on which the edit occured. So, if getAffectedObjects is needed, then so is getAffectedPage, etc. The other deleted method is Command#getResult(). getResult() was originally used to break a "paste" operation into two command. The first command behaved as a factory, and the second command used the factory's result to perform the paste. The last time I searched for senders of these methods, there was only one usage for each. Interfaces: CommandStack and Command are not interfaces in the proposal. This was to allow for future additions. But, I think making Command an interface is probably the best approach. Implementation: EMF's AbstractCommand is complex. It assumes that isExecutable() is expensive to calculate, and therefore must be cached. This is not true in general. The proposed implementation is a "bare-bones" one. Finally, the package name could not contain "emf" if it were part of platform.
If we intend to do a common command framework , we should try to improve the existing command frameworks before using them as the common framework I would like to mention some drawbacks or at least things i don't like in EMF command framework 1- returning value of methods a- Command: execute(),undo() and redo() throws runtime exception if an error occured in them ,becasue they return void , and this is bad to try to catch runtime exception , and affects the performance b- Command: canExecute() , canUndo() and canRedo() return boolean to say that there's an error and the command can't do the operation , and this doesn't describe why the operation can't be performed c- CommandStack: execute(Command),undo() and redo() return void , and if the operation failed (the command that it executes ,undo or redo throws runtime exception) , it just log this error , and there's no way to retrieve it and to know if the operation succeeded or not both of these sets of methods better return IStatus Object , and for the command stack it must return the IStatus that's return from the command it performs the operation on. 2-CommandStack: Events fired by CommandStack does not carry any information 3-Command: It's documented that canExecute() , canUndo() and canRedo() must be called before execute() , undo() and redo() , but this is only documented but not forced by code , and it better be forced , so Command can be an abstract class not an interface //contains some psedo code final IStatus execute(){ IStatus s=canExecute(); if(s.sevirity!=OK){ return s;} return doExecute() } abstract IStatus doExecute();
I think you missed something. Before discussing complete implementation details it is necessary to evaluate, what integration is necessary. We can't just copy one framework, put it into a core plugin and say: "Hey, here it is." BTW, changing methods can...() to return IStatus instead of boolean is not a good idea. There are a lot more things to think about. Just to mention a few: - Tasks of this framework - Possible framework clients - Framework use cases - How to represent the Commands/CommandStack/EditDomain? - Is a generic integration possible? - Which implementation/integration level should be achieved? In a next "in deep look" you can discuss functions and return codes. Sometimes it is ok to return status objects but anyway functions can also throw errors (for example CoreException), which is a better way to indicate execution errors. We also should think about transaction concepts because currently the model and the stack seems to be in an undefined state after such an error, there is no fallback. BTW, have you ever tried executing a command, which cannot be undone? all other operations in the stack before are lost, there should be more warnigns about this behavior (like PopUps....).
*** Bug 51060 has been marked as a duplicate of this bug. ***
deferred.
not sure why a plan item would be marked critical
Because this bug is important to integrating EMF and GEF technologies.
Let's not ignore this for another release.
> 2-CommandStack: Events fired by CommandStack does not carry any information For 3.1, GEF is adding a new notification events: PRE/POST_EXECUTE PRE/POST_UNDO PRE/POST_REDO The events contain the command being undone/redone. Although now that I think about it that's not perfect. We may want to allow clients to create a group of commands which are rolled back and redone atomically. For example: COMMAND, COMMAND, <MARKER>, COMMAND, COMMAND, </MARKER> Commands 3&4 are a single unit. So maybe we provide all commands being undone/redone, or we fire some more changes for each of these sub-commands. One known use of this notification is to disable and enable UI updates, for example, selection synchronization, Action enabled state, Control.setRedraw (boolean)., etc. When operating on multiple objects, all of these things occur multiple times and slow down the application, so such notification is necessary to improve performance.
Can this feature request be (re) prioritized higher for 3.1 plan? I'm asking on behalf of WTP project. The work around is for unrelated components to pre-req some common component which provides this function, causing awkward dependancies among components not otherwise related except for a desire to provide good Undo experience for user. One challange now, though, is to do this in a way that does not break API compatibility with/between components such as EMF, GEF, and VE (they all have their own "common" command(stack) framework). In WTP, not having this common command stack at the platform level causes, for example, some plugins to pre-req an EMF plugin, so, by convention, all can do the same thing. This is not as modular as we'd like to be.
This is missing functionality -- moving to enhancement. I'm taking ownership for this bug, but this does not mean that it's priority has changed.
Please set target milestone
The plan is to set the target milestone once this item is being worked on.
see also bug 1887
I don't see why an undo service is needed. There is already an undo service. It's the global action registry, and you hook into it by registering actions for undo/redo. The preferred solution should include Actions which invoke undo/redo, and monitor the stack for changes.
We are adding the following API to CommandStack in GEF: /** * Constant indicating notification prior to executing a command (value is 1). */ public static final int STATE_PRE_EXECUTE = 1; /** * Constant indicating notification prior to redoing a command (value is 2). */ public static final int STATE_PRE_REDO = 2; /** * Constant indicating notification prior to undoing a command (value is 4). */ public static final int STATE_PRE_UNDO = 4; /** * Constant indicating notification after a command has been executed (value is 8). */ public static final int STATE_POST_EXECUTE = 8; /** * Constant indicating notification after a command has been redone (value is 16). */ public static final int STATE_POST_REDO = 16; /** * Constant indicating notification after a command has been undone (value is 32). */ public static final int STATE_POST_UNDO = 32; /** * Appends the listener to the list of command stack listeners. * Multiple adds result in multiple notifications. * @since 3.1 * @param listener the event listener */ public void addCommandStackEventListener(CommandStackEventListener listener){ ... } /** * Removes the first occurrence of the specified listener. * @param listener the listener */ public void removeCommandStackEventListener(CommandStackEventListener listener){ ... } The listener looks like: public interface CommandStackEventListener { /** * Sent when an event occurs on the command stack. {@link CommandStackEvent#getDetail()} * can be used to identify the type of event which has occurred. * @since 3.1 * @param event the event */ void stackChanged(CommandStackEvent event); } See comment 14 for motivation for these changes.
I'd like to put forward a view from EMF. Unfortunately, the discussion seems to be occuring at so many different levels that it's difficult to gauge where minds are on the topic. Perhaps the owner of this bug can offer some direction: is the intent to start with EMF's simple command framework (or one of its offspring), and tweak it to make it platform-happy? Or is it to start from scratch, and consider more complex notions of status, transactions, etc? It seems that the majority of comments presuppose the former approach, so I should put this on the table: I don't think that Randy's proposed API in bug 39034 would be sufficient for EMF and its clients. Certainly, we don't need to force our abstract command implementation on others, but I don't believe we could give up on getResult() and getAffectedObjects(). The claim has been made that they should be removed because they're not needed for their intended purpose, but I fail to see how they are so offensive. For that matter, I haven't seen any alternatives proposed. Our experience has shown that by providing a control for the objects selected after the command is executed or undone, getAffectedObjects() facilitates very simple command reuse and composition. How should this be better achieved?
David, In that case, for general text editors we need to add: Point getExecuteSelectionRange(); Point getUndoSelectionRange(); Point getRedoSelectionRange(); Where each method returns a pair of offsets indicating the selection range and caret placement within a Text widget. As far as getResult(), EMF is using Command as if it were a Factory. So the alternative is to create a CopyCommand which takes a Factory instead of a Command which is a factory.
Why wouldn't execute/undo/redo return affected objects? (i.e. the objects to select). The problem is that in case of element deletion I might want to select the element parent, when the command is undone - to select the element itself. The comment #14 also points one of the issues. In our (GEF-based) project I'm going to try to implement "mergable" commands. I.e. we have a slider control that controls the rectangle radius. The slider is in the property view and the rectangle is in editor. So I issue a command each time a slider is moved. It happens that there is a lot of commands issued that change the radius by one point. I want to modify the command stack that way so the command is notified what the previous command is and they could "merge" for the undo/redo purposes. The merge logic will be put in the command class of the command that is put later. Though I have some doubts but wanted to share tjust in case someone will get some better idea and implement it in the platform :)
Concerning comment #13 I would suggest to return these values from the execute/undo/redo methods of the commands and the abilty to plug some kind of handlers - so in the case of GEF the handler will know that these methods return the model objects and selects corresponding edit parts, in the case of the text editor it will know that the value is the text range. It should be possible to change the default handler though.
Eugene, I agree that the object to be selected in the UI change based on Undo and Redo. But you can never add enough methods to the Command interface to allow all UIs to extract the necessary information. that was my point with selection ranges. What about a multi-page editor? The command would have to tell you which page to flip to. The solution is staring us in the face. Just have the command do this stuff. So when the command is executed, it sets the selection range, and scrolls to the appropriate location. I believe this is how the TextEditor works in Eclipse. We could extend this concept and have the command be a visitor on various UI contexts.
Randy, I don't see good grounds for your "slippery slope" concerns. First, I'm not sure why you'd propose three methods, when a single method could return different results after execute, undo and redo -- that's how getAffectedObjects() is currently defined in EMF. Secondly, getAffectedObjects() has a return type of Collection: there's no reason that the collection couldn't include a Point, or anything else. That's the intent behind using a generic type: it creates plasticity, allowing the editor to interpret the result in a way that makes sense for the situation. The two propsed solutions aren't sufficiently flexible for EMF. Simply returning the affected objects from execute(), undo() and redo() doesn't work because we execute the command and get the affected objects in different places. Our usual pattern is to create and execute the command in an action, and to have command stack listeners defined in the editor that respond to an executed command by refreshing the appropriate viewers. As for the command setting the selection directly, that's entirely inappropriate for EMF. Our editing framework completely separates the model from the UI, making item providers UI-independent. So, item providers (which usually customize commands) have no knowledge of how to update the UI themselves. They need a plastic way to communicate that "these are the objects that were changed" to the editor. Moreover, the act of setting the selection, itself, cannot be composed. If I want to define a CompoundCommand that uses two different commands to do two things, affecting two sets of objects, it can easily call getAffectedObjects() on each and combine the result. Relying on the two commands to set the selection in execute() couldn't work, since the second one would unset the selection from the first. Note that the multi-page editor problem is purely a UI concept. Presumably the editor already has enough information to determine which page to flip to based on which "logical" objects were affected.
> Presumably the editor already has enough information..to flip Every multipage editor I know of is displaying the same model on multiple pages. For example, Page Designer, PDE's plugin.xml. The model does't tell you where the change was made. Removing UI dependecies can be done by layering the UI-specific portions on top of the "pure model" commands (which have String labels on them for display in a UI). > That's the intent behind using a generic type: it creates plasticity So much so that if you browse WSAD you will find many examples of misuse. For example, using it to identify which resources in a set were dirtied and require saving.
> For example, Page Designer, PDE's plugin.xml. The Plug-in Manifest Editor doesn't switch pages when you undo, either (I don't have the Page Designer installed to check). If the user can already see the undo on the current page, I don't believe it would be helpful to switch pages. > layering the UI-specific portions on top of the "pure model" commands ...simply requiring a complete redesign of EMF.Edit. Generic commands created by generic actions (for cut, paste, delete, etc.) have no knowledge of the UI elements that are displaying the affected objects, so they can't control updating. And, even then, that still doesn't address the issue of command composition.
> But you can never add enough methods to the Command interface to > allow all UIs to extract the necessary information. that was my point > with selection ranges. What about a multi-page editor? Custom handlers facility will solve this solution. BTW, these handlers could be something like the command stack listeners (they receive command executed event and are able to call the EMF getAffectedObjects method, for example). getAffectedObjects can return TextSelection for the last (exec/undo/redo) operation.
We should try to keep it simple. There is a lot information that everybody wants to put into the commands and command stack (eg. new selection, page switching etc.). Why not allowing to add data to commands: Command#setData(Object key, Object value) Command#getData(Object key) After that, every listener is free to intepret the data it wants to. The Command should be submitted in the CommandStackChangeEvent. We could also move this part out of the listeners into a special command data handler, which can "manipulate" the command stack by mergin commands etc.
I think that every suggestion so far would work. But for compatibility, EMF can just define an extended interface which provides getResult and getAffectedObjects, and implement this extension on their abstractCommand.
I'm beginning a prototype for an undo model that can meet the needs of text, refactoring, GEF, EMF, while trying to minimize the impact to any one of those components. It is documented in http://dev.eclipse.org/viewcvs/index.cgi/% 7Echeckout%7E/platform-ui-home/R3_1/undo-redo-proposal/undo-redo% 20support.html. This is in the early investigation stage - the goal is a prototype demonstrating that text and refactoring undo can work together under this framework. I'll report interesting results here. I'm also interested in collecting scenarios that involve shared undo histories between different kinds of views and editors.
I've read the proposal and it looks like every editor will have its operations stored in one giant stack. While I'm sure this stack will be hidden inside a (big) black box, it isn't very efficient or easy to implement. And I don't see any reason to interleave unrelated operations from different editors. It makes a lot more sense for each editor to have it's own stack. I say this because 1) the implementation maps directly to the user's mental model, and 2) the implementation is easy, and 3) The application does not give up control over its stack. The stack is essentially that editor's context. A "workbench operation service" would notify to the editor that an operation is happening (such as a refactor), and the editor would decide to place an entry onto its stack identifying that workspace-wide Operation (and possibly doing some actions in response). Undo for the editor is strightforward and does not involve the workbench until the top of the stack identifies an operation. Similarly, if the workbench wants to undo the refactor, it checks with the editor, which realizes that that operation is covered by N subsequent text edits, and indicates (UI TBD??) that those text edits must also be rolled back. This approach is much more migration/compatibility-friendly. An application can integrate into many workbench-wide operations without changing their API defining command/commandstack. I have questions about scenario #4. If a workbench-wide action is performed, how is it that an editor is capable of undoing only the portion relevant to it? How does it know what happened to its resource(s)? IMO, this information is not usually captured by the editor, JDT is the exception. I didn't see any scenarios that deal with CVS interaction or restoring from local history.
In regards to the question about scenario #4 in comment 34, an editor only undoing the local portion of a refactoring operation is what happens in R3.0.1, but is not what is being proposed. It was there for comparison. You say that editors having their own stack maps more directly to the user's model. As soon as you make entries in your editor's stack for workspace operations you are in effect violating this local model. I think we have similar desires for the user model, and we are debating an implementation detail for storing the history. A prototype that gets refactoring and text communicating properly will help resolve much of this swirl. I can factor the prototype so that adding operations to the history and assigning contexts from the text editor can be done in one of two ways: 1 - as described in the proposal 2 - keep a local stack and instead of assigning a context, make a local representation for my stack. Then we can compare the performance characteristics. The straightforward case (the last N edits were in one place and purely local) is pretty simple in both implementations. We need to see how it holds up when there are many parts interested in the same operations and maintaining their local markers. A local stack per part seems obvious to many (see also bug 1887) but I worry about keeping all the "copies" in synch when many views/editors decide they are interested in a more "global" operation. I also think some contexts (such as the workspace) are not related to any particular part and so who owns that history? You mention "the workbench wants to undo the refactor, it checks with the editor"...which implies that there is some workbench-level undo history. So are you saying there is still a black box, just smaller and supporting one global context? Or that the navigator, for example, has its own stack? Is the workbench service you describe keeping a workbench history or is it just a place for any view/editor to inform listeners about operations, allowing listeners to add local markers representing certain operations to their local stacks? I'm not tied to either implementation, and believe that the meaty issues (the ability to find out about an operation and decide it is of importance to some part, representing in some way that my part cares about your operation, handling the rollback situations) are common for both data structures... The global history simply provides a uniform way to access the history for shared contexts. The local history means that a third party (such as the workbench) has to keep operations for shared contexts.
IStatus canExecute(); IStatus canRedo(); IStatus canUndo(); void execute(IProgressMonitor); void redo(IProgressMonitor); void undo(IProgressMonitor); I don't see any reason why can... methods should return an IStatus object. The should perform quickly and thus, should be implemented lightweight. I'm not aware of any UI feedback where an IStatus result is useful for can... methods. Note that we are talking about a general purpose undo framework and not a refactoring wizard. Can you do this? OK! Fine! Can you do this? WARNING! You can't? What you mean? Can you do this? CANCEL! What? Should I cancel the workbench? Can you do this? ERROR! Does it mean you can't? Are you sick? Is you code bad? At least 3 results allow a broad range of interpetion. A simple boolean return value is suitable. IMHO, it's more important to let the execution methods (execute, redo, undo) return an IStatus object to indicate the operation result. There is nothing in the proposal that covers and error state during the operation. Should the executions methods throw RuntimeExceptions?
Yes, we are debating implementation details. The majority of commands are going to be local editor commands. Workbench operations are rare, especially undoable ones. So the number of markers that will be on stacks will be small. Using the proposed implementation, each operation needs to be stamped with at least one context, which is another form of redundant copies. To minimize marker count, you can identify conflicts up front instead of later at undo-time using approval. If a refactor operation involves resources XY&Z, editors of AB&C can inspect the operation up front and avoid putting a marker on their local stacks since the operation doesn't affect them (back to mental models again). When an editor is closed, removing its operations from a single stack is going to be less trivial. With local stacks, you just signal that that context is dying, and let it get GC'ed. I'm not sure on which stack a refactor would occur. Maybe JDT would use a workbench-wide "resource operations" stack. You don't want it on a view of course. I think the workspace DOES need to own and manage a "resource stack" because certain things are going to cause that stack's operations to get flushed. For example, catching up with CVS, replace with local history, project delete, re- importing source, etc. Someone needs to monitor these changes, and proactively flush operations which can no longer be undone. This would imply that the "workspace" stack contains "workspace" operations, which know the resources they involve. Is there some way to avoid this?
Re: comment 36. I understand your concerns. Will work through scenarios. I've gone back and forth on this myself and need to go through the refactoring flows in more detail. Re: comment 37. Agree that markers can be minimized up front. I would expect editors to listen to the operations history and "adopt" the operation (avoiding stating how it's done...assign context/make marker) up front if interested. Agree that local edits case is most common case and users/code should not pay for introduction of more global contexts. I still think there is the potential for different kinds of "more global" contexts (apart from the workspace) and a unified place to keep history is useful. I'll avoid guessing about this and will report as prototype evolves.
The 2 are equivalent, but in one case the application gives up control over something it used to own: its stack. This is going to cause migration problems.
The proposal at http://dev.eclipse.org/viewcvs/index.cgi/%7Echeckout%7E/platform-ui- home/R3_1/undo-redo-proposal/undo-redo%20support.html has been updated to reflect the prototype and investigation. I built a prototype on M3 that integrated refactoring undo, text undo, and some sample actions. The new proposal is much more specific than the original one, although the framework is similiar in spirit. Some changes to argument types, return types, etc. were necessary to integrate the existing undo strategies. I am currently porting the prototype to M4 and will determine how to make code available for the curious. In the meantime, please keep directing comments on the proposal to this bug.
A couple of comments on the proposed APIs: - The main consideration with Java interfaces is that it is difficult to add methods in the future if the interface is something that clients are expected to *implement*. Where feasible, an abstract class provides more flexibility than an interface, since it's always possible to add new methods (with a suitable default implementation) withoutbreaking clients. The proposal mentions a number of interfaces (although it's unclear in some cases whether clients are expected to implement them or to just call methods on them). IOperation is clearly one that clients are supposed to implement, and as such would be a candidate for an abstract class. But'll you'll need to weigh the increased flexibility that comes with the abstract class from the decreased flexibility for retrofitting (while an existing client may be able to change their existing operation class to implement your new IOperation, it may be harder for them to move their existing operation class under your new abstract Operation class). OperationHistoryEvent - It's generally a bad idea to expose fields other than constants. Provide getHistory() and getOperation() accessors instead of history and operations fields.
I also noticed that the event class has several boolean methods that identifiy the type of event. Most of the other examples I have seen in Eclipse use integer contants to identify the different event types and have a single getType() method. Also, two questions also came to mind when I read the proposal. The first involves the relationship between concurrent operations (i.e. using Jobs) and command execution. At first glace, I don't see how concurrency fits into the puzzle. Do you foresee the possibility of supporting background undo/redo? If so, you may want to include a section on this. If not, you may want to state it explicitly so clients know what to expect. My second question is a bit harder to phrase and involves the IOperationApprover. The approver is expected to prompt and I don't see where the approver gets it's UI context (i.e. Shell). Prompting needs a Shell to parent dialogs. Is it expected that all approvers will have access to their own UI context (e.g. editors and views have access to the part site so approvers connected to these parts would be OK)?
I don't like having add and remove contexts on the operation. It still seems like doing things backwards. I don't understand why each context doesn't have it's own command stack. A common scenario for existing command users goes as followed. UI elements are asked to contribute operations. The caller may then take multiple operations and compound them, or maybe there is just one contribution and it is executed. The operation provider may have also chosen to compound several operations together. You end up with a bug tree-like structure of ops, and only the root is passed around. It doesn't make sense for every node in this tree to have set/remove context, and it wastes memory for them to support this uncalled API. The "context" is really the editor's local commandstack instance (or lack of one in this case). The undo stack should be the context. But instead, where putting the equivalent of setStack on each operation. If you rename the methods as follows, the backwardsness may be more obvious: interface IOperation { addUndoStack(..) removeUndoStack(..) hasUndoStack(..) getUndoStacks(..) .. }
Re: comment #41. Thanks for the suggestions. The interface IOperation is the initial integration point for the various command frameworks that exist. Clients can adopt the interface without necessarily using the shared operation history or listeners. IOperationHistory is provided as an interface so that RCP apps can implement their own without buying into the policies of the workbench implementation. The sketchiest interface is IOperationContext and I think it's a wise suggestion to consider an abstract implementation, as this may evolve with further use. The public fields in the listeners are now removed. Re: comment #42. I agree that the boolean event type methods are less common than checking the type. I looked at various event classes to see what was common, and upon seeing this technique, I liked the readability. It's easy to change if this is considered difficult or odd to work with. Comments? As for jobs, it has been suggested that operations could perform some help with progress reporting or job management. If the operation itself is performed in the background, one could expect the undo/redo to be performed in the background. I have avoided saying anything about jobs until I've actually written some operations that use them. Any special knowledge of jobs would probably be in a job-oriented subclass of AbstractOperation that clients could use??? Not sure yet. IOperationApprover and the UI: I understand your question. It's also hard to explain without looking at code. IOperationApprover is defined generically and it is assumed that any knowledge of the UI needed to do prompting would be carried in the operation or its contexts. The workbench installs an IOperationApprover that recognizes linear undo violations and consults the contexts as to whether this is allowed. The context used by text editors knows the text editor and therefore the shell or any other UI info needed to do prompting is there. Re: comment #43. "Backwards" depends on where you stand. Contexts look surprising from an implementation perspective when you are only worried about your local edits. In that case you can think of a context as a filter on a global history. When commands/operations span multiple contexts, the concept is helpful. All I can say is that clients have said it helps address the shared cases. In the initial prototype, I had a version of the text undo manager that "wrapped" workbench-oriented operations and assigned to its local stack. I got to delete a lot of code when instead I could just use the workbench operation history and assign my context to operations of interest.
Another clarification re: jobs. The operation history tracks a sequential history of operations and in the near future, the workbench will only support a linear-style undo of operations rather than a more selective undo. The framework itself can support less strict models and theoretically one could build a history that allowed concurrent undo/redo, which has characteristics similar to a direct/selective undo. However, our workbench operation history won't support this, and we will have to ensure that an action that triggers a concurrent operation only adds the operation to the workbench history after it's actually completed. The undo and redo methods also assume that the status of execution is reported on return.
Another alternative to having IOperation#addContext() would be: IOperationHistory#execute(IOperation, IOperationContext[], ...) which goes along with undo(context), and redo (context). What is the mechanism for responding to an operation which is under execution, and appending some additional operation and context to the history? For example, a resource gets deleted in the navigator, so an opened UML editor deletes any views refering to the types declared in that resource. The undo should be atomic, so that both operations get undone together. Couldn't OperationHistoryEvent just have "int getDetail()" which returns a constant for whatever happened instead of multiple boolean methods. This would allow you to write a switch statement in your listener. > assign my context to operations of interest What does this mean exactly. Do you have a scenario where you are just watching operations get executed and tagging them? IOperationHistory#remove(IOperation op) is not a convenient method. Contexts may have been added to operations I executed, and those contexts could still be valid even though my context is going away. IOperationHistory#purge(context) would be much more convenient. What happens if I execute an operation and just leave it hanging around in the history forever?? Can the client configure the amount of history kept for their context? If an operation can't be undone, or approvers don't allow it to be undone, shouldn't it just be purged to free up memory?
If an operation attempts to acquire a lock being used by a background job, and that job attempts to place an operation on the stack once it has completed, couldn't this result in deadlock?
Re: comment #47: Obviously, any time you are acquiring locks, you must do so in a manner that does not lead to the possibility of deadlock. I would suspect that operations would only acquire locks while being executed, undone or redone and they would do so in a manner that was not deadlock prone. I do think that supporting concurreny in the operations and undo/redo stack does pose certain challanges, especially since the stack is sequential and concurrent jobs may or may not be (as was stated in comment 45). I wonder if it would be possible to support background undo/redo that would allow the user to do other things except another undo/redo. This exclusion may also have to include the execution of operations that would add to the stack. My original statement was that I would like to see the issue of concurreny addressed in the proposal. I think the most we would need to shoot for initially is to support concurrent operations that are added to the stack after they complete and have undo/repo done in the foreground. Background undo/redo could be pursued at a latter time if it was felt to be warranted.
My concern was that the operationhistory itself was synchronized to support being accessed from the job thread, in which case the history provides a secondary lock.
Re: comment #46: Contexts can get added at any time, not just initially, so the dynamic protocol needs to be there. It is a common scenario to watch for operations added to the history and tag them. For example, a text editor watches for operations with the workbench context, and tags any operation that has elements related to what is being edited. (In this way a refactoring operation could appear in the undo history for a source editor). Operations that in turn execute other operations need to use compound operations to ensure that all of the execution/undo/redo happen together. Otherwise, you'll have the inner operation added first and the outer operation added later, with no relationship between them. The remove protocol is for an operation, not a context. To purge a context, use dispose(context, true, true). Whether an operation gets removed on failure depends on the status severity (see the javadoc on the undo/redo methods). If there is an ERROR severity the operation will get removed, otherwise it's up to the caller to decide what to do, hence the remove protocol. It depends how aggressive you want to be on purging the stack. The workbench action handlers are aggressive because they enforce a linear policy - they purge a context when a failure occurs. But other RCP apps may want to do something different, hence it's left to the client to decide on the policy. If you execute an operation and just leave it hanging in the history (you never undo it, remove it, or dispose of its context), then it will just hang around until the limit is reached, when it will eventually get bumped out as the oldest operation. Another way is that the editor/view that created the context disposes of the context and the operation will be removed, assuming it doesn't have another context assigned to it. This is how local text edits disappear. The limit is currently global - there may be a case for context-based limits, but we decided to wait for the scenario that proves it. In the prototype, text editors increase the limit when they start and decrease it when they are closed. So in theory they actually have a larger history than expected. This could be a problem if you use up the global space in one editor and expect to go back to another editor and undo there...(local stack behavior). We need to get user feedback on that issue and can enforce local limits if need be.
Another method could be added to append contexts (and perhaps an additional operation) to the operation which is under execution.
> This could be a problem if you use up the global space in one > editor and expect to go back to another editor and undo > there...(local stack behavior). We need to get user feedback > on that issue and can enforce local limits if need be. Does this mean that the prototy that is currently beeing worked on does not provide local stacks? If I switch from one editor to another I expect to have different undo stacks with the same limits. Ideally it keeps all operations starting from the last save. That's like the majority of text editors are working. Most flush the stack if the file is saved but that's configurable in the preferences. See also bug 80137 comment 2 for user feedback.
Gunnar, the user's mental model of having local command stacks for each editor is implemented using a global command stack with tags on each command to identify which local command stack it conceptually belongs to. When an editor undoes a command, it looks for the top-most command on the global stack with the appropriate tag on it. I think most people agree that the undo limit must be maintained per editor, and not as one global limit. I think what the text editor is doing now is a temporary hack.
The 3.1M5 milestone contains the initial implementation of the undoable operation framework. The proposal at http://dev.eclipse.org/viewcvs/index.cgi/%7Echeckout%7E/platform-ui- home/R3_1/undo-redo-proposal/undo-redo%20support.html has been updated to reflect the latest design. The work for putting the text undo manager and refactoring undo manager on top of this framework is underway and should appear soon. These efforts have resulted in valuable feedback and some changes to the API since the original proposal. Although the API may still evolve considerably, I encourage those interested to try out the new framework and provide feedback. If you have a problem with specific API, or find bugs in the implementation, please open new bugs against Platform UI rather than annotate this bug. If it's a broad issue, please annotate this bug with a reference to the new bug so that those watching this bug will know about it. This bug can still be used to answer general questions about the strategy. If there are workbench actions you think should be undoable, open bugs against Platform UI to help us prioritize which ones to implement. FYI: Here are specific bugs already opened against the framework: bug 84444: OperationHistory and "invalid" operations bug 84446: OperationHistory should protect itself better against runtime exceptions bug 86243: Undo/Redo implementation should propagate CoreExceptions bug 86244: IOperationHistory#closeOperation should only add the operation to the undo stack if it is valid
> These efforts have resulted in valuable feedback Where is the feedback documented?
Feedback so far has been in posts to this bug (comments since #41), new bugs from the refactoring work (listed in comment #55), and prototype exchange with the text and refactoring teams. Most of the comments from this bug report are incorporated into the M5 implementation or proposal(context limits, int event types, specific discussion of Jobs, etc) though I've not responded to them individually.
I have some comments on naming conventions: - EMF, GEF, and the Text Team use the term "command". Why aren't we sticking with this name (for operations)? Why is something NEW and different being introduced with this already-in-use term? I thought the purpose of this work was to unify the existing implementations. This name hijacking makes it confusing. - The same for "stack" and "history". IMO, a history would be read-only. Everyone knows you can't change history. - What is the relationship between the "commands" package, and the "commands.operations" package? i.e., why are operations under the commands namespace? - "Undoable" should be removed from IUndoableOperation. - fields should not be prefixed with "f". There is a coding convention that mentions this somewhere. I know eclipse doesn't follow ONE coding convention, but it would be nice to be consistent at the component or plug-in level.
Can't speak for other components but the work that's going into Platform Text, JDT Text and JDT UI will have the f prefixes ;-)
That's funny, the team which made prefixes obsolete (advanced syntax highlighting in 3.0) are the ones still using it. But, you don't see the prefix in other core classes, SWT, or "base" Jface, etc.
Ever been color blind? ;-)
re: comment #57: - The name "Command" is indeed the most prevalent outside the platform, but it already has meaning in the platform, and will be even more dominant in the new contributions architecture. Commands are the bridge between the UI (formerly actions) and the behavior (handlers). Handlers collect parameters and do the work. We envision that more and more handlers will create operations as they become undoable. So we can't use that name. I haven't thought of any name that everyone else likes, but IUndoableOperation has been the least offensive so far. IOperation was the original candidate name but there is an update IOperation as well as the Jface operations package. Since this interface is to be adopted by so many clients for the purpose of providing undo, we opted for the more specific name. We had the same problem with "context." All the good names are taken, so we ended up with IUndoableOperation and IUndoContext. If you feel strongly that these are bad choices, could you open up a separate naming bug with suggestions and then ideas and votes can be added. I won't attempt to rename again until there is clear consensus. Much of the discussion of names has been informal during the prototype phase and there's been no name so far that everyone is happy with. I feel strongly about what names *not* to use (commands, contexts, stack) but I am not strongly attached to any one name. - stack implies strict LIFO. With multiple undo contexts involved, the access is not strict LIFO. Further, an application can implement a history with different undo policies. The DefaultOperationHistory implements a "per context LIFO" strategy, but the interface allows for direct/selective undo also, and this may become more prevalent with concurrent operations later on. Hence the avoidance of "stack." History was my best suggestion, but I'm open to others. I don't like "list." - as the contributions architecture evolves, we hope to see a common pattern of commands that execute handlers which collect parameters and execute undoable operations. So there is a semantic relationship between commands and undoable operations. A practical reason they are in the same plug-in is that they are both intended to be used by stand-alone JFace users independently of the Eclipse runtime platform and have restricted/planned use of core classes, so they are grouped together rather than creating two plug-ins with these same restrictions. - I'm all for fCoding fConventions ;-) Seriously, I will follow agreed-upon guidelines but changing names in the internal implementation will be low prio for me until I feel like the API and functionality issues are sorted out.
The "f" or "m_" or "c_" prefixes should not be used in code maintained by the Platform/UI team. I apologize that I did not notice this earlier when committing your patches. Susan: could you please provide a patch changing the member variable names? I agree it's low priority. If you're going to be rushed coming into the API freeze (M6), then it can wait until after (M7).
I guess I was hanging out in platform text too long while writing this code ;-) Opened bug #86502 for this one.
re comment 60, yes, I'm red-green color blind. re comment 61, Command is new for 3.1 in core.commands. ICommand was introduced in UI for 3.0 and it has an unfortunate name choice which several people commented on during 3.0 development. But, that interface is deprecated after its 1st release, so maybe there is a chance to clean up names? "Command" in GEF and EMF dates back further and has more use than the one in UI. I still don't see why operations is a subpackage of commands. AFAIK they are completely separate concepts. Sure, some command handlers may execute an operation. But, commands may be invoked by an SWT button so maybe widgets.commands.operations ;-) ;-) I wish I had a better suggestion for command+handler to free up "command" for its rightful purpose <g>.
Susan: can we declare success on this plan item?
yes, we can. I've been waiting for the refactoring code that uses the new undo to go live, and it has, so I am marking this one fixed. Bugs/issues in the implementation can be tracked in their own bugs. Marking fixed.
Verified in I20050509-2010.
