#575 TCoolEdit: Undo is not working correctly

[Ognyan Chernokozhev]

The Undo command in TCoolEdit, and thus in OWLMaker is not working correctly. Instead of restoring a deleted character, it restores the character that follows it.

Steps to reproduce:

1. Create a new text document in either the CoolDemo example or OWLMaker
2. Type the word 'Test'
3. Go to the 's' character and delete it with Backspace
4. Press Ctrl+Z to undo the deletion
5. Instead of 's', the character 't' is inserted, so the text becomes 'Tett'

[Ognyan Chernokozhev]

The bug may be in TCoolEdit::DeleteCharBack, in the line

buffer.AddUndoNode(new TUndoDeleteCharBack(buffer, curPos.row, curPos.col, move(undoSel)));

This adds the character after the character that was deleted. Shouldn't it be curPos.col - 1 here?

Alternatively, the conde in TUndoDeleteCharBack can be fixed to take the previous character, as it is called in two places.

[Ognyan Chernokozhev]

Checking the various code paths, the behavior is even worse if you try to undo the deletion of the CRLF between two lines, as it crashes the program.

Steps to repro:

1. Create a new text document in either the CoolDemo example or OWLMaker
2. Type the word 'Test', then press Enter and type the same word again
3. Go to the start of the second line and press Backspace to delete the CRLF and join the twow lines, so that the text becomes TestTest
4. Press Ctrl+Z to unde the deletion
5. If the program is built in Release mode, it simply closes. If it is built in Debug mode, it shows several Precondition and Assert messages, the last one being "vector subscript out of range", afterwards it fails with an unhandled exception.

[Ognyan Chernokozhev]

Pushed a possible fix in [r6993] that seems to handle both undo cases - backspacing a charater in the middle of a line, and backspacing at the beginning of a line that joins it ti the previous line.

[Vidar Hasfjord]

Another Undo issue is that it doesn't work in overwrite mode:

1. Open a new file.
2. Type "ABC".
3. Press Insert on the keyboard to enter overwrite mode.
4. Move the cursor over B and press space.
5. You now have "A C" with the cursor over C.
6. Press Ctrl+Z to undo.
7. The cursor moves back between A and C, but B is not restored.

Edit: This issue has now been fixed [r8136]. The fix has been merged into branches/7-coolprj-dev [r8137] (records svn:mergeinfo only; the bug was coincidentally fixed in [r7890] as part of undo/redo overhaul on this development branch).

[Erwin Lotter]

This issue is fixed in branches/7-coolprj-dev .

[Vidar Hasfjord]

@elotter wrote:

This issue is fixed in branches/7-coolprj-dev .

Which revision?

[Erwin Lotter]

Hi @vattila,

these are the revisions related to the Undo/Redo implementation until I reported it:

[r7890]
CHG: Added a TUndoNode* to TUndoNode for general undoing of selection changes.
BUG: 'UndoSel' objects given to some TUndoNode derived classes are never deleted.
CHG: Typedef'd TRedoNode as TUndoNode to make TUndoNode usable as TRedoNode.
NEW: Added Redo() member to TUndoNode.
NEW: Implemented redo capability to TUndoInsertChar.

[r7891]
NEW: TUndoInsertChar accumulates subsequently entered keys, reducing the amout of TUndoInsertChar objects needed.

[r7892]
NEW: Implemented redo capability for TUndoDeleteSelection.

[r7893]
BUG: TUndoDeleteSelection.Redo() missed to supply new position.
NEW: TUndoPaste::Redo().
CHG: Moved the copying of selections to TCoolFileBuffer for reuse in TUndoPaste::Redo().

[r7894]
NEW: Redo() for TUndoKeyTab, TUndoKeyEnter, TUndoReplaceText classes.

[r7895]
NEW: TUndoKeyTabify::Redo().

[r7898]
BUG: Control-Delete didn't allow undoing selection deletion.

[r7902]
NEW: TUndoDeleteChar::Redo(), TUndoDeleteCharBack::Redo(), TUndoDeleteWord::Redo(), TUndoDeleteWordBack::Redo().

[r7903]
CHG: TUndoClearAll.Undo() restores selection.
NEW: TUndoClearAll.Redo().

[r7904]
CHG: TUndoDragDropMoveExt replaced by TUndoDeleteSelection.

[r7905]
CHG: TUndoDragDropCopy replaced by TUndoPaste, improved undoing of unselecting.
CHG: TUndoDragDropMove, TUndoDragDropCopy replaced by TUndoPaste.

[r7906]
CHG: TUndoNode::GetDescription(), TUndoNode::GetRedoDescription() create the descriptions now.
CHG: Removed the TRedoXXX classes.
CHG: Added missing Undo/Redo description strings.

[r7908]
BUG: TUndoPaste::Undo() did not clear the selection introduced by dropping external text.

---

Later there have been the following fixes and improvements:

[r7909]
NEW: One more TUndo class to fine-tune the selectioning on drag&drop text insertion.

[r7920]
BUG: TUndoDeleteSelection::Redo() missed to restore the selection before deleting it.

[r7930]
CHG: Optimized Undo/Redo description generation.

[r7932]
BUG: Undoing the deletion of a column selection in TUndoDeleteSelection::Undo() placed the restored selection incorrectly, when the selection was made from left to right.

[r7933]
CHG: Improved reconstruction of cursor position in TUndoDeleteLine.

[r7934]
CHG: Tab moves can't be undone any longer.

[r7938]
NEW: Added Undo/Redo to TCoolEdit::Untabify().

[r7950]
BUG: TUndoDeleteSelection::Redo() set the cursor position incorrect.

[r7965]
BUG: TUndoDeleteLine.Undo() did not restore the last character of a deleted line.

[r8029]
CHG: Postponed selection-redo in TUndoKeyUnTabSelection, TUndoKeyTabify and TUndoMoveLines until the core rodo is done to avoid debug exeptions due to selections beyond EOL.

[r8031]
NEW: Added TCoolEdit::Undo() and Redo().
CHG: Renamed
TUndoKeyTabify -> TUndoIndentSelection,
TUndoKeyUnTabSelection -> TUndoUnindentSelection.

[r8032]
CHG: Postponed selection-redo in TUndoPaste until the core rodo is done to avoid debug exeptions due to selections beyond EOL.

[r8042]
CHG: Made TUndoInsertChar handling inserted and overtyped characters in the same instance.
NEW: Nested TUndoInsertChar for multiple lines or carets.

[r8069]
CHG: TUndoInsertChar revised and simplified.

[Vidar Hasfjord]

@elotter wrote:

these are the revisions related to the Undo/Redo implementation until I reported it

Super!

However, I have a hard time identifying fixes to issues that still apply to the trunk. In particular, the failure to undo inserted characters in overwrite mode (mentioned by me earlier in this ticket comment thread [#1df3]) is still an issue on the trunk, but has been fixed on the development branch. Can you guide me towards the revisions/code to look at?

Edit: After some investigation, I found that the bug was coincidentally fixed in [r7890] as part of undo/redo overhaul.

Same goes for any other issues with undo not working correctly on the trunk. The only other issue I can recall (apart from the unfinished column selection mode), is that undo doesn't restore the selection properly in all circumstances. It would be nice to make a minimal fix for this as well on the trunk, without rewriting the code, if possible.

[Erwin Lotter]

After some investigation, I found that the bug was coincidentally fixed in [r7890] as part of undo/redo overhaul.

Ah, sorry, I didn't realize that your question was specifically about this point.

[Vidar Hasfjord]

@elotter wrote:

Ah, sorry, I didn't realize that your question was specifically about this point.

No worries — its easy for key changes to get buried in big overhauls like this. It is tedious and hard to meticulously follow the Coding Standards and consistently link everything (revision to ticket, and from ticket to all relevant revisions). Remember that the revision log can be edited to fix typos, improve documentation and hook up everything (e.g. use TortoiseSVN's log view to edit). Considering you are unfamiliar with the tools and procedures, you are doing pretty well!

Now, regarding my earlier comment ("undo doesn't restore the selection properly in all circumstances [, and it] would be nice to make a minimal fix for this as well on the trunk"). Can it be done? Or does it depend on your comprehensive rewrite of the Undo functionality?

PS. And it is "trunk" (as in "trunk of a tree"), not "trunc" (as in "truncation"). :-)

[Erwin Lotter]

Remember that the revision log can be edited to fix typos, ...

Good to know. Although I use (Tortoise)Git for more than 20 years now as a backup tool, I never really needed the revision management, which seems to be much more powerful than I thought.

Can it be done? Or does it depend on your comprehensive rewrite of the Undo functionality?

At first glance it looks easy to do. The base class TUndoNode should be untouched there.

[Vidar Hasfjord]

With [r8229] and [r8350], the Undo and Redo functionality on the trunk has been completely redesigned and reimplemented. See [feature-requests:#266], and please review and test!