From 2df8f29b6486910535e05e4ca40d1b91b848871d Mon Sep 17 00:00:00 2001 From: Peng Xiao Date: Thu, 22 Feb 2024 09:37:59 +0000 Subject: [PATCH] refactor(core): use fractional indexing for sorting (#5809) use https://github.com/rocicorp/fractional-indexing to enable better sorting logic for crdt app --- packages/frontend/core/package.json | 1 + .../page-properties-manager.ts | 26 +++- .../affine/page-properties/table.tsx | 130 +++++++++++------- .../page-properties/tags-inline-editor.css.ts | 2 +- .../core/src/hooks/use-affine-adapter.ts | 27 ++-- .../modules/workspace/properties/schema.ts | 2 +- .../affine-local/e2e/page-properties.spec.ts | 42 ++++++ yarn.lock | 1 + 8 files changed, 157 insertions(+), 74 deletions(-) diff --git a/packages/frontend/core/package.json b/packages/frontend/core/package.json index dd0ca9739..0bac4ff59 100644 --- a/packages/frontend/core/package.json +++ b/packages/frontend/core/package.json @@ -61,6 +61,7 @@ "cssnano": "^6.0.1", "dayjs": "^1.11.10", "foxact": "^0.2.20", + "fractional-indexing": "^3.2.0", "graphql": "^16.8.1", "idb": "^8.0.0", "image-blob-reduce": "^4.1.0", diff --git a/packages/frontend/core/src/components/affine/page-properties/page-properties-manager.ts b/packages/frontend/core/src/components/affine/page-properties/page-properties-manager.ts index 1e929c2d3..bbc173168 100644 --- a/packages/frontend/core/src/components/affine/page-properties/page-properties-manager.ts +++ b/packages/frontend/core/src/components/affine/page-properties/page-properties-manager.ts @@ -6,6 +6,7 @@ import type { } from '@affine/core/modules/workspace/properties/schema'; import { PagePropertyType } from '@affine/core/modules/workspace/properties/schema'; import { DebugLogger } from '@affine/debug'; +import { generateKeyBetween } from 'fractional-indexing'; import { nanoid } from 'nanoid'; import { getDefaultIconName } from './icons-mapping'; @@ -216,16 +217,13 @@ export class PagePropertiesManager { } getOrderedCustomProperties() { - return Object.values(this.getCustomProperties()).sort( - (a, b) => a.order - b.order + return Object.values(this.getCustomProperties()).sort((a, b) => + a.order > b.order ? 1 : a.order < b.order ? -1 : 0 ); } largestOrder() { - return Math.max( - ...Object.values(this.properties.custom).map(p => p.order), - 0 - ); + return this.getOrderedCustomProperties().at(-1)?.order ?? null; } getCustomPropertyMeta(id: string): PageInfoCustomPropertyMeta | undefined { @@ -247,7 +245,7 @@ export class PagePropertiesManager { return; } - const newOrder = this.largestOrder() + 1; + const newOrder = generateKeyBetween(this.largestOrder(), null); if (this.properties.custom[id]) { logger.warn(`custom property ${id} already exists`); } @@ -260,6 +258,20 @@ export class PagePropertiesManager { }; } + moveCustomProperty(from: number, to: number) { + // move from -> to means change from's order to a new order between to and to -1/+1 + const properties = this.getOrderedCustomProperties(); + const fromProperty = properties[from]; + const toProperty = properties[to]; + const toNextProperty = properties[from < to ? to + 1 : to - 1]; + const args: [string?, string?] = + from < to + ? [toProperty.order, toNextProperty?.order ?? null] + : [toNextProperty?.order ?? null, toProperty.order]; + const newOrder = generateKeyBetween(...args); + this.properties.custom[fromProperty.id].order = newOrder; + } + hasCustomProperty(id: string) { return !!this.properties.custom[id]; } diff --git a/packages/frontend/core/src/components/affine/page-properties/table.tsx b/packages/frontend/core/src/components/affine/page-properties/table.tsx index ade00f603..c1a0cc237 100644 --- a/packages/frontend/core/src/components/affine/page-properties/table.tsx +++ b/packages/frontend/core/src/components/affine/page-properties/table.tsx @@ -40,7 +40,7 @@ import { restrictToParentElement, restrictToVerticalAxis, } from '@dnd-kit/modifiers'; -import { arrayMove, SortableContext, useSortable } from '@dnd-kit/sortable'; +import { SortableContext, useSortable } from '@dnd-kit/sortable'; import * as Collapsible from '@radix-ui/react-collapsible'; import clsx from 'clsx'; import { use } from 'foxact/use'; @@ -54,6 +54,7 @@ import { Suspense, useCallback, useContext, + useEffect, useMemo, useState, } from 'react'; @@ -95,7 +96,12 @@ type PropertyVisibility = PageInfoCustomProperty['visibility']; const editingPropertyAtom = atom(null); const modifiers = [restrictToParentElement, restrictToVerticalAxis]; -const SortableProperties = ({ children }: PropsWithChildren) => { + +interface SortablePropertiesProps { + children: (properties: PageInfoCustomProperty[]) => React.ReactNode; +} + +const SortableProperties = ({ children }: SortablePropertiesProps) => { const manager = useContext(managerContext); const properties = useMemo( () => manager.getOrderedCustomProperties(), @@ -110,6 +116,14 @@ const SortableProperties = ({ children }: PropsWithChildren) => { }, }) ); + // use localProperties since changes applied to upstream may be delayed + // if we use that one, there will be weird behavior after reordering + const [localProperties, setLocalProperties] = useState(properties); + + useEffect(() => { + setLocalProperties(properties); + }, [properties]); + const onDragEnd = useCallback( (event: DragEndEvent) => { if (!draggable) { @@ -120,22 +134,22 @@ const SortableProperties = ({ children }: PropsWithChildren) => { const toIndex = properties.findIndex(p => p.id === over?.id); if (fromIndex !== -1 && toIndex !== -1 && fromIndex !== toIndex) { - const newOrdered = arrayMove(properties, fromIndex, toIndex); - manager.transact(() => { - newOrdered.forEach((p, i) => { - manager.updateCustomProperty(p.id, { - order: i, - }); - }); - }); + manager.moveCustomProperty(fromIndex, toIndex); + setLocalProperties(manager.getOrderedCustomProperties()); } }, [manager, properties, draggable] ); + + const filteredProperties = useMemo( + () => localProperties.filter(p => manager.getCustomPropertyMeta(p.id)), + [localProperties, manager] + ); + return ( - {children} + {children(filteredProperties)} ); @@ -167,6 +181,7 @@ const SortablePropertyRow = ({ transition, active, isDragging, + isSorting, } = useSortable({ id: property.id, }); @@ -175,10 +190,10 @@ const SortablePropertyRow = ({ transform: transform ? `translate3d(${transform.x}px, ${transform.y}px, 0)` : undefined, - transition, + transition: isSorting ? transition : undefined, pointerEvents: manager.readonly ? 'none' : undefined, }), - [manager.readonly, transform, transition] + [isSorting, manager.readonly, transform, transition] ); return ( @@ -305,7 +320,6 @@ export const PagePropertiesSettingsPopup = ({ }: PagePropertiesSettingsPopupProps) => { const manager = useContext(managerContext); const t = useAFFiNEI18N(); - const properties = manager.getOrderedCustomProperties(); const menuItems = useMemo(() => { const options: MenuItemOption[] = []; @@ -321,35 +335,37 @@ export const PagePropertiesSettingsPopup = ({ options.push('-'); options.push([ - {properties.map(property => { - const meta = manager.getCustomPropertyMeta(property.id); - assertExists(meta, 'meta should exist for property'); - const Icon = nameToIcon(meta.icon, meta.type); - const name = meta.name; - return ( - - - - -
+ properties.map(property => { + const meta = manager.getCustomPropertyMeta(property.id); + assertExists(meta, 'meta should exist for property'); + const Icon = nameToIcon(meta.icon, meta.type); + const name = meta.name; + return ( + - {name} -
- - - ); - })} + + + +
+ {name} +
+ + + ); + }) + } , ]); return renderMenuItemOptions(options); - }, [manager, properties, t]); + }, [manager, t]); return ( { + setLocalPropertyMeta(meta); + }, [meta]); + + useEffect(() => { + setLocalProperty(property); + }, [property]); + const handleFinishEditing = useCallback(() => { onFinishEditing(); manager.updateCustomPropertyMeta(meta.id, localPropertyMeta); @@ -752,10 +776,6 @@ export const PagePropertiesTableBody = ({ style, }: PagePropertiesTableBodyProps) => { const manager = useContext(managerContext); - - const properties = useMemo(() => { - return manager.getOrderedCustomProperties(); - }, [manager]); return (
- {properties - .filter( - property => - manager.isPropertyRequired(property.id) || - (property.visibility !== 'hide' && - !(property.visibility === 'hide-if-empty' && !property.value)) - ) - .map(property => ( - - ))} + {properties => + properties + .filter( + property => + manager.isPropertyRequired(property.id) || + (property.visibility !== 'hide' && + !( + property.visibility === 'hide-if-empty' && !property.value + )) + ) + .map(property => ( + + )) + }
{manager.readonly ? null : } diff --git a/packages/frontend/core/src/components/affine/page-properties/tags-inline-editor.css.ts b/packages/frontend/core/src/components/affine/page-properties/tags-inline-editor.css.ts index c7789b707..f6199bf0a 100644 --- a/packages/frontend/core/src/components/affine/page-properties/tags-inline-editor.css.ts +++ b/packages/frontend/core/src/components/affine/page-properties/tags-inline-editor.css.ts @@ -27,7 +27,7 @@ export const inlineTagsContainer = style({ export const tagsMenu = style({ padding: 0, transform: - 'translate(-3px, calc(-3px + var(--radix-popper-anchor-height) * -1))', + 'translate(-3.5px, calc(-3.5px + var(--radix-popper-anchor-height) * -1))', width: 'calc(var(--radix-popper-anchor-width) + 16px)', overflow: 'hidden', }); diff --git a/packages/frontend/core/src/hooks/use-affine-adapter.ts b/packages/frontend/core/src/hooks/use-affine-adapter.ts index ed826996d..f055a71b3 100644 --- a/packages/frontend/core/src/hooks/use-affine-adapter.ts +++ b/packages/frontend/core/src/hooks/use-affine-adapter.ts @@ -1,33 +1,36 @@ import type { Workspace } from '@toeverything/infra'; import { useService } from '@toeverything/infra/di'; -import { use } from 'foxact/use'; -import { useEffect, useMemo, useState } from 'react'; +import { useDebouncedState } from 'foxact/use-debounced-state'; +import { useEffect, useMemo } from 'react'; import { WorkspacePropertiesAdapter } from '../modules/workspace/properties'; -import { useAllBlockSuitePageMeta } from './use-all-block-suite-page-meta'; function getProxy(obj: T) { return new Proxy(obj, {}); } const useReactiveAdapter = (adapter: WorkspacePropertiesAdapter) => { - use(adapter.workspace.blockSuiteWorkspace.doc.whenSynced); - const [proxy, setProxy] = useState(adapter); - // fixme: this is a hack to force re-render when default meta changed - useAllBlockSuitePageMeta(adapter.workspace.blockSuiteWorkspace); + // hack: delay proxy creation to avoid unnecessary re-render + render in another component issue + const [proxy, setProxy] = useDebouncedState(adapter, 0); useEffect(() => { // todo: track which properties are used and then filter by property path change // using Y.YEvent.path function observe() { - requestAnimationFrame(() => { - setProxy(getProxy(adapter)); - }); + setProxy(getProxy(adapter)); } + const disposables: (() => void)[] = []; + disposables.push( + adapter.workspace.blockSuiteWorkspace.meta.pageMetasUpdated.on(observe) + .dispose + ); adapter.properties.observeDeep(observe); + disposables.push(() => adapter.properties.unobserveDeep(observe)); return () => { - adapter.properties.unobserveDeep(observe); + for (const dispose of disposables) { + dispose(); + } }; - }, [adapter]); + }, [adapter, setProxy]); return proxy; }; diff --git a/packages/frontend/core/src/modules/workspace/properties/schema.ts b/packages/frontend/core/src/modules/workspace/properties/schema.ts index 40ace3d8a..4eaa6a1c1 100644 --- a/packages/frontend/core/src/modules/workspace/properties/schema.ts +++ b/packages/frontend/core/src/modules/workspace/properties/schema.ts @@ -88,7 +88,7 @@ const WorkspaceAffinePropertiesSchemaSchema = z.object({ }); const PageInfoCustomPropertyItemSchema = PageInfoItemSchema.extend({ - order: z.number(), + order: z.string(), }); const WorkspacePagePropertiesSchema = z.object({ diff --git a/tests/affine-local/e2e/page-properties.spec.ts b/tests/affine-local/e2e/page-properties.spec.ts index 41e61fb67..42672fe43 100644 --- a/tests/affine-local/e2e/page-properties.spec.ts +++ b/tests/affine-local/e2e/page-properties.spec.ts @@ -241,3 +241,45 @@ test('create a required property', async ({ page }) => { ) ).toContainText('Required'); }); + +test('delete a required property', async ({ page }) => { + await openWorkspaceProperties(page); + await addCustomProperty(page, 'Text', true); + + await page + .locator('[data-testid="custom-property-row"]:has-text("Text")') + .getByRole('button') + .click(); + + await page + .getByRole('menuitem', { + name: 'Set as required property', + }) + .click(); + + await page + .locator('[data-testid="custom-property-row"]:has-text("Text")') + .getByRole('button') + .click(); + + await page + .getByRole('menuitem', { + name: 'Delete property', + }) + .click(); + await page + .getByRole('button', { + name: 'Confirm', + }) + .click(); + + // close workspace settings + await page.keyboard.press('Escape'); + + await waitForEditorLoad(page); + + // check if the property is removed from page properties + await expect( + page.locator('[data-testid="page-property-row-name"]:has-text("Text")') + ).not.toBeVisible(); +}); diff --git a/yarn.lock b/yarn.lock index 8c91b248c..f188b39b1 100644 --- a/yarn.lock +++ b/yarn.lock @@ -354,6 +354,7 @@ __metadata: express: "npm:^4.18.2" fake-indexeddb: "npm:^5.0.0" foxact: "npm:^0.2.20" + fractional-indexing: "npm:^3.2.0" graphql: "npm:^16.8.1" html-webpack-plugin: "npm:^5.5.3" idb: "npm:^8.0.0"