Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Introduce operations for complex actions instead of using deltas to compose them #4196

Closed
scofalik opened this issue Oct 10, 2017 · 3 comments
Assignees
Labels
package:engine status:discussion type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@scofalik
Copy link
Contributor

Originally reported here, I am extracting this to a separate issue as it seems that we will be interested in doing this.

As time passes and more issues pop out, I started to have a feeling, that deltas were not a good idea. They are much harder to manage than we originally thought. They gave us good, fast start and hopes that we can easily solve some issues (like add row x add column conflict). But I think it's time to think whether we want them.

By deltas I mean the idea that we have an entity that is composed of operations. When I write about "removing deltas" I mean removing the concept. Instead, we would have to introduce all missing operations, so we would have MergeOperation, SplitOperation, etc., which are now handled by MergeDelta or SplitDelta.

So back to deltas. They seemed a good idea. Unfortunately, they have very real drawbacks:

  • algorithms around transformation are much more complicated (I don't mean the transformation algorithms themselves but all the loops and hoops that lead to firing "transform opA by opB"),
  • transformation algorithms for deltas are more difficult and grasp than we initially thought, so they are very abstract and you need a lot of experience and imagination when you want to fix/change anything in them,
  • transformation bugs are more difficult to track because we have multiple layers of transformations and debugging also takes longer simply because there are more "clicks" to make and more steps to check,
  • special cases for delta transformations have to account that deltas may be in weird states,
  • some special cases are very difficult / impossible to solve when deltas are composed from operations without having their own logic.

Of course, we need to remember, that:

  • before we decide for such radical refactor, we need to test whether operations will fit all the goals we have, for example -- hypothetical SplitOperation may be need to transformed to MoveOperation -- we lose some information and it might lead to some "impossible to solve scenarios" too,
  • if we decide for such refactoring, it will take probably 2-3 weeks to make initial re-write and then probably like two months to test and stabilize everything,
  • we will need to write transformations for all new operations pairs (note, that deltas transformations already got very complicated too, and I hope that transformations for singular operations will be simpler).

All in all, I think that if operations will be enough to reach our goals, it would be a HUGE relief to get rid of deltas. It will have highly positive impact to the project's complexity and future maintaining.

@Reinmar
Copy link
Member

Reinmar commented Oct 10, 2017

if we decide for such refactoring, it will take probably 2-3 weeks to make initial re-write and then probably like two months to test and stabilize everything,

Wonderful :D

before we decide for such radical refactor, we need to test whether operations will fit all the goals we have

I think that we should not even think about reducing the number of types of changes at this point. If we have 10 deltas we need to have 10 operations after this change. So for all deltas which don't have operations yet, we need to create new operations.

This, in fact, may make it easier to quickly roll out an initial version of this refactoring. First, we'd get rid of deltas initially and move their transformations to operations. Then, we might think on how to improve this internally. Doing this step by step may be a lot easier.

@scofalik
Copy link
Contributor Author

+1 for doing this step by step. First introduce MergeOperation and the like, then remove deltas transformations and move them to operations transformations. If that's stable, remove the whole infrastructure that are handling deltas. This is the biggest step.

@scofalik scofalik changed the title Drop the deltas idea Introduce operations for complex actions instead of using deltas to compose them Oct 10, 2017
@scofalik
Copy link
Contributor Author

scofalik commented Oct 10, 2017

I have an example where we cannot really introduce special case for delta transformations, even though theoretically the example is easy.

https://github.com/ckeditor/ckeditor5-engine/issues/1161

Here I propose that maybe we should introduce MergeDelta x RemoveDelta transformation. Case is easy: <p>foo</p><p></p><p>bar</p>. Consider merging bar to the empty paragraph, but empty paragraph is in meantime removed. Without special case, bar is merged to empty paragraph and removed with it. That's very bad UX as user expects that bar will be merged to foo in that case.

So, the transformation case is simple. If left-side merge element was removed, merge to its previous sibling instead.

Easy? Easy. Undoable.

Not only I would have to transform two pairs of operations by hand which could lead to issues.

The problem is that because MergeDelta is composed of MoveOperation and RemoveOperation, there must be a targetPosition in MoveOperation. It has to target after the last children of left-side merge element. Unfortunately, we can't know what is that offset during transformation, so we can't script this special behavior.

If we had MergeOperation that has all the logic inside and is atomic, all I would have to write is:

mergeOperation.position.getShiftedBy( -removeOperation.howMany );

Since "targetPosition" would be resolved on execution, I do not have to worry about it during transformation.

@pjasiun pjasiun assigned pjasiun and scofalik and unassigned pjasiun May 8, 2018
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 20 milestone Oct 9, 2019
@mlewand mlewand added module:model status:discussion type:improvement This issue reports a possible enhancement of an existing feature. package:engine labels Oct 9, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
package:engine status:discussion type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

4 participants