Postby karx11erx » Mon May 04, 2015 10:22 am
The main problem is that for convenience undoManager.Begin often is called at the beginning of a function that might change mine data, and if an error occurs, either Unroll or End are called although no game data has been changed. Unroll will erroneously restore a previous mine data state, while End will leave an empty state buffer. The undo manager should have a Cancel function, simply allowing to discard all backups made "just in case".
In the concrete case of AddSegment, the underlying Create function is preceded by undoManager.Begin. If Create returns an error, then in only has done some checks and hasn't altered mine data at all. In that case, the backup that has been created in AddSegment for the case it would change mine data can (and should) be discarded. There are more cases than that.
Unroll has the prolem that the undoManager accumulates changes via its Begin - End mechanics, which can be nested. The undoManager will only create a new mine data backup when Begin is called at nesting level zero. So if an error occurs deep in some nested undo Begins, unrolling will revert all the way back through all changes made since the Begin call at level zero.
However, I think that shouldn't mess up the mine data. Only if a Begin hasn't a matching End / Unroll, it should.
I have added CUndoManager::Cancel (char* szFunction, int dataFlags) to allow fixing that particular problem with the undo manager.
