Move selection methods to Writer, protect DocumentSelection #1237

ma2ciek · 2018-01-15T15:48:48Z

Suggested merge commit message (convention)

other: Moved selection methods to Writer, introduced LiveSelection. Closes ckeditor/ckeditor5#4221.

coveralls · 2018-01-15T15:53:24Z

Coverage decreased (-0.2%) to 99.767% when pulling 2447683 on t/1209 into 47291eb on master.

coveralls · 2018-01-16T09:26:03Z

Coverage decreased (-0.3%) to 99.723% when pulling 92c91ce on t/1209 into 47291eb on master.

coveralls · 2018-01-16T09:33:01Z

Coverage decreased (-0.3%) to 99.723% when pulling a200add on t/1209 into 47291eb on master.

coveralls · 2018-01-16T09:59:23Z

Changes Unknown when pulling 8bddd7d on t/1209 into ** on master**.

coveralls · 2018-01-16T10:10:01Z

Coverage decreased (-0.2%) to 99.752% when pulling 38bfce5 on t/1209 into e55814b on master.

coveralls · 2018-01-16T10:30:04Z

Coverage decreased (-0.2%) to 99.752% when pulling a19980b on t/1209 into e55814b on master.

coveralls · 2018-01-16T10:34:57Z

Coverage decreased (-0.2%) to 99.752% when pulling a19980b on t/1209 into e55814b on master.

coveralls · 2018-01-16T10:46:26Z

Coverage decreased (-0.2%) to 99.752% when pulling ea88b5f on t/1209 into e55814b on master.

coveralls · 2018-01-16T11:00:04Z

Coverage decreased (-0.2%) to 99.767% when pulling e045b18 on t/1209 into e55814b on master.

coveralls · 2018-01-16T14:22:00Z

Coverage decreased (-0.2%) to 99.796% when pulling 002738d on t/1209 into e55814b on master.

coveralls · 2018-01-16T15:19:37Z

Coverage decreased (-0.2%) to 99.796% when pulling a012c2c on t/1209 into e55814b on master.

coveralls · 2018-01-16T15:49:49Z

Coverage decreased (-0.2%) to 99.796% when pulling 25f275e on t/1209 into e55814b on master.

coveralls · 2018-01-17T15:38:24Z

Coverage decreased (-0.2%) to 99.825% when pulling 6d4da68 on t/1209 into e55814b on master.

coveralls · 2018-01-17T15:47:49Z

Coverage decreased (-0.2%) to 99.825% when pulling 504e221 on t/1209 into e55814b on master.

coveralls · 2018-01-17T16:03:50Z

Coverage decreased (-0.2%) to 99.839% when pulling 2bad633 on t/1209 into e55814b on master.

coveralls · 2018-01-18T10:35:55Z

Coverage remained the same at 100.0% when pulling 8e0feed on t/1209 into fd2f7c1 on master.

pjasiun · 2018-01-24T13:17:44Z

src/model/documentselection.js


 /**
 * `DocumentSelection` is a special selection which is used as the
 * {@link module:engine/model/document~Document#selection document's selection}.
 * There can be only one instance of `DocumentSelection` per document.
 *
+ * `DocumentSelection` is a proxy to {@link module:engine/model/liveselection~LiveSelection} that provides


Live Selection should not be public and should not be mentioned in the documentation for public classes.

pjasiun · 2018-01-24T13:19:37Z

src/model/documentselection.js

 		/**
-		 * Document which owns this selection.
+		 * TODO


pjasiun · 2018-01-24T13:29:36Z

src/model/liveselection.js

@@ -0,0 +1,674 @@
+/**


I would make LiveSelection private or protected. It should be used only by the DocumentSelection and could be even defined there.

Both classed could go to the same file.

pjasiun · 2018-01-24T13:59:17Z

src/model/selection.js

 	 *
-	 * @fires change
+	 * @private
 	 */


Private methods should go to the end of this file.

pjasiun · 2018-01-24T14:02:45Z

src/model/utils/deletecontent.js

@@ -82,7 +83,11 @@ export default function deleteContent( model, selection, options = {} ) {
 			schema.removeDisallowedAttributes( startPos.parent.getChildren(), writer );
 		}

-		selection.setCollapsedAt( startPos );
+		if ( selection instanceof DocumentSelection ) {
+			selection._setTo( startPos );


The writer should be used here.

pjasiun · 2018-01-26T14:41:16Z

src/model/documentselection.js

 		/**
-		 * Document which owns this selection.
+		 * Selection used internally by that class (`DocumentSelection` is a proxy to that selection).
 		 *
 		 * @protected


Shouldn't this be private?

It's a very useful property in tests.

pjasiun · 2018-01-26T14:45:22Z

src/model/documentselection.js

+	 * @param {Number|'end'|'before'|'after'} [offset] Offset or one of the flags. Used only when
+	 * first parameter is a {@link module:engine/model/item~Item model item}.
+	 */
+	_setFocus( itemOrPosition, offset ) {


All protected method should go after all public methods.

pjasiun · 2018-01-26T15:03:00Z

src/model/documentselection.js

+		this._selection.destroy();
+	}
+
+	getAttributeKeys() {


pjasiun · 2018-01-26T15:10:20Z

src/model/writer.js

+
+	/**
+	 * Sets attribute(s) on the selection. If attribute with the same key already is set, it's value is overwritten.
+	 *


Samples needed.

pjasiun · 2018-01-26T16:18:51Z

Selection constructor could use setTo and accepts all its parameters. Then we should be able to remove Selection.createFromSelection.

…romSelection().

…mSelection().

ma2ciek added the status:review- label Jan 15, 2018

ma2ciek force-pushed the t/1209 branch from 8bddd7d to 38bfce5 Compare January 16, 2018 10:05

ma2ciek force-pushed the t/1209 branch 2 times, most recently from ff03f2a to a19980b Compare January 16, 2018 10:26

ma2ciek force-pushed the t/1209 branch from a19980b to ea88b5f Compare January 16, 2018 10:41

ma2ciek force-pushed the t/1209 branch from 0b6790b to a012c2c Compare January 16, 2018 14:43

ma2ciek force-pushed the t/1209 branch from 8091ad2 to b4dbc96 Compare January 18, 2018 10:28

ma2ciek force-pushed the t/1209 branch 2 times, most recently from 2704d59 to d8029cf Compare January 24, 2018 10:58

pjasiun reviewed Jan 24, 2018

View reviewed changes

src/model/documentselection.js Outdated

/**

* Document which owns this selection.

* TODO

Copy link

pjasiun Jan 24, 2018

Choose a reason for hiding this comment

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

TODO

pjasiun reviewed Jan 24, 2018

View reviewed changes

src/model/selection.js Outdated

*

* @fires change

* @private

*/

Copy link

pjasiun Jan 24, 2018

Choose a reason for hiding this comment

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

Private methods should go to the end of this file.

pjasiun reviewed Jan 24, 2018

View reviewed changes

ma2ciek added 11 commits January 26, 2018 14:27

Fixed usage of DocumentSelection and Writer.

adf1043

Made LiveSelection a private class.

8e8748a

Added missing tests for insertContent and deleteContent.

93fd62d

Added missing test for autoparagraphing at custom selection.

9793128

Added last missing test, removed dead code.

12ab52f

Improved selection tests.

cd67ed5

Removed circular imports.

1163f2e

Added DocumentSelection#getSelectedElement().

4e2752b

Added comment to the setTo method.

3c74bca

Fixed introduced bug in DomConverter.

cf28a1f

Added more restricted types in setTo().

8cd280c

ma2ciek force-pushed the t/1209 branch from 650a046 to 8cd280c Compare January 26, 2018 13:27

ma2ciek added 2 commits January 26, 2018 14:41

Added small improvements to dev-utils/model.js.

92fda51

Added small improvements to model/writer.js.

c864df9

ma2ciek force-pushed the t/1209 branch from 4e80fa9 to c864df9 Compare January 26, 2018 13:52

pjasiun reviewed Jan 26, 2018

View reviewed changes

src/model/documentselection.js Outdated

this._selection.destroy();

}

getAttributeKeys() {

Copy link

pjasiun Jan 26, 2018

Choose a reason for hiding this comment

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

Docs?

pjasiun reviewed Jan 26, 2018

View reviewed changes

ma2ciek added 3 commits January 26, 2018 16:21

Fixed small issues with DocumentSelection.

39a1362

Improved docs in Writer.

427e841

Merge branch 'master' into t/1209

5b2ca2f

ma2ciek changed the title ~~Move selection methods to Writer, introduce LiveSelection~~ Move selection methods to Writer, protect DocumentSelection Jan 26, 2018

ma2ciek added 3 commits January 26, 2018 23:46

Enhanced ModelSelection#constructor(), removed ModelSelection.createF…

1640a51

…romSelection().

Enhanced ViewSelection#constructor(), removed ViewSelection.createFro…

8f29e39

…mSelection().

Aligned code fragments to new Selection constructor.

8e0feed

ma2ciek removed the status:review- label Jan 29, 2018

pjasiun merged commit 7db1fee into master Jan 29, 2018

pjasiun deleted the t/1209 branch January 29, 2018 11:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move selection methods to Writer, protect DocumentSelection #1237

Move selection methods to Writer, protect DocumentSelection #1237

ma2ciek commented Jan 15, 2018 •

edited by pomek

Loading

coveralls commented Jan 15, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 17, 2018

coveralls commented Jan 17, 2018

coveralls commented Jan 17, 2018

coveralls commented Jan 18, 2018 •

edited

Loading

pjasiun Jan 24, 2018

pjasiun Jan 24, 2018

pjasiun Jan 24, 2018

pjasiun Jan 24, 2018

pjasiun Jan 24, 2018

pjasiun Jan 24, 2018

pjasiun Jan 26, 2018

ma2ciek Jan 26, 2018

pjasiun Jan 26, 2018

pjasiun Jan 26, 2018

pjasiun Jan 26, 2018

ma2ciek Jan 26, 2018

pjasiun commented Jan 26, 2018

Move selection methods to Writer, protect DocumentSelection #1237

Move selection methods to Writer, protect DocumentSelection #1237

Conversation

ma2ciek commented Jan 15, 2018 • edited by pomek Loading

Suggested merge commit message (convention)

coveralls commented Jan 15, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 16, 2018

coveralls commented Jan 17, 2018

coveralls commented Jan 17, 2018

coveralls commented Jan 17, 2018

coveralls commented Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjasiun commented Jan 26, 2018

ma2ciek commented Jan 15, 2018 •

edited by pomek

Loading

coveralls commented Jan 18, 2018 •

edited

Loading