Skip to content
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

Improve performance of getSelectedBlocks #17582

Merged
merged 13 commits into from
Dec 19, 2024

Conversation

filipsobol
Copy link
Member

@filipsobol filipsobol commented Dec 3, 2024

Suggested merge commit message (convention)

Other (engine): Improve performance of getSelectedBlocks when selection contains only one block element. Closes #17629.


This greatly improves how long it takes to make a selection over a big table.


Use Squash and merge, as it's a fairly simply change with too many commits.

@filipsobol filipsobol requested a review from scofalik December 3, 2024 12:53
@scofalik
Copy link
Contributor

scofalik commented Dec 3, 2024

Looks good and is a simple and safe fix.

However, since it only targets a specific scenario (one element in selection), I am afraid we may need to revisit it.

The problem with current implementation is that it only looks at elementEnd while it seems it could look at elementStart as well. If the treewalker traverses over elementStart and this is a top-most unvisited block element, we should "jump" over it, as we never return blocks inside blocks:

 * **Note:** `getSelectedBlocks()` returns blocks that are nested in other non-block elements
 * but will not return blocks nested in other blocks.

Of course, we should respect this special case that is mentioned later:

* **Special case**: Selection ignores first and/or last blocks if nothing (from user perspective) is selected in them.

Maybe it's even possible to limit the number of isUnvisitedTopBlock() calls with some smart observations, but what I proposed seems like a safe change as well. One difficulty is that, AFAIR, it is not possible currently to change TreeWalker#position. There is .skip() but it simply traverses the tree, so that's exactly what we want to avoid. We would need something like .jumpToPosition( position ) and this should basically reset the treewalker and set it to that position. Alternatively, we could create a new treewalker and overwrite the current one.

packages/ckeditor5-engine/src/model/treewalker.ts Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/model/selection.ts Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/model/selection.ts Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/model/selection.ts Outdated Show resolved Hide resolved
@filipsobol
Copy link
Member Author

I implemented the suggestions. The jumpTo will not prevent tree walkers from going out of boundary, which is best described here:

* Moves treewalker {@link #position} to provided `position`. Treewalker will
* continue traversing from that position.
*
* If the provided position is before the start boundary, the position will be
* set to the start boundary. If the provided position is after the end boundary,
* the position will be set to the end boundary.
* This is done to prevent the treewalker from traversing outside the boundaries.

@filipsobol filipsobol requested a review from scofalik December 11, 2024 15:10
Copy link
Contributor

@scofalik scofalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also apply mentioned changes in the view tree walker.

packages/ckeditor5-engine/src/model/treewalker.ts Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/model/treewalker.ts Outdated Show resolved Hide resolved
filipsobol and others added 2 commits December 16, 2024 16:48
Co-authored-by: Szymon Cofalik <s.cofalik@cksource.com>
@filipsobol filipsobol requested a review from scofalik December 16, 2024 15:56
scofalik
scofalik previously approved these changes Dec 19, 2024
packages/ckeditor5-engine/src/view/treewalker.ts Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/view/treewalker.ts Outdated Show resolved Hide resolved
packages/ckeditor5-engine/src/view/treewalker.ts Outdated Show resolved Hide resolved
@scofalik scofalik merged commit 2a114f4 into master Dec 19, 2024
3 of 9 checks passed
@scofalik scofalik deleted the improve-performance-of-getSelectedBlocks branch December 19, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve performance of getSelectedBlocks
2 participants