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

Differ#getChanges() could return items for remove entries #6830

Closed
scofalik opened this issue May 15, 2020 · 5 comments
Closed

Differ#getChanges() could return items for remove entries #6830

scofalik opened this issue May 15, 2020 · 5 comments
Labels
domain:dx This issue reports a developer experience problem or possible improvement. package:engine resolution:expired This issue was closed due to lack of feedback. squad:core Issue to be handled by the Core team. status:stale type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@scofalik
Copy link
Contributor

📝 Provide a description of the improvement

At the moment, Differ#getChanges() for "remove" entries does not return the removed item. Instead position and length are returned.

What is returned now is fine for remove conversion but it feels bad when used in post-fixers. Now, if you want to react to removing particular element, you need to get changes together with changes in graveyard and check if the element was inserted into a graveyard. This is not intuitive at all.

Moreover, there are some difficulties when using the above "workaround". For example, I don't know from which position the element was removed.

@scofalik scofalik added type:improvement This issue reports a possible enhancement of an existing feature. domain:dx This issue reports a developer experience problem or possible improvement. labels May 15, 2020
@Dolov

This comment has been minimized.

@Reinmar Reinmar added this to the nice-to-have milestone Jun 22, 2020
@jodator
Copy link
Contributor

jodator commented Oct 14, 2020

I've spiked this when trying to figure out why table reconversion fails: 77c8115.

Looks easy to implement. It might be also required for table partial reconversion.

The problem in tables touches the way how triggerBy handles events and that there's no way to "consume" remove changes. I need to override the default remove converter (block it in case of table reconversion when a row was removed) because the default removes converter uses a heuristic to detect what view element is to be removed (because it does not have access to removed element and can't check if the view element was already removed).

@jodator
Copy link
Contributor

jodator commented Oct 15, 2020

I've spiked this when trying to figure out why table reconversion fails: 77c8115.

This will not work for the current writer.move() operation.

It could be a new kind of conversion operation to handle but I don't know the impact of such change. So it is hard to tell.

OTOH maybe the remove opearation handler could actually check if the model is in greaveyard or not (remove vs move).

@Reinmar Reinmar added squad:core Issue to be handled by the Core team. and removed squad:dx labels Sep 9, 2021
@pomek pomek removed this from the nice-to-have milestone Feb 21, 2022
@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added the resolution:expired This issue was closed due to lack of feedback. label Nov 12, 2023
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:dx This issue reports a developer experience problem or possible improvement. package:engine resolution:expired This issue was closed due to lack of feedback. squad:core Issue to be handled by the Core team. status:stale type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

6 participants