Community
Participate
Working Groups
Build: I20061031-0656 Undo of resource changes only works when the Navigator view has focus. This feels really subtle and I'm wondering if it will be a barrier to people discovering and using it. For example, if I create a new file, focus is immediately given to the editor so you don't even know the undo option is there unless you click back into the navigator. Likewise if a user happens to switch to the Outline or some other view, I could see them getting frustrated that undo stops working. I don't really know the right answer here - I realize separate parts can have different undo stacks. Maybe if the new active part has no undo stack or its stack is empty, you should keep the previous part's undo stack active? I don't know much about the implementation details so I'm a bit fuzzy, but it feels wrong as a user that I always have to click back into the Navigator to get undo to work...
From Susan (email): The issue with showing undo history from an editor is really a sticky one. The editors keep their own undo history, and in some cases there is overlap. We did a lot of work in 3.1 to make good overlaps happen. For example, in JDT, when a refactoring is performed and it affects the open editor, those refactorings show up in the editor's undo history, along with the localized text changes. But in those cases, the refactoring changes actually caused text changes so it feels "correct" to a user that they are included. It would be possible for us to better here, but it takes changes in either the text undo manager or some tricks with composite operations to smoothly integrate a more global operation into the text undo stack. Also note that if we put superfluous changes in the undo history for an editor, then users who just want to Ctrl-Z to back out of some text changes could get confused.
I agree mixing resource change undo with the text undo stack doesn't seem right either.
marking for 3.3 (no milestone yet). Something to either solve or document. I agree it's confusing, especially when the action you want to undo caused an editor to open. It's not apparent right away why the undo is not available.
I also agree that merging the undo histories is a bad idea: text editor users expect undo to be disabled unless they start typing.
Dani: how about the case where the file has just been created? It makes sense to me as a user that since nothing has happened yet in the editor, undo could reasonably undo the file creation. As soon as the editor has any contents, the editor undo stack should take over. Of course I'm saying this without understanding how the undo stacks are managed... maybe this case is too complicated to be worthwhile.
I agree that currently it is also strange that I have to go to the Package Explorer to undo a rename. Hence I would not (only) special case the new file but the non-dirty file. Idea: instead of merging the stacks we could maybe provide an action that accesses the global stack while the file is unchanged and the local stack when there are local changes.
interesting idea. That would be a matter of having the editors use a different undo context when first creating the undo and redo actions, and then resetting the context when the document changed. Dani, do you want me to explore this post-M3 and provide an experimental patch?
Mmh, I rather thought about hacking this into the action and not messing with the editor's context. I won't have much time to work on this since other items have a higher priority.
The editor is currently using the stock undo/redo action handlers, and their undo contexts can be reset. I'll play around with this some in the M4 timeframe and report back...
Created attachment 53242 [details] AbstractTextEditor162964.patch Dani and John - give this patch a whirl. Kind of cool for the case John mentions, but possibly some unintended side effects... The patch is set up to monitor the undo history. When there is no history for the local editor, the undo context for the editor is switched to the workbench (global) context. Undo and redo are treated separately, so if you undo until the file is not dirty, you'll see an undo for the last global operation, but a redo for any local typing the user has done. Groovy. And scary. It plays really nice for the scenario John reported, but issues I see: - if you are quickly Ctrl-Z'ing through the undo stack, you could accidentally cause your file to be deleted by undoing the original creation of the file. Yes, you could redo the creation of the file, but the local undo history for the file is gone, so you couldn't then redo all the typing. - for that matter, if the last global operation didn't have anything to do with your file at all, you might cause something really unintended to happen, but at least you don't lose your file and your redo history. You might not even notice? Scary that you could be unintentionally whirling through the global undo history. There are things we can do to refine the switch to the global context, such as: - only switch over to global undo when there is no undo and no redo history, to avoid accidental global undo when the user was working with local edits. If there is no undo or redo history, the user hasn't typed anything yet. - modify the operation approvers that are already installed which warn when a non-local operation is occurring. Have them watch for these cases and warn the user with option to proceed... If you guys think this is worth pursuing, I can investigate further.
In my opinion it is important that we do not allow to do a global undo once local changes are on the stack. Hacking this into the text editor seems wrong to me. I would expect that the fix is not in the text editor but in the Undo/RedoActionHandlers since the same problem applies to all parts that modify local content and use those handler. As far as I can see those handlers could nicely do the redirection.
Right, but if there are no changes on the stack (because the user has undone them all) do you still mean there should be no global changes...? I have to think about the action handlers. I'm just not sure the behavior is universally desired, so I'm not sure it should go there, although I agree it's a convenient place to implement it.
>Right, but if there are no changes on the stack (because the user has undone >them all) do you still mean there should be no global changes...? The global history should be there as long as no change happened in the local stack but once there was a change it should never come back because otherwise we run into the ugly scenarios where e.g. a refactoring is undone because of hitting Ctrl+Z too often. I fairly often hit Ctrl+Z until my file is no longer dirty.
cc'ing some other UI team members for general comment. Now that resource navigator supports undo, what do we want to do to make the "workbench undo history" available from other views? The workbench undo history is the history of the more global undo operations. In IDE-space, this refers to refactoring changes, resource modifications, etc. Currently you can see it from the navigator or package explorer, and you can see subsets of it in the marker views (subsetted by marker type). Where else would you expect to see it...(see John's initial comment #0)... - should it be visible from editors that have no local changes yet? - should we install undo history on outline views...and should it be the same as the outliner's editor? - should views that don't otherwise register an undo action handler get one that shows the workbench undo history?
> should views that don't otherwise register an undo action handler get one > that shows the workbench undo history? This would be my feeling. It should be a global action as you can create delete resources in potentially plenty of places.
Paul and/or Duong: Is there a way to set up a LabelRetargetAction so that there is some default handler installed unless a part specifically retargets it? Or would we need to make this part of the behavior of WorkbenchPart? I'm not sure what the best way is to accomplish a default handler for a retargetable action. I'm still not sure this is a good idea...for example, I would expect an outliner to show the same undo history as its editor, not the workbench-wide history. The alternative is to explicitly decide what views and editors show undo histories and explicitly assign the handler.
>I would expect an >outliner to show the same undo history as its editor, not the workbench-wide >history. Yes, of course but how's that scenario different from the editor one? Already now it uses the same setup as the editor and hence if the global action would do the routing then it worked exactly like in the editor, right?
(In reply to comment #16) > Paul and/or Duong: > Is there a way to set up a LabelRetargetAction so that there is some default > handler installed unless a part specifically retargets it? Or would we need to > make this part of the behavior of WorkbenchPart? I'm not sure what the best > way is to accomplish a default handler for a retargetable action. Hmmm... In an editor, there's already an active handler for CTRL+Z, the editor's handler. The editor's handler gets to decide if it's enabled or disabled. I think your desired behaviour would be the handler deciding to delegate back to the global context if there have been no local changes. In the case where the part doesn't offer undo/redo, you can add a default handler to the undo command. That will be executed if there are no competing active handlers, and could just walk the global undo context. PW
Sorry for confusion, my comments referred to the idea of setting a default handler for views that don't set one. Simply that it may point out cases where a different undo context should be installed, rather than getting the global history for free. I'm sure we'll find those cases as we go. Paul, it's still not obvious to me how to set up the default handler...can you point me to an example of where/when this should done in the workbench? Can I do this using the existing LabelRetargetAction defined for Undo/Redo in WorkbenchActionBuilder or should I be doing so with a command extension?
(In reply to comment #19) > > Paul, it's still not obvious to me how to set up the default handler...can you > point me to an example of where/when this should done in the workbench? Can I > do this using the existing LabelRetargetAction defined for Undo/Redo in > WorkbenchActionBuilder or should I be doing so with a command extension? Normally you can set a default handler in the command definition for org.eclipse.ui.edit.undo. That would give Undo a handler to fall back on if (in the common case) the active part didn't offer to do the Undo work. All commands are like headless retargetable actions, and in 3.3 going forward we would use the menus API + commands + handlers. But the initial 3.3 menus stuff won't be available until M4. In the short term, I'm not sure what to do. I could investigate the possibility of replace the Undo LabelRetargetAction with an action that could delegate to the undo command ... but there might be some unpredictable behaviour. setGlobalActionHandler(*) would continue to work, though, since that currently generates ActionHandlerProxys for each IAction (I think :-) PW
Thanks, Paul. I don't see spending time hacking in a temp solution to LabelRetargetAction if everything is migrating to commands later in the cycle. I've retargeted (heh heh) this bug for M5. I'll go ahead and work on the issue of using the global undo context when there is no activity in the assigned context and annotate here when that part is working. But this bug will stay open until we can install the default handler everywhere...
Changed the OperationHistoryActionHandler so that it operates on the workbench undo context if there has been no OperationHistoryEvent sent for the assigned context. As soon as any event involving the assigned context is received, then that context will be used. This can be verified with this scenario: - create a new file from the new file wizard - an editor for the new file opens - note that "undo create file" appears in the undo menu. You can undo/redo the file creation. - subsequent edits to the new file cause the editor undo commands to appear in the menu, and if you undo all the way back, you will get a disabled undo menu. The workbench undo history never reappears (as desired). Note that this will occur in any text editor that is not dirtied. You will have access to the workbench undo history until activity is registered in the editor. This is available >20061113. The remaining work on this bug is to install the Undo/Redo handlers as the default handlers for undo/redo. This will happen when the new menu/commands story is in place. Assigning this bug to Kim since I'll be on leave when that happens...
Comment on attachment 53242 [details] AbstractTextEditor162964.patch this is now handled in the action handler
>Note that this will occur in any text editor that is not dirtied. You will >have access to the workbench undo history until activity is registered in the >editor. What happens in this scenario: 1. open editor 2. type 3. Undo ==> editor no longer dirty ? As outlined in comment 13, we should not allow a global undo in this case.
Global undo will not reappear in that case. Once the action handler sees *any* activity for the designated undo context, it no longer uses the global undo context.
In a GMF editor each time the workbench part changes a new UndoActionHandler is created for a new workbench part site and new context. Due to the new 'activeContext' field in OperationHistoryActionHandler, the newly created UndoActionHandler has 'activeContext' equal to false and hence if you had some undoable operations on the stack, Undo will be disabled until a new operation is pushed on the stack. For example: 1) Added a shape on the diagram (Undo is enabled) 2) Switch to package explorer (Undo is disabled, which is ok) 3) Switch back to the editor part (Undo is still disabled - but should be enabled, since there are operations on the stack) Is the above something that will be taken care within this issue? Are we expected to use the same action handlers all the time and not create new ones when part changes?
For my previous comment the GMF defect: 173361
I don't know why you would want to create a new action handler and undo context each time your editor becomes active. That sounds like the underlying problem to me. Note that text editors do not have this problem, they retain their context and handler until they close. This is the expected pattern.
The new UndoActionHandler is created for the Global Undo. The new context is not created, the context remains the same. Only the action handler is created for the already existing context, which has operations on the stack already. When the handler is created 'activeContext' is false, but there are operations on the stack. Since #getUndoContext() returns platform context if 'activeContext' is false, Global 'Undo' is disabled, although the editor context has operations on the stack. So, my impression that instead of: final IUndoContext getUndoContext() { // If no context was specified, or the specified one is not // in use, use the workbench context. if (undoContext == null || !contextActive) { return PlatformUI.getWorkbench().getOperationSupport().getUndoContext(); } return undoContext; } perhaps it may be better to have: final IUndoContext getUndoContext() { // If no context was specified, or the specified one is not // in use, use the workbench context. if (undoContext == null || getOperation() == null) { return PlatformUI.getWorkbench().getOperationSupport().getUndoContext(); } return undoContext; } Doesn't it look like a problem (i.e. the 'contextActive' check)? It felt like the discussion here at least partially goes about something related. In any case, if this doesn't look like a problem i could take a look at how you deal with global undo/redo in the text editors.
I see the bug now. I've opened bug #184604 to address the problem, which is that activeContext should initialize based on whether there is local undo history. However, it is "working as designed" that once activeContext becomes true, the handler should never reference the global history. This is to support the requirement in comment #24 and comment #25. Once there is any activity in the local context, we never refer back to the global context. If you want your action handlers to refer back to the global context whenever the local stack is empty, then you would want to create your own custom action handler, but be warned that this would be inconsistent with the text editors. I still recommend that you change your code so that the action handler is only created once. Not just for garbage reduction, but also because the action handlers register part listeners and operation history listeners, and there will be lots of extra notifications and churn happening if you are creating a new action handler every time your editor becomes active. Finally, I'm going to close this bug as fixed since we addressed the initial problem John brought up, and have opened bug #184607 to discuss installing a default undo command handler when we migrate the action handlers to commands and handlers (per comment #20 and comment #21)
This bug slipped through the verification cracks somehow. Susan, using the steps you posted in comment #22 I'm getting different results depending on the file created. If I create a text file it works fine but if I create a java file it does not. Is this expected?
yeah, I just marked this fixed pending migrating to commands, so the fixed part never got verified. Not all creation paths support undo, but the way to verify this bug is to check the undo menu from the package (or project) explorer. If "Undo New File" appears there, then it should appear in the new editor. So I think you've sufficiently verified it.
Good 'nuff. Marking verified in I20070501-0010
Per bug #200424 and others, we are pulling this feature. If you read the discussion in this bug, you'll see we agreed it was something not too important, but cool if it could be made to work right. It's not working well.
