Bug 374435

Comment 1 Silenio Quarti 2012-04-04 14:17:08 EDT

Do you have steps to reproduce this? I tried the following in demo.js but it did not cause the problem:

	function test() {
		log("test");
		var undoStack = mSetup.undoStack;
		undoStack.startCompoundChange();
		mSetup.view.setText("silenio", 10, 10);
		undoStack.startCompoundChange();
		mSetup.view.setText("quarti", 20, 20);
		undoStack.endCompoundChange();
		undoStack.endCompoundChange();
	}

Hard to say what is the correct behaviour for nested compound changes. After I run the test above there is only one undo operation in the stack as if the middle start/end are noops.

I can see a few options when startCompoundChange() is called inside a compound change:

1) it is a noop
2) it ends the previous compound change
3) it actually adds a new level to the undo stack.

1) and 2) are some what easier to implement
3) is harder because the undo operation is not done in the same order the changes where made. So we would have to update the offsets of every change made after the nested change is ended.

Comment 2 Mihai Sucan 2012-04-04 14:34:28 EDT

Hello Silenio!

Thank you for looking into this!

(In reply to comment #1)
> Do you have steps to reproduce this? I tried the following in demo.js but it
> did not cause the problem:
> 
>     function test() {
>         log("test");
>         var undoStack = mSetup.undoStack;
>         undoStack.startCompoundChange();
>         mSetup.view.setText("silenio", 10, 10);
>         undoStack.startCompoundChange();
>         mSetup.view.setText("quarti", 20, 20);
>         undoStack.endCompoundChange();
>         undoStack.endCompoundChange();
>     }

Breakage happens when you do undo/redo before the second endCompoundChange().


> Hard to say what is the correct behaviour for nested compound changes. After I
> run the test above there is only one undo operation in the stack as if the
> middle start/end are noops.
> 
> I can see a few options when startCompoundChange() is called inside a compound
> change:
> 
> 1) it is a noop
> 2) it ends the previous compound change
> 3) it actually adds a new level to the undo stack.
> 
> 1) and 2) are some what easier to implement
> 3) is harder because the undo operation is not done in the same order the
> changes where made. So we would have to update the offsets of every change made
> after the nested change is ended.

I can see 1) and 2) causing problems further along the road for us - problems as in "unexpected behavior". Ideally we should have 3) which nests compound changes. At this point the breakage (exceptions) we have is probably the most important thing to fix.

For 3) you could merge the inner-most compound changes when endCompoundChange() happens.

For example:

1. changes...
2. startCompoundChange()
   3.1. changes...
   3.2. startCompoundChange()
        3.3.1. changes...
        ... allow individual undo/redo of changes here, at this level.
        ... probably make it an error to do undo() outside of the current nested compound change.
   3.4. endCompoundChange()
        ... merge the changes in 3.3.1 into the current compound changes as one change - effectively losing all track of changes.
        ... allow undo/redo of changes at this level. What happened between 3.2 and 3.4 is considered one change.
4. endCompoundChange()
   ... same as above (merge changes)

Would this still need updating offsets?

Comment 3 John Arthorne 2015-05-05 14:46:56 EDT

Closing as part of a mass clean up of inactive bugs. Please reopen if this problem still occurs or is relevant to you. For more details see:

https://dev.eclipse.org/mhonarc/lists/orion-dev/msg03444.html