Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Implementing 'is' methods to classes related to view and to model #1736

Merged
merged 33 commits into from
Aug 14, 2019
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
3816afb
Add 'is' method to model's classes.
May 8, 2019
b8c10d8
Add unit test for 'is' methods.
May 8, 2019
960bb38
Replace spaces with tabs.
May 9, 2019
87dc29b
Remove checks for no parameters, as those shouldn't be supported.
May 9, 2019
cfe3b86
Add and update 'is()' method for view components.
May 9, 2019
146bd43
Fix editableelement test to actually use EditableElement class.
May 9, 2019
9d80ec3
Add unit tests for view components.
May 9, 2019
101819e
Add prefix model to missingo ones.
May 9, 2019
c725da1
Add 'is()' method to operations.
May 9, 2019
67a5a74
Update docs description for engine/model classes.
May 10, 2019
e239f4f
Add documentation for view's classes.
May 10, 2019
c2863de
Some additioanl tweaks to do cumentation description.
May 10, 2019
264457d
Improve code spacing.
May 10, 2019
608ce64
Fix and add test to have coverage on 100%.
May 10, 2019
cf2bce2
Revert "Add 'is()' method to operations."
Jun 17, 2019
21db0af
Remove is() method from unnecessary model's classes.
Jun 17, 2019
ecea81a
Remove unit test which are no longer valid.
Jun 19, 2019
290a40c
Remove is() method from unnecessary view classes.
Jun 19, 2019
c9e7719
Remove unit test for classes without 'is()' method.
Jun 26, 2019
4e07dec
Fix docs entries to non existing places.
Jun 26, 2019
b961330
Unify names of unit tests.
Jun 28, 2019
76f8a51
Apply suggestions from code review
msamsel Jul 25, 2019
0fb8821
Revert change replacing spaces to tabs.
Jul 25, 2019
3aa2407
Update docs for View, and replace trig with regexp in is() method.
Jul 26, 2019
887ba88
Improve description to model's is method.
Jul 26, 2019
2ef7d9e
Merge branch 'master' into t/1667
Jul 29, 2019
de6f1a2
Merge branch 'master' into t/1667
Reinmar Aug 8, 2019
f33f445
Refined the documentation.
Reinmar Aug 8, 2019
96f6213
Fix wrong example in docs.
Aug 12, 2019
a1885ed
Correct description for models' classes.
Aug 12, 2019
45c4c87
Improve docs for views classes.
Aug 13, 2019
78e28c2
Minor changes in samples for is() method.
Aug 13, 2019
4315a6b
Merge branch 'master' into t/1667
Reinmar Aug 14, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/model/documentfragment.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,13 +118,13 @@ export default class DocumentFragment {
/**
* Checks whether given model tree object is of given type.
*
* Read more in {@link module:engine/model/node~Node#is}.
* Read more in {@link module:engine/model/node~Node#is `Node#is()`}.
*
* @param {String} type
* @returns {Boolean}
*/
is( type ) {
return type == 'documentFragment';
return type == 'documentFragment' || type == 'model:documentFragment';
}

/**
Expand Down
9 changes: 8 additions & 1 deletion src/model/documentselection.js
Original file line number Diff line number Diff line change
Expand Up @@ -374,15 +374,22 @@ export default class DocumentSelection {
* const selection = new DocumentSelection( ... );
*
* selection.is( 'selection' ); // true
* selection.is( 'model:selection' ); // true
* selection.is( 'documentSelection' ); // true
* selection.is( 'model:documentSelection' ); // true
* selection.is( 'view:selection' ); // false
* selection.is( 'node' ); // false
* selection.is( 'model:node' ); // false
* selection.is( 'element' ); // false
*
* @param {String} type
* @returns {Boolean}
*/
is( type ) {
return type == 'selection' || type == 'documentSelection';
return type == 'selection' ||
type == 'model:selection' ||
type == 'documentSelection' ||
type == 'model:documentSelection';
}

/**
Expand Down
10 changes: 8 additions & 2 deletions src/model/element.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,14 @@ export default class Element extends Node {
* obj instanceof Element; // true
*
* obj.is( 'element' ); // true
* obj.is( 'model:element' ); // true
* obj.is( 'listItem' ); // true
* obj.is( 'model:listItem' ); // true
* obj.is( 'element', 'listItem' ); // true
* obj.is( 'model:element', 'listItem' ); // true
* obj.is( 'text' ); // false
* obj.is( 'model:text' ); // false
* obj.is( 'view:element' ); // false
* obj.is( 'element', 'image' ); // false
*
* Read more in {@link module:engine/model/node~Node#is `Node#is()`}.
Expand All @@ -108,10 +113,11 @@ export default class Element extends Node {
* @returns {Boolean}
*/
is( type, name = null ) {
const cutType = type.replace( 'model:', '' );
msamsel marked this conversation as resolved.
Show resolved Hide resolved
if ( !name ) {
return type == 'element' || type == this.name || super.is( type );
return cutType == 'element' || cutType == this.name || super.is( type );
} else {
return type == 'element' && name == this.name;
return cutType == 'element' && name == this.name;
}
}

Expand Down
7 changes: 7 additions & 0 deletions src/model/liveposition.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,13 @@ export default class LivePosition extends Position {
this.stopListening();
}

/**
* @inheritDoc
*/
is( type ) {
return type == 'livePosition' || type == 'model:livePosition' || super.is( type );
}

/**
* Creates a {@link module:engine/model/position~Position position instance}, which is equal to this live position.
*
Expand Down
7 changes: 7 additions & 0 deletions src/model/liverange.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,13 @@ export default class LiveRange extends Range {
this.stopListening();
}

/**
* @inheritDoc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You send people to Range#is() which documents that:

Checks whether given object is of range type.

Which means that you still don't know how to use this method to check for a live range

*/
is( type ) {
return type == 'liveRange' || type == 'model:liveRange' || super.is( type );
}

/**
* Creates a {@link module:engine/model/range~Range range instance} that is equal to this live range.
*
Expand Down
12 changes: 12 additions & 0 deletions src/model/markercollection.js
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,18 @@ class Marker {
return this._liveRange.toRange();
}

/**
* Checks whether given object is of `marker` type.
*
* Read more at {@link module:engine/model/node~Node#is `Node#is()`}.
*
* @param {String} type
* @returns {Boolean}
*/
is( type ) {
return type == 'marker' || type == 'model:marker';
}

/**
* Binds new live range to the marker and detach the old one if is attached.
*
Expand Down
10 changes: 8 additions & 2 deletions src/model/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,12 @@ export default class Node {
* This method is useful when processing model tree objects that are of unknown type. For example, a function
* may return {@link module:engine/model/documentfragment~DocumentFragment} or {@link module:engine/model/node~Node}
* that can be either text node or element. This method can be used to check what kind of object is returned.
* All checked types might be prefixed with `model:` to narrow search exclusively to model's objects.
* That should prevent of situation where `view:node` accidentally might be considered as `model:node`.
* Types are defined as name of the class written in [camelCase](https://en.wikipedia.org/wiki/Camel_case) notation.
* E.g. class `LiveRange` will get type `liveRange`.
*
* There is more classes in model which follows similar naming convention. Check corresponding elements documentation for more details.
*
* obj.is( 'node' ); // true for any node, false for document fragment and text fragment
* obj.is( 'documentFragment' ); // true for document fragment, false for any node
Expand All @@ -477,11 +483,11 @@ export default class Node {
* obj.is( 'textProxy' ); // true for text proxy object
*
* @method #is
* @param {'element'|'rootElement'|'text'|'textProxy'|'documentFragment'} type
* @param {String} type
* @returns {Boolean}
*/
is( type ) {
return type == 'node';
return type == 'node' || type == 'model:node';
}
}

Expand Down
12 changes: 12 additions & 0 deletions src/model/position.js
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,18 @@ export default class Position {
}
}

/**
* Checks whether given object is of `position` type.
*
* Read more at {@link module:engine/model/node~Node#is `Node#is()`}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sending people to Node#is() from Position (which isn't a node) seems confusing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But see my other comment below about mentioning that this is a convention.

*
* @param {String} type
* @returns {Boolean}
*/
is( type ) {
return type == 'position' || type == 'model:position';
}

/**
* Checks if two positions are in the same parent.
*
Expand Down
12 changes: 12 additions & 0 deletions src/model/range.js
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,18 @@ export default class Range {
return this.containsPosition( pos ) || this.start.isEqual( pos );
}

/**
* Checks whether given object is of `range` type.
*
* Read more at {@link module:engine/model/node~Node#is `Node#is()`}.
*
* @param {String} type
* @returns {Boolean}
*/
is( type ) {
return type == 'range' || type == 'model:range';
}

/**
* Two ranges are equal if their {@link #start} and {@link #end} positions are equal.
*
Expand Down
5 changes: 3 additions & 2 deletions src/model/rootelement.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,11 @@ export default class RootElement extends Element {
* @inheritDoc
*/
is( type, name ) {
const cutType = type.replace( 'model:', '' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing blank line after var def.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, what's "cut type"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's type without module: at the beginning to simplify logic in further lines.
Statement like:

( ( type == 'rootElement' || type == 'module:rootElement'  ) && name == this.name ) || super.is( type, name );

seems to be a little to complex in my opinion.

if ( !name ) {
return type == 'rootElement' || super.is( type );
return cutType == 'rootElement' || super.is( type );
} else {
return ( type == 'rootElement' && name == this.name ) || super.is( type, name );
return ( cutType == 'rootElement' && name == this.name ) || super.is( type, name );
}
}

Expand Down
4 changes: 3 additions & 1 deletion src/model/selection.js
Original file line number Diff line number Diff line change
Expand Up @@ -629,14 +629,16 @@ export default class Selection {
* const selection = new Selection( ... );
*
* selection.is( 'selection' ); // true
* selection.is( 'model:selection' ); // true
* selection.is( 'node' ); // false
* selection.is( 'element' ); // false
* selection.is( 'view:selection' ); // false
*
* @param {String} type
* @returns {Boolean}
*/
is( type ) {
return type == 'selection';
return type == 'selection' || type == 'model:selection';
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/model/text.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export default class Text extends Node {
* @inheritDoc
*/
is( type ) {
return type == 'text' || super.is( type );
return type == 'text' || type == 'model:text' || super.is( type );
}

/**
Expand Down
6 changes: 3 additions & 3 deletions src/model/textproxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,15 +173,15 @@ export default class TextProxy {
}

/**
* Checks whether given model tree object is of given type.
* Checks whether given object is of `textProxy` type.
*
* Read more in {@link module:engine/model/node~Node#is}.
* Read more in {@link module:engine/model/node~Node#is `Node#is()`}.
*
* @param {String} type
* @returns {Boolean}
*/
is( type ) {
return type == 'textProxy';
return type == 'textProxy' || type == 'model:textProxy';
}

/**
Expand Down
5 changes: 3 additions & 2 deletions src/view/attributeelement.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,11 @@ export default class AttributeElement extends Element {
* @inheritDoc
*/
is( type, name = null ) {
const cutType = type && type.replace( 'view:', '' );
msamsel marked this conversation as resolved.
Show resolved Hide resolved
if ( !name ) {
return type == 'attributeElement' || super.is( type );
return cutType == 'attributeElement' || super.is( type );
} else {
return ( type == 'attributeElement' && name == this.name ) || super.is( type, name );
return ( cutType == 'attributeElement' && name == this.name ) || super.is( type, name );
}
}

Expand Down
5 changes: 3 additions & 2 deletions src/view/containerelement.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,11 @@ export default class ContainerElement extends Element {
* @inheritDoc
*/
is( type, name = null ) {
const cutType = type && type.replace( 'view:', '' );
if ( !name ) {
return type == 'containerElement' || super.is( type );
return cutType == 'containerElement' || super.is( type );
} else {
return ( type == 'containerElement' && name == this.name ) || super.is( type, name );
return ( cutType == 'containerElement' && name == this.name ) || super.is( type, name );
}
}
}
Expand Down
7 changes: 3 additions & 4 deletions src/view/documentfragment.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,15 +94,14 @@ export default class DocumentFragment {
}

/**
* Checks whether given view tree object is of given type.
*
* Read more in {@link module:engine/view/node~Node#is}.
* Checks whether given view tree object is of given type following the convention set by
* {@link module:engine/view/node~Node#is `Node#is()`}.
*
* @param {String} type
* @returns {Boolean}
*/
is( type ) {
return type == 'documentFragment';
return type == 'documentFragment' || type == 'view:documentFragment';
}

/**
Expand Down
9 changes: 8 additions & 1 deletion src/view/documentselection.js
Original file line number Diff line number Diff line change
Expand Up @@ -281,15 +281,22 @@ export default class DocumentSelection {
* const selection = new DocumentSelection( ... );
*
* selection.is( 'selection' ); // true
* selection.is( 'view:selection' ); // true
* selection.is( 'documentSelection' ); // true
* selection.is( 'view:documentSelection' ); // true
* selection.is( 'node' ); // false
* selection.is( 'view:node' ); // false
* selection.is( 'model:selection' ); // false
* selection.is( 'element' ); // false
*
* @param {String} type
* @returns {Boolean}
*/
is( type ) {
return type == 'selection' || type == 'documentSelection';
return type == 'selection' ||
type == 'documentSelection' ||
type == 'view:selection' ||
type == 'view:documentSelection';
}

/**
Expand Down
40 changes: 20 additions & 20 deletions src/view/downcastwriter.js
Original file line number Diff line number Diff line change
Expand Up @@ -788,26 +788,26 @@ export default class DowncastWriter {
}

/**
* Wraps elements within range with provided {@link module:engine/view/attributeelement~AttributeElement AttributeElement}.
* If a collapsed range is provided, it will be wrapped only if it is equal to view selection.
*
* If a collapsed range was passed and is same as selection, the selection
* will be moved to the inside of the wrapped attribute element.
*
* Throws {@link module:utils/ckeditorerror~CKEditorError} `view-writer-invalid-range-container`
* when {@link module:engine/view/range~Range#start}
* and {@link module:engine/view/range~Range#end} positions are not placed inside same parent container.
*
* Throws {@link module:utils/ckeditorerror~CKEditorError} `view-writer-wrap-invalid-attribute` when passed attribute element is not
* an instance of {@link module:engine/view/attributeelement~AttributeElement AttributeElement}.
*
* Throws {@link module:utils/ckeditorerror~CKEditorError} `view-writer-wrap-nonselection-collapsed-range` when passed range
* is collapsed and different than view selection.
*
* @param {module:engine/view/range~Range} range Range to wrap.
* @param {module:engine/view/attributeelement~AttributeElement} attribute Attribute element to use as wrapper.
* @returns {module:engine/view/range~Range} range Range after wrapping, spanning over wrapping attribute element.
*/
* Wraps elements within range with provided {@link module:engine/view/attributeelement~AttributeElement AttributeElement}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why touching this file in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the first stage when I add is() method, my editor fix those spaces by replacing them with tabs. So it sees to be leftover. I'll revert it.

* If a collapsed range is provided, it will be wrapped only if it is equal to view selection.
*
* If a collapsed range was passed and is same as selection, the selection
* will be moved to the inside of the wrapped attribute element.
*
* Throws {@link module:utils/ckeditorerror~CKEditorError} `view-writer-invalid-range-container`
* when {@link module:engine/view/range~Range#start}
* and {@link module:engine/view/range~Range#end} positions are not placed inside same parent container.
*
* Throws {@link module:utils/ckeditorerror~CKEditorError} `view-writer-wrap-invalid-attribute` when passed attribute element is not
* an instance of {@link module:engine/view/attributeelement~AttributeElement AttributeElement}.
*
* Throws {@link module:utils/ckeditorerror~CKEditorError} `view-writer-wrap-nonselection-collapsed-range` when passed range
* is collapsed and different than view selection.
*
* @param {module:engine/view/range~Range} range Range to wrap.
* @param {module:engine/view/attributeelement~AttributeElement} attribute Attribute element to use as wrapper.
* @returns {module:engine/view/range~Range} range Range after wrapping, spanning over wrapping attribute element.
*/
wrap( range, attribute ) {
if ( !( attribute instanceof AttributeElement ) ) {
throw new CKEditorError( 'view-writer-wrap-invalid-attribute', this.document );
Expand Down
5 changes: 3 additions & 2 deletions src/view/editableelement.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,11 @@ export default class EditableElement extends ContainerElement {
* @inheritDoc
*/
is( type, name = null ) {
const cutType = type && type.replace( 'view:', '' );
if ( !name ) {
return type == 'editableElement' || super.is( type );
return cutType == 'editableElement' || super.is( type );
} else {
return ( type == 'editableElement' && name == this.name ) || super.is( type, name );
return ( cutType == 'editableElement' && name == this.name ) || super.is( type, name );
}
}

Expand Down
5 changes: 3 additions & 2 deletions src/view/element.js
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,11 @@ export default class Element extends Node {
* @returns {Boolean}
*/
is( type, name = null ) {
const cutType = type.replace( 'view:', '' );
if ( !name ) {
return type == 'element' || type == this.name || super.is( type );
return cutType == 'element' || cutType == this.name || super.is( type );
} else {
return type == 'element' && name == this.name;
return cutType == 'element' && name == this.name;
}
}

Expand Down
Loading