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

Modify SchemaContext #4250

Closed
pjasiun opened this issue Jan 23, 2018 · 4 comments
Closed

Modify SchemaContext #4250

pjasiun opened this issue Jan 23, 2018 · 4 comments
Assignees
Labels
package:engine type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@pjasiun
Copy link

pjasiun commented Jan 23, 2018

At this moment, the schema is not mutable and there is no easy way to create one schema based on another. The simplest way right now is to create a new definition and the new schema based on it. Even the current docs do not look very simple:

https://github.com/ckeditor/ckeditor5-engine/blob/d00c24c0fefad47f6a48abe8bd43c71ee8fddf58/src/model/schema.js#L1018

You also, need to remember about all these fixes done in the SchemaContext creator if you want to get items from the position manually:

https://github.com/ckeditor/ckeditor5-engine/blob/d00c24c0fefad47f6a48abe8bd43c71ee8fddf58/src/model/schema.js#L877-L890

This is why I think that schema SchemaContext should have methods like add/delete or push/pop to create one context based one another. These methods could modify the context object, but if we want to keep a context immutable (as @Reinmar suggested) these methods could return the new context object (do not change the current one).

Also, checkChild and checkAttribute should accept SchemaContext instance. To do so, we could make SchemaContext constructor accepts SchemaContext instance as a parameter. In such case, the same (or even the same instance) should be returned from the constructor.

@Reinmar
Copy link
Member

Reinmar commented Jan 25, 2018

Off topic:

	constructor( context ) {
		if ( Array.isArray( context ) ) {
			if ( context[ 0 ] && typeof context[ 0 ] != 'string' && context[ 0 ].is( 'documentFragment' ) ) {
				context.shift();
			}
		}
		else {
			// `context` is item or position.
			// Position#getAncestors() doesn't accept any parameters but it works just fine here.
			context = context.getAncestors( { includeSelf: true } );

			if ( context[ 0 ].is( 'documentFragment' ) ) {
				context.shift();
			}
		}

		this._items = context.map( mapContextItem );
	}

Why is the documentFragment check implemented twice? It can be done once, after the if-else part, once context is an array.

@Reinmar
Copy link
Member

Reinmar commented Jan 25, 2018

Also, checkChild and checkAttribute should accept SchemaContext instance. To do so, we could make SchemaContext constructor accepts SchemaContext instance as a parameter. In such case, the same (or even the same instance) should be returned from the constructor.

I'd go with the same instance. Since it's immutable anyway, that doesn't matter.

@pjasiun
Copy link
Author

pjasiun commented Jan 31, 2018

We decided to call it concatand it will accept any form of the schema definition, which can be string now too (it is useful also in other cases). Such string will be treated as a name of a single element.

pjasiun referenced this issue in ckeditor/ckeditor5-engine Jan 31, 2018
Feature: Convert view to model using position. Closes #1213. Closes #1250.

BREAKING CHANGE: `DataController#parse`, `DataController#toModel`, `ViewConversionDispatcher#convert` gets `SchemaContextDefinition` as a contex instead of `String`.
BREAKING CHANGE: `ViewConversionApi#splitToAllowedParent` has been introduced.
BREAKING CHANGE: `ViewConversionApi#storage` has been introduced.
BREAKING CHANGE: `ViewConsumable` has been merged to `ViewConversionApi`.
BREAKING CHANGE: Format od data object passed across conversion callback has been changed.
Feature: `Schema#findAllowedParent` has been introduced.
Feature: `SchemaContext#concat` has been introduced.
@Reinmar
Copy link
Member

Reinmar commented Jan 31, 2018

I'm afraid that I don't like the new method and other changes. I don't understand why we needed it.

Plus, I'd ask for clarification about this:

Also, checkChild and checkAttribute should accept SchemaContext instance. To do so, we could make SchemaContext constructor accepts SchemaContext instance as a parameter. In such case, the same (or even the same instance) should be returned from the constructor.

Because once it landed it just looks very... unnecessary.

EDIT: OK, that paragraph explains the "why". Sorry – I focused so much on concat() that I thought that it was added because of it.

Let's talk further under ckeditor/ckeditor5-engine#1263.

@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:model 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

No branches or pull requests

4 participants