Move selection methods to writer

[pjasiun]

We decided that writer should be the only proper way to modify model and it should include selection changes. We had plenty of bugs because of the selection changes out of the enqueueChanges block, and it should be fixed.

At this point we have following methods to modify document selection:

addRange( range, isBackward = false )
removeAllRanges()
setRanges( newRanges, isLastBackward = false )
setTo( selectable )
setIn( element )
setOn( item )
setCollapsedAt( itemOrPosition, offset )
collapseToStart()
collapseToEnd()
moveFocusTo( itemOrPosition, offset )
removeAttribute( key )
setAttribute( key, value )
setAttributesTo( attrs )
refreshAttributes()

We should use this occasion to rethink if we can limit this list. Then selection methods should be changed to protected (and starts from underscore) and proper writter methods should be added.

At the same time the question is if only document selection modification should be protected or any selection.

[pjasiun]

I reviewed the current selection API and how it is used, and we should simplify it.

I propose to introduce a new set of methods:

setRange( range );
setRange( range, isBackward );
setRange( [ range1, range2 ], isBackward );
setRange( position );
setRange( item, offset ); // same as https://github.com/ckeditor/ckeditor5-engine/blob/a8e0f3732d2ef6dbc2e604103edb393df8e6b89f/src/model/position.js#L662
setRange( item, 'end' ); // setRange( item, 'before' ); / setRange( item, 'after' ); 
setRange( null ); // removes all ranges
setRange( selection )

setFocus( itemOrPosition, offset )

removeAttribute( key );

setAttribute( key, value );
setAttribute( { key1: value1, key2: value2  );

Old API to new API porting:

addRange( range, isBackward = false ) -> // get ranges as array, add range to the array, set array as ranges
removeAllRanges() -> setRange( null );
setRanges( newRanges, isLastBackward = false ) -> setRange( newRanges, isLastBackward = false );
setTo( selectable )  -> setRange( selection )
setIn( element )  -> setRange( Range.createIn( element ) )
setOn( item ) -> setRange( Range.createOn( item ) );
setCollapsedAt( itemOrPosition, offset ) -> setRange( itemOrPosition, offset )
collapseToStart() -> setRange( selection.getFirstPosition() );
collapseToEnd() -> setRange( selection.getLastPosition() );
moveFocusTo( itemOrPosition, offset ) -> setFocus( itemOrPosition, offset )
removeAttribute( key ) -> removeAttribute( key ); // same
setAttribute( key, value ) -> setAttribute( key, value ); // same
setAttributesTo( attrs ) -> // for ( attr ) removeAttribute; setAttribute( newAttrs );
refreshAttributes() -> // private method

The new API should be available for the Selection class.

DocumentSelection should not extends selection since the API for selection modifications should not be public there. DocumentSelection should keep selection as the protected property. All get methods should be provided on DocumentSelection (as proxy), while setters should be available only through the writter:

writer.setSelection -> selection.setRange
writer.setSelectionFocus -> selection.setFocus
setSelectionAttribute -> selection.setAttribute
removeSelectionAttribute -> selection.removeAttribute

You can also remove writer.clearAttributes and merge writer.setAttribute with writer.setAttributes so attributes and selection attributes APIs will be the same.

Note that if during refactoring tests it appears that there is a missing method, feel free to add it. However, think whether that API isn't only needed for tests, are there a least 2 real (not tests) use cases for such additional method.

[Reinmar]

The setRange() method is not named correctly. Look at its use cases:

setRange( range );
setRange( range, isBackward );
setRange( [ range1, range2 ], isBackward );
setRange( position );
setRange( item, offset );
setRange( item, 'end' );
setRange( null );
setRange( selection )

In only two cases (per 8) you set it by a range. In most cases you just set it to a given location.

Note that the fact that internally selection stores a range (which isn't even true – it keeps live ranges) is an implementation detail. By having a method called setRange() we make it leak to the world which is unnecessary (although, I'm not saying that it's bad per se). Plus, this method simply isn't about setting that ranges but setting the selection to the given location(s).

Therefore, I'd override one thing in PJ's proposal and call this method setTo(). It will work beautifully with all the possible execution scenarios:

setTo( range );
setTo( range, isBackward );
setTo( [ range1, range2 ], isBackward );
setTo( position );
setTo( item, offset );
setTo( item, 'end' );
setTo( null );
setTo( selection )

However, since we have setTo() then I'm also unsure now about setFocus() (whether it should not be setFocusTo() as well). However2, I'm hesitating about the whole "setting the focus" instead of "moving the focus" so let's leave it as PJ proposed for now. We can always change it later.

[Reinmar]

There's one more thing which I don't like in this proposal, but I don't want to block it so I moved the discussion to #742.

[ma2ciek]

I'd stick with the setTo() method too. For now the setTo() method calls internally setRanges() and it seems ok. The problem I see here is the fact, that the new method must handle many cases and it'd be slightly harder to document them correctly.

[scofalik]

I understand that in model.Writer it'll be selectionSetTo()?

[ma2ciek]

I've found that we can overload methods in JSDoc, so we can end up with the following docs:

[Reinmar]

I'm unsure about overloading methods in JSDoc. We haven't done that so far so there would be no consistency and it might not work well (e.g. what about links to methods?). Additionally, I'd be worried that people who're not used to classical OOP (I'm not) would not think to check all forms of setTo(), which will mean poor discoverability.

The other option is to document it like this:

/** 
 * Bla bla
 *
 * This method allows to set the selection in multiple ways:
 *
 * * `range` and `isBackward=false` – it does bla bla
 * * `position` – it does bla bla
 * * ...
 *
 * @param {*} args... Read more about the arguments in the method description.
 */

This will be rather understandable. If properly formatted it will be even more understandable than what we usually have because it would contain examples and examples are gold. However, being understandable for human doesn't make it understandable for machines. I'm sure that many people want to convert JSDoc notation to TypeScripts's definitions and with {*} args... that won't work well.

However, taken all this, I'd not risk overloading methods in JSDoc.

[Reinmar]

I understand that in model.Writer it'll be selectionSetTo()?

As @pjasiun wrote: writer.setSelection(). Alternatively, writer.setSelectionTo(), but I'd go with what @pjasiun proposed.

[ma2ciek]

I'm unsure about overloading methods in JSDoc. We haven't done that so far so there would be no consistency and it might not work well (e.g. what about links to methods?). Additionally, I'd be worried that people who're not used to classical OOP (I'm not) would not think to check all forms of setTo(), which will mean poor discoverability.

Yep, it was just my finding and this approach have some downvotes, e.g. two same HTML's ids for same methods, probably lower readability.

I'm sure that many people want to convert JSDoc notation to TypeScripts's definitions and with {*} args... that won't work well.

I agree, but e.g. module:engine/model/writer~Writer won't work either. I don't know any existing JSDoc to Typescript Declaration File converter, but supporting above syntax won't be easy I guess. Or maybe it can be done from the JSDoc's JSON output, but it'd require quite a lot of work too.

I'll stick with the original proposal as it might have fewer cons.

[ma2ciek]

DocumentSelection should not extends selection since the API for selection modifications should not be public there. DocumentSelection should keep selection as the protected property. All get methods should be provided on DocumentSelection (as proxy), while setters should be available only through the writter

I'd add setters to the DocumentSelection but mark them as protected. Otherwise, we'll get some internal DocumentSelection's code in the Writer and weird property chains, e.g. this.model.document.selection._selection.setTo().

[pjasiun]

The setRange() method is not named correctly.

The idea behind calling it setRange was that selection is ranges + attributes. Since there are separate methods to set attributes this method should change only range. setTo was my first choice too, but it suggests that it changes both.

On the other hand, if it will change attributes to these in the focus position, it makes sense to call it setTo.

However, documentSelection.setTo( selection ) is the tricky one: should it take ranges from selection parameter or from the focus position? I think that the focus position is a better choice, to keep it consistent. Attributes should be changed separately if one wants to use specific attributes.

So I'm fine with setTo, but keep in mind to mention in the documentation that is always set attributes to these in the focus position.

I'm unsure about overloading methods in JSDoc.

Yea, I was very excited that we can do it, but then agreed with you that it will be problematic.

[ma2ciek]

One more question. Should tests to the Selection class test actually DocumentSelection? I've found that some of them do it. E.g. https://github.com/ckeditor/ckeditor5-engine/blob/2592bf116e8210ed2b2f5d3b86815e385be2f0a0/tests/model/selection.js#L951

[pjasiun]

I would test actually document selection. These tests do not need to be as precise as document selection tests but should check if the proper method was called to do what it suppose to do.

I believe it is safer to have integration tests in such cases. They will tell us if these methods do what they should do, and should work, even if we will change the implementation in the future.

[ma2ciek]

Hmm. That makes some sense. I was just surprised that tests to the Selection class have started failing after my changes in the DocumentSelection.