Improvements in insertNodes

[GermanJablo]

• In insertNodes I divided the process into two cases: All elements of the array are inline, or at least one element is not inline. That makes insertNodes about 10 lines longer, but much easier to reason with and refactor.
• I renamed $basicInsertStrategy to $wrapInlineNodes and moved it from clipboard to insertNodes. Now insertNodes and paste should not have different behaviors, which also made refactoring more difficult. Thanks to this, an incorrect test was detected and corrected: "'white-space: pre (no touchy) (2)".
• Tests about "disjoint sibling text node" were removed. Before the previous PR where I changed insertNodes, these tests verified that an error was thrown in that case. This does not happen now. However, the case with "disjoint sibling text node" has already been tested in the G Docs pasting suite.
• Instead of iterating over each node and inserting it, I defined an insertRangeAfter method in LexicalNode that can be used to let users define how the nodes should be pasted. This PR makes Modularize the paste process with a new INSERT_NODES_COMMAND #4900 obsolete as it offers a simpler solution.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 24, 2023 5:08pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 24, 2023 5:08pm

[GermanJablo]

locally sometimes some tests fail me due to this attribute that playwright uses. I don't know if it happens to any of you too.
It seems to be a bug:
microsoft/playwright#22004

[acywatson]

I'm good here - gonna let @zurfyx take a pass since he's more familiar with the new $insertNodes logic at the moment.

[zurfyx]

Thank you GermanJablo! Sorry for the late review

[zurfyx]

Do we have tests for the correct behavior of this? I think it's important to ensure that block + block will remain on future iterations.

[GermanJablo]

Yes, this case (block + text) and also the one you mention (block + block) are covered in the copy paste suite

[zurfyx]

Can you share some more thoughts on why we need this additional method on the API? I feel like insertAfter already covers most use cases and otherwise a spread (array) params like we have for append will do. The signature is also a bit confusing in my opinion, I understand the performance aspect of it but it's a hard trade-off.

For the sake of insertNodes I would personally have this function as a module level.

insertRangeAfter(node: LexicalNode, ...other params)

[GermanJablo]

Yes, two reasons.

1. First, it provides a way to modify the behavior of multiple nodes inserted after another. For example, in my case I want that when several blocks are pasted, the indent level of the initial block is added to all of them. This is not currently possible. Using PASTE_COMMAND would mean having to repeat hundreds of lines of code. See Modularize the paste process with a new INSERT_NODES_COMMAND #4900

2. Secondly, these methods offer a much more performant API than splice and append, optimized for the double linked list structure that uses lexical. See Deprecate splice, append, getChildIndex and getIndexWithinParent #5194

I don't recommend using a spread array param, since you almost always insert nodes that are already siblings anyway, and that would require an additional iteration loop.

I also wouldn't "hide" it inside the insertNodes module for internal use. This use case is not only useful for copy-paste. For example, in my application you can drag and drop a series of blocks to another position.

[zurfyx]

@GermanJablo - The two points you made are totally valid, but I also think that having an ergonomic API and backward compatibility to certain extent is very important.

In my opinion, linked lists are a fairly advanced CS concept and even with a good understanding of them, having firstToInsert and lastToInsert would still lead me to some trial and error to fully understand how the function works.

As for performance, I would take into account real-life use cases. insertNodes may be worth optimizing, a plugin which handles clipboard paste once every 5 min can probably do with a function that takes slightly longer to initialize. At the end of the day the cost is a linear O(n).

I propose the following to unblock this PR. We hide this function from the public API and I bring this and #5194 up for team discussion to gather more opinions.

[GermanJablo]

This change is backward compatible.

...other params would be O(n), but lastToInsert is O(1). Yes, in insertNodes you have to iterate all the nodes anyway. But this may not always be the case. A real life example of this is dragging consecutive blocks. firstToInsert and lastToInsert are obtained immediately from the anchor and focus of the selection.

I understand that ...other params may be a little easier to understand, but do you think that even with the JSDOC comments lastToInsert is difficult to understand?

[zurfyx]

Isn't the function implementation O(2n)? The while loop + for loop. I believe the additional for loop isn't necessary but in any case we're now talking about O(3n) which is the same order of magnitude.

IMO the function is not immediately obvious, it's a sort of slice based function and the preconditions are that the first and the last have to be children of the same parent. To me having insertAfter(node1, node2, node3) is more intuitive and iff optimal performance a must have I'd probably consider a utility that does this for you

$insertChildrenAfter(refNode: LexicalNode, parent: ElementNode);

[GermanJablo]

Actually, I haven't finished the function. I'm simply iterating and applying insertAfter on each node to save time. But the idea is to correct the implementation later.

My idea of the algorithm is:

• loop to check from firstToInsert to lastToInsert with 3 goals: (1) check that lastToInsert is a later sibling of firstToInsert, (2) calculate the distance between those two nodes (count++) , needed to update the size property of the newParent and oldParent and (3) update the parent property of the nodes in the range.

If we use ...params, you will be doing the same job + additionally:

• You need an additional iteration to store the nodes to be inserted in an array (plus memory space)
• modifying the size of newParent and oldParent N times.
• Modifying nextSibling and prevSibling 3N times (for each node moved, unnecessarily).

Yes, we are talking about the same order of magnitude. The 2 cases are O(N)... the difference is that in the first case fewer iterations are done, and also fewer things in the iterations. I wouldn't be surprised if the second implementation is 3-5X slower when there are many nodes.

Please correct me if I'm wrong

[zurfyx]

I would be more inclined to keep against $isTextNode and $isElementNode as it would give us type-safetiness, it's currently typed as any

[GermanJablo]

Or better yet: #5205
? 😅

[zurfyx]

Is this part of the command abstraction part? Like code blocks?

[GermanJablo]

In the current version of insertNodes it was pretending to be lexical/list agnostic, but it wasn't.
That's why one of the current conditions of isMergeable is (!firstBlock.isEmpty() || !$isRootOrShadowRoot(firstBlock.getParentOrThrow()));.

Surely anyone who reviews that will not understand that condition (in fact it's still hard for me lol), so I came clean with a more declarative condition.