Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 370606

Summary:	Problems with UndoStack and deletions at the beginning of the document
Product:	[ECD] Orion	Reporter:	Mihai Sucan <mihai.sucan>
Component:	Editor	Assignee:	Mihai Sucan <mihai.sucan>
Status:	RESOLVED FIXED	QA Contact:
Severity:	normal
Priority:	P2	CC:	eclipse.felipe, mihai.sucan, Silenio_Quarti
Version:	unspecified
Target Milestone:	0.4 RC1
Hardware:	All
OS:	All
Whiteboard:

Description Mihai Sucan

2012-02-03 15:23:26 EST

UndoStack fails to properly commit deletion changes when the happen at offset 0.

STR:

1. Open Orion with some code and go to line 0, offset 0.
2. Delete several chars.
3. Press Ctrl-Z then Ctrl-Y.

By now you can see that undo/redo fail to work.

Problem: UndoStack tracks changes by storing a negative _undoStart for deletion and these changes are committed together in a single change. However, when _undoStart is 0 ... the commitUndo function fails to record the removal.

Fix: track the undo type separately.

Will submit a proposed patch.

Comment 1 Mihai Sucan

2012-02-03 15:36:48 EST

Proposed patch:

https://github.com/mihaisucan/orion.client/tree/bug-370606

Please review and let me know if this fix can land. Thank you!

Comment 2 Mihai Sucan

2012-02-03 15:38:12 EST

Please note that I also made a change to ignore empty changes. For example if the user presses Backspace at the beginning of the document or delete at the end, they are stored as individual empty changes that are confusing when one tries to undo/redo across multiple changes in the document.

Comment 3 Silenio Quarti

2012-02-06 11:54:15 EST

(In reply to comment #2)
> Please note that I also made a change to ignore empty changes. For example if
> the user presses Backspace at the beginning of the document or delete at the
> end, they are stored as individual empty changes that are confusing when one
> tries to undo/redo across multiple changes in the document.

This looks like a bug in the text model. We should not be getting Chaging/Changed events if there is no change in the document.

Comment 4 Silenio Quarti

2012-02-06 12:29:59 EST

The other changes are fine to be committed, but I believe we should fix textModel.js for invalid/empty Changing/Changed events.  We can just add this line to setText() after the undefined checks. 


			if (start === end && text === "") { return; }

Comment 5 Mihai Sucan

2012-02-06 12:34:33 EST

Thank you Silenio!

Shall I make the change you suggested in TextModel setText()? If yes, shall I land the updated patch?

Comment 6 Silenio Quarti

2012-02-06 12:44:57 EST

I fixed the textModel problem:

http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=cec71bddaf32251c34d3728df5da13c130d14f33

Mihai, please go ahead and land your updated changes.  Thanks.

Comment 7 Mihai Sucan

2012-02-06 16:19:27 EST

Thank you Silenio!

I have updated the patch and landed:

http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=3ce24b94f1d8103b16b9cf16f2f50a6302d43b18

Comment 8 Silenio Quarti

2012-02-06 17:04:37 EST

Mihai, I just found a bug with the new code:

1) Type a letter "a"
2) Type backspace to remove letter "a"
3) Type Ctrl+z to undo change

Note that "aa" gets inserted. It should be only "a".

Comment 9 Silenio Quarti

2012-02-06 17:28:14 EST

(In reply to comment #8)
> Mihai, I just found a bug with the new code:
> 
> 1) Type a letter "a"
> 2) Type backspace to remove letter "a"
> 3) Type Ctrl+z to undo change
> 
> Note that "aa" gets inserted. It should be only "a".

I committed a fix for this problem. I will leave the bug open because it needs more testing.

http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=27177e9a3dc70c20b4877e3eab3adfff1d56e342

Comment 10 Mihai Sucan

2012-02-07 08:25:53 EST

(In reply to comment #9)
> (In reply to comment #8)
> > Mihai, I just found a bug with the new code:
> > 
> > 1) Type a letter "a"
> > 2) Type backspace to remove letter "a"
> > 3) Type Ctrl+z to undo change
> > 
> > Note that "aa" gets inserted. It should be only "a".
> 
> I committed a fix for this problem. I will leave the bug open because it needs
> more testing.
> 
> http://git.eclipse.org/c/orion/org.eclipse.orion.client.git/commit/?id=27177e9a3dc70c20b4877e3eab3adfff1d56e342

Thanks for your fix! Tested and it seems to work fine for me.

Comment 11 Silenio Quarti

2012-02-07 14:10:51 EST

Closing