BUG: Ops added to undo/redo stack twice
Hey, TLF Team...
I'm seeing a bug with what gets added to the undo stack when combining begin/end composite operation with operations that are done in begin/end flow op event handlers during the duration of the begin/end composite op calls. I've filed a few bugs through Flex SDK bug system a while back and they didn't seem to funnel through to you right away, so I thought I'd post this one here. This isn't quite a blocker for us right now, but we'd really like to see this one fixed for release if possible.
Steps to Reproduce:
1. Call EditManager.beginCompositeOperation().
2. Call EditManager.doOperation( OpA ).
3. Call EditManager.doOperation( OpB ).
4. Handle FlowOperationEvent.FLOW_OPERATION_BEGIN event for OpB and within the event handler call EditManager.doOperation( OpB_PreOp ).
5. ...allow OpB to do its thing.
6. Handle FlowOperationEvent.FLOW_OPERATION_END event for OpB and within the event handler call EditManager.doOperation( OpB_PostOp ).
7. Call EditManager.doOperation( OpC ).
8. Call EditManager.endCompositeOperation().
Result:
Observe the CompositeOperation (the stack of ops contained within it) that gets added to the undo stack. It will contain something similar to this:
- CompositeOperation (overall composite op created as result of begin/end composite operation calls)
- OpA
- OpB_PreOp
- OpB_PostOp
- CompositeOperation (created as a result of doOp calls during FlowOp begin/end handlers)
- OpB_PreOp (same obj ref as item 'b' above)
- OpB
- OpB_PostOp (same obj ref as item 'c' above)
- OpB_PreOp (same obj ref as item 'b' above)
- OpC
As you can see, when we do operations during the flow op begin/end handlers AND we are already in a composite operation context, the ops done during begin/end handlers get added to the undo/redo stack in two places.
Also, we compile with 'CONFIG::debug = true' during development and in this case get the assertion error 'adding non-contiguous operation to composite operation' during the finalizeDo call for OpB_PostOp during parentOperation.addOperation(op). The reason for the assertion error is that when we call parentOperation.addOperation(OpB_PostOp), parentOperation's ops array only contains OpB_PreOp, whose endGeneration does not match up with OpB_PostOp's beginGeneration due to model changes done during OpB execution.
Expected Result:
OpB_PreOp and OpB_PostOp should only be added to the undo/redo stack in one place. Since we are already in a composite operation in this case, maybe the composite wrapper created for OpB_PreOp/OpB/OpB_PostOp would not be needed, and OpB could be added in the appropriate location in the ops stack. Otherwise, preventing the pre/post ops from getting added to the outer composite op seems like that would fix it also.
Related Note:
In cases where we do an operation as a result of flow op begin event, if another handler of that event calls preventDefault(), we would prefer that our 'PreOp' not get executed either -- or that it would get immediately undone when edit manager realizes that the original op was prevented. We basically get that behavior by default with post ops, since the flow op end event never fires in that case. Our workaround for now is to use lower priority listeners for the handlers that do pre ops so we can check for defaultPrevented ourselves before we actually do the pre op. We have also seen some issues with the undo/redo stack breaking due to generation number gaps in certain cases where we do pre ops and then prevent default on the original op, but we have not really gotten to the bottom of those issues yet. I realize that not everyone will want to have 'pre' ops prevented if the originating op is prevented. We may even have specific cases where that's true. I don't have any good ideas right now about how that type of thing could be handled, but I just wanted to throw it out there.
Thanks,
Brent
