Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Do not use element reference in DifferItemInsert and DifferItemRemove #15800

Closed
scofalik opened this issue Feb 5, 2024 · 1 comment · Fixed by #15804 · May be fixed by Klantinteractie-Servicesysteem/KISS-frontend#929
Closed
Assignees
Labels
package:engine squad:collaboration Issue to be handled by the Collaboration team. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@scofalik
Copy link
Contributor

scofalik commented Feb 5, 2024

📝 Provide a description of the improvement

Recently, we introduced _element property for DifferItemInsert and DifferItemRemove. It was done in #15649.

The differ represents all changes as insertions, deletions and attribute changes. So, the rename operation is represented by two changes: remove + insert. But we had no good way to link such two changes together and figure out that it was actually a rename. We also have "refresh" mechanism, which forces an element to reconvert, by adding remove+insert changes.

And we needed this to solve some problems regarding track changes, to understand if a user renamed an element, or removed+inserted an element, as we want to present such operations in a different way.

To solve that, we introduced _element property, which was a reference to the changed element. Now for each DifferItemInsert entry you can look if there is corresponding DifferItemRemove entry.

The PR was accepted but the solution was not received positively, and I can understand why. We decided to use it anyway, but implement _element as internal (private) property and fix it later. We don't like this solution because we made a hard effort earlier to avoid passing the reference in diff items. Our idea was that diff items should have all the information to handle the change given in simple form. You should not need to use the element reference.

It is important for remove entries -- we don't want people to base on references when handling remove entries, as it will lead to incorrect results. The element reference in remove entry will be outdated, especially when it comes to its position. The deletion position was in the model (the element position before it was removed), while the element's actual position will be in the graveyard root. Also, the element could have been renamed, or change its attributes, and the reference will have the new values, not the old ones. So, long story short, if we can avoid it, we will still make an effort not to pass the element reference in diff items.

Also, it is confusing now, that we have name and attributes next to "full" element reference. Why/when should I use this or that? Is there difference? If no, why we have name and attributes? And so on...

So, instead of _element we need to propose something else. My proposal is:

For DiffItemInsert:

action: 'insert' | 'refresh' | 'rename';
before?: {
	name: string;
	attributes: Map<string, unknown>;
}

Where action is refresh or rename when that was the original "trigger", while insert is the default.

For DiffItemRemove just action, as attributes and name are already in "before" state. Of course action should be 'remove' | 'refresh' | 'rename'.

I am not super happy with before name though, so I am open to suggestions if you have something more fitting.

@scofalik scofalik added type:improvement This issue reports a possible enhancement of an existing feature. package:engine squad:collaboration Issue to be handled by the Collaboration team. labels Feb 5, 2024
@Witoso
Copy link
Member

Witoso commented Feb 5, 2024

I am not super happy with before name though, so I am open to suggestions if you have something more fitting.

Not an exact but similar case, React uses (or used?) prev prefix, like prevProps, maybe prev or previous might sound better?

scofalik added a commit that referenced this issue May 30, 2024
Feature (engine): Introduced `DiffItemInsert#action`, `DiffItemInsert#before` and `DiffItemRemove#action` which give more information about the change that happened in the model. Refer to the API docs to learn more. Closes #15800.
@CKEditorBot CKEditorBot added this to the iteration 75 milestone May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine squad:collaboration Issue to be handled by the Collaboration team. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
4 participants