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

Allow for storing additional data related to roots #13395

Closed
scofalik opened this issue Feb 3, 2023 · 7 comments · Fixed by #13759
Closed

Allow for storing additional data related to roots #13395

scofalik opened this issue Feb 3, 2023 · 7 comments · Fixed by #13759
Assignees
Labels
squad:collaboration Issue to be handled by the Collaboration team. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@scofalik
Copy link
Contributor

scofalik commented Feb 3, 2023

📝 Provide a description of the new feature

Allow for storing additional data related to roots. We already have RootAttributeOperation and it is possible to set an attribute on a root element using writer.setAttribute(). However it is hard to use them outside of the editing pipeline. There's no simple API that would allow to store "metadata" that is connected with the document but is not the document data itself.

This is especially important for applications that might want to use multi-root editors. If an application "document" is built from multiple, separated editing areas, it is common that these editing areas may also have their own properties that can be changed by users as they edit the document.

It should be possible to easily save and load such additional data and it also should be propagated in real-time collaborative editing.

@scofalik scofalik added type:feature This issue reports a feature request (an idea for a new functionality or a missing option). squad:collaboration Issue to be handled by the Collaboration team. labels Feb 3, 2023
@scofalik scofalik self-assigned this Feb 3, 2023
@CKEditorBot CKEditorBot added the status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. label Feb 21, 2023
@CKEditorBot CKEditorBot added status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. and removed status:planned Set automatically when an issue lands in the "Sprint backlog" column. We will be working on it soon. labels Mar 14, 2023
@scofalik
Copy link
Contributor Author

scofalik commented Mar 24, 2023

Some final decisions to be made regarding the API. Do you have an opinion on naming for "additional data" and APIs related with it? We've been talking about root attributes, but currently I call it metadata.Why not attributes but metadata?

  1. I wanted to cut any connotations with regular attributes and how they are used. Also, we have some features that use root attributes, and I don't want it to be mixed with the new thing.
  2. I want to it to be clear that we are introducing something new with new possibilities/different purpose (someone who heard about root attributes would think that this isn't something new).
  3. I want to have a different API for this (esp. on writer to differentiate from regular attributes) and have it easier to be used (so you don't need for example specify a converter or somehow tell the editor that given root attribute is special).
  4. I want people to think about it as data, as something custom, arbitrary, to be saved and loaded.

Another proposal is properties but that's close to attributes. metadata OTOH is close to just "data".

One problem with metadata in general is that this concept is new to rich-text editor integrators. Data is pretty obvious, you want to save what you edited and you know that you have to do it.

With metadata it is not obvious why or when you should save it, especially since no official plugins use it yet (and it will be very rare). In docs, for such plugins, we will need to visibly highlight that the plugin uses metadata and that data must be saved.

Some of the API proposals:

writer.setRootMetadata( key: string, value: unknown, root: RootElement );
editor.getMetadata(); // -> { rootA: { ... }, rootB: { ... } } or .`getMetadata( rootName: string )` as with `getData()`

Editor.create( ..., { metadata: { rootA: { ... }, rootB: { ... } } } );
Editor.create( ..., { metadata: { ... } } ); // Single root version.

@oleq
Copy link
Member

oleq commented Mar 24, 2023

Reposting my comment from Slack:

TBH I’m not sure if this new API should be in the Editor namespace. For instance editor.getMetadata(); .To me, the Editor namespace feels like a place for the integrators only, e.g. setData(), getData() , enableReadOnlyMode(), destroy().This whole metadata concept is for developers who build things using the framework. This is an advanced API. It should be behind editor.model and editor.editing (I know it touches both, so probably somewhere else).


A separate topic is that “metadata” is very vague. It could be anything: content/document metadata? Editor (as a HTML component) metadata? Something related to SEO? I think we should make it clear that this is related to multiple roots and the structure of the edited document.

@Reinmar
Copy link
Member

Reinmar commented Mar 24, 2023

What worries me here is:

  • On the view layer we have attributes and custom data
  • On the model we have attributes and (? is there something more ?)
  • On the editor level and model root level we have data and metadata

It’d IMO look better if metadata would be only visible on the editor level. If they didn’t leak anyhow to the writer. Then, we would have data and the accompanying metadata. Do you think there’s an option to rething what we have in the writer?Another problem I can see: when looking at the editor intereface, there’s data and then there’s metadata. If you saw it for the first time would you expect comments or track changes information to be metadata? I’m afraid so. Is this mechanism (on the editor layer) capable of becoming a store for comments too?

I remember you were thinking about the future applications of metadata and how the editor interface could look like. That’s where I’d move back for a moment.

@scofalik
Copy link
Contributor Author

scofalik commented Mar 24, 2023

TL;DR:

Let's move the API to multi-root editor, and change as few things in engine and core as possible. If possible, make changes only in ckeditor5-editor-multi-root. We don't know all the (future) requirements and if something changes, it will be breaking for a smaller audience. This way it will be easier to commit to some decisions now.


Fantastic, a two opposite opinions on where it should go ;).

Let's first clarify, that the changes must go through the model, so they are:

  1. Propagated to other (remote) clients.
  2. Synchronized (made in a correct order) with other changes that are happening in the model/editor.
  3. Probably, it would be beneficial to have a reasonable API to react to them, hopefully resembling/using other mechanisms that we have.

In the end, metadata is kind of data, just stored in a different way.

Thankfully, this is a part where we all agree (as I clarified with @Reinmar in F2F talk).

The question is: do we know everything about metadata? What kind of uses for it can we imagine? Do we think there are more requirements we can learn and how probably it is that it can evolve in a "breaking" way?

Here, I have to admit to @Reinmar that we do not have the whole data gathered. We focused on multi-root uses, with some very strict use cases, like section editing. It may be risky to introduce it heavily on the engine/core side as this might mean more breaking changes in the future.

I personally think that root attributes -- or maybe general data stored synchronized between remote clients -- will be beneficial even for "single root" editors, and something that we may want to have. But I don't have good examples on hand. Instead, it will be better to focus on multi-root use cases.

This means that it will be better to keep as much of the API as possible in multi-root editor. If breaking change happens (e.g. we will introduce a similar (but different) general scenario and what is proposed here will be obsolete) then it will concern smaller audience.

Now, answering some of your questions:

A separate topic is that “metadata” is very vague. It could be anything: content/document metadata? Editor (as a HTML component) metadata? Something related to SEO? I think we should make it clear that this is related to multiple roots and the structure of the edited document.

  1. Initially, I didn't want to have it just for multi-roots, I wanted it to have in "single root" editors too.
  2. I also didn't want to suggest what can be there and what cannot be there. OTOH, guides and API docs should explain it because, as I wrote, this is a less intuitive topic than just "data".

To me, the Editor namespace feels like a place for the integrators only.

And you are correct. But it has the same reason to be on Editor as Editor.getData(). It is kind of data, it is something that you want to save with regular data, keep and load to the editor. If you treat the document in a more holistic way and you accept e.g. structured document with sections as a document, it is natural, that sections properties are part of the document data. After saving the document, you may want to use the properties to display things in a different way on your website, or have them handled, managed, post-processed, shared in a different way, etc.

I think that what confuses you and why you don't like it is because we don't have any official plugins using it. I can imagine this may change if we want to explore "section editing space" more and propose some own solutions.

If you saw it for the first time would you expect comments or track changes information to be metadata? I’m afraid so.

Yes, and I don't have a great idea how to communicate it in a better way other than just have good docs (and examples). As I wrote, it is a problem that you don't know when you should be interested in saving this metadata.

Is this mechanism (on the editor layer) capable of becoming a store for comments too?

Not in its current form, where metadata is related to roots.

I remember you were thinking about the future applications of metadata and how the editor interface could look like. That’s where I’d move back for a moment.

I... don't remember it exactly :D, not sure if I had some concrete ideas :D.

@scofalik
Copy link
Contributor Author

scofalik commented Mar 26, 2023

New proposal (in PR):

  1. Removed concept of metadata. There's no metadata anywhere. No changes in writer. Root attributes are called root attributes. Simply.
  2. There's much less new code compared to before, which is a plus.
  3. Minimal changes in core/engine. Introduced changes outside multi-root editor are:
    1. Added RootAttributeOperation handling in the Differ. However, it should be treated as a past omission.
    2. Added EditorConfig#rootsAttributes. This has a note that it is handled only in multi-root editor.
    3. There will be changes in RealTimeCollaborationClient. It will check if rootsAttributes is set.
  4. Other changes are on multi-root editor:
    1. Added callback for DataController#init which uses rootsAttributes config value.
    2. Added MultiRootEditor#getRootsAttributes().
  5. Writer#setAttribute() is used to set root attributes. You don't need to add any prefix, etc.
  6. MultiRootEditor#getRootsAttributes() return these attributes, that were set in rootsAttributes config value.
    1. Integrator will need to specify all "special" root attribute keys in the config.
    2. If root doesn't have an attribute, null value must be passed. The attribute "entry" must be in the config.
    3. Attributes added through MultiRootEditor#addRoot() are included as well. This is especially important if you have an integration where all roots are added dynamically (you init the editor with no roots).

This way we have:

  1. Much smaller PR.
  2. Changes limited mostly to MultiRootEditor scope. The only addition is config prop.
  3. The feature is still easy to use (I wanted to avoid things like editor.registerDataRootAttributeKey( 'foo' )).
  4. RTC does not need to be heavily changed. Although, it has to know about config.rootsAttributes.
  5. There's no new concept so there's less confusion. config.rootsAttributes and editor.getRootsAttributes() are better at hinting what are you dealing with. The solution is more focused on the actual problem-to-solve.
  6. There's one small drawback. If we introduce a feature that uses roots attributes, the integrator will have to know that they have to start using config.rootsAttributes and "init" it with null values.

Example usage:

MultiRootEditor.create( ..., {
    rootAttributes: {
        rootId1: { order: 10, ownerId: 'userId' },
        rootId2: { order: 20, ownerId: null }
    }
} ).then( editor => {
    editor.model.document.getRoot( 'rootId1' ).getAttribute( 'order' ); // 10.
    editor.model.document.getRoot( 'rootId2' ).hasAttribute( 'ownerId' ); // false.
} );
editor.model.change( writer => {
    const root = editor.model.document.getRoot( 'rootId2' );

    writer.setAttribute( 'order', 0, root );
} );
editor.model.document.on( 'change:data', () => {
    for ( const change of editor.model.document.differ.getChangedRoots() ) {
        if ( change.attributes && change.attributes.order ) {
            const root = editor.model.document.getRoot( change.name );

            // ...
        }
    }
} );
editor.getRootsAttributes();
/*
    {
        rootId1: { order: 10, ownerId: 'userId' },
        rootId2: { order: 0, ownerId: null }
    }
*/

@Reinmar
Copy link
Member

Reinmar commented Mar 27, 2023

6. There's one small drawback. If we introduce a feature that uses roots attributes, the integrator will have to know that they have to start using config.rootsAttributes and "init" it with null values.

Why do they have to init attributes with null

Also, it sounds like it applies e.g. to HTMLComments too. So I guess I misunderstood something.

@scofalik
Copy link
Contributor Author

Why do they have to init attributes with null

Since getRootsAttributes() returns only what was set in config.rootsAttributes, then, if we introduce a feature requiring to load and save an attribute, it will have to be specified in rootsAttributes. It doesn't have to be null but it have to be set.

Unless, of course, at that point, we introduce something like editor.registerRootAttributeKey( 'foo' ).

Also, it sounds like it applies e.g. to HTMLComments too. So I guess I misunderstood something.

HTML comments feature uses roots attributes without any changes. Attributes used by that feature will not be available in getRootsAttributes().

scofalik added a commit that referenced this issue Mar 28, 2023
Other (engine): `RootAttributeOperation` is now correctly handled by `Differ`. Root attribute changes will be returned in `Differ#getChangedRoots()`.

Internal (editor-multi-root): Introduced API to easily save and load root attributes. Introduced `EditorConfig#rootsAttributes` and `MultiRootEditor#getRootsAttributes()`. Closes #13395.

Internal (editor-multi-root): Introduced `MultiRootEditor#getFullData()`. The method returns document data for all roots in multi-root editor.

Internal (editor-multi-root): `MultiRootEditor#addRoot()` has a new option `attributes`.

Internal (engine): `model.Writer#detachRoot()` will now remove attributes from the detached root.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Mar 28, 2023
@CKEditorBot CKEditorBot added this to the iteration 61 milestone Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squad:collaboration Issue to be handled by the Collaboration team. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants