| Re: [platform-ui-dev] Undo/Redo Proposal |
See attached with my comments.
(See attached file: undo_redo.html)
"Randy Giffen/OTT/OTI"
<Randy_Giffen@xxxxxxx> To: platform-ui-dev@xxxxxxxxxxx
Sent by: cc:
platform-ui-dev-admin@e Subject: [platform-ui-dev] Undo/Redo Proposal
clipse.org
11/20/01 09:59 AM
Please respond to
platform-ui-dev
Attached are proposed changes to Undo/Redo to implement a global Undo/Redo
stack for the workbench.
Title: Undo Redo Proposal
Thus, in a way, the name global is a bit misleading in the undo/redo case. The actions do not support a global undo/redo stack. They support a targeted undo/redo directed at, and implemented by, the active part. This is typically not what most users expect. In the case of an IDE, they are using the UI to manipulate (make changes to) a model. They expect that undo/redo will allow them to back out of or redo these changes regardless of what part of the ui they are currently working.
This architecture requires that parts maintain their own undo/redo stack (although theoretically some parts may decide to implement a shred stack among themselves).
Another problem is that actions that don't belong to a view (e.g. like the "Refactor" menu which gets retargeted to the active view) can't access the undo/redo actions in the edit menu.
DS> I like this design in general. As you say, the current design is modal in nature, so it is impossible to undo an action, which you may have just performed, unless the target part is active. This makes it very easy to lose the undo feature.
IUndoAction would have the following definition
public interface IUndoAction {
String getLabel();
void undo();
void redo();
boolean isUndoValid();
boolean isRedoValid();
void dispose();
}
DS> I am concerned about the use of the term "Action". IUndoAction is not a subtype of IAction, and has none of the standard methods like run, getImage, etc. I think this object is actually a "record of an action" which can be undone, rather than the action itself. So I might call it IUndoFrame or IUndoRecord.
Comment: Should we consider having an abstract UndoAction rather than an IUndoAction? In some cases this would allow us to modify API in a non-breaking way but it means that clients will not be able to use an existing type to implement IUndoAction (probably a rare case).
DS> From a purely Java standpoint, there is no reason for an abstract superclass unless you can provide some default implementation. Given the nature of the methods in IUndoAction, I don't think you can actually provide much implementation. So, the ONLY reason to create UndoAction is for interface evolution. If the public API for the IUndoStack is simple, like just one method for adding items ( addFrame(IUndoAction) ), and we do not expose IUndoAction ourselves in any getter methods, I think this can evolve the interface if needed by adding methods like addFrame(IUndoAction2). Does anybody but the workbench really care if there is anything on the undo stack? If not, there is no need to expose anything else within the API.
The manager will ensure that the top undo action and top redo action are valid by calling the isUndoValid() and isRedoValid() api methods. If the undo/redo action is no longer valid it will be punted and the next one moved to the top.
DS> Will we update the enablement of the menu item whenever the edit menu is about to show? If the user invokes the Undo accelerator (Ctrl+Z) will we check the enablement of the action at that point, and return silently if disabled?
The valid check is required as an undo action cannot validate itself simply by listening to model changes. It would also somehow need additional information if the received delta will have an undo which will be put onto the undo stack. Consider the following case: A Java refactoring renames file A.java to B.java. Then the user goes to the navigator and renames B.java to C.txt. If the navigator pushes an undo action onto the stack, the refactoring undo is still valid since the user must first perform the undo for "rename B.java to C.txt" before he can execute the undo for "rename A.java to B.java". If the refactoring kernel listened to model changes only, without knowing that there is an undo for the renaming of B.java to C.txt, the kernel would invalidate the undo for rename A.java to B.java.
Note that we are only going to validate the top undo/redo actions. Validating others on the stack would be too complicated. Consider the case where one renames a compilation unit. In this case we have two changes, one that renames the top level type and one that actual renames the file. To be able to figure out if the undo for this refactoring is still valid, we have to check if both changes are still valid. To do so we have to simulate the undo of the rename of the file to be able to check if we still can undo the renaming of the top level type. This is necessary since the rename of the top level type doesn't know anything about the rename of the file. For example:
Rename A.java to B.java.
We would add tool bar buttons for undo and redo with a menu button to the right like the "new" button. The menu button would show the current undo or redo stack and let you select at any level in the stack. Again it is important that IUndoActions be able to validate themselves since this will determine the enablement of these buttons. The undo and redo actions in the Edit menu would show the label for the action currently at the top of their stacks.
DS> The "arrow" idea seems incompatable with many of the previous statements. In particular, "we are only going to validate the top undo/redo actions". If it is only possible to accurately determine if the top actions are valid, how can you possibly expose the entire undo stack and let the user go back n steps? This may be an invalid operation.
Once the new api is available, it is likely than many more actions would become undoable (ex. resource rename, move etc.).
DS> I disagree. These actions are theoretically
undoable now. The only reason you can't undo them is because the
Navigator has absolutely no support for undo / redo, and core has absolutely
no support for undo. If these conditions were met, the undo / redo
actions would work for resource rename, move, etc.
DS> If an action is performed, but no IUndoAction is added to the IUndoStack, what will appear on the IUndoStack and in the undo menu? Will it show the text for an older, possibly obsolete action? How will you ensure consistent use of the IUndoStack throughout the product?
DS> I am not convinced that this is an acceptable breaking change. For those parts which do not expose undo / redo within their context menu, undo / redo will be completely broken, and the part functionally incomplete. If the part has a global action handler for undo / redo, it might be better to delegate to those handlers temporarily, while the part is active. Perhaps some indication of the local nature of undo could be given in the menus. If this approach is taken, at least existing parts will continue to work.
If you make a breaking change, every editor on the planet not derived from AbstractTextEditor will be broken. If we're willing to accept this, let's give up on API compatability.
2) The text widget implements its own undo. For example create a new
task and edit its description note that there is an undo item in the widgets
popup menu. This undo is local to the widget. Currently Undo is not enabled
under edit in this case since the tasklist does not supply a global action
handler for this purpose (There is no api on Text to allow programmatic
undo). However if we have a global stack it is likely that Edit>Undo would
be enabled but it would perform something different than undo on the popup.
Thus should we:
1) We could ask SWT for undo API on text in this
case.
2) Continue to allow Undo/Redo to be global actions
for which we simply define a default behavior (which will not be retargeted
by 95% of the parts).
3) It is not clear that an IDE built using the workbench has only a
single model. For example, suppose I
i) Edit some .java file in the Java perspective
ii) Switch to the Team perspective and create something in the Repositories
view
iii) Switch back to the Java perspective, decide I don't like my change
and press Undo.
A global stack will cause the item I created in the team perspective
to be removed (this will perhaps happen silently).
But this is exactly what some would expect. They
find it confusing to have more than one undo stack in an application.
DS> You have a number of choices. There could be one undo stack for each page, each window, or the entire app. What would I expect as a user? Given that a page == a window, there are only 2 choices: window and app.
If I'm in an editor, and I make a change, I would be surprised if an undo action for that editor appeared in an entirely different window. If we say that each window is a separate task, good task separation should be maintained, and each window should have a separate undo stack.
If you adopt a window approach perhaps the method to get the undo stack should be on the window, instead. For convenience within a part there should also be a method on IWorkbenchPartSite to get the undo stack, too, so there is no need to traverse upwards.
4) How do we define what can go on the undo redo stack? Do we limit it to model changes? For example opening an editor could have an undo that would allow the user to close the editor and return to the previously active part.
DS> In most applications, the undo stack contains
model actions, but does not contain UI actions. For instance, if
I move a toolbar in Netscape Composer, an undo action does not appear for
the action. The Undo stack only contains document centric items.
I suspect this represents a general belief that the user is more interested
in interaction with the document, not the user interface, so you don't
clutter the undo stack with ui gestures. UI interaction is
also unconscious for the user, whereas doc interaction is conscious (the
focus of attention).