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

Fixed: Empty element should be merged instead of removed in deleteContents #1163

Merged
merged 2 commits into from
Oct 11, 2017

Conversation

scofalik
Copy link
Contributor

@scofalik scofalik commented Oct 11, 2017

Suggested merge commit message (convention)

Fix: deleteContent algorithm will use merging to "remove" empty element which will ensure a better conflict resolution on collaborative editing. Closes ckeditor/ckeditor5#4195.

@Reinmar
Copy link
Member

Reinmar commented Oct 11, 2017

Eeeem... as you wrote this is just a workaround and it's a workaround for a collab scenario where three blocks are merged simultaneously by two users. Additionally, this scenario leads to an incorrect data, not to a crash.

Taken that, I'm against doing this change. Avoiding using the remove op may help in the meantime but will need to be reverted later on. Unless, of course, you think that such algorithms should use merge instead of remove?

@scofalik
Copy link
Contributor Author

scofalik commented Oct 11, 2017

I proposed this PR anyway because this issue is:

  • not difficult to reproduce,
  • gives a very bad result,
  • we won't fix it in transformations soon (requires refactor).

Also, it is not 100% clear that this should be a remove or merge. I wouldn't even say that this is a workaround - just a worse solution. But, both solutions are reasonable. I wanted to fix this in transformations, but this is impossible. So in the end, I wanted to at least fix what can be easily fixed. So we won't be stuck with a bug for a couple of months.

@Reinmar
Copy link
Member

Reinmar commented Oct 11, 2017

OK, then let's change it. I can see that the code actually got simpler and after looking at the scenario again I realised that it makes sense to use merge there.

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.

Using RemoveDelta in deleteContent leads to wrong results in collaboration
2 participants