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

Text nodes simplification #3756

Closed
pjasiun opened this issue Jun 13, 2016 · 5 comments · Fixed by ckeditor/ckeditor5-engine#537
Closed

Text nodes simplification #3756

pjasiun opened this issue Jun 13, 2016 · 5 comments · Fixed by ckeditor/ckeditor5-engine#537
Assignees
Labels
package:engine type:task This issue reports a chore (non-production change) and other types of "todos".
Milestone

Comments

@pjasiun
Copy link

pjasiun commented Jun 13, 2016

The original concept was to have every character as a separate node. It did no work.

Then we had an idea to merge them together and make them act like a separate characters. This is why we have CharacterProxy which is a Node, TextProxy which is not a Node, Text to insert a text with attributes and private NodeListText to keep compressed text. This is why I have no idea how to copy NodeList it the efficient way or I need to think event time which API I should use in each case.

No more lies!

This lie, does not make much sense, since we decided that even TreeWalker return whole texts by default.

First we should apply some change, easy to agree:

  • Text is a Node,
  • Text and NodeListText should be merge into a single class,
  • CharacterProxy and TextProxy should be merge into a single class (TextProxy or TextFragment),
  • NodeList has no hidden internal structures, is contains Texts.

Then we will see what this change will bring. It may change the way Positions work and change TreeWalker API, but I think that we should keep the change as simple as possible at first and then introduce new features to it.

Also, note that having only Text and TextFragment (for what TreeWalker returns when you start iterating inside a text or ask for single characters) we will have the same classes in model and the view.

Related: https://github.com/ckeditor/ckeditor5-engine/issues/350, https://github.com/ckeditor/ckeditor5-engine/issues/351, https://github.com/ckeditor/ckeditor5-engine/issues/353, https://github.com/ckeditor/ckeditor5-engine/issues/357, https://github.com/ckeditor/ckeditor5-engine/issues/321, https://github.com/ckeditor/ckeditor5-engine/issues/428.

@pjasiun
Copy link
Author

pjasiun commented Jun 22, 2016

https://github.com/ckeditor/ckeditor5-engine/issues/255 view TreeWalker introduce architecture similar to what we want to have in the model.

An additional change we could apply here is merging TEXT and CHARACTER types into one (text).

Also if Text will be a node NodeList should return Text as child, but we could have additional getItemAt method, which provide the "old" API, for instance for OT.

@scofalik scofalik self-assigned this Jul 1, 2016
@scofalik
Copy link
Contributor

scofalik commented Jul 1, 2016

In F2F discussion we come to conclusion that it is worthy to consider removing NodeList and creating ModelWriter.

Why so?

We already have this approach in view. In fact, in view and in model we have same problem: we have a basic structure, in which texts are nodes but we need some easy method to operate on those texts when we have a position inside a text node and we want to do something there, i.e. change attributes of part of text or insert something inside text node.

Right now we solve this same issue differently both in model and in view. In model, there is no ViewWriter. Instead, for historical reasons, we have NodeList which tried to provide character-is-singular-node API for texts that were inserted as a whole, internally processed, kept as a whole, but presented as single characters. This made sense for some time but the point of this issue is to get rid of this overcomplication.

Still, in model, we need to operate on single characters, because we might want to change attributes of part of text node or remove only part of text node. So we would need to have dobule API in ModelNode and ModelElement - one "basic" API to operate on nodes as they are, and second API to operate on "offsets" which would enable inserting/removing/changing inside text nodes. This second API is now already provided thanks to NodeList.

There are multiple pros of such division:

  • You have one API, so you don't have to explain differences between operating on indexes and operating on offsets, which leads to better user experience.
  • In many cases users will be interested in basic API.
  • The API will be similar for view and model.
  • It makes sense that you have a basic API and then you have a tool/external interface/worker/writer that lets you do more complicated stuff. Normally you could, i.e., change attribute on part of node by, removing text node, creating two/three text nodes and insert them again. Now, ModelWriter will have those features built-in. It makes perfect sense, much better than having some sort of NodeList that pretends to do things.
  • Getting rid of NodeList will bring further simplifications.

I am sure that we need to refactor this part also, while working on this issue.

@pjasiun
Copy link
Author

pjasiun commented Jul 1, 2016

It would be great to replace TextProxy, CharacterProxy, Text, NodeList and NodeListText with a simple Text and a Writer.

@scofalik
Copy link
Contributor

scofalik commented Jul 1, 2016

And TextProxy but still, we get rid of three "what-the-fuck" classes and introduce simpler and more clean division.

@pjasiun
Copy link
Author

pjasiun commented Jul 1, 2016

Yep, that is true.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 2 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:task This issue reports a chore (non-production change) and other types of "todos". 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:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants