Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Commit

Permalink
Merge pull request #1347 from ckeditor/t/1226
Browse files Browse the repository at this point in the history 
Other: Refactored how markers removal is converted from the model to the view. Closes #1226.
  • Loading branch information
Piotr Jasiun authored Mar 16, 2018
2 parents 651cc58 + a721f95 commit f6de5f5
Show file tree
Hide file tree
Showing 19 changed files with 985 additions and 576 deletions.
82 changes: 7 additions & 75 deletions src/controller/editingcontroller.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,9 @@

import RootEditableElement from '../view/rooteditableelement';
import View from '../view/view';
import ViewWriter from '../view/writer';
import Mapper from '../conversion/mapper';
import DowncastDispatcher from '../conversion/downcastdispatcher';
import {
insertText,
remove
} from '../conversion/downcast-converters';
import { insertText, remove } from '../conversion/downcast-converters';
import { convertSelectionChange } from '../conversion/upcast-selection-converters';
import {
convertRangeSelection,
Expand All @@ -28,7 +24,7 @@ import mix from '@ckeditor/ckeditor5-utils/src/mix';

/**
* Controller for the editing pipeline. The editing pipeline controls {@link ~EditingController#model model} rendering,
* including selection handling. It also creates the {@link ~EditingController#view view document} which builds a
* including selection handling. It also creates the {@link ~EditingController#view view} which builds a
* browser-independent virtualization over the DOM elements. The editing controller also attaches default converters.
*
* @mixes module:utils/observablemixin~ObservableMixin
Expand All @@ -41,7 +37,7 @@ export default class EditingController {
*/
constructor( model ) {
/**
* Editing model.
* Editor model.
*
* @readonly
* @member {module:engine/model/model~Model}
Expand Down Expand Up @@ -78,14 +74,17 @@ export default class EditingController {
const selection = doc.selection;
const markers = this.model.markers;

// Whenever model document is changed, convert those changes to the view (using model.Document#differ).
// Do it on 'low' priority, so changes are converted after other listeners did their job.
// Also convert model selection.
this.listenTo( doc, 'change', () => {
this.view.change( writer => {
this.downcastDispatcher.convertChanges( doc.differ, writer );
this.downcastDispatcher.convertSelection( selection, markers, writer );
} );
}, { priority: 'low' } );

// Convert selection from view to model.
// Convert selection from the view to the model when it changes in the view.
this.listenTo( this.view.document, 'selectionChange', convertSelectionChange( this.model, this.mapper ) );

// Attach default model converters.
Expand All @@ -97,53 +96,6 @@ export default class EditingController {
this.downcastDispatcher.on( 'selection', convertRangeSelection(), { priority: 'low' } );
this.downcastDispatcher.on( 'selection', convertCollapsedSelection(), { priority: 'low' } );

// Convert markers removal.
//
// Markers should be removed from the view before changes to the model are applied. This is because otherwise
// it would be impossible to map some markers to the view (if, for example, the marker's boundary parent got removed).
//
// `removedMarkers` keeps information which markers already has been removed to prevent removing them twice.
const removedMarkers = new Set();

// We don't want to render view when markers are converted, so we need to create view writer
// manually instead of using `View#change` block. See https://github.com/ckeditor/ckeditor5-engine/issues/1323.
const viewWriter = new ViewWriter( this.view.document );

this.listenTo( model, 'applyOperation', ( evt, args ) => {
// Before operation is applied...
const operation = args[ 0 ];

for ( const marker of model.markers ) {
// Check all markers, that aren't already removed...
if ( removedMarkers.has( marker.name ) ) {
continue;
}

const markerRange = marker.getRange();

if ( _operationAffectsMarker( operation, marker ) ) {
// And if the operation in any way modifies the marker, remove the marker from the view.
removedMarkers.add( marker.name );
this.downcastDispatcher.convertMarkerRemove( marker.name, markerRange, viewWriter );
// TODO: This stinks but this is the safest place to have this code.
this.model.document.differ.bufferMarkerChange( marker.name, markerRange, markerRange );
}
}
}, { priority: 'high' } );

// If an existing marker is updated through `model.Model#markers` directly (not through operation), just remove it.
this.listenTo( model.markers, 'update', ( evt, marker, oldRange ) => {
if ( oldRange && !removedMarkers.has( marker.name ) ) {
removedMarkers.add( marker.name );
this.downcastDispatcher.convertMarkerRemove( marker.name, oldRange, viewWriter );
}
} );

// When all changes are done, clear `removedMarkers` set.
this.listenTo( model, '_change', () => {
removedMarkers.clear();
}, { priority: 'low' } );

// Binds {@link module:engine/view/document~Document#roots view roots collection} to
// {@link module:engine/model/document~Document#roots model roots collection} so creating
// model root automatically creates corresponding view root.
Expand Down Expand Up @@ -174,23 +126,3 @@ export default class EditingController {
}

mix( EditingController, ObservableMixin );

// Helper function which checks whether given operation will affect given marker after the operation is applied.
function _operationAffectsMarker( operation, marker ) {
const range = marker.getRange();

if ( operation.type == 'insert' || operation.type == 'rename' ) {
return _positionAffectsRange( operation.position, range );
} else if ( operation.type == 'move' || operation.type == 'remove' || operation.type == 'reinsert' ) {
return _positionAffectsRange( operation.targetPosition, range ) || _positionAffectsRange( operation.sourcePosition, range );
} else if ( operation.type == 'marker' && operation.name == marker.name ) {
return true;
}

return false;
}

// Helper function which checks whether change at given position affects given range.
function _positionAffectsRange( position, range ) {
return range.containsPosition( position ) || !range.start._getTransformedByInsertion( position, 1, true ).isEqual( range.start );
}
133 changes: 60 additions & 73 deletions src/conversion/downcast-converters.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -517,15 +517,16 @@ export function remove() {

/**
* Function factory, creates a converter that converts marker adding change to the view ui element.
* The view ui element that will be added to the view depends on passed parameter. See {@link ~insertElement}.
* In a case of collapsed range element will not wrap range but separate elements will be placed at the beginning
*
* The view ui element, that will be added to the view, depends on passed parameter. See {@link ~insertElement}.
* In a case of a non-collapsed range, the ui element will not wrap nodes but separate elements will be placed at the beginning
* and at the end of the range.
*
* **Note:** unlike {@link ~insertElement}, the converter does not bind view element to model, because this converter
* uses marker as "model source of data". This means that view ui element does not have corresponding model element.
* This converter binds created {@link module:engine/view/uielement~UIElement}s with marker name using
* the {@link module:engine/conversion/mapper~Mapper#bindElementToMarker}.
*
* @param {module:engine/view/uielement~UIElement|Function} elementCreator View ui element, or function returning a view element, which
* will be inserted.
* @param {module:engine/view/uielement~UIElement|Function} elementCreator View ui element or a function returning a view element
* which will be inserted.
* @returns {Function} Insert element event converter.
*/
export function insertUIElement( elementCreator ) {
Expand Down Expand Up @@ -563,50 +564,42 @@ export function insertUIElement( elementCreator ) {

// Add "opening" element.
viewWriter.insert( mapper.toViewPosition( markerRange.start ), viewStartElement );
conversionApi.mapper.bindElementToMarker( viewStartElement, data.markerName );

// Add "closing" element only if range is not collapsed.
if ( !markerRange.isCollapsed ) {
viewWriter.insert( mapper.toViewPosition( markerRange.end ), viewEndElement );
conversionApi.mapper.bindElementToMarker( viewEndElement, data.markerName );
}

evt.stop();
};
}

/**
* Function factory, creates a default downcast converter for removing {@link module:engine/view/uielement~UIElement ui element}
* Function factory, returns a default downcast converter for removing {@link module:engine/view/uielement~UIElement ui element}
* basing on marker remove change.
*
* @param {module:engine/view/uielement~UIElement|Function} elementCreator View ui element, or function returning
* a view ui element, which will be used as a pattern when look for element to remove at the marker start position.
* This converter unbinds elements from marker name.
*
* @returns {Function} Remove ui element converter.
*/
export function removeUIElement( elementCreator ) {
export function removeUIElement() {
return ( evt, data, conversionApi ) => {
// Create two view elements. One will be used to remove "opening element", the other for "closing element".
// If marker was collapsed, only "opening" element will be removed.
data.isOpening = true;
const viewStartElement = elementCreator( data, conversionApi.writer );

data.isOpening = false;
const viewEndElement = elementCreator( data, conversionApi.writer );
const elements = conversionApi.mapper.markerNameToElements( data.markerName );

if ( !viewStartElement || !viewEndElement ) {
if ( !elements ) {
return;
}

const markerRange = data.markerRange;
conversionApi.mapper.unbindElementsFromMarkerName( data.markerName );

const viewWriter = conversionApi.writer;

// When removing the ui elements, we map the model range to view twice, because that view range
// may change after the first clearing.
if ( !markerRange.isCollapsed ) {
viewWriter.clear( conversionApi.mapper.toViewRange( markerRange ).getEnlarged(), viewEndElement );
for ( const element of elements ) {
viewWriter.clear( ViewRange.createOn( element ), element );
}

// Remove "opening" element.
viewWriter.clear( conversionApi.mapper.toViewRange( markerRange ).getEnlarged(), viewStartElement );

evt.stop();
};
}
Expand Down Expand Up @@ -778,6 +771,9 @@ export function wrap( elementCreator ) {
*
* If the highlight descriptor will not provide `id` property, name of the marker will be used.
*
* This converter binds created {@link module:engine/view/attributeelement~AttributeElement}s with marker name using
* the {@link module:engine/conversion/mapper~Mapper#bindElementToMarker}.
*
* @param {module:engine/conversion/downcast-converters~HighlightDescriptor|Function} highlightDescriptor
* @return {Function}
*/
Expand Down Expand Up @@ -809,7 +805,17 @@ export function highlightText( highlightDescriptor ) {
viewWriter.wrap( viewSelection.getFirstRange(), viewElement, viewSelection );
} else {
const viewRange = conversionApi.mapper.toViewRange( data.range );
viewWriter.wrap( viewRange, viewElement );
const rangeAfterWrap = viewWriter.wrap( viewRange, viewElement );

for ( const element of rangeAfterWrap.getItems() ) {
if ( element.is( 'attributeElement' ) && element.isSimilar( viewElement ) ) {
conversionApi.mapper.bindElementToMarker( element, data.markerName );

// One attribute element is enough, because all of them are bound together by the view writer.
// Mapper uses this binding to get all the elements no matter how many of them are registered in the mapper.
break;
}
}
}
};
}
Expand All @@ -828,6 +834,9 @@ export function highlightText( highlightDescriptor ) {
*
* If the highlight descriptor will not provide `id` property, name of the marker will be used.
*
* This converter binds altered {@link module:engine/view/containerelement~ContainerElement}s with marker name using
* the {@link module:engine/conversion/mapper~Mapper#bindElementToMarker}.
*
* @param {module:engine/conversion/downcast-converters~HighlightDescriptor|Function} highlightDescriptor
* @return {Function}
*/
Expand Down Expand Up @@ -863,14 +872,16 @@ export function highlightElement( highlightDescriptor ) {
}

viewElement.getCustomProperty( 'addHighlight' )( viewElement, descriptor, conversionApi.writer );

conversionApi.mapper.bindElementToMarker( viewElement, data.markerName );
}
};
}

/**
* Function factory, creates a converter that converts model marker remove to the view.
* Function factory, creates a converter that converts removing model marker to the view.
*
* Both text nodes and elements are handled by this converter by they are handled a bit differently.
* Both text nodes and elements are handled by this converter but they are handled a bit differently.
*
* Text nodes are unwrapped using {@link module:engine/view/attributeelement~AttributeElement} created from provided
* highlight descriptor. See {link module:engine/conversion/downcast-converters~highlightDescriptorToAttributeElement}.
Expand All @@ -886,6 +897,8 @@ export function highlightElement( highlightDescriptor ) {
*
* If the highlight descriptor will not provide `id` property, name of the marker will be used.
*
* This converter unbinds elements from marker name.
*
* @param {module:engine/conversion/downcast-converters~HighlightDescriptor|Function} highlightDescriptor
* @return {Function}
*/
Expand All @@ -902,33 +915,28 @@ export function removeHighlight( highlightDescriptor ) {
return;
}

const viewRange = conversionApi.mapper.toViewRange( data.markerRange );

// Retrieve all items in the affected range. We will process them and remove highlight from them appropriately.
const items = new Set( viewRange.getItems() );
// View element that will be used to unwrap `AttributeElement`s.
const viewHighlightElement = createViewElementFromHighlightDescriptor( descriptor );

// First, iterate through all items and remove highlight from those container elements that have custom highlight handling.
for ( const item of items ) {
if ( item.is( 'containerElement' ) && item.getCustomProperty( 'removeHighlight' ) ) {
item.getCustomProperty( 'removeHighlight' )( item, descriptor.id, conversionApi.writer );
// Get all elements bound with given marker name.
const elements = conversionApi.mapper.markerNameToElements( data.markerName );

// If container element had custom handling, remove all it's children from further processing.
for ( const descendant of ViewRange.createIn( item ) ) {
items.delete( descendant );
}
}
if ( !elements ) {
return;
}

// Then, iterate through all other items. Look for text nodes and unwrap them. Start from the end
// to prevent errors when view structure changes when unwrapping (and, for example, some attributes are merged).
const viewHighlightElement = createViewElementFromHighlightDescriptor( descriptor );
const viewWriter = conversionApi.writer;
conversionApi.mapper.unbindElementsFromMarkerName( data.markerName );

for ( const item of Array.from( items ).reverse() ) {
if ( item.is( 'textProxy' ) ) {
viewWriter.unwrap( ViewRange.createOn( item ), viewHighlightElement );
for ( const element of elements ) {
if ( element.is( 'attributeElement' ) ) {
conversionApi.writer.unwrap( ViewRange.createOn( element ), viewHighlightElement );
} else {
// if element.is( 'containerElement' ).
element.getCustomProperty( 'removeHighlight' )( element, descriptor.id, conversionApi.writer );
}
}

evt.stop();
};
}

Expand Down Expand Up @@ -962,10 +970,10 @@ function _prepareDescriptor( highlightDescriptor, data, conversionApi ) {
* is not provided in descriptor - default priority will be used.
*
* @param {module:engine/conversion/downcast-converters~HighlightDescriptor} descriptor
* @return {module:engine/conversion/downcast-converters~HighlightAttributeElement}
* @returns {module:engine/view/attributeelement~AttributeElement}
*/
export function createViewElementFromHighlightDescriptor( descriptor ) {
const viewElement = new HighlightAttributeElement( 'span', descriptor.attributes );
const viewElement = new ViewAttributeElement( 'span', descriptor.attributes );

if ( descriptor.class ) {
viewElement._addClass( descriptor.class );
Expand All @@ -975,32 +983,11 @@ export function createViewElementFromHighlightDescriptor( descriptor ) {
viewElement._priority = descriptor.priority;
}

viewElement._setCustomProperty( 'highlightDescriptorId', descriptor.id );
viewElement._id = descriptor.id;

return viewElement;
}

/**
* Special kind of {@link module:engine/view/attributeelement~AttributeElement} that is created and used in
* marker-to-highlight conversion.
*
* The difference between `HighlightAttributeElement` and {@link module:engine/view/attributeelement~AttributeElement}
* is {@link module:engine/view/attributeelement~AttributeElement#isSimilar} method.
*
* For `HighlightAttributeElement` it checks just `highlightDescriptorId` custom property, that is set during marker-to-highlight
* conversion basing on {@link module:engine/conversion/downcast-converters~HighlightDescriptor} object.
* `HighlightAttributeElement`s with same `highlightDescriptorId` property are considered similar.
*/
class HighlightAttributeElement extends ViewAttributeElement {
isSimilar( otherElement ) {
if ( otherElement.is( 'attributeElement' ) ) {
return this.getCustomProperty( 'highlightDescriptorId' ) === otherElement.getCustomProperty( 'highlightDescriptorId' );
}

return false;
}
}

/**
* Object describing how the marker highlight should be represented in the view.
*
Expand Down
7 changes: 6 additions & 1 deletion src/conversion/downcastdispatcher.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,11 @@ export default class DowncastDispatcher {
* @param {module:engine/view/writer~Writer} writer View writer that should be used to modify view document.
*/
convertChanges( differ, writer ) {
// Before the view is updated, remove markers which have changed.
for ( const change of differ.getMarkersToRemove() ) {
this.convertMarkerRemove( change.name, change.range, writer );
}

// Convert changes that happened on model tree.
for ( const entry of differ.getChanges() ) {
if ( entry.type == 'insert' ) {
Expand All @@ -135,7 +140,7 @@ export default class DowncastDispatcher {
}
}

// After the view is updated, convert markers which has changed.
// After the view is updated, convert markers which have changed.
for ( const change of differ.getMarkersToAdd() ) {
this.convertMarkerAdd( change.name, change.range, writer );
}
Expand Down
Loading

0 comments on commit f6de5f5

Please sign in to comment.