T/643 Markers in engine #700
Conversation
|
Related PRs: Link to CI build with branch configured to have all related branches https://travis-ci.org/ckeditor/ckeditor5/builds/179751995 |
|
Follow-ups: Also in undo: |
| if ( this._from.type == 'element' ) { | ||
| // Converting from model element to view attribute is unsupported. | ||
| if ( this._from.type != 'attribute' ) { | ||
| // Only converting from attribute to attribute is supported. |
There was a problem hiding this comment.
You should throw or call console.error.
There was a problem hiding this comment.
I will throw CKEditorError.
| * a view attribute element, which will be used for wrapping. | ||
| * @returns {Function} Add marker converter. | ||
| */ | ||
| export function wrapMarker( elementCreator ) { |
There was a problem hiding this comment.
What is the difference between wrap and wrapMarker? From the view point of view, there is nothing like marker to be wrapped.
There was a problem hiding this comment.
As far as name is concerned, I agree that the name could be different, i.e. wrapRange (while wrap could be changed to wrapItem, same for unwraps).
The functions (wrap and wrapMarker) could be merged. But there are couple of differences between them:
- Consumable type. It's taken from
eventNameToConsumableType( evt.name )and would be invalid forwrapMarker. This would have to change. dataobject passed to the converter is different. We can try to unify them a bit though.wrapgets:item,range,attributeKey,attributeOldValueandattributeNewValue.wrapMarkergetsrangeandname. We could adddata.itemtowrapMarkerdataand make it equal todata.range, but that's it.- There's difference if view element is created using "element creator function":
wrap:elementCreator( data.attributeNewValue, data, consumable, conversionApi );
wrapMarker:elementCreator( data, consumable, conversionApi );
It would have to be something like:
elementCreator( data.attributeNewValue || data.name, data, consumable, conversionApi );and still,elementCreatorfunctions would have to be different for converting items and ranges which means that we need to write about it in docs and it might be confusing. - Both converters have special behavior that are individual for them.
wraphas to "unwrap" part of view if attribute changed.wrapMarkerhas to support collapsed ranges. Those would be just twoifs standing next to each other, operating on different values and "active" depending ondatacontent. We can put them together in one function, but on the same basis we could put everything into one class.
All in all the merging would not be as seamless as it looks on first sight so I am against it. Let's change wrapMarker to wrapRange and wrap to wrapItem, same for unwrap and keep them separated.
There was a problem hiding this comment.
You're right. Maybe we should call them explicit: wrapInAttribute and wrapInMarker?
There was a problem hiding this comment.
And the documentation should stress the difference.
| // @param {engine.view.Position} viewPosition Position to start from when looking for furthest zero length position. | ||
| // @param {engine.conversion.Mapper} mapper Mapper to use when looking for furthest zero length position. | ||
| // @returns {engine.view.Position} Furthest zero length position. | ||
| function findFurthestZeroLengthPoint( viewPosition, mapper ) { |
There was a problem hiding this comment.
I will rename it to enlargeViewPosition and it will return a view.Range.
| // that passed ranges are contained within one container element). | ||
| // @param {engine.view.Range} viewRange Range to break. | ||
| // @returns {Array.<engine.view.Range>} Flat ranges that combines into passed `viewRange`. | ||
| function breakViewRangeOnFlat( viewRange ) { |
There was a problem hiding this comment.
For now, I will just move it. I thought about making this a default behavior of methods like viewWriter.remove or viewWriter.wrap but those methods return values and I think it would be too many changes in this PR.
|
Remember to fix API docs. This PR has been made before we reformatted them. |
| } | ||
| } | ||
| } | ||
| }, { priority: 'lowest' } ); |
There was a problem hiding this comment.
These two functions (for insert and move) should be moved to the separate functions like all other.
There was a problem hiding this comment.
I'll move them to model-to-view-converters.js.
| }, { priority: 'lowest' } ); | ||
|
|
||
| this.modelToView.on( 'move', ( evt, data ) => { | ||
| const pos = data.sourcePosition._getTransformedByInsertion( data.targetPosition, data.item.offsetSize ); |
There was a problem hiding this comment.
Why do you need to transform this position here?
There was a problem hiding this comment.
To make conversion easier, data.sourcePosition for move change is in state "before" operation happened. However, both data.targetPosition and markers state are already "after" operation. To check whether moved part of document was inside a range or was moved to the range, we need to transform sourcePosition.
It's only transforming by insertion, because move sourcePosition (in other words, start of moved range) could not change due to removing that range:
ab[cde]f <- if cde is moved, source position is after b and will stay there. Only change can happen if range is moved before letter b.
There was a problem hiding this comment.
Buuuuut, when I look at the conditions I am not sure why exactly I check if ( marker.range.start.isEqual( pos ) || marker.range.end.isEqual( pos ) ) {. I remember this condition went through some iterations.
There was a problem hiding this comment.
Checking with the start is because when you have a (marker) range and then you "cut" something out of it, the transformed range always starts or ends on sourcePosition. Examples:
ab[cd{e]f}g -> move {} -> ab[cd]^g -> sourcePosition will be after d
a{b[c}de]fg -> move {} -> a^[de]fg -> sourcePosition will be after a
That's why it makes sense to check marker.range.start and marker.range.end. But I will take a look if that loop cannot be simplified.
EDIT: OTOH a[bc{d}ef]g here it won't work this way. I will check this part again.
| * Stores range to marker name bindings for added ranges. | ||
| * | ||
| * @private | ||
| * @member {Map} #_markerNames |
There was a problem hiding this comment.
Why this is called "names" if it contains ranges?
There was a problem hiding this comment.
Because it keeps range => name mapping. It makes sense when you write:
const name = this._markerNames.get( range );. It does not contain ranges, ranges are keys.
There was a problem hiding this comment.
Since we'll implement getting, removing and updating markers by name, it makes sense to store name => range mapping now. I'll rename this property to _nameToRange.
| this.modelToView.on( 'insert', ( evt, data ) => { | ||
| const pos = data.range.start; | ||
|
|
||
| for ( let [ range, name ] of this.model.markers._markerNames ) { |
There was a problem hiding this comment.
You iterate here over the private property. I believe this.model.markers should be iterable and return [ name, range ].
There was a problem hiding this comment.
I'll make MarkersCollection iterable and it will return an object with name and range properties.
There was a problem hiding this comment.
Maybe the array is better to keep it consistent with https://github.com/ckeditor/ckeditor5-engine/blob/master/src/model/node.js#L299-L309
| this.modelToView.on( 'move', ( evt, data ) => { | ||
| const pos = data.sourcePosition._getTransformedByInsertion( data.targetPosition, data.item.offsetSize ); | ||
|
|
||
| for ( let [ range, name ] of this.model.markers._markerNames ) { |
…lgorithms are hooked.
…cumentChange, .createFromRanges.
…ters, now transformation is always "spreading" and "not sticky".
…e range if it was collapsed and insertion was at that position.
…eRange automatic transformation.
…ion was in an element which part was in moved range.
…ge to consumable values.
…leForRange to #_createConsumableForType.
| * @param {module:engine/model/liverange~LiveRange} newLiveRange Live range to be added. | ||
| * @returns {Boolean} `true` if `oldLiveRange` was found and changed, `false` otherwise. | ||
| */ | ||
| updateRange( oldLiveRange, newLiveRange ) { |
There was a problem hiding this comment.
Maybe it should be update( name, newLiveRange )?
There was a problem hiding this comment.
Will be update( nameOrRange, newLiveRange )
| * @param {module:engine/model/liverange~LiveRange} liveRange Live range to be added as a marker to markers collection. | ||
| * @param {String} markerName Name to be associated with given `liveRange`. | ||
| */ | ||
| addRange( liveRange, markerName ) { |
There was a problem hiding this comment.
It's markers collection why I'm adding range? I'm adding range with a name so marker (even addMarker event is fired). So addMarker would be better. But since there is nothing else I can add maybe simple add?
Also, it's more natural to have a name as the first parameter, so:
add( name, range )?
| } | ||
|
|
||
| this._markerNames.set( liveRange, markerName ); | ||
| this.fire( 'addMarker', markerName, Range.createFromRange( liveRange ) ); |
There was a problem hiding this comment.
As above, maybe simple add event like in the Collection class?
| * @param {module:engine/model/liverange~LiveRange} liveRange Live range to be removed from markers collection. | ||
| * @returns {Boolean} `true` is passed `liveRange` was found and removed from the markers collection, `false` otherwise. | ||
| */ | ||
| removeRange( liveRange ) { |
There was a problem hiding this comment.
As above. It would be also nice to remove the marker by name.
There was a problem hiding this comment.
Will be remove( nameOrRange )
|
|
||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
Will add get( name )
|
I have corrected all raised issues and updated docs. |
| const range = LiveRange.createFromRange( model.selection.getFirstRange() ); | ||
|
|
||
| ranges.push( range ); | ||
| model.markers.addRange( range, 'highlight:' + color ); |
There was a problem hiding this comment.
It looks like the manual tests is not updated.
| * {@link module:engine/conversion/modelconversiondispatcher~ModelConversionDispatcher}). | ||
| * | ||
| * modelDispatcher.on( 'addMarker:searchResult', wrapRange( new ViewAttributeElement( 'span', { class: 'searchResult' } ) ) ); | ||
| * |
There was a problem hiding this comment.
I would like to see here the difference between wrapItem and this, so no one in the future asks again can we merge these two converters. I would be nice to have it written directly: "In contrast to wrapItem...".
| * Markers are similar to adding and converting attributes on nodes. The difference is that attribute is connected to | ||
| * a given node (i.e. a character is bold no matter if it gets moved or content around it changes). Markers on the | ||
| * other hand are continuous ranges (i.e. if a character from inside of marker range is moved somewhere else, marker | ||
| * range is shrunk and the character does not have any attribute or information that it was in the marked range). |
There was a problem hiding this comment.
Also, what is most important, markers let you find quickly the marked range without searching through the whole document.
| } | ||
|
|
||
| /** | ||
| * Returns range for given name. |
| /** | ||
| * Returns range for given name. | ||
| * | ||
| * @private |
There was a problem hiding this comment.
Maybe this method could be public, it might be useful. Then update and remove could simply get a name.
There was a problem hiding this comment.
Or we could keep it simple and drop the possibility of updating or removing marker by the range and remove this method. We copy range very ofter and it is a potential problem that someone uses a copy of a range to remove or update a marker. KISS, let's use names.
There was a problem hiding this comment.
I don't agree. I understand that you requested additional possibility to pass name instead of range. That was backed up by arguments and made sense. But why remove existing feature that works, is tested and it's not like this is 100LOC.
There was a problem hiding this comment.
Still, range instance is a terrible ID. Also, it is not a point to introduce more features that we need at this stage. Then we need to maintain them, what is usually more costly than introducing them. If we are unsure we will need this feature we should not bring it.
| @@ -271,6 +273,36 @@ export function mergeContainers( position ) { | |||
| } | |||
|
|
|||
| /** | |||
| * Breaks given `range` on a set of {@link module:engine/view/range~Range ranges}, that each are contained within a | |||
| * {@link module:engine/view/containerelement~ContainerElement container element}. After `range` is broken, it's "pieces" can | |||
| * be used by {@link module:engine/view/writer~writer} (which expects that passed ranges are contained within on container element). | |||
There was a problem hiding this comment.
be used by {@link module:engine/view/writer~writer}
*writer.wrap?
(which expects that passed ranges are contained within on container element).
*one?
There was a problem hiding this comment.
Well, it applies to all methods in writer, so first one is correct, but the other is not.
There was a problem hiding this comment.
all methods in writer
Others then this one.
Docs: changes in docs.
Tests: Removed failing test for functionallity that is not possible at this moment to implement.
|
I fixed one more thing and then merged master into this. I also merged master to related branches in |
|
When testing this with |
|
Two followups: #740 and #741 |
This PR introduces markers implementation in CKEditor5 engine. Fixes ckeditor/ckeditor5#3856.
Other than that some changes and fixes has been done:
Mapperposition mapping events have changed, now callbacks receive position mapped by default algorithm (previously default mapping was a lowest priority callback). This way default mapping can be used in custom callbacks.LiveRangetransformation has been changed.LiveRangeis not sticky anymore (which was weird from UX perspective).Range.createFromRangeswas introduced, it glues multiple ranges into one. Useful after a range is transformed.view.Writer.moveandModelConversionDispatcher#convertMovewere fixed.Related PRs and follow-up issues will be linked in comments below.