Ckeditor5: Implement is() in model/Selection and model/DocumentSelection

[oleq]

I found out that the only way to check if anything is (Document)Selection is by instanceof which means additional imports.

This is is a problem I discovered when writing a converters guide with quick solutions for various use–cases. The code snippets don't require any imports (plugins defined as functions) and it is possible to use them with pre–build editors, which is very convenient. Forcing people to import means building the editor and seriously hits the DX.

Before:

import ModelSelection from '@ckeditor/ckeditor5-engine/src/model/selection';
import DocumentSelection from '@ckeditor/ckeditor5-engine/src/model/documentselection';

function AddClassToAllLinks( editor ) {
    editor.conversion.for( 'downcast' ).add( dispatcher => {
        dispatcher.on( 'attribute:linkHref', ( evt, data, conversionApi ) => {
            const viewWriter = conversionApi.writer;
            const viewSelection = viewWriter.document.selection;
            const viewElement = viewWriter.createAttributeElement( 'a', { class: 'my-class' }, { priority: 5 } );

            if ( data.item instanceof ModelSelection || data.item instanceof DocumentSelection ) {
                viewWriter.wrap( viewSelection.getFirstRange(), viewElement );
            } else {
                viewWriter.wrap( conversionApi.mapper.toViewRange( data.range ), viewElement );
            }
        } );
    } );
}

After:

function AddClassToAllLinks( editor ) {
    editor.conversion.for( 'downcast' ).add( dispatcher => {
        dispatcher.on( 'attribute:linkHref', ( evt, data, conversionApi ) => {
            const viewWriter = conversionApi.writer;
            const viewSelection = viewWriter.document.selection;
            const viewElement = viewWriter.createAttributeElement( 'a', { class: 'my-class' }, { priority: 5 } );

            if ( data.item.is( 'modelSelection' ) || data.item.is( 'documentSelection' ) ) {
                viewWriter.wrap( viewSelection.getFirstRange(), viewElement );
            } else {
                viewWriter.wrap( conversionApi.mapper.toViewRange( data.range ), viewElement );
            }
        } );
    } );
}

cc @scofalik @jodator

[ma2ciek]

I couldn't find a ticket but it's been proposed somewhere in the past. I agree that we should implement it.

Note that we need it also in the view/Selection and view/DocumentSelection.

[jodator]

Less imports! :+1:

I'd propose selection and documentSelection for Model and View selections. The model is reduntant (we do not have modelElement in ModelElement.is()) also you'd like to catch both here so DocumentSelection.is( 'selection' ) === true should be true.

[pjasiun]

Note that is should be consistent:

data.item.is( 'selection' ) || data.item.is( 'documentSelection' )

or

data.item.is( 'modelSelection' ) || data.item.is( 'modelDocumentSelection' )

Since we have element.is( 'element' ) (even though there is view element and model elemet) I believe the first option in better.

Also, I think that we should have just one: item.is( 'selection' ) should be true for both selection and document selection. We have 2 separate classes because you should not modify document selection directly, you need to use change block to it. It means that document selection has less public methods than selection and document selection cannot extend the selection class.

However, in most cases, you do not care which one it is since they have the same getter interface. Also, I believe that if you want to change the selection you know which class you have (usually you create the selection and set it in the same place). This is why I think that a single item.is( 'selection' ) might be enough.

[Reinmar]

Since we have element.is( 'element' ) (even though there is view element and model elemet) I believe the first option in better.

👍

Also, I think that we should have just one: item.is( 'selection' ) should be true for both selection and document selection.

I'm not sure here... One doesn't inherit from the other. But @pjasiun made a good point that you can still do:

foo.is( 'selection' ) && !foo.is( 'documentSelection' )

if you want to check if it's the base selection. Taken that, I'd agree with @pjasiun on this.

[Reinmar]

I reported a followup to review also other objects: https://github.com/ckeditor/ckeditor5-engine/issues/1667. There's more classes which miss this method, plus we should review some instanceof checks.