T/1299: Create writer#updateMarker() method. #1388

jodator · 2018-03-28T14:05:19Z

Suggested merge commit message (convention)

Feature: Introduce writer#updateMarker() method. Closes ckeditor/ckeditor5#4276.

BREAKING CHANGE: The writer#setMarker() method is used only to create a new marker and it does not accept a marker instance as a parameter. To update existing marker use writer#updateMarker() method.
BREAKING CHANGE: The options.usingOperation option in writer#setMarker() is now a required one.
BREAKING CHANGE: The range parameter was removed. Use options.range instead.

Additional information

Other repositories that required changes:

ckeditor5-link: https://github.com/ckeditor/ckeditor5-link/tree/t/ckeditor5-engine/1299
ckeditor5-undo: https://github.com/ckeditor/ckeditor5-undo/tree/t/ckeditor5-engine/1299

Suggested merge message for other repositores:

Aligned code to changes in writer#setMarker method.

… marker.

…ng writer.setMarker() API.

# Conflicts: # tests/model/document.js

coveralls · 2018-03-28T14:17:27Z

Coverage decreased (-0.1%) to 99.878% when pulling 49b99b4 on t/1299 into 47e4f9f on master.

oskarwrobel · 2018-03-28T15:09:32Z

Since we have a separate method for updating marker we can rename setMarker to addMarker.

pjasiun · 2018-03-30T15:18:43Z

src/model/writer.js

 	 *
-	 * 		setMarker( markerName, range );
+	 * 		setMarker( markerName, { range } );


It should be updateMarker.

pjasiun · 2018-03-30T15:18:50Z

src/model/writer.js

-	 *
-	 * 		setMarker( range )
+	 * 		setMarker( marker, { range, usingOperation: true } );
+	 * 		setMarker( markerName, { range, usingOperation: true } );


It should be updateMarker.

pjasiun · 2018-03-30T15:19:58Z

tests/model/writer.js

 		} );

 		return marker;
 	}

+	function updateMarker( markerOrNameOrRange, rangeOrManagedUsingOperations, options ) {
+		model.enqueueChange( batch, writer => {
+			writer.updateMarker( markerOrNameOrRange, rangeOrManagedUsingOperations, options );


updateMarker has different parameters.

pjasiun · 2018-03-30T15:22:20Z

Since we have a separate method for updating marker we can rename setMarker to addMarker.

I agree with @oskarwrobel. We used to have setMarker to make it similar to setAttribute. But since these methods have different parameters it does not matter and addMarker makes more sense.

pjasiun · 2018-03-30T15:25:11Z

src/model/writer.js

+	 * @param {Object} options
+	 * @param {Boolean} options.usingOperation Flag indicated whether the marker should be added by MarkerOperation.
+	 * @param {module:engine/model/range~Range} options.range Marker range.
+	 * See {@link module:engine/model/markercollection~Marker#managedUsingOperations}.


This see is for usingOperation. Not for range.

pjasiun · 2018-03-30T15:34:43Z

src/model/writer.js

 		}

-		const currentMarker = this.model.markers.get( markerName );
+		const range = options && options.range;
+		const usingOperation = !!options && !!options.usingOperation;


Maybe I do not understand something or was not clear enough, but if I call writer.updateMarker( name, { range } ); (with usingOperation not specified), then usingOperation should not change. So only range should change. This code looks like it changes usingOperation to false in such case. Also, I can not find a test which checks this.

…ration when usingOperation is not specified.

pjasiun · 2018-04-04T14:47:52Z

src/model/writer.js

-				throw new CKEditorError( 'writer-setMarker-no-range: Range parameter is required when adding a new marker.' );
-			}
+		const hasUsingOperationDefined = !!options && typeof options.usingOperation == 'boolean';
+		const usingOperation = !!options && !!options.usingOperation; // : currentMarker.managedUsingOperations;


// : currentMarker.managedUsingOperations;?

pjasiun · 2018-04-04T15:03:07Z

tests/model/writer.js

 			const spy = sinon.spy();

 			model.on( 'applyOperation', spy );

-			setMarker( 'name', range, { usingOperation: true } );
+			updateMarker( marker, { usingOperation: true } );


You don't change range nor usingOperation. I do not get what this test is about.

pjasiun · 2018-04-04T15:03:22Z

tests/model/writer.js

+
+			sinon.assert.calledOnce( spy );
+
+			updateMarker( 'name', { range } );


As above. You don't change range nor usingOperation. I do not get what this test is about.

pjasiun · 2018-04-04T15:12:30Z

Tests for updateMarker are bad. First, there is no point to set the same range or usingOperation value in addMarker and then in updateMarker in that many tests. It might be a good idea to have one or two tests like this to be sure that nothing bad happens if the range or usingOperation parameter is the same, but it should be an edge case.

At the same time, there are missing tests to check if updateMarker really update markers. It should be possible to change the value of usingOperation or range or both using this method and this is what should be deeply tested.

pjasiun · 2018-04-04T15:14:22Z

src/model/writer.js

-	 *
-	 * 		setMarker( range )
+	 * 		updateMarker( marker, { range, usingOperation: true } );
+	 * 		updateMarker( markerName, { range, usingOperation: true } );


There should be no range in a sample to show that you can update only usingOperation: true parameter.

pjasiun · 2018-04-04T15:15:45Z

src/model/writer.js

 				applyMarkerOperation( this, markerName, currentMarker.getRange(), null );
 			}

-			return this.model.markers._set( markerName, newRange, usingOperation );
-		}
+			this.model.markers._set( markerName, range, hasUsingOperationDefined ? usingOperation : currentMarker.managedUsingOperations );


What if range is not defined?

jodator · 2018-04-05T09:28:29Z

OK to have this clear, the updateMarker() should

updateMarker( marker, { range } ) - change marker's range (usingOperation as defined in addMarker()
updateMarker( marker, { usingOperation: true } ) - change marker's mangedUsingOperation to true (if usingOperation was not defined in addMarker()
updateMarker( marker, { usingOperation: false } ) - change marker's mangedUsingOperation to false (if usingOperation was defined in addMarker()
updateMarker( marker, { range, usingOperation ) - change marker's range and change it's managedUsingOperation if it is different then current marker's mangedUsingOperation.

…perations.

jodator · 2018-04-05T11:47:58Z

@pjasiun OK I've revisited the test and I hope that all cases are handled. In particular I've added (changed) that if a marker was created not using operations and we update it with a new range the operations that is registered is for the updated range directly).

And vice versa if a manged marker is updated to be not managed by operations with a new range the operation that removes the marker is for current range but the marker is updated to a new range.

Other: Rename `setMarker` to `addMarker` (see ckeditor/ckeditor5-engine#1388).

jodator added 7 commits March 28, 2018 10:15

Changed: Make usingOperation option required for writer.setMarker().

e143b41

Changed: The writer.setMarker() method should not update the existing…

c71470c

… marker.

Added: Create writer.updateMarker() method.

a514ea7

Tests: Fix engine tests after adding writer.updateMarker() and changi…

4d01075

…ng writer.setMarker() API.

Aligned code to changes in writer#setMarker method.

b27da03

Docs: Update writer#setMarker() and writer#updateMarker() documentation.

8ec2b61

Merge branch 'master' into t/1299

4c78d41

# Conflicts: # tests/model/document.js

jodator requested a review from pjasiun March 28, 2018 14:26

pjasiun reviewed Mar 30, 2018

View reviewed changes

src/model/writer.js Outdated

*

* setMarker( markerName, range );

* setMarker( markerName, { range } );

Copy link

pjasiun Mar 30, 2018

Choose a reason for hiding this comment

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

It should be updateMarker.

pjasiun reviewed Mar 30, 2018

View reviewed changes

pjasiun added the status:review- label Mar 30, 2018

jodator added 4 commits April 4, 2018 12:57

Docs: Update writer documentation.

2370bda

Tests: Update helper method in model writer tests.

87f54d8

Other: Rename writer.setMarker() to writer.addMarker().

7d00bb6

Fixed: Writer#updateMarker() should not change markers mangedUsingOpe…

76f0608

…ration when usingOperation is not specified.

jodator removed the status:review- label Apr 4, 2018

pjasiun reviewed Apr 4, 2018

View reviewed changes

pjasiun added the status:review- label Apr 4, 2018

Changed: Properly handle marker update with changes to be manged by o…

49b99b4

…perations.

pjasiun added status:review+ and removed status:review- labels Apr 5, 2018

pjasiun merged commit bed647f into master Apr 6, 2018

pjasiun deleted the t/1299 branch April 6, 2018 08:43

pjasiun pushed a commit to ckeditor/ckeditor5-undo that referenced this pull request Apr 6, 2018

Merge pull request #86 from ckeditor/t/ckeditor-engine/1299

45ff636

Other: Rename `setMarker` to `addMarker` (see ckeditor/ckeditor5-engine#1388).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

T/1299: Create writer#updateMarker() method. #1388

T/1299: Create writer#updateMarker() method. #1388

jodator commented Mar 28, 2018 •

edited by pomek

Loading

coveralls commented Mar 28, 2018 •

edited

Loading

oskarwrobel commented Mar 28, 2018

pjasiun Mar 30, 2018

pjasiun Mar 30, 2018 •

edited

Loading

pjasiun Mar 30, 2018

pjasiun commented Mar 30, 2018

pjasiun Mar 30, 2018 •

edited

Loading

pjasiun Mar 30, 2018

pjasiun Apr 4, 2018

pjasiun Apr 4, 2018

pjasiun Apr 4, 2018

pjasiun commented Apr 4, 2018

pjasiun Apr 4, 2018

pjasiun Apr 4, 2018

jodator commented Apr 5, 2018

jodator commented Apr 5, 2018


		sinon.assert.calledOnce( spy );

		updateMarker( 'name', { range } );

T/1299: Create writer#updateMarker() method. #1388

T/1299: Create writer#updateMarker() method. #1388

Conversation

jodator commented Mar 28, 2018 • edited by pomek Loading

Suggested merge commit message (convention)

Additional information

coveralls commented Mar 28, 2018 • edited Loading

oskarwrobel commented Mar 28, 2018

Choose a reason for hiding this comment

pjasiun Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjasiun commented Mar 30, 2018

pjasiun Mar 30, 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

pjasiun commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodator commented Apr 5, 2018

jodator commented Apr 5, 2018

jodator commented Mar 28, 2018 •

edited by pomek

Loading

coveralls commented Mar 28, 2018 •

edited

Loading

pjasiun Mar 30, 2018 •

edited

Loading

pjasiun Mar 30, 2018 •

edited

Loading