Undo Feature. #1

scofalik · 2016-03-18T15:12:47Z

Undo Feature. Fixes ckeditor/ckeditor5#2683.

pjasiun · 2016-03-30T12:52:39Z

src/undocommand.js

+			}
+		}
+
+		this.fire( 'undo', undoBatch );


Because redo is also an UndoCommand the event should be called reverted. It is strange that redo command fire undo event.

pjasiun · 2016-03-30T13:15:42Z

Beside these small changes mentioned above, two biggest missing parts are: selection handling and UI. If you do not want to handle them as a part of this ticket please create follow-ups. On the other hand selection may change undo so much that there is no point in considering undo without selection.

fredck · 2016-03-30T13:22:04Z

there is no point in considering undo without selection

Agree... selection is in fact one of the essential things related to undo.

scofalik · 2016-05-04T10:55:53Z

I've implemented selection handling. It should work really well / perfect with non-selective undo where batches are not divided by other batches or when mixed delta is not conflicting with deltas from undone batch.

I did expanded implementation of range transformation so I expect that the selection should work okay in selective undo or mixed conflicting deltas too, but just don't expect wonders.

This comment is more suitable for ckeditor5-engine repo because that's where the magic happens, but I wanted to sum up the work here aswell.

Changes that are needed for this branch to work are on ckeditor5-engine branch t/389.

pjasiun · 2016-05-04T13:45:36Z

tests/undocommand.js

+		} );
+	} );
+
+	describe( '_doExecute', () => {


There should not be tests for private class. If you want to test this method you should change it to "protected".

In fact, what should be done is:

_doExecute should be private as it is called by _execute

_execute should be tested.

I'll change it.

…changing to WeakMap.

scofalik · 2016-05-04T14:37:21Z

I added selection direction restoring. I've changed tests so they test protected _execute method instead of private _doExecute. I also removed a test for addBatch method as all it tested was private members.

I hope it is okay to merge now.

pjasiun · 2016-05-04T16:14:15Z

src/undocommand.js

+			batch,
+			{
+				ranges: Array.from( this.editor.document.selection.getRanges() ),
+				isBackward: this.editor.document.selection.isBackward


Still: why now selection.clone()?

I've already written why in one of comments (it got outdated and hidden). Selection uses LiveRanges and would get transformed if we saved Selection. This, and other reasons.

Reinmar · 2016-05-09T11:51:43Z

BTW. which issue is this PR closing?

scofalik · 2016-05-09T12:35:09Z

#3

Reinmar · 2016-05-11T10:58:51Z

src/undo.js

+		} );
+
+		// Whenever batch is reverted by undo command, add it to redo history.
+		this._undoCommand.listenTo( this._redoCommand, 'revert', ( evt, batch ) => {


Shouldn't be the feature listening? In 99% of situation we should be using listenTo() on this, because the class which adds a listener is interested in removing it while it's gone.

You are right. I probably thought that this solution looks cool but it will be better if feature is listening.

Reinmar · 2016-05-11T11:28:36Z

I'm missing some more integrational tests, like:

Use composer to change the doc (perhaps create couple undo steps at once).
Check commands' states.
Undo and see that the data and selection are correct.

Those tests should be written with our test utils, so they are easy to read.

Having some low level tests is fine, but in a long run they will be hard to maintain, read and extend. That's when the easy-to-write and easy-to-read higher level tests are useful. I'd add some now (in a separate test file most likely) to have easier start in the future and easier review now.

scofalik · 2016-05-12T07:47:10Z

I could create some integrational tests using model test utils and see if model is fine but right now composer only has deleteContents so I'd have to create batches by hand anyway, just like in UndoCommand tests. I would just not have to add them to UndoCommand instance. Same with selection, I'd have to manually set it.

I looked at utils/src/diff.js and utils/src/difftochanges.js and those maybe could be used to automatically generate batches. Or, if we would be able to diff to model trees, we could use setData tool.

Reinmar · 2016-05-12T08:49:35Z

Doing changes is one part of a test. Second is setting data and checking the output. Currently you're building a document by creating lots of deltas, then do some operations (or even not – some tests starts with undo._execute()) and then some checks. This is fine (I guess), but it's not something that you can read and extend quickly.

By doing simple:

setData( doc, '<p>foo[bar]</p>' );
doc.composer.deleteContents( doc.batch(), doc.selection ); // You can also use some deltas.
expect( getData( doc ) ).to.equal( '<p>foo[]</p>' );

You will be able to quickly generate more test cases which underneath do quite similar things as you did with the low level API.

E.g. let's imagine that we find a bug in a very specific case in the future. The first thing to do will be to code a test for it exactly like I showed. So we'll be definitely adding those. I'd just like to have some already, so I can verify on couple of cases that the undo performs as expected. I'm not able to do this quickly based on the existing tests.

scofalik · 2016-05-12T14:53:04Z

I've added integration tests. Unfortunately, some of them fail, but not directly because of undo algorithms. I've commented them. I will create followup ticket for them in ckeditor5-engine.

scofalik · 2016-05-12T14:53:16Z

I also fixed other issues you pointed out.

scofalik · 2016-05-13T06:09:54Z

I commented out tests that failed and created followup issue in ckeditor5-engine

Reinmar · 2016-05-13T08:15:10Z

I reported https://github.com/ckeditor/ckeditor5-undo/issues/6 for the ignored tests.

Reinmar · 2016-05-13T08:24:53Z

src/undocommand.js

+		if ( !batch ) {
+			batchIndex = this._batchStack.length - 1;
+		} else {
+			for ( let i = 0; i < this._batchStack.length; i++ ) {


You could've used: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find. I'll fix it.

Reinmar · 2016-05-13T08:30:09Z

http://tests.ckeditor.dev:1030/build/amd/tests/undo/undo#adding%20and%20removing%20content%20multiple%20remove%20and%20undo is failing for me. Could you check?

…ions. Tests: Fixed the way how feature is initialized and made sure to use beforeEach().

scofalik · 2016-05-13T09:56:47Z

It needed a change in ckeidtor5-engine that I had only locally. Already fixed by ckeditor/ckeditor5-engine#414

scofalik force-pushed the undo branch from c055ebf to ccf6af9 Compare March 18, 2016 15:26

pjasiun reviewed Mar 30, 2016
View reviewed changes

pjasiun added the status:review- label Mar 30, 2016

CKSourceBOT removed the status:review- label Mar 30, 2016

scofalik force-pushed the undo branch 2 times, most recently from 3f0d5f2 to 66be332 Compare May 4, 2016 10:36

scofalik added 4 commits May 4, 2016 12:39

Undo Feature initial commit.

3c348a6

Changes according to changes in CKE5-core.

4420f98

Added: undo.UndoCommand selection restoring + docs update.

9eff507

Tests, docs: undo.UndoFeature minor tweaks.

f89097a

scofalik force-pushed the undo branch from 66be332 to f89097a Compare May 4, 2016 10:39

Fixes: UndoFeature/UndoCommand changes Map/Set to WeakMap/WeakSet.

322c80e

pjasiun reviewed May 4, 2016
View reviewed changes

Changed: UndoCommand now restores selection direction. + fixes after …

2eda14d

…changing to WeakMap.

pjasiun reviewed May 4, 2016
View reviewed changes

Changed: removing commented code + docs.

36484d6

Reinmar reviewed May 11, 2016
View reviewed changes

Changed: Fixes after review.

3a5542e

Tests: Commented out tests that fail.

89885e9

Reinmar reviewed May 13, 2016
View reviewed changes

Reinmar force-pushed the undo branch from bd8a9f2 to d03eff6 Compare May 13, 2016 09:45

Renamed _batchStack to more correct _items and made minor simplificat…

bdf3ba9

…ions. Tests: Fixed the way how feature is initialized and made sure to use beforeEach().

Reinmar force-pushed the undo branch from d03eff6 to bdf3ba9 Compare May 13, 2016 09:55

Reinmar added 2 commits May 13, 2016 12:00

Minor API docs corrections.

859f639

Minor improvement.

fdc695a

Reinmar merged commit 27226ef into master May 13, 2016

Reinmar deleted the undo branch May 13, 2016 10:54

Reinmar mentioned this pull request Sep 14, 2016

📅 News and updates: December 7, 2015 – August 11, 2016 ckeditor/ckeditor5-design#159

Open

Reinmar mentioned this pull request Oct 9, 2019

Fix undo cases ignored in #1 ckeditor/ckeditor5#2685

Closed

scofalik mentioned this pull request Oct 9, 2019

Fixes for OT to correct undo. ckeditor/ckeditor5#3710

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Undo Feature. #1

Undo Feature. #1

scofalik commented Mar 18, 2016 •

edited by pomek

Loading

pjasiun Mar 30, 2016

pjasiun commented Mar 30, 2016

fredck commented Mar 30, 2016

scofalik commented May 4, 2016

pjasiun May 4, 2016

scofalik May 4, 2016 •

edited

Loading

scofalik commented May 4, 2016 •

edited

Loading

pjasiun May 4, 2016

scofalik May 4, 2016

Reinmar commented May 9, 2016

scofalik commented May 9, 2016

Reinmar May 11, 2016

scofalik May 12, 2016

Reinmar commented May 11, 2016 •

edited

Loading

scofalik commented May 12, 2016 •

edited

Loading

Reinmar commented May 12, 2016 •

edited

Loading

scofalik commented May 12, 2016

scofalik commented May 12, 2016

scofalik commented May 13, 2016

Reinmar commented May 13, 2016

Reinmar May 13, 2016

Reinmar commented May 13, 2016

scofalik commented May 13, 2016 •

edited

Loading

Undo Feature. #1

Undo Feature. #1

Conversation

scofalik commented Mar 18, 2016 • edited by pomek Loading

pjasiun Mar 30, 2016

Choose a reason for hiding this comment

pjasiun commented Mar 30, 2016

fredck commented Mar 30, 2016

scofalik commented May 4, 2016

pjasiun May 4, 2016

Choose a reason for hiding this comment

scofalik May 4, 2016 • edited Loading

Choose a reason for hiding this comment

scofalik commented May 4, 2016 • edited Loading

pjasiun May 4, 2016

Choose a reason for hiding this comment

scofalik May 4, 2016

Choose a reason for hiding this comment

Reinmar commented May 9, 2016

scofalik commented May 9, 2016

Reinmar May 11, 2016

Choose a reason for hiding this comment

scofalik May 12, 2016

Choose a reason for hiding this comment

Reinmar commented May 11, 2016 • edited Loading

scofalik commented May 12, 2016 • edited Loading

Reinmar commented May 12, 2016 • edited Loading

scofalik commented May 12, 2016

scofalik commented May 12, 2016

scofalik commented May 13, 2016

Reinmar commented May 13, 2016

Reinmar May 13, 2016

Choose a reason for hiding this comment

Reinmar commented May 13, 2016

scofalik commented May 13, 2016 • edited Loading

scofalik commented Mar 18, 2016 •

edited by pomek

Loading

scofalik May 4, 2016 •

edited

Loading

scofalik commented May 4, 2016 •

edited

Loading

Reinmar commented May 11, 2016 •

edited

Loading

scofalik commented May 12, 2016 •

edited

Loading

Reinmar commented May 12, 2016 •

edited

Loading

scofalik commented May 13, 2016 •

edited

Loading