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

Consider making the insert/append/remove view/model API protected #3923

Closed
Reinmar opened this issue Dec 20, 2016 · 3 comments · Fixed by ckeditor/ckeditor5-engine#1328
Closed
Assignees
Labels
package:engine type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Dec 20, 2016

Currently I can't think of a case in which you should use methods like Node.remove() or Element.appendChildren(). In all cases which I know you either use deltas (when operating on the model) or model/view writer (when converting).

So, we can make this API protected. In other words, view and model nodes should be immutable, unless you're using the writer or deltas.

Also, we should consider simplifying the writer (at least the model one), because it requires working with ranges which is a pain. I'm constantly changing:

el.appendChildren( foo );

into:

modelWriter.insert( ModelRange.createOn( el ), foo );

Related issue: https://github.com/ckeditor/ckeditor5-engine/issues/737.

@Reinmar
Copy link
Member Author

Reinmar commented Dec 20, 2016

Once we'll get rid of the public API from nodes, we can perhaps reintroduce it using the writers. Although, perhaps keeping writer and deltas as separate tools is better, because then you'll need to consciously choose which to use.

@pjasiun
Copy link

pjasiun commented Dec 20, 2016

Agree that there is no more good reason to keep them public. One can say that they create the API of this class but it the API which should not be used publically.

The reason why the writer is not used there is because of the circular dependencies. In both view and model, positions, ranges and writer create a layer on top of nodes. It solved a lot of circular dependencies, but it also means that nodes can't use writers, it needs to be included separately.

In general, levels of the model API is the part which should be reviewed before the release. I remember we had a global ticket about this somewhere.

@pjasiun
Copy link

pjasiun commented Dec 21, 2017

It's almost exactly a year since the last comment here and the code is finally ready to keep everything as one API (new writer API). Note that it's not only nodes API what need to be changed to protected, but also selection and markers (and maybe something else).

szymonkups referenced this issue in ckeditor/ckeditor5-engine Mar 9, 2018
Other: Methods which modify the model's and view's tree are now protected and shouldn't be used directly in the code. Iinstance of `Writer` should be used instead. Closes #738.
szymonkups referenced this issue in ckeditor/ckeditor5-undo Mar 9, 2018
Other: Aligned code to the changes API in `ckeditor5-engine`. Read more ckeditor/ckeditor5-engine#738.
szymonkups referenced this issue in ckeditor/ckeditor5-list Mar 9, 2018
Other: Aligned code to the changes API in `ckeditor5-engine`. Read more ckeditor/ckeditor5-engine#738.
szymonkups referenced this issue in ckeditor/ckeditor5-clipboard Mar 9, 2018
Other: Aligned code to the changes API in `ckeditor5-engine`. Read more ckeditor/ckeditor5-engine#738.
szymonkups referenced this issue in ckeditor/ckeditor5-image Mar 9, 2018
Other: Aligned code to the changes API in `ckeditor5-engine`. Read more ckeditor/ckeditor5-engine#738.
szymonkups referenced this issue in ckeditor/ckeditor5-typing Mar 9, 2018
Other: Aligned code to the changes API in `ckeditor5-engine`. Read more ckeditor/ckeditor5-engine#738.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 14 milestone Oct 9, 2019
@mlewand mlewand added type:improvement This issue reports a possible enhancement of an existing feature. 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:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants