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

T/1053 Incorrectly set context.forceWeakRemove for MergeDelta during OT #1054

Merged
merged 5 commits into from
Aug 9, 2017

Conversation

scofalik
Copy link
Contributor

@scofalik scofalik commented Jul 31, 2017

Suggested merge commit message (convention)

Fixed: Fixed bug when additional list item has been created when undoing setting block quote on list followed by splitting list item in that list. Closes ckeditor/ckeditor5#4132.


Additional information

  1. When working on this issue I discovered another one. RemoveOperation x MoveOperation was changing context.isStrong value directly, which affected further transformations. Now context.isStrong is assigned to a different variable and that variable is changed if needed.

  2. Tests for this issue will be written in ckeditor5-undo.

@scofalik
Copy link
Contributor Author

Tests: ckeditor/ckeditor5-undo#69

@scofalik scofalik changed the title T/1053 T/1053 Incorrectly set context.forceWeakRemove for MergeDelta during OT Jul 31, 2017
@Reinmar
Copy link
Member

Reinmar commented Aug 4, 2017

Tests for this change need to also be in this package. There can be additional integration tests in ckeditor5-undo but we need some basic verification here. Otherwise, someone (most likely you) will complain that he changed something in ckeditor5-engine, tests passed, but they failed in ckeditor5-undo ;).

@scofalik
Copy link
Contributor Author

scofalik commented Aug 8, 2017

I will write the test but you have to be aware that these kinds of tests are difficult to write in "non-integrational" way.

It's even hard to name a test case because these are all "private" switches and flags of closed and complicated process.

@Reinmar
Copy link
Member

Reinmar commented Aug 8, 2017

But I didn't say that the test must be non-integrational. I just want it to be here. And there's a whole range of approaches and API levels between tests in the undo package and unit tests (which we don't want). In other words – I'd like to see as high-level test as you could implement in here.

@scofalik
Copy link
Contributor Author

scofalik commented Aug 8, 2017

I know I know :). I just wanted to explain why I haven't written any test in the first place.

@scofalik
Copy link
Contributor Author

scofalik commented Aug 9, 2017

I've added additional test (and also found another conceptual problem - with how MoveOperations are broken into multiple operations - and fixed it).

@Reinmar Reinmar merged commit a6c6167 into master Aug 9, 2017
@Reinmar Reinmar deleted the t/1053 branch August 9, 2017 12:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OT: incorrectly set context.forceWeakRemove for MergeDelta
2 participants