-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
I/8054: Model#insertContent should use fewer operations #8773
Conversation
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.
LGTM.
Minor comments mostly on code style and requests for better in-code comments.
I had some doubts but I think I eventually understood everything correctly. Please clarify where I commented.
handleNodes( nodes, parentContext ) { | ||
nodes = Array.from( nodes ); | ||
handleNodes( nodes ) { | ||
for ( const node of Array.from( nodes ) ) { |
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.
If nodes
param is iterable, you shouldn't need to make an array out of 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.
Those nodes are inserted one by one to the temporary document fragment (it removes them from the original collection) so it invalidates the iterator.
// <p>x^y</p> + <p>z</p> => <p>x</p>^<p>y</p> + <p>z</p> => <p>x</p><p>z</p><p>y</p> => <p>xz[]y</p> | ||
// but: | ||
// <p>x</p><p>^</p><p>z</p> + <p>y</p> => <p>x</p><p>y</p><p>z</p> (no merging) | ||
// <p>x</p>[<img>]<p>z</p> + <p>y</p> => <p>x</p><p>y</p><p>z</p> (no merging, note: after running deleteContents |
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.
In Insertion
constructor you only pass position. I am not sure if it makes sense to describe this scenario here. I understand that you wanted to show other examples but it might be even confusing.
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.
This was moved from the previous location, I'll add this to one of the merging methods.
if ( this._documentFragment.getChild( 0 ) == this._firstNode ) { | ||
this.writer.insert( this._firstNode, this.position ); | ||
this.position = livePosition.toPosition(); | ||
} | ||
|
||
// Insert the remaining nodes from document fragment. | ||
if ( !this._documentFragment.isEmpty ) { | ||
this.writer.insert( this._documentFragment, this.position ); | ||
} |
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.
Okay, so I finally understood that this whole fragment is like that so we have an insert operation only for the really first node that I ever inserted in the whole insertContent process, because only the first node can be merged.
I didn't understand why we check if if ( this._documentFragment.getChild( 0 ) == this._firstNode ) {
instead of doing it always but then I realized that there can be multiple calls to _insertPartialFragment()
during one insertion. But then I didn't understand why it always checks with the very first node, but now I understand that's because only the very first node can be merged.
Yeah this is optimal but a little confusing.
How about changing the comment to:
// If the very first node of the whole insertion process is inserted, insert it separately for OT reasons (undo).
// Note: there can be multiple calls to `_insertPartialFragment()` during one insertion process.
// Note: only the very first node can be merged so we have to do separate operation only for 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.
👍
|
||
this.writer.merge( mergePosLeft ); | ||
if ( this._firstNode === this._lastNode ) { | ||
this._firstNode = this._lastNode = mergePosLeft.nodeBefore; |
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.
Please do it in two lines of code :).
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.
But actually, I wonder... Why/when we want to change _firstNode
? Isn't it that we want to merge only the very first node? Even if we do some splitting etc.?
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.
Okay, I think I finally got it. Is it a case of:
<p>A^A</p>
paste <p>X</p>
<p>A</p>^<p>A</p>
<p>A</p><p>X</p><p>A</p>
<p>AX</p><p>A</p>
<p>AXA</p>
?
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 in this case we would have keep references to wrong (removed due to merge) node.
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.
👍
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 added a comment about that.
// <p>x</p>[]<p>y</p> => <p>x[]</p><p>y</p> | ||
this.position = Position._createAt( mergePosRight.nodeBefore, 'end' ); | ||
|
||
// OK: <p>xx[]</p> + <p>yy</p> => <p>xx[]yy</p> (when sticks to previous) |
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 know this is old comment, but I'd add this before it, because I didn't know what this example talks about:
// Explanation of setting position stickiness to `'toPrevious'`:
// NOK: <p>xx[]</p> + <p>yy</p> => <p>xxyy[]</p> (when sticks to next) | ||
const livePosition = LivePosition.fromPosition( this.position, 'toPrevious' ); | ||
|
||
// See comment above on moving `_affectedStart`. |
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.
Change this to "See comment in mergeSiblingsOfFirstNode()
...".
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.
BTW. shouldn't this be called mergeSiblingOfFirstNode()
(no "s") or simply mergeOnLeft()
?
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.
👍
Waiting for feedeback from @ckeditor/qa-team |
I've found a regression in Track Changes:
3. Copy it and paste at the end of the first paragraph.
|
This was caused by a changed order of operations, before this change it was:
In this PR:
I've just pushed the fix for that and for other review comments. |
* <p>x^y</p> + <p>z</p> => <p>x</p>^<p>y</p> + <p>z</p> => <p>x</p><p>z</p><p>y</p> => <p>xz[]y</p> | ||
* but: | ||
* <p>x</p><p>^</p><p>z</p> + <p>y</p> => <p>x</p><p>y</p><p>z</p> (no merging) | ||
* <p>x</p>[<img>]<p>z</p> + <p>y</p> => <p>x</p><p>y</p><p>z</p> (no merging, note: after running deleteContents |
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 still don't think this should be here. This is just incorrect example, it shouldn't be here. Insertion
class does not operate on a range.
I think this whole comment (I mean all three examples without the last) should be somewhere else (handleNodes()
? or Insertion
class description?).
Suggested merge commit message (convention)
Other (engine): Optimized
Model#insertContent()
to use as few operations as possible to reduce the time needed to handle pasting large content into the editor. Closes #8054. Closes #715.Additional information