Bug 126016 - [Undo] Allow appending operations to the history during an ongoing operation
Summary: [Undo] Allow appending operations to the history during an ongoing operation
Status: NEW
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.2   Edit
Hardware: PC Windows XP
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: Platform UI Triaged CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-02-01 10:13 EST by Randy Hudson CLA
Modified: 2019-09-06 15:31 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 Randy Hudson CLA 2006-02-01 10:13:15 EST
In some cases the application model will notify listeners of changes, and a listener will determine that additional work is required. For example, if a UMLClass is being deleted from its namespace, then a diagram which is displaying that UMLClass will need to delete its view of that item. When it receives notification of the class' namespace changing (to null), it should be able to append an operation to the operation that is underway in an atomic way.

pseudocode:
if (operationHistory.isCurrentlyExecuting())
     operationHistory.appendOperation(additionStuffToDo, myContexts[]);

The contexts of the ongoing OP and the appended one would get effictively unioned.

The workaround (for the client) would be to declare an extended operation interface that allows chaining. But, I still don't see a getter method that tells me the current status of the history, such as whether an operation is currently being executed. Or a way to grab that operation for "chaining" to it.
Comment 1 Randy Hudson CLA 2006-02-01 10:16:41 EST
moving. I didn't realize that "core.commands" was UI
Comment 2 Susan McCourt CLA 2006-02-07 13:44:51 EST
This is a rather long response (sorry).  Please read and comment as we are approaching API freeze.  It's important to have this discussion now.

What you want to achieve is implemented, but backwards than how you describe it.  Instead of the creator of the subsequent operation saying, "chain my op to the currently open op", the caller of the original operation says, "I am about to do a bunch of work that is going to cause other things to happen.  Chain everyone to me until I'm done."  

See IOperationHistory.openOperation(ICompositeOperation operation).

This accomplishes what you describe, but the original op made the decision to chain, not the triggered ops.  This is how a refactoring operation gets "unioned" with local editor changes that it may trigger.  The editor operations (and therefore their contexts) get added to the refactoring op and the refactoring is now visible from the editor's undo history.

This should accomplish what you describe in your pseudocode, since the history itself did just what you describe.
if (operationHistory.isCurrentlyExecuting())
     operationHistory.appendOperation(additionStuffToDo, myContexts[]);

But as soon as you want the outcome to be described by pseudocode such as:
if (operationHistory.isCurrentlyExecuting()) {
     op = operationHistory.getCurrentlyExecutingOperation();
     if (op has something to do with my stuff)
        operationHistory.appendOperation(additionStuffToDo, myContexts[]);
}

then we have a problem, because the current API doesn't allow you to decide whether your stuff gets added to the open operation.  I'm aware of inherent theoretical problems with this approach (for example a background task unrelated to the open operation adds to the history while something else is executing), but this approach seems to have held up so far.  

Another important issue with this approach:  It's common that when you chain operations like this, you don't want to actually undo all the sub-operations, because you'll get them for free when you undo the original refactoring operation.  If some model change event caused listeners to do additional work, then the undo of the original op will cause another model event, which will cause listeners to undo their additional work.  

The main benefit of the chaining is to get the contexts added to the original op, rather than to undo or redo each of them individually.  This is kinda cool, because listeners can remain unaware of the whole issue.  They add their undoable operation to the history when they do work, not worrying about whether something else triggered it, and it gets added to the open composite.  The implementation of ICompositeOperation that we use for the open operation is TriggeredOperations, which is not a true/typical composite.  It absorbs the contexts of the added operations but does not undo/redo its suboperations when it is asked to undo/redo.  It gets rid of them.  This is how refactoring and text undo were made to "merge" together without either one knowing about the other.  

Given your problem description, I think the existing implementation will "just work."  But if you have conditions you want to check before chaining ops, then we have to work through that scenario.

If you think this will work, but you need a true composite implementation of ICompositeOperation to pass to openOperation(...), then we can add one.  I've not done so simply because it's not been needed yet.  
Comment 3 Randy Hudson CLA 2006-02-07 15:42:59 EST
The way I understand it, the original executer of the operation must have prior knowledge that cascaded operations exist, and call openOperation(). I think it makes sense to support compounding without prior knowledge.

Refactoring is a special case because it's invertible. Imagine if you were deleting a java class, and the triggered operation was that the plugin.xml references to that class were removed. Restoring the resource will not bring back the references.
Comment 4 Susan McCourt CLA 2006-02-07 20:16:01 EST
Good example of the true composite vs. triggered op.  I'm working on resource-level undo right now (undo add/delete/edit markers, resources, etc.).  So that will surface a need for a true composite and if so, I'll add one.

As for having another (explicit) chaining mechanism, I'm going to hold off on introducing anything different until there's a specific use case.  Anything API I add for the purpose of theory is sure to get it wrong somehow.
Comment 5 Susan McCourt CLA 2006-02-12 20:35:29 EST
Removing a milestone tag on this one.
Workspace-level undo code in 3.2 is likely to remain internal, and any attempt to deal with side effects/chaining will be done internally.  When it's more mature we can look at which parts should become API, but there's nothing worth rushing into the API at this point.  So far, the interfaces in the undo framework seem sufficient.
Comment 6 Susan McCourt CLA 2006-07-07 18:56:05 EDT
marking 3.3 so we can look at this issue while implementing resource undo
Comment 7 Susan McCourt CLA 2006-09-12 17:13:43 EDT
investigate this approach during M3.
- the API to find out if an operation is executing is needed in order to provide efficient validation of a context.  See bug #87494.
- the composition/chaining issue should also be investigated.  It is proving useful for things like restoring markers or properties when restoring a resource.  Not sure if this will become API in a general sense or just a chaining mechanism used for resource operations, but as Randy states in comment #0, you need to know if something is executing and possibly which operation it is.
Comment 8 Susan McCourt CLA 2006-10-12 14:24:42 EDT
Removing a milestone for now.
Workspace undo is able to deal with these issues for now without adding API.  There may still be a case for this in the future, but we need more experience.

- The new WorkspaceUndoMonitor (used in bug #87494) is using the existing listener support to keep track of whether an operation is executing, undoing, or redoing.  This seems like a reasonable way to handle it for now.  There may be a need to make this public in some class at a later time, as it's wasteful for everyone to have to hook these listeners to get the same info.

- Chaining may still come into play here, but a generic solution is dangerous, since different composites behave in different ways.  A third party observer could respond to some event (resource change event, operation history event, etc.) by getting the currently executing operation and adding to it if it is a composite.  I think our approach in the short term will be including more specific support - such as the client checking whether it's a specific type of op they know about, such as a workspace operation, and if so, chaining themselves to it.

Basically I agree the proposed ideas are good ideas, but am not making this API as part of the undoable workspace operation support.  I'd like some time for the workspace undo support to mature before we open up some of the solutions as API.
Comment 9 Susan McCourt CLA 2009-07-09 16:55:46 EDT
As per http://wiki.eclipse.org/Platform_UI/Bug_Triage_Change_2009
Comment 10 Eclipse Webmaster CLA 2019-09-06 15:31:01 EDT
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.