From 14ed3077268a67b46ba6d63ce6cced300f605326 Mon Sep 17 00:00:00 2001 From: Filip Sobol Date: Fri, 29 Nov 2024 15:32:05 +0100 Subject: [PATCH] Other (list): Skip already visited list elements during reconversion and post fixing for better performance. --- .../ckeditor5-list/src/list/converters.ts | 11 +++++---- .../ckeditor5-list/src/list/listediting.ts | 13 +++++----- .../src/list/utils/postfixers.ts | 12 ++++++++-- .../tests/list/utils/postfixers.js | 24 ++++++++++++------- 4 files changed, 39 insertions(+), 21 deletions(-) diff --git a/packages/ckeditor5-list/src/list/converters.ts b/packages/ckeditor5-list/src/list/converters.ts index 033748da35b..7f8611e2442 100644 --- a/packages/ckeditor5-list/src/list/converters.ts +++ b/packages/ckeditor5-list/src/list/converters.ts @@ -138,32 +138,33 @@ export function reconvertItemsOnDataChange( const changes = model.document.differ.getChanges(); const itemsToRefresh = []; const itemToListHead = new Map(); + const visited = new Set(); const changedItems = new Set(); for ( const entry of changes ) { if ( entry.type == 'insert' && entry.name != '$text' ) { - findAndAddListHeadToMap( entry.position, itemToListHead ); + findAndAddListHeadToMap( entry.position, itemToListHead, visited ); // Insert of a non-list item. if ( !entry.attributes.has( 'listItemId' ) ) { - findAndAddListHeadToMap( entry.position.getShiftedBy( entry.length ), itemToListHead ); + findAndAddListHeadToMap( entry.position.getShiftedBy( entry.length ), itemToListHead, visited ); } else { changedItems.add( entry.position.nodeAfter! ); } } // Removed list item. else if ( entry.type == 'remove' && entry.attributes.has( 'listItemId' ) ) { - findAndAddListHeadToMap( entry.position, itemToListHead ); + findAndAddListHeadToMap( entry.position, itemToListHead, visited ); } // Changed list attribute. else if ( entry.type == 'attribute' ) { const item = entry.range.start.nodeAfter!; if ( attributeNames.includes( entry.attributeKey ) ) { - findAndAddListHeadToMap( entry.range.start, itemToListHead ); + findAndAddListHeadToMap( entry.range.start, itemToListHead, visited ); if ( entry.attributeNewValue === null ) { - findAndAddListHeadToMap( entry.range.start.getShiftedBy( 1 ), itemToListHead ); + findAndAddListHeadToMap( entry.range.start.getShiftedBy( 1 ), itemToListHead, visited ); // Check if paragraph should be converted from bogus to plain paragraph. if ( doesItemBlockRequiresRefresh( item as Element ) ) { diff --git a/packages/ckeditor5-list/src/list/listediting.ts b/packages/ckeditor5-list/src/list/listediting.ts index 39ca5726439..505128ae9a2 100644 --- a/packages/ckeditor5-list/src/list/listediting.ts +++ b/packages/ckeditor5-list/src/list/listediting.ts @@ -755,6 +755,7 @@ function modelChangePostFixer( listEditing: ListEditing ) { const changes = model.document.differ.getChanges(); + const visited = new Set(); const itemToListHead = new Map(); const multiBlock = listEditing.editor.config.get( 'list.multiBlock' ); @@ -775,30 +776,30 @@ function modelChangePostFixer( } } - findAndAddListHeadToMap( entry.position, itemToListHead ); + findAndAddListHeadToMap( entry.position, itemToListHead, visited ); // Insert of a non-list item - check if there is a list after it. if ( !entry.attributes.has( 'listItemId' ) ) { - findAndAddListHeadToMap( entry.position.getShiftedBy( entry.length ), itemToListHead ); + findAndAddListHeadToMap( entry.position.getShiftedBy( entry.length ), itemToListHead, visited ); } // Check if there is no nested list. for ( const { item: innerItem, previousPosition } of model.createRangeIn( item as Element ) ) { if ( isListItemBlock( innerItem ) ) { - findAndAddListHeadToMap( previousPosition, itemToListHead ); + findAndAddListHeadToMap( previousPosition, itemToListHead, visited ); } } } // Removed list item or block adjacent to a list. else if ( entry.type == 'remove' ) { - findAndAddListHeadToMap( entry.position, itemToListHead ); + findAndAddListHeadToMap( entry.position, itemToListHead, visited ); } // Changed list item indent or type. else if ( entry.type == 'attribute' && attributeNames.includes( entry.attributeKey ) ) { - findAndAddListHeadToMap( entry.range.start, itemToListHead ); + findAndAddListHeadToMap( entry.range.start, itemToListHead, visited ); if ( entry.attributeNewValue === null ) { - findAndAddListHeadToMap( entry.range.start.getShiftedBy( 1 ), itemToListHead ); + findAndAddListHeadToMap( entry.range.start.getShiftedBy( 1 ), itemToListHead, visited ); } } diff --git a/packages/ckeditor5-list/src/list/utils/postfixers.ts b/packages/ckeditor5-list/src/list/utils/postfixers.ts index 8484bf4e038..c7067217999 100644 --- a/packages/ckeditor5-list/src/list/utils/postfixers.ts +++ b/packages/ckeditor5-list/src/list/utils/postfixers.ts @@ -7,7 +7,7 @@ * @module list/list/utils/postfixers */ -import type { Position, Writer } from 'ckeditor5/src/engine.js'; +import type { Element, Position, Writer } from 'ckeditor5/src/engine.js'; import { SiblingListBlocksIterator, type ListIteratorValue } from './listwalker.js'; import { getListItemBlocks, isListItemBlock, ListItemUid, type ListElement } from './model.js'; @@ -17,10 +17,12 @@ import { getListItemBlocks, isListItemBlock, ListItemUid, type ListElement } fro * @internal * @param position The search starting position. * @param itemToListHead The map from list item element to the list head element. + * @param visited A set of elements that were already visited. */ export function findAndAddListHeadToMap( position: Position, - itemToListHead: Map + itemToListHead: Map, + visited: Set ): void { const previousNode = position.nodeBefore; @@ -42,6 +44,12 @@ export function findAndAddListHeadToMap( for ( const { node } of new SiblingListBlocksIterator( listHead, 'backward' ) ) { listHead = node; + if ( visited.has( listHead ) ) { + return; + } + + visited.add( listHead ); + if ( itemToListHead.has( listHead ) ) { return; } diff --git a/packages/ckeditor5-list/tests/list/utils/postfixers.js b/packages/ckeditor5-list/tests/list/utils/postfixers.js index 4921f915f88..980c7e2c1a9 100644 --- a/packages/ckeditor5-list/tests/list/utils/postfixers.js +++ b/packages/ckeditor5-list/tests/list/utils/postfixers.js @@ -43,8 +43,9 @@ describe( 'List - utils - postfixers', () => { const fragment = parseModel( input, schema ); const position = model.createPositionAt( fragment, 1 ); const itemToListHead = new Map(); + const visited = new Set(); - findAndAddListHeadToMap( position, itemToListHead ); + findAndAddListHeadToMap( position, itemToListHead, visited ); const heads = Array.from( itemToListHead.values() ); @@ -62,8 +63,9 @@ describe( 'List - utils - postfixers', () => { const fragment = parseModel( input, schema ); const position = model.createPositionAt( fragment, 2 ); const itemToListHead = new Map(); + const visited = new Set(); - findAndAddListHeadToMap( position, itemToListHead ); + findAndAddListHeadToMap( position, itemToListHead, visited ); const heads = Array.from( itemToListHead.values() ); @@ -81,8 +83,9 @@ describe( 'List - utils - postfixers', () => { const fragment = parseModel( input, schema ); const position = model.createPositionAt( fragment, 3 ); const itemToListHead = new Map(); + const visited = new Set(); - findAndAddListHeadToMap( position, itemToListHead ); + findAndAddListHeadToMap( position, itemToListHead, visited ); const heads = Array.from( itemToListHead.values() ); @@ -101,10 +104,11 @@ describe( 'List - utils - postfixers', () => { const fragment = parseModel( input, schema ); const position = model.createPositionAt( fragment, 3 ); const itemToListHead = new Map(); + const visited = new Set(); itemToListHead.set( fragment.getChild( 2 ), fragment.getChild( 1 ) ); - findAndAddListHeadToMap( position, itemToListHead ); + findAndAddListHeadToMap( position, itemToListHead, visited ); const heads = Array.from( itemToListHead.values() ); @@ -123,10 +127,11 @@ describe( 'List - utils - postfixers', () => { const fragment = parseModel( input, schema ); const position = model.createPositionAt( fragment, 4 ); const itemToListHead = new Map(); + const visited = new Set(); itemToListHead.set( fragment.getChild( 2 ), fragment.getChild( 1 ) ); - findAndAddListHeadToMap( position, itemToListHead ); + findAndAddListHeadToMap( position, itemToListHead, visited ); const heads = Array.from( itemToListHead.values() ); @@ -145,8 +150,9 @@ describe( 'List - utils - postfixers', () => { const fragment = parseModel( input, schema ); const position = model.createPositionAt( fragment, 4 ); const itemToListHead = new Map(); + const visited = new Set(); - findAndAddListHeadToMap( position, itemToListHead ); + findAndAddListHeadToMap( position, itemToListHead, visited ); const heads = Array.from( itemToListHead.values() ); @@ -166,8 +172,9 @@ describe( 'List - utils - postfixers', () => { const fragment = parseModel( input, schema ); const position = model.createPositionAt( fragment, 5 ); const itemToListHead = new Map(); + const visited = new Set(); - findAndAddListHeadToMap( position, itemToListHead ); + findAndAddListHeadToMap( position, itemToListHead, visited ); const heads = Array.from( itemToListHead.values() ); @@ -188,8 +195,9 @@ describe( 'List - utils - postfixers', () => { const fragment = parseModel( input, schema ); const position = model.createPositionAt( fragment, 3 ); const itemToListHead = new Map(); + const visited = new Set(); - findAndAddListHeadToMap( position, itemToListHead ); + findAndAddListHeadToMap( position, itemToListHead, visited ); const heads = Array.from( itemToListHead.values() );