From 691c8309118a6b87c9d1f4ca6147f4a0e57402d7 Mon Sep 17 00:00:00 2001 From: Flrande Date: Tue, 7 Nov 2023 11:02:22 +0800 Subject: [PATCH 1/4] fix(store): `deleteBlock` should delete children recursively by default --- packages/store/src/workspace/page.ts | 102 ++++++++++++++++----------- 1 file changed, 60 insertions(+), 42 deletions(-) diff --git a/packages/store/src/workspace/page.ts b/packages/store/src/workspace/page.ts index 5cb8b23d0848..885c1c42be7e 100644 --- a/packages/store/src/workspace/page.ts +++ b/packages/store/src/workspace/page.ts @@ -2,7 +2,8 @@ import { assertExists, Slot } from '@blocksuite/global/utils'; import { uuidv4 } from 'lib0/random.js'; import * as Y from 'yjs'; -import { BaseBlockModel, internalPrimitives } from '../schema/base.js'; +import type { BaseBlockModel } from '../schema/base.js'; +import { internalPrimitives } from '../schema/base.js'; import type { IdGenerator } from '../utils/id-generator.js'; import { assertValidChildren, @@ -562,51 +563,69 @@ export class Page extends Space { deleteBlock( model: BaseBlockModel, options: { - bringChildrenTo: 'parent' | BaseBlockModel | false; - } = { - bringChildrenTo: false, - } + bringChildrenTo?: BaseBlockModel; + } = {} ) { if (this.readonly) { console.error('cannot modify data in readonly mode'); return; } - const parent = this.getParent(model); - const index = parent?.children.indexOf(model) ?? -1; + const { bringChildrenTo } = options; - if (index > -1) { - parent?.children.splice(parent.children.indexOf(model), 1); - } - if (bringChildrenTo instanceof BaseBlockModel) { - model.children.forEach(child => { - this.schema.validate(child.flavour, bringChildrenTo.flavour); - }); - // When bring children to parent, insert children to the original position of model - if (bringChildrenTo === parent && index > -1) { - parent.children.splice(index, 0, ...model.children); - } else { - bringChildrenTo.children.push(...model.children); - } - } + + const yModel = this._yBlocks.get(model.id) as YBlock; + const yModelChildren = yModel.get('sys:children') as Y.Array; + + const parent = this.getParent(model); + assertExists(parent); + const yParent = this._yBlocks.get(parent.id) as YBlock; + const yParentChildren = yParent.get('sys:children') as Y.Array; + const modelIndex = yParentChildren.toArray().indexOf(model.id); this.transact(() => { - this._yBlocks.delete(model.id); + if (modelIndex > -1) { + yParentChildren.delete(modelIndex, 1); + } - if (parent) { - const yParent = this._yBlocks.get(parent.id) as YBlock; - const yChildren = yParent.get('sys:children') as Y.Array; + if (bringChildrenTo) { + // validate children flavour + model.children.forEach(child => { + this.schema.validate(child.flavour, bringChildrenTo.flavour); + }); - if (index > -1) { - yChildren.delete(index, 1); + if (bringChildrenTo.id === parent.id) { + // When bring children to parent, insert children to the original position of model + yParentChildren.insert(modelIndex, yModelChildren.toArray()); + } else { + const yBringChildrenTo = this._yBlocks.get( + bringChildrenTo.id + ) as YBlock; + const yBringChildrenToChildren = yBringChildrenTo.get( + 'sys:children' + ) as Y.Array; + yBringChildrenToChildren.push(yModelChildren.toArray()); } - if (bringChildrenTo instanceof BaseBlockModel) { - this.updateBlock(bringChildrenTo, { - children: bringChildrenTo.children, + } else { + // delete children recursively + const deleteChildren = (id: string) => { + const yBlock = this._yBlocks.get(id) as YBlock; + + const yChildren = yBlock.get('sys:children') as Y.Array; + yChildren.forEach(id => { + deleteChildren(id); }); - } - parent.childrenUpdated.emit(); + this._yBlocks.delete(id); + }; + + yModelChildren.forEach(id => { + deleteChildren(id); + }); } + + this._yBlocks.delete(model.id); + + parent.childrenUpdated.emit(); }); } @@ -703,18 +722,17 @@ export class Page extends Space { yChildren.forEach((id: string) => { const index = model.childMap.get(id); - if (Number.isInteger(index)) { - const hasChild = this._blockMap.has(id); - - if (!hasChild) { - visited.add(id); - this._handleYBlockAdd(visited, id); - } + assertExists(index); + const hasChild = this._blockMap.has(id); - model.children[index as number] = this._blockMap.get( - id - ) as BaseBlockModel; + if (!hasChild) { + visited.add(id); + this._handleYBlockAdd(visited, id); } + + model.children[index as number] = this._blockMap.get( + id + ) as BaseBlockModel; }); } From f790673c41e8eb5612e1004cb708cadc046e1bf1 Mon Sep 17 00:00:00 2001 From: Flrande Date: Tue, 7 Nov 2023 11:28:16 +0800 Subject: [PATCH 2/4] test(store): add test --- .../src/__tests__/workspace.unit.spec.ts | 247 +++++++++++++++++- 1 file changed, 242 insertions(+), 5 deletions(-) diff --git a/packages/store/src/__tests__/workspace.unit.spec.ts b/packages/store/src/__tests__/workspace.unit.spec.ts index c4fdb11b9963..0d65d3198492 100644 --- a/packages/store/src/__tests__/workspace.unit.spec.ts +++ b/packages/store/src/__tests__/workspace.unit.spec.ts @@ -435,23 +435,260 @@ describe('addBlock', () => { }); describe('deleteBlock', () => { - it('can delete single model', async () => { + it('delete children recursively by default', async () => { const page = await createTestPage(); - page.addBlock('affine:page', { - title: new page.Text(), + const pageId = page.addBlock('affine:page', {}); + const noteId = page.addBlock('affine:note', {}, pageId); + page.addBlock('affine:paragraph', {}, noteId); + page.addBlock('affine:paragraph', {}, noteId); + assert.deepEqual(serializeWorkspace(page.doc).spaces[spaceId].blocks, { + '0': { + 'prop:title': '', + 'sys:children': ['1'], + 'sys:flavour': 'affine:page', + 'sys:id': '0', + }, + '1': { + 'prop:background': '--affine-background-secondary-color', + 'prop:hidden': false, + 'prop:index': 'a0', + 'prop:xywh': '[0,0,800,95]', + 'sys:children': ['2', '3'], + 'sys:flavour': 'affine:note', + 'sys:id': '1', + }, + '2': { + 'prop:text': '', + 'prop:type': 'text', + 'sys:children': [], + 'sys:flavour': 'affine:paragraph', + 'sys:id': '2', + }, + '3': { + 'prop:text': '', + 'prop:type': 'text', + 'sys:children': [], + 'sys:flavour': 'affine:paragraph', + 'sys:id': '3', + }, }); + + const deletedModel = page.getBlockById('1') as BaseBlockModel; + page.deleteBlock(deletedModel); + assert.deepEqual(serializeWorkspace(page.doc).spaces[spaceId].blocks, { '0': { + 'prop:title': '', 'sys:children': [], 'sys:flavour': 'affine:page', 'sys:id': '0', + }, + }); + }); + + it('bring children to parent', async () => { + const page = await createTestPage(); + + const pageId = page.addBlock('affine:page', {}); + const noteId = page.addBlock('affine:note', {}, pageId); + const p1 = page.addBlock('affine:paragraph', {}, noteId); + page.addBlock('affine:paragraph', {}, p1); + page.addBlock('affine:paragraph', {}, p1); + + assert.deepEqual(serializeWorkspace(page.doc).spaces[spaceId].blocks, { + '0': { 'prop:title': '', + 'sys:children': ['1'], + 'sys:flavour': 'affine:page', + 'sys:id': '0', + }, + '1': { + 'prop:background': '--affine-background-secondary-color', + 'prop:hidden': false, + 'prop:index': 'a0', + 'prop:xywh': '[0,0,800,95]', + 'sys:children': ['2'], + 'sys:flavour': 'affine:note', + 'sys:id': '1', + }, + '2': { + 'prop:text': '', + 'prop:type': 'text', + 'sys:children': ['3', '4'], + 'sys:flavour': 'affine:paragraph', + 'sys:id': '2', + }, + '3': { + 'prop:text': '', + 'prop:type': 'text', + 'sys:children': [], + 'sys:flavour': 'affine:paragraph', + 'sys:id': '3', + }, + '4': { + 'prop:text': '', + 'prop:type': 'text', + 'sys:children': [], + 'sys:flavour': 'affine:paragraph', + 'sys:id': '4', }, }); - page.deleteBlock(page.root as BaseBlockModel); - assert.deepEqual(serializeWorkspace(page.doc).spaces[spaceId].blocks, {}); + const deletedModel = page.getBlockById('2') as BaseBlockModel; + const deletedModelParent = page.getBlockById('1') as BaseBlockModel; + page.deleteBlock(deletedModel, { + bringChildrenTo: deletedModelParent, + }); + + assert.deepEqual(serializeWorkspace(page.doc).spaces[spaceId].blocks, { + '0': { + 'prop:title': '', + 'sys:children': ['1'], + 'sys:flavour': 'affine:page', + 'sys:id': '0', + }, + '1': { + 'prop:background': '--affine-background-secondary-color', + 'prop:hidden': false, + 'prop:index': 'a0', + 'prop:xywh': '[0,0,800,95]', + 'sys:children': ['3', '4'], + 'sys:flavour': 'affine:note', + 'sys:id': '1', + }, + '3': { + 'prop:text': '', + 'prop:type': 'text', + 'sys:children': [], + 'sys:flavour': 'affine:paragraph', + 'sys:id': '3', + }, + '4': { + 'prop:text': '', + 'prop:type': 'text', + 'sys:children': [], + 'sys:flavour': 'affine:paragraph', + 'sys:id': '4', + }, + }); + }); + + it('bring children to other block', async () => { + const page = await createTestPage(); + + const pageId = page.addBlock('affine:page', {}); + const noteId = page.addBlock('affine:note', {}, pageId); + const p1 = page.addBlock('affine:paragraph', {}, noteId); + const p2 = page.addBlock('affine:paragraph', {}, noteId); + page.addBlock('affine:paragraph', {}, p1); + page.addBlock('affine:paragraph', {}, p1); + page.addBlock('affine:paragraph', {}, p2); + + assert.deepEqual(serializeWorkspace(page.doc).spaces[spaceId].blocks, { + '0': { + 'prop:title': '', + 'sys:children': ['1'], + 'sys:flavour': 'affine:page', + 'sys:id': '0', + }, + '1': { + 'prop:background': '--affine-background-secondary-color', + 'prop:hidden': false, + 'prop:index': 'a0', + 'prop:xywh': '[0,0,800,95]', + 'sys:children': ['2', '3'], + 'sys:flavour': 'affine:note', + 'sys:id': '1', + }, + '2': { + 'prop:text': '', + 'prop:type': 'text', + 'sys:children': ['4', '5'], + 'sys:flavour': 'affine:paragraph', + 'sys:id': '2', + }, + '3': { + 'prop:text': '', + 'prop:type': 'text', + 'sys:children': ['6'], + 'sys:flavour': 'affine:paragraph', + 'sys:id': '3', + }, + '4': { + 'prop:text': '', + 'prop:type': 'text', + 'sys:children': [], + 'sys:flavour': 'affine:paragraph', + 'sys:id': '4', + }, + '5': { + 'prop:text': '', + 'prop:type': 'text', + 'sys:children': [], + 'sys:flavour': 'affine:paragraph', + 'sys:id': '5', + }, + '6': { + 'prop:text': '', + 'prop:type': 'text', + 'sys:children': [], + 'sys:flavour': 'affine:paragraph', + 'sys:id': '6', + }, + }); + + const deletedModel = page.getBlockById('2') as BaseBlockModel; + const moveToModel = page.getBlockById('3') as BaseBlockModel; + page.deleteBlock(deletedModel, { + bringChildrenTo: moveToModel, + }); + + assert.deepEqual(serializeWorkspace(page.doc).spaces[spaceId].blocks, { + '0': { + 'prop:title': '', + 'sys:children': ['1'], + 'sys:flavour': 'affine:page', + 'sys:id': '0', + }, + '1': { + 'prop:background': '--affine-background-secondary-color', + 'prop:hidden': false, + 'prop:index': 'a0', + 'prop:xywh': '[0,0,800,95]', + 'sys:children': ['3'], + 'sys:flavour': 'affine:note', + 'sys:id': '1', + }, + '3': { + 'prop:text': '', + 'prop:type': 'text', + 'sys:children': ['6', '4', '5'], + 'sys:flavour': 'affine:paragraph', + 'sys:id': '3', + }, + '4': { + 'prop:text': '', + 'prop:type': 'text', + 'sys:children': [], + 'sys:flavour': 'affine:paragraph', + 'sys:id': '4', + }, + '5': { + 'prop:text': '', + 'prop:type': 'text', + 'sys:children': [], + 'sys:flavour': 'affine:paragraph', + 'sys:id': '5', + }, + '6': { + 'prop:text': '', + 'prop:type': 'text', + 'sys:children': [], + 'sys:flavour': 'affine:paragraph', + 'sys:id': '6', + }, + }); }); it('can delete model with parent', async () => { From 93f44757b339859423705342ddf46d03b9d0c4b1 Mon Sep 17 00:00:00 2001 From: Flrande Date: Wed, 8 Nov 2023 11:30:03 +0800 Subject: [PATCH 3/4] fix: fix --- .../blocks/src/_legacy/service/json2block.ts | 8 +++- packages/store/src/workspace/page.ts | 39 ++++++++++++------- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/packages/blocks/src/_legacy/service/json2block.ts b/packages/blocks/src/_legacy/service/json2block.ts index e8e404254bf9..993831fbd955 100644 --- a/packages/blocks/src/_legacy/service/json2block.ts +++ b/packages/blocks/src/_legacy/service/json2block.ts @@ -143,7 +143,9 @@ export async function json2block( assertExists(lastMergedModel); lastMergedModel.text?.join(splitSuffixModel.text as Text); - page.deleteBlock(splitSuffixModel); + page.deleteBlock(splitSuffixModel, { + deleteChildren: false, + }); } /** @@ -201,7 +203,9 @@ export async function json2block( } if (isFocusedBlockEmpty && !shouldMergeFirstBlock) { - page.deleteBlock(focusedBlockModel); + page.deleteBlock(focusedBlockModel, { + deleteChildren: false, + }); } } diff --git a/packages/store/src/workspace/page.ts b/packages/store/src/workspace/page.ts index 7bd87ff8d4f4..e01bee0033a8 100644 --- a/packages/store/src/workspace/page.ts +++ b/packages/store/src/workspace/page.ts @@ -567,14 +567,23 @@ export class Page extends Space { model: BaseBlockModel, options: { bringChildrenTo?: BaseBlockModel; - } = {} + deleteChildren?: boolean; + } = { + deleteChildren: true, + } ) { if (this.readonly) { console.error('cannot modify data in readonly mode'); return; } - const { bringChildrenTo } = options; + const { bringChildrenTo, deleteChildren } = options; + if (bringChildrenTo && deleteChildren) { + console.error( + 'Cannot bring children to another block and delete them at the same time' + ); + return; + } const yModel = this._yBlocks.get(model.id) as YBlock; const yModelChildren = yModel.get('sys:children') as Y.Array; @@ -609,21 +618,23 @@ export class Page extends Space { yBringChildrenToChildren.push(yModelChildren.toArray()); } } else { - // delete children recursively - const deleteChildren = (id: string) => { - const yBlock = this._yBlocks.get(id) as YBlock; + if (deleteChildren) { + // delete children recursively + const dl = (id: string) => { + const yBlock = this._yBlocks.get(id) as YBlock; - const yChildren = yBlock.get('sys:children') as Y.Array; - yChildren.forEach(id => { - deleteChildren(id); - }); + const yChildren = yBlock.get('sys:children') as Y.Array; + yChildren.forEach(id => { + dl(id); + }); - this._yBlocks.delete(id); - }; + this._yBlocks.delete(id); + }; - yModelChildren.forEach(id => { - deleteChildren(id); - }); + yModelChildren.forEach(id => { + dl(id); + }); + } } this._yBlocks.delete(model.id); From e5fa0de4b74ccbf26cd4577b9f54f6cc8b46d333 Mon Sep 17 00:00:00 2001 From: Flrande Date: Wed, 8 Nov 2023 15:13:58 +0800 Subject: [PATCH 4/4] fix: fix --- .../components/rich-text/markdown/utils.ts | 8 +- .../rich-text/rich-text-operations.ts | 4 +- packages/lit/src/utils/range-synchronizer.ts | 25 ++- tests/selection/native.spec.ts | 164 ++++++++++++++++-- 4 files changed, 175 insertions(+), 26 deletions(-) diff --git a/packages/blocks/src/_common/components/rich-text/markdown/utils.ts b/packages/blocks/src/_common/components/rich-text/markdown/utils.ts index fa82c2e5e48b..36e08005366a 100644 --- a/packages/blocks/src/_common/components/rich-text/markdown/utils.ts +++ b/packages/blocks/src/_common/components/rich-text/markdown/utils.ts @@ -47,7 +47,9 @@ export function convertToList( children: model.children, ...otherProperties, }; - page.deleteBlock(model); + page.deleteBlock(model, { + deleteChildren: false, + }); const id = page.addBlock('affine:list', blockProps, parent, index); asyncFocusRichText(page, id); @@ -78,7 +80,9 @@ export function convertToParagraph( text: model.text?.clone(), children: model.children, }; - page.deleteBlock(model); + page.deleteBlock(model, { + deleteChildren: false, + }); const id = page.addBlock('affine:paragraph', blockProps, parent, index); asyncFocusRichText(page, id); diff --git a/packages/blocks/src/_common/components/rich-text/rich-text-operations.ts b/packages/blocks/src/_common/components/rich-text/rich-text-operations.ts index 018a91e26819..9489df53bf3e 100644 --- a/packages/blocks/src/_common/components/rich-text/rich-text-operations.ts +++ b/packages/blocks/src/_common/components/rich-text/rich-text-operations.ts @@ -396,7 +396,9 @@ function handleListBlockBackspace(page: Page, model: ExtendedModel) { children: model.children, }; page.captureSync(); - page.deleteBlock(model); + page.deleteBlock(model, { + deleteChildren: false, + }); const id = page.addBlock('affine:paragraph', blockProps, parent, index); asyncFocusRichText(page, id); return true; diff --git a/packages/lit/src/utils/range-synchronizer.ts b/packages/lit/src/utils/range-synchronizer.ts index b58db4f13f78..32775794f899 100644 --- a/packages/lit/src/utils/range-synchronizer.ts +++ b/packages/lit/src/utils/range-synchronizer.ts @@ -164,19 +164,18 @@ export class RangeSynchronizer { endText.delete(0, to.length); startText.join(endText); } - }); - // make each delete operation in one transaction to ensure - // `deleteBlock` works correctly - // For example: - // aaa - // bbb - // In this case, if we delete `aaa` firstly, then delete `bbb`, - // the `deleteBlock` will fail when it delete `bbb` because `aaa` is already deleted - // but `deleteBlock` still try to get the parent of `bbb` which is `aaa` - blocks.slice(1).forEach(block => { - this.root.page.transact(() => { - this.root.page.deleteBlock(block.model); - }); + + blocks + .slice(1) + // delete from lowest to highest + .reverse() + .forEach(block => { + const parent = this.root.page.getParent(block.model); + assertExists(parent); + this.root.page.deleteBlock(block.model, { + bringChildrenTo: parent, + }); + }); }); const newSelection = this._selectionManager.getInstance('text', { diff --git a/tests/selection/native.spec.ts b/tests/selection/native.spec.ts index 05c5f25fff9c..9fac3bf685d2 100644 --- a/tests/selection/native.spec.ts +++ b/tests/selection/native.spec.ts @@ -39,6 +39,7 @@ import { pressShiftTab, pressTab, redoByKeyboard, + resetHistory, scrollToTop, selectAllByKeyboard, SHORT_KEY, @@ -101,27 +102,170 @@ test('native range delete', async ({ page }) => { test('native range delete with indent', async ({ page }) => { await enterPlaygroundRoom(page); - await initEmptyParagraphState(page); - await initThreeParagraphs(page); - await assertRichTexts(page, ['123', '456', '789']); + const { noteId } = await initEmptyParagraphState(page); + + await focusRichText(page); + await type(page, '123'); + await pressEnter(page); + await type(page, '456'); + await pressEnter(page); + await type(page, '789'); + await pressEnter(page); + await type(page, 'abc'); + await pressEnter(page); + await type(page, 'def'); + await pressEnter(page); + await type(page, 'ghi'); + await resetHistory(page); - // test indent case await focusRichText(page, 1); await pressTab(page); await focusRichText(page, 2); + await pressTab(page, 2); + await focusRichText(page, 4); await pressTab(page); - await pressTab(page); + await focusRichText(page, 5); + await pressTab(page, 2); + + // 123 + // 456 + // 789 + // abc + // def + // ghi + + await assertStoreMatchJSX( + page, + ` + + + + + + + + + + + +`, + noteId + ); + + await dragBetweenIndices(page, [0, 2], [4, 1]); + + // 12|3 + // 456 + // 789 + // abc + // d|ef + // ghi - await dragBetweenIndices(page, [0, 2], [2, 1]); await pressBackspace(page); - await assertBlockCount(page, 'paragraph', 1); - await assertRichTexts(page, ['1289']); + await assertStoreMatchJSX( + page, + ` + + + +`, + noteId + ); await waitNextFrame(page); await undoByKeyboard(page); - await assertRichTexts(page, ['123', '456', '789']); + + await assertStoreMatchJSX( + page, + ` + + + + + + + + + + + +`, + noteId + ); await redoByKeyboard(page); - await assertRichTexts(page, ['1289']); + await assertStoreMatchJSX( + page, + ` + + + +`, + noteId + ); }); test('native range delete by forwardDelete', async ({ page }) => {