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

Implement rename operation #3675

Closed
2 tasks done
Reinmar opened this issue Apr 19, 2016 · 9 comments
Closed
2 tasks done

Implement rename operation #3675

Reinmar opened this issue Apr 19, 2016 · 9 comments
Assignees
Labels
package:engine type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Apr 19, 2016

Needed for cases like:

  • enter inside a fully selected non-paragraph block (should result in empty paragraph),
  • switching between formats,
  • converting paragraphs to list items.

It could be done in two steps:

  • Implement delta application logic.
  • Add OT support for that delta. Eventually add also the rename operation which will solve issues like:
    • you lose reference to the element being renamed when renaming it (if it's implemented as a delta - insert+move+remove),
    • OT conflicts.

Cons:

  • We're unsure whether this delta makes semantic sense. When changing formats there's a question what to do with attributes. The attributes of the previous element may not make sense on the new element. So is it really semantical renaming if issues like this may appear?

Pros:

  • Such delta will resolve some cases in OT. Especially in the p->li case where many elements need to be renamed at once, doing this with insert+move+remove could create tricky situations.
@pjasiun
Copy link

pjasiun commented Apr 19, 2016

The problem is implementing such delta using existing operations will break the reference to the element. Now we can insert, move children, set attributes and remove, so if user keep the reference to the object he will still have original object, but in graveyard.

We could implement new operation type which simply change the tag name, what seems to be the cleanest solution. But them the change is deeper, beside OT for operations, converters have to handle new change type.

@Reinmar Reinmar self-assigned this Apr 19, 2016
@Reinmar
Copy link
Member Author

Reinmar commented Apr 19, 2016

We could implement new operation type which simply change the tag name, what seems to be the cleanest solution. But them the change is deeper, beside OT for operations, converters have to handle new change type.

Couldn't the dispatcher trigger rename operation as insert+move+remove?

@pjasiun
Copy link

pjasiun commented Apr 19, 2016

Yes, this is the handling I was thinking about.

@Reinmar
Copy link
Member Author

Reinmar commented Apr 19, 2016

OK, another argument for doing this as an operation is live position. If I have a live position inside a an element that I'm renaming it may end up in a graveyard rather than in the new element.

@Reinmar Reinmar changed the title Implement rename delta Implement rename operation Apr 20, 2016
@Reinmar
Copy link
Member Author

Reinmar commented Apr 20, 2016

I'll implement the first part of this ticket in ckeditor/ckeditor5-engine#368.

@Reinmar Reinmar removed their assignment Apr 20, 2016
@scofalik scofalik self-assigned this Aug 11, 2016
@scofalik
Copy link
Contributor

scofalik commented Aug 11, 2016

Renaming appears to be more tricky than I thought. We have to consider few things if go with RenameOperation. Changes in model are easy. It's just element.name = newName;, done. Change has rename type. Problems start during conversion.

We could "close" the case right in ModelConversionDispatcher. For rename change, we could fire convertRemove() and convertInsertion(). This solution is very clean. First of all, we do not introduce any new events for conversion. Second of all, we use already defined callbacks (keep in mind that some of them might be custom so it's good that they got reused and don't have to be tweaked for different kind of event). Third, changing element name may, theoretically, have an impact on how that element is rendered or how attributes for that element work or how it's children are rendered. In this approach, we re-render all the children so it's clean.

The drawback is that if big element gets changed or many element get changed, we re-render a lot of stuff. There is also a problem with any embeded content, like MathJax or anything we use iframes for, because they will be reloaded.

So next idea was to do convertInsertion(), convertMove() and convertRemove() for rename change type. Insertion convertion would be only on the renamed element (not it's children). Then children are moved from "old" element to "new", and then "old" element is removed. However I fear that there might be some problems with model - view mappings, because convertInsertion() will make some mappings, so same instance of model will be mapped to two elements in view.

Another idea was to implement viewWriter.rename() that would take responsibility of renaming given element. Then model-to-view converter for rename change type would be just firing that writer method. However, that method cannot just change element name because that might result in incorrect conversion. We would have to insert new view element and remove old. But then we don't know in the writer what kind of element we should create and how. So in fact, we still need convertInsertion() or rename converter would have to be more complicated.

Another problem is schema corectness. Model has to be always correct according to schema. So I propose that RenameOperation will:

  1. remove all attributes from the renamed element that are not correct with schema,
  2. remove all children from the renamed element that are not correct with schema.
    The question is whether these operations should be somehow done by OT or should they be silent? We could enqueue those changes and make batches but I am not sure whether this is really important. If same happens at the both sides of OT we should be fine.

So that would sum up some problems with rename operation. I think that I will try to come up with an approach that does not re-render children of renamed element. It will be probably some variation of solution with convertInsertion(), convertMove(), convertRemove() approach. I don't know whether I will do these operations in ModelConversionDispatcher or whether I will introduce rename event.

@scofalik
Copy link
Contributor

This has been resolvedi n ckeditor/ckeditor5-engine#584 by introducing RenameOperation and providing OT for that operation. The operation's type is rename so appropriate conversion mechanism and default converter has been also introduced.

@scofalik
Copy link
Contributor

BTW, OT for the delta will be added in a separate issue ckeditor/ckeditor5-engine#582.

@scofalik
Copy link
Contributor

scofalik commented Jul 7, 2017

Renaming is already handled by RenameOperation and RenameDelta.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 3 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

No branches or pull requests

4 participants