From 320d0df60d27dc5c187a6d4d5321eedf8b5ab7b8 Mon Sep 17 00:00:00 2001 From: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com> Date: Thu, 4 Jul 2024 05:56:57 -0700 Subject: [PATCH] Desktop, Mobile: Fixes #10674: Fix sidebar performance regression with many nested notebooks (#10676) --- .../gui/Sidebar/hooks/useSidebarListData.ts | 9 +- .../components/side-menu-content.tsx | 12 ++- .../shared/side-menu-shared.test.ts | 50 ++++++++++- .../lib/components/shared/side-menu-shared.ts | 82 ++++++++++--------- 4 files changed, 106 insertions(+), 47 deletions(-) diff --git a/packages/app-desktop/gui/Sidebar/hooks/useSidebarListData.ts b/packages/app-desktop/gui/Sidebar/hooks/useSidebarListData.ts index e0c2343e9..f110722dd 100644 --- a/packages/app-desktop/gui/Sidebar/hooks/useSidebarListData.ts +++ b/packages/app-desktop/gui/Sidebar/hooks/useSidebarListData.ts @@ -1,7 +1,7 @@ import { useMemo } from 'react'; import { FolderListItem, HeaderId, HeaderListItem, ListItem, ListItemType, TagListItem } from '../types'; import { FolderEntity, TagsWithNoteCountEntity } from '@joplin/lib/services/database/types'; -import { renderFolders, renderTags } from '@joplin/lib/components/shared/side-menu-shared'; +import { buildFolderTree, renderFolders, renderTags } from '@joplin/lib/components/shared/side-menu-shared'; import { _ } from '@joplin/lib/locale'; import CommandService from '@joplin/lib/services/CommandService'; import Setting from '@joplin/lib/models/Setting'; @@ -35,10 +35,13 @@ const useSidebarListData = (props: Props): ListItem[] => { }); }, [props.tags]); + const folderTree = useMemo(() => { + return buildFolderTree(props.folders); + }, [props.folders]); const folderItems = useMemo(() => { const renderProps = { - folders: props.folders, + folderTree, collapsedFolderIds: props.collapsedFolderIds, }; return renderFolders(renderProps, (folder, hasChildren, depth): FolderListItem => { @@ -50,7 +53,7 @@ const useSidebarListData = (props: Props): ListItem[] => { key: folder.id, }; }); - }, [props.folders, props.collapsedFolderIds]); + }, [folderTree, props.collapsedFolderIds]); return useMemo(() => { const foldersHeader: HeaderListItem = { diff --git a/packages/app-mobile/components/side-menu-content.tsx b/packages/app-mobile/components/side-menu-content.tsx index b03c25d13..b1da60fa5 100644 --- a/packages/app-mobile/components/side-menu-content.tsx +++ b/packages/app-mobile/components/side-menu-content.tsx @@ -8,7 +8,7 @@ import Synchronizer from '@joplin/lib/Synchronizer'; import NavService from '@joplin/lib/services/NavService'; import { _ } from '@joplin/lib/locale'; import { ThemeStyle, themeStyle } from './global-style'; -import { isFolderSelected, renderFolders } from '@joplin/lib/components/shared/side-menu-shared'; +import { buildFolderTree, isFolderSelected, renderFolders } from '@joplin/lib/components/shared/side-menu-shared'; import { FolderEntity, FolderIcon, FolderIconType } from '@joplin/lib/services/database/types'; import { AppState } from '../utils/types'; import Setting from '@joplin/lib/models/Setting'; @@ -560,8 +560,16 @@ const SideMenuContentComponent = (props: Props) => { items.push(renderSidebarButton('folder_header', _('Notebooks'), 'folder')); + const folderTree = useMemo(() => { + return buildFolderTree(props.folders); + }, [props.folders]); + if (props.folders.length) { - const result = renderFolders(props, renderFolderItem); + const result = renderFolders({ + folderTree, + collapsedFolderIds: props.collapsedFolderIds, + }, renderFolderItem); + const folderItems = result.items; items = items.concat(folderItems); } diff --git a/packages/lib/components/shared/side-menu-shared.test.ts b/packages/lib/components/shared/side-menu-shared.test.ts index 67427851a..f8ef94918 100644 --- a/packages/lib/components/shared/side-menu-shared.test.ts +++ b/packages/lib/components/shared/side-menu-shared.test.ts @@ -1,6 +1,6 @@ import { FolderEntity } from '../../services/database/types'; import { getTrashFolder, getTrashFolderId } from '../../services/trash'; -import { renderFolders } from './side-menu-shared'; +import { buildFolderTree, renderFolders } from './side-menu-shared'; const renderItem = (folder: FolderEntity, hasChildren: boolean, depth: number) => { return [folder.id, hasChildren, depth]; @@ -86,8 +86,52 @@ describe('side-menu-shared', () => { order: ['1', getTrashFolderId(), '2'], }, ], - ])('should render folders', (props, expected) => { - const actual = renderFolders(props, renderItem); + + // Should not render id: 4 because it's contained within the child of a collapsed folder. + [ + { + collapsedFolderIds: ['2'], + folders: [ + { + id: '1', + parent_id: '', + deleted_time: 0, + }, + { + id: '2', + parent_id: '', + deleted_time: 0, + }, + { + id: '3', + parent_id: '2', + deleted_time: 0, + }, + { + id: '4', + parent_id: '3', + deleted_time: 0, + }, + getTrashFolder(), + ], + notesParentType: 'Folder', + selectedFolderId: '', + selectedTagId: '', + }, + { + items: [ + ['1', false, 0], + ['2', true, 0], + [getTrashFolderId(), false, 0], + ], + order: ['1', '2', getTrashFolderId()], + }, + ], + ])('should render folders (case %#)', (props, expected) => { + const actual = renderFolders({ + folderTree: buildFolderTree(props.folders), + collapsedFolderIds: props.collapsedFolderIds, + }, renderItem); expect(actual).toEqual(expected); }); diff --git a/packages/lib/components/shared/side-menu-shared.ts b/packages/lib/components/shared/side-menu-shared.ts index 25cc4f14c..06afee576 100644 --- a/packages/lib/components/shared/side-menu-shared.ts +++ b/packages/lib/components/shared/side-menu-shared.ts @@ -1,39 +1,10 @@ -import Folder from '../../models/Folder'; -import BaseModel from '../../BaseModel'; import { FolderEntity, TagEntity, TagsWithNoteCountEntity } from '../../services/database/types'; -import { getDisplayParentId, getTrashFolderId } from '../../services/trash'; +import { getDisplayParentId } from '../../services/trash'; import { getCollator } from '../../models/utils/getCollator'; export type RenderFolderItem = (folder: FolderEntity, hasChildren: boolean, depth: number)=> T; export type RenderTagItem = (tag: TagsWithNoteCountEntity)=> T; -function folderHasChildren_(folders: FolderEntity[], folderId: string) { - if (folderId === getTrashFolderId()) { - return !!folders.find(f => !!f.deleted_time); - } - - for (let i = 0; i < folders.length; i++) { - const folder = folders[i]; - const folderParentId = getDisplayParentId(folder, folders.find(f => f.id === folder.parent_id)); - if (folderParentId === folderId) return true; - } - - return false; -} - -function folderIsCollapsed(folders: FolderEntity[], folderId: string, collapsedFolderIds: string[]) { - if (!collapsedFolderIds || !collapsedFolderIds.length) return false; - - while (true) { - const folder: FolderEntity = BaseModel.byId(folders, folderId); - if (!folder) throw new Error(`No folder with id ${folder.id}`); - const folderParentId = getDisplayParentId(folder, folders.find(f => f.id === folder.parent_id)); - if (!folderParentId) return false; - if (collapsedFolderIds.indexOf(folderParentId) >= 0) return true; - folderId = folderParentId; - } -} - interface FolderSelectedContext { selectedFolderId: string; notesParentType: string; @@ -48,21 +19,36 @@ type ItemsWithOrder = { order: string[]; }; -interface RenderFoldersProps { +interface FolderTree { folders: FolderEntity[]; + parentIdToChildren: Map; + idToItem: Map; +} + +interface RenderFoldersProps { + folderTree: FolderTree; collapsedFolderIds: string[]; } +function folderIsCollapsed(context: RenderFoldersProps, folderId: string) { + if (!context.collapsedFolderIds || !context.collapsedFolderIds.length) return false; + + while (true) { + const folder = context.folderTree.idToItem.get(folderId); + const folderParentId = getDisplayParentId(folder, context.folderTree.idToItem.get(folder.parent_id)); + if (!folderParentId) return false; + if (context.collapsedFolderIds.includes(folderParentId)) return true; + folderId = folderParentId; + } +} + function renderFoldersRecursive_(props: RenderFoldersProps, renderItem: RenderFolderItem, items: T[], parentId: string, depth: number, order: string[]): ItemsWithOrder { - const folders = props.folders; - for (let i = 0; i < folders.length; i++) { - const folder = folders[i]; + const folders = props.folderTree.parentIdToChildren.get(parentId ?? '') ?? []; + const parentIdToChildren = props.folderTree.parentIdToChildren; + for (const folder of folders) { + if (folderIsCollapsed(props, folder.id)) continue; - const folderParentId = getDisplayParentId(folder, props.folders.find(f => f.id === folder.parent_id)); - - if (!Folder.idsEqual(folderParentId, parentId)) continue; - if (folderIsCollapsed(props.folders, folder.id, props.collapsedFolderIds)) continue; - const hasChildren = folderHasChildren_(folders, folder.id); + const hasChildren = parentIdToChildren.has(folder.id); order.push(folder.id); items.push(renderItem(folder, hasChildren, depth)); if (hasChildren) { @@ -81,6 +67,24 @@ export const renderFolders = (props: RenderFoldersProps, renderItem: RenderF return renderFoldersRecursive_(props, renderItem, [], '', 0, []); }; +export const buildFolderTree = (folders: FolderEntity[]): FolderTree => { + const idToItem = new Map(); + for (const folder of folders) { + idToItem.set(folder.id, folder); + } + + const parentIdToChildren = new Map(); + for (const folder of folders) { + const displayParentId = getDisplayParentId(folder, idToItem.get(folder.parent_id)) ?? ''; + if (!parentIdToChildren.has(displayParentId)) { + parentIdToChildren.set(displayParentId, []); + } + parentIdToChildren.get(displayParentId).push(folder); + } + + return { folders, parentIdToChildren, idToItem }; +}; + const sortTags = (tags: TagEntity[]) => { tags = tags.slice(); const collator = getCollator();