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

Model, engine and document organisation #4211

Closed
pjasiun opened this issue Nov 20, 2017 · 4 comments
Closed

Model, engine and document organisation #4211

pjasiun opened this issue Nov 20, 2017 · 4 comments
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 Nov 20, 2017

At this point, we have a mess. The document is at the same time:

  • document data structure with roots, selection, and history,
  • the model entry point with methods like transformDeltas (which has nothing to do with the document), enqueueChanges or even schema (which is mostly used in converters which operates on data outside the document).

Also, after changes proposed in https://github.com/ckeditor/ckeditor5-engine/issues/858#issuecomment-344947666, the document will fire change event also for changes which do not change the document (changes in document fragments or detached elements). This is the same change event which is overused (and should be replaced with post fixers https://github.com/ckeditor/ckeditor5-engine/issues/1101).

This is why I proposed to change the structure to this:
screen shot 2017-11-20 at 16 29 38

The current model writer class will be part of the batch API, the new writer will be more similar to the view writer.

The key concept is split between document, which is a read-only data structure, and writer which is the way to modify the document, but it can also modify any element or document fragment. It will be the writer who fires event about applied operations (currently it is document#change event), it will be writer#operation. document#change will be a more semantic event, the proper event to use by plugins, most probably the result of the document diff util https://github.com/ckeditor/ckeditor5-engine/issues/1172.

Note, that this way any util which listens on applied operations (markers, differ), can work on any subtree, while these focused on the document can filter only document operation or listen on document events.

Also, model and view will have a very similar architecture: read-only document and writer to modify it. It won't be identical because you will have to create a batch, to modify the model in batches, but this is because the problems are different (view does not need the concept of batches).

Another change is to split model and document. Now, the document will be... only a document. It will not be model entry point anymore. Schema, default transformations, transformDeltas, creating writer will be moved to the Model class.

@pjasiun
Copy link
Author

pjasiun commented Nov 20, 2017

This ticket can be done together with https://github.com/ckeditor/ckeditor5-engine/issues/678.

@pjasiun
Copy link
Author

pjasiun commented Nov 21, 2017

Now, Batch merges 2 responsibilities: it's both collection of deltas, representing single undo step and the util to modify the document (and other elements). At the same time, Writer do not do much: creates batches and fire events. This is why I propose to move all modifications methods to writer. It will be more natural to write:

writer.insert( element, position );

A writer will have batch as a property to which all deltas are saved. Creating writer you will be able to say to which batch this writer is saving changes:

const writer = new Writer( batch );
writer.insert( element, position );

Writer constructor should create a new batch by default.

const writer = new Writer();
writer.insert( element, position ); // works on the new batch

It also solves the problem with methods and cases when a batch is not really needed. It was strange that you call batch.createElement, when the batch is not involved in creating the element. Also, when you modify a detached element, which is not tracked by undo, batch is not really needed.

At the same time, it means that writer, will not be able to fire operation event since multiple writers will be used. However, I find model a perfectly fine class to fire such event.

The only inconvenience in the new solution is that you will have to use two writers if you want to have 2 separate undo steps. However, I think that:

  1. it's not a very popular case, usually all changes in your features suppose to be a single undo step, creating multiple undo steps, or adding something to an old one is an advance case, for advance users;
  2. when you think that each writer is saving changes to its batch, so if you want to work on 2 separate batches you need 2 separate writers, it's not that strange anymore.

pjasiun referenced this issue in ckeditor/ckeditor5-engine Dec 14, 2017
Other: Refactor: engine/model reorganization, introducing new chnage and enqueueChange block, split batch/writer. Related: #1186.
@pjasiun
Copy link
Author

pjasiun commented Dec 19, 2017

Lets consider it closed by ckeditor/ckeditor5-engine@5be1ad6. Details will be handled in follow-up.

@pjasiun pjasiun closed this as completed Dec 19, 2017
@pjasiun
Copy link
Author

pjasiun commented Dec 21, 2017

In the PR which closes this issue:

We decided not to introducing engine class because of reasone described here: https://github.com/ckeditor/ckeditor5-engine/issues/678#issuecomment-352760701.

@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 module:controller 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

No branches or pull requests

2 participants