-
Notifications
You must be signed in to change notification settings - Fork 40
T/1172-b Conversion refactor #1206
Conversation
…n)wrapping and merging.
…alidate()` method.
… `DocumentFragment` (in model and view).
…on()`. Changed: `conversion.Mapper` will not unbind elements that got rebound to other elements.
…red `conversion.ModelConversionDispatcher`.
src/conversion/mapper.js
Outdated
@@ -368,7 +379,7 @@ export default class Mapper { | |||
* We are in the text node so we can simple find the offset. | |||
* <p>fo<b>ba|r</b>bom</p> -> expected offset: 2, actual offset: 2 -> position found | |||
* | |||
* @private | |||
* @protected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be still private.
src/conversion/mapper.js
Outdated
* @returns {module:engine/view/position~Position} Corresponding view position. | ||
*/ | ||
toViewPosition( modelPosition ) { | ||
toViewPosition( modelPosition, isPhantom = false ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of isPhantom
-> options.isPhantom
.
const viewStart = conversionApi.mapper.toViewPosition( data.position ); | ||
|
||
const modelEnd = data.position.getShiftedBy( data.length ); | ||
const viewEnd = conversionApi.mapper.toViewPosition( modelEnd, true ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const viewEnd = conversionApi.mapper.toViewPosition( modelEnd, { isPhantom: true } );
would be more readable.
|
||
// If marker's range is not collapsed - consume all items inside. | ||
for ( const value of markerRange ) { | ||
if ( !consumable.consume( value.item, eventName ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why consume is removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some time there were no consumables and I forgot to bring back this part of code.
this.convertInsert( Range.createFromPositionAndShift( entry.position, entry.length ) ); | ||
} else if ( entry.type == 'remove' ) { | ||
this.convertRemove( entry.position, entry.length, entry.name ); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment missing.
src/model/differ.js
Outdated
// Check all changed elements. | ||
for ( const element of this._changesInElement.keys() ) { | ||
// If the element is inside other changed element, skip changes in this element. | ||
if ( this._isInsertedOrRemoved( element ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comment that it's all about children.
src/model/differ.js
Outdated
const changes = this._changesInElement.get( element ).sort( ( a, b ) => { | ||
if ( a.offset === b.offset ) { | ||
if ( a.type != b.type ) { | ||
return a.type == 'remove' ? -1 : 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment needed.
src/model/differ.js
Outdated
|
||
for ( const change of changes ) { | ||
if ( change.offset > offset ) { | ||
actions.push( ...'e'.repeat( change.offset - offset ).split( '' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment "fill holes between end of one change and beginning of another with 'e'".
// - 'e' for 'equal' - when item at that position did not change, | ||
// - 'i' for 'insert' - when item at that position was inserted, | ||
// - 'r' for 'remove' - when item at that position was removed, | ||
// - 'a' for 'attribute' - when item at that position has it attributes changed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A single sample would be great.
// Add diff item. | ||
diffs.push( { | ||
type: 'attribute', | ||
position: range.start, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's strange that I get both position and range.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
position
is useful for further sorting (as other changes has position
too), but I will remove it from the final results. BTW. attribute change item also has length
which is not used at all - I will remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that I was wrong, both length
and position
are used in further processing but also both are not really needed later so they can be removed before changes are returned by getChanges()
src/model/differ.js
Outdated
old.howMany = inc.offset - old.offset; | ||
|
||
// Unshift to prevent further processing of this change. | ||
changes.unshift( { type: 'attribute', offset: incEnd, howMany: howMany - old.howMany, count: this._changeCount++ } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment why you add change in the first place is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And since you unshift because you know that this function is called in the loop, the loop should be moved inside the function.
src/model/differ.js
Outdated
const howManyAfter = howMany - old.howMany - inc.howMany; | ||
|
||
// Unshift to prevent further processing of this change. | ||
changes.unshift( { type: 'attribute', offset: inc.offset, howMany: howManyAfter, count: this._changeCount++ } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
src/model/differ.js
Outdated
const changeItem = { type: 'attribute', offset: oldEnd, howMany: incEnd - oldEnd, count: this._changeCount++ }; | ||
|
||
for ( const oldChange of changes ) { | ||
this._handleChange( changeItem, oldChange, changes ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
src/model/differ.js
Outdated
} | ||
|
||
/** | ||
* Calculates diff between old model tree state (before all the buffered operations) and the new model tree state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(before all the buffered operations)
-> (since the last {@link #reset})
?
src/conversion/mapper.js
Outdated
* return; | ||
* } | ||
* | ||
* const sibling = modelPosition.nodeBefore; // Might crash for phantom position that does not exist in model. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modelPosition.nodeBefore
-> data.modelPosition.nodeBefore
?
} | ||
}; | ||
} | ||
|
||
/** | ||
* Function factory, creates a default model-to-view converter for removing {@link module:engine/view/uielement~UIElement ui element} | ||
* basing on marker remove change. | ||
* Function factory, creates a converter that converts model marker add/change/remove to the view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add, change?
* evt.stop(); | ||
* } ); | ||
* | ||
* Callback that "overrides" other callback: | ||
* Callback that overrides other callback: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove this sample, it is too complicated, after reading it I understand less than before. Instead you could add a regular convresion for attribute->wrap (like bold).
c51ce54
to
57d8e04
Compare
Suggested merge commit message (convention)
Other: Conversion refactoring. Introduced
model.Differ
. Changes now will be converted after all changes in a change block are done. Closes ckeditor/ckeditor5#4202.