Undo/Redo system porting from v2

[Garry Yao]

Undo/Redo system need to be ported from v2, the features could be summed up as below:

• Undo system restore/retrieve document status from both selection and content
• All commands that modify the document could support undo/redo feature,but each command has chance to optionaly declare whether would hooked with undo systemwhen they're registed.

Note: Since from v3 all keystroke will pre-bind to command, so keystrokes also hooked with undo system through command interface.

• Undo feature itself performed as a command, A button plugin is required for interfacing this command to end user and keystroke pair of 'Ctrl-Z' and 'Ctrl-Y' too.
• Support empty/reset all the undo snapshots for clean up, and A button plugin is required for interfacing this reset
• snapshot for undo system could be recorded in two forms:
  1. One snapshot for each record action, this apply for almost every functional commands, but a few special keystroke commands that also being recorded in this way including:
     1. 'Enter'
     2. 'ShiftEnter'
     3. 'CtrlBackspace'
     4. 'Delete'
     5. 'Tab'
     6. 'Ctrl-X'
     7. 'Ctrl-V'
  2. One snapshot for a serials of record actions, this apply for most keystroke commands.

Test cases ported from #915 :

• 1. Select text, change style, undo
    Expected results: 
    - text style restored, 
    - selection restored.
  
  2a. Type one ANSI charater in the editor after it's loaded
    Expected results: 
    - undo button is enabled, 
    - undo can be performed by clicking the toolbar button
    - undo can be performed by Ctrl-Z
  2b. Undo once
    Expected results: 
    - redo button is enabled,
    - redo can be performed by clicking the toolbar button 
    - redo can be performed by Ctrl-Y
    - previous contents restored
    - previous caret position restored
  2c. Redo once
    Expected results:
    - undo button is enabled, 
    - undo can be performed by clicking the toolbar button
    - undo can be performed by Ctrl-Z
    - changed contents restored
    - changed caret position restored
  
  3. Type more than 25 ANSI characters in the editor
    Expected results:
    - One undo level should be generated for every 25 characters typed
  
  4. Pressing Ctrl-Z repeatedly after the undo button greyed out
    Expected results:
    - Nothing visible happens
    - Undo/redo system should still work normally afterwards
  
  5. Pressing Ctrl-Y repeatedly after the redo button greyed out
    Expected results:
    - Nothing visible happens
    - Undo/redo system should still work normally afterwards
  
  6a. Select some text, Ctrl-X, repeat N times
    Expected results:
    - The selected text blocks are cut
  6b. Undo once
    Expected results:
    - The last cut operation was restored
    - The selection or caret position was restored
  6c. Undo N-1 times
    Expected results:
    - The cut operations were restored in a step-by-step manner
    - The selections or caret positions in each step were restored
  
  7. Repeat 6a-6c with Del and Ctrl-V
  
  8a. Toolbar commands that change the document
    Procedure:
    - Select a toolbar command that changes the document (e.g. insert smiley)
    - Carry out the neccessary steps to make the change effective (e.g. choose smiley, ok)
    Expected results:
    - One undo step is generated after the change
    - Ctrl-Z or clicking "Undo" undoes the change
    - Ctrl-Y or clicking "Redo" after the previous point redoes the change
  8b. Repeat the "procedure" part of 8a to repeat the same change to the document N times
    Expected results:
    - N undo steps are generated after the changes
    - Ctrl-Z/Undo button and Ctrl-Y/Redo button would undo/redo the changes in a stepwise manner
  9a. Pressing Ctrl-Z breaks the undo/redo system
  

[Garry Yao]

Major features completed first for incoming beta release, uncompleted features include:

1. Undo/Redo command UI
2. Reset Undo/Redo feature
3. Serials command recording

[Garry Yao]

Name convention revised

[Garry Yao]

code style revised

[Frederico Caldeira Knabben]

The patches are a bit confusing. 2763.2.patch has nothing to do with the ticket, or just partially. In the other hand, 2763.patch contains the code related to the ticket, but I could identify several things that needs to be discussed. Other than that, the coding style is not confirming to our standards.

Anyway, let's discuss about it online Garry. We may code this feature together.

[Garry Yao]

I hope this time make it better...

[Frederico Caldeira Knabben]

Garry, can you provide a new patch over the trunk this time?

Also, I've noted the new "serializable" feature for the bookmarks. While the intention is good, it's not a good thing for us in this case. The undo system executes several times during the document lifetime. Using the current bookmarks, you are constantly adding nodes to the DOM.

The above is not necessary as a undo snapshot is "stable". It contains a DOM tree and the selection position can safely be reconstructed by using the node counting system used in V2 (CreateBookmark2). So, I would do a step back an use that non intrusive bookmark system again.

I'll go deeper on the review as soon as I'm able to patch the trunk with this.

[Garry Yao]

Revise to apply to trunk

[Garry Yao]

Replying to fredck:

Also, I've noted the new "serializable" feature for the bookmarks. While the intention is good, it's not a good thing for us in this case. The undo system executes several times during the document lifetime. Using the current bookmarks, you are constantly adding nodes to the DOM.

I don't quite understand the last point since the bookmark nodes will also be removed just like ordinary bookmark in CKEDITOR.dom.range::moveToBookmark.

The above is not necessary as a undo snapshot is "stable". It contains a DOM tree and the selection position can safely be reconstructed by using the node counting system used in V2 (CreateBookmark2). So, I would do a step back an use that non intrusive bookmark system again.

My original consideration on the unobtrusive bookmark (CreateBookmark2) is that it might be weaker than current one from PF.

[Garry Yao]

New addings in this patch:

*Align with trunk *Adding toolbar UI
*Adding CKEDITOR.editor::getMode

Lacks in this patch:

*Now the redo/undo doesn't hook into all dialog based commands, it's related to #2817. *Due to lack of important keystrokes like Backspace and Delete, it doesn't work in such case.

[Frederico Caldeira Knabben]

Replying to garry.yao:

I don't quite understand the last point since the bookmark nodes will also be removed just like ordinary bookmark in CKEDITOR.dom.range::moveToBookmark.

Be sure there is a lot of thinking behind everything I've said. I just avoided wasting time explaining the whys, but here it is: it's a matter of performance and simplification.

In your current implementation you are doing the following operations to get an undo snapshot:

1. Save contents snapshot.
2. Create the bookmarks (includes DOM nodes).
3. Save the contents snapshot again, this time with the DOM nodes.
4. Select the bookmarks again, restoring the current selection.

DOM operations are notably slow, and the above operations are totally DOM based.

Due to your choice, you also need to have two content snapshot saved per image (steps 1 and 3), which is an unnecessary duplication.

And them, you are forced to do selection manipulation to remove the bookmark nodes (step 4), which is also unneeded as you already have the selection before saving.

Other than that, you get a lot of DOM fragmentation, breaking text nodes to add bookmark nodes. This also impacts in the overall performance of the editor operations.

The solution I'm proposing is to not touch the DOM or the selection. Grabbing an image means the following in this case:

1. Save the contents snapshot.
2. Save the bookmarks.

My original consideration on the unobtrusive bookmark (CreateBookmark2) is that it might be weaker than current one from PF.

You should give more details regarding your consideration. Based on my explanation, I can just see the opposite.

Lacks in this patch

The scope of this ticket is introducing the Undo System, which makes it possible to Undo/Redo. We don't need to make things Undoable/Readoable here, so these lacks are not a big issue (for this ticket).

---

Other than the above, your coding style is not yet matching our standards. There are so many things to fix in this sense that it's easier to just go ahead fixing instead of explaining them here. (You must adapt yourself to the project, not the opposite).

This is a critical feature, so to avoid wasting time, I'll be adding a new patch here, based on your patch. I would ask you to please compare the two patches, identifying the differences and coming with your comments if you think something is wrong.

[Frederico Caldeira Knabben]

This new patch is based on Garry's work, on the very base at least. It also fixes some issues we are used to have with the current V2 system.

Note that our intention here is introducing the Undo System, not making all current operations in the editor undoable. For that, we'll be opening further tickets.

[Frederico Caldeira Knabben]

Ops, forgot to mention. This patch depends on #2909, which patch must be applied first.

[Frederico Caldeira Knabben]

New patch... forgot to add the new files to it.

[Martin Kou]

The proposed patch does not work for multi-step undo/redos.

e.g.

1. Open replacebyclass.html in Firefox 3.
2. Perform a few align operations on the first paragraph - center, right, justify, right, center, left, etc.
3. Press undo.
4. The system skips all the intermediate steps and goes back to the first snapshot.

Another fault pattern would be the undo system is unable to undo beyond a certain undo snapshot, e.g.

1. Open replacebyclass.html in Firefox 3.
2. Justify center.
3. Type some random text into the paragraph to ensure Image::equals() returns false.
4. Repeat steps 2, 3 for a few times with other justify commands.
5. Try to undo a few times.
6. The undo system is stuck in one of the snapshots and is unable to go back to the first snapshot.

The bugs seem to have to do with the type checking logic in the UndoManager.

[Garry Yao]

Fred, the updated patch should has better performance due to the new bookmark system you adapted, and I also see the code quality improvement, it's a good example for me. Here, I just provide some of my opinions toward the logic details when performing the undo/redo, since I found in your patch many changes toward this part, some of which were those I introduced in my old patch to slightly distinguish the behaviors from v2 codes:

1. Either undo or redo only take care document modification along with selection changes, which mean stand-alone selection changes should not be recorded.
2. Undo record the snapshot right before a command, while redo record the snapshot right after it, both of themselves need not participant in snapshots.
3. Due-stacks mechanism was introduce for easy accessing snapshots according to the same history index, though it could be combined, but is mistakable and complex during the navigation.

[Garry Yao]

Provide one test case for undo doesn't restore selection changes.

• Procedures:
  1. Open replacebyclass.html;
  2. Bold some text;
  3. Perform undo
• Expected results:
  • Bolded text restore along with selection, undo state if off now.
• Actual results:
  • Undo state if on, if perform undo again at this moment, selection was restore to the beginning of text.

[Frederico Caldeira Knabben]

This new patch simplified, in some aspects, the system. The basic idea is that we are able to undo/redo only if we have content differences between the snapshots.

I've also fixed a bug in the event system for DOM objects, as well as introduced an automatism in the command execution, which will not execute if the command is disabled.

All cases reported in previous comments now pass properly.

[Frederico Caldeira Knabben]

Fixed with [3086]. Click here for more info about our SVN system.