Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Do not register batches which have only non-document operations #80

Merged
merged 3 commits into from
Feb 22, 2018

Conversation

scofalik
Copy link
Contributor

@scofalik scofalik commented Feb 21, 2018

Suggested merge commit message (convention)

Fix: Do not register batches which have only non-document operations. Closes ckeditor/ckeditor5#2719. Closes ckeditor/ckeditor5#781.

@coveralls
Copy link

coveralls commented Feb 21, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling d99b618 on t/79 into 9d1536c on master.

Copy link
Member

@Reinmar Reinmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing integration test.

@@ -65,6 +65,13 @@ export default class UndoEngine extends Plugin {

this.listenTo( this.editor.model, 'applyOperation', ( evt, args ) => {
const operation = args[ 0 ];

// Do not register batch if the operation is not a document operation.
// This prevents from creating empty undo steps, where all operations where non-document operations.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link to the bug is missing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you consider this completely obvious that undo should not record these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I consider everything obvious :). I'll add one more line of comment to make it more obvious.

@Reinmar
Copy link
Member

Reinmar commented Feb 22, 2018

But I confirm that the fix work.

@scofalik
Copy link
Contributor Author

I've completed requested changes.

@Reinmar Reinmar merged commit 60ac1ab into master Feb 22, 2018
@Reinmar Reinmar deleted the t/79 branch February 22, 2018 22:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants