Desktop: Resolves #4251: Refactor sidebar to better handle thousands of tags and notebooks #10331

personalizedrefrigerator · 2024-04-18T22:31:53Z

Summary

This pull request refactors the desktop sidebar to:

Avoid re-generating the list of tags and folders when the selection changes.
Render the currently visible notes and/or tags, rather than all notes and tags on each re-render.
Have Playwright tests.
- To support this change, several items that were missing accessibility labels have been them. This includes the "new note" button and the expand/unexpand folder buttons.

With this pull request,

Adding tags is faster.
Switching notebooks is faster.
Moving a large notebook with thousands of items to the trash is still slow.
Expanding a large notebook with many nested subnotebooks is still slow (though doesn't seem to be as slow as before).

Fixes #4251.

Notes

This should only affect desktop. A similar pull request may need to be created to refactor the mobile sidebar.
At present, this pull request assumes that all notebook, tag, and header items have the same height in the sidebar.
- At present, this height is 30px.
- This may not be the case if the user installs a theme like the ReMoods theme or has changed Joplin's notebook list styles with custom CSS.
Much of the logic that was originally in Sidebar.tsx has been moved to the useOnRenderItem.ts hook.
The following issues seem to have been present before this PR and are still present:
- When dragging one folder to another, the note count for the parent folder may briefly go negative.
- When adding a new notebook, the notebook is initially added below the trash folder, then moved to the correct location. This seems to be because we set doRefreshFolders to true instead of now here (true causes a delayed update, while now updates without delay).

To-do

Test manually: How much faster is it?
Automated tests
- Potentially add more: Include tests for note count and adding/removing tags.
Follow-up pull request: Make finding which notebooks are children of the current notebook faster. At present, it seems to be in $\mathcal{O}(n^2)$.
- As a result, Joplin is still slow with many deeply-nested notebooks.

Manual testing

On Ubuntu 23.10, I have manually verified that:

Dragging a note to a notebook heading moves that note to the target notebook.
Clicking on the "NOTEBOOKS" header hides/shows all notebooks
Clicking on the "TAGS" header hides/shows all tags
Selecting a notebook and/or tag outside of the viewport with the up/down arrow keys scrolls the viewport
Note counts update when dragging one notebook to another
It is still possible to use "go to anything" to search and scroll through search results with arrow keys
- GoToAnything uses ItemList, which is refactored by this pull request.
On mobile (Android 10 emulator), with 4-5 notebooks, it is still possible to select and expand/collapse notebooks in the sidemenu.

Note that this pull request has automated tests, which additionally tests:

Expanding/collapsing notebooks
Dragging one notebook to another
Dragging one notebook to the "all notes" heading
Creating new notebooks using the "+" button
Changing focus using the up and down arrow keys

Screen recordings

Faster notebook switching

faster-notebook-switching.mp4

Faster tagging

In the recording below, the main screen is open in the background in both copies of Joplin. The numbers of notes/tags/notebooks in both profiles are similar to in the tests done here.

faster-tagging.mp4

Fixes laurent22#4251

packages/app-desktop/gui/Sidebar/listItemComponents/FolderItem.tsx

elements

laurent22 · 2024-04-19T08:40:30Z

With this pull request is it faster to render the list of tags?

Also I think the problem that this user is having is due to having many tags, and every time a sync operation is performed the tag list is rendered (slowly) again. I'm wondering if your PR would help with this? See also here for example: https://discourse.joplinapp.org/t/desktop-ui-is-too-slow-to-work-efficiently/23657/10?u=laurent

laurent22

It's quite a big refactoring and difficult to review but I didn't notice any obvious issue other than the folder/notebook issue. Splitting the sidebar into multiple components should definitely help with optimising it.

laurent22 · 2024-04-19T10:54:22Z

packages/app-desktop/gui/Sidebar/NotebookAndTagList.tsx

+}
+
+
+const NotebookAndTagList: React.FC<Props> = props => {


Should be FolderAndTagList. There are several instances of "notebook" being used in this pull request instead of "folder" so if you could change everything to "folder" that would be great. As always, we only use the term "notebook" in user-facing string, and only "folder" internally.

I've made this change in most places.

For now, I've left sidebar.spec.ts as-is, because Playwright is accessing user-facing labels (which use "notebook") to run the test. I can change the internal variable names there well if desired.

I would prefer we use "folder" there too if that's ok, including in test descriptions, and even when creating folders for test (So "Folder A" instead of "Notebook A").

Basically the only time we should use the term "notebook" is in strings to be translated or in documentation. Ideally we'd change everything to use only one term or the other, but that would be more trouble than it's worth.

Got it! This should be addressed by 97ec1a9.

personalizedrefrigerator · 2024-04-19T12:57:47Z

With this pull request is it faster to render the list of tags?

Yes. This pull request refactors the sidebar to use an ItemList, which only renders the tags/folder headers that are currently visible on the screen.

It still needs to load the sorted list of tags and folders, but does so less often than before this pull request.

Additionally, switching notebooks/tags should be faster (see the screen recording above):

Rather than re-rendering the full sidebar when the selected notebook/tag changes, we now only update the currently selected sidebar index (see useSelectedSidebarIndex.ts). This is partially to make it easier to manage keyboard focus and partially for performance.
If the notebook list or tag list needs to be regenerated (e.g. because a notebook is added), useSidebarListData.ts caches the other list. As a result, adding a tag does not force the list of notebooks to be regenerated.
- Previously, we would both re-generate this data and render it as components.

Also I think the problem that this user is having is due to having many tags, and every time a sync operation is performed the tag list is rendered (slowly) again. I'm wondering if your PR would help with this?

This pull request should help with that (see the "Faster tagging" video above for an example).

personalizedrefrigerator · 2024-04-19T15:33:28Z

packages/app-desktop/gui/Sidebar/FolderAndTagList.tsx

+				itemRenderer={onRenderItem}
+				onKeyDown={onKeyEventHandler}
+
+				itemHeight={30}


I'm not sure I like hardcoding the item height as 30... Doing so makes it difficult to force a larger-than-30px sidebar font with custom CSS.

Aside from measuring one of the children (e.g. with .clientHeight), I'm not sure how else to do this, however.

packages/app-desktop/gui/Sidebar/listItemComponents/AllNotesItem.tsx

packages/app-desktop/gui/Sidebar/hooks/useSidebarListData.ts

packages/app-desktop/gui/Sidebar/styles/index.ts

packages/app-desktop/gui/Sidebar/listItemComponents/ExpandIcon.tsx

The focus handler test is occasionally failing in CI. Hypothesizing that this issue is caused by the note counter updating (and perhaps causing the sidebar to be re-focused), the text selector for folder A is updated

Rather than using mainWindow.keyboard.press, uses locator.press, which seems to be the recommended way to test keyboard events (per the docs)

added to the note list

packages/app-desktop/integration-tests/models/Sidebar.tsx

laurent22 · 2024-04-25T12:54:55Z

There's a conflict on the Sidebar file

…erf-improvement

packages/app-desktop/gui/Sidebar/hooks/useOnRenderItem.tsx

personalizedrefrigerator added 11 commits April 18, 2024 12:03

Refactor sidebar to use ItemList

5e9659b

Fixes laurent22#4251

Remove StyledExpandLink styled component

1d7efbc

Remove unused refs and comments

88f139b

Fix context menus

1c83c4c

Fix expand/collapse header behavior

cd1b198

Move useListContainerHeight to its own file

e6acd60

Keyboard shortcuts for switching folders

92def11

Move list item components into a new folder

3eaf3c7

Remove now mostly unused selection calculation

754ab01

Improve focus/shortcut handling

63cbdf3

Add spacer below the note list section

d773b5d

personalizedrefrigerator changed the title ~~Desktop: Resolves #4251: Refactor sidebar to better handle thousands tags and notebooks~~ Desktop: Resolves #4251: Refactor sidebar to better handle thousands of tags and notebooks Apr 18, 2024

personalizedrefrigerator commented Apr 19, 2024

View reviewed changes

packages/app-desktop/gui/Sidebar/listItemComponents/FolderItem.tsx Outdated Show resolved Hide resolved

personalizedrefrigerator added 2 commits April 18, 2024 22:08

Sidebar: Tests and improved accessibility

d3d7c32

Don't show redundant orange outline focus indicator for most sidebar

f2677bb

elements

personalizedrefrigerator marked this pull request as ready for review April 19, 2024 06:01

laurent22 reviewed Apr 19, 2024

View reviewed changes

personalizedrefrigerator added 3 commits April 19, 2024 06:06

Rename notebook -> folder

484e1ce

Remove extra spaces in sidebar.spec.ts

5f18044

side-menu-shared: De-inline types

2b40bb5

personalizedrefrigerator commented Apr 19, 2024

View reviewed changes

personalizedrefrigerator added 7 commits April 19, 2024 08:34

Remove unnecessary braces

ae1c6c1

Fix focusElementSideBar command

8a9ea53

Refactor and test useFocusHandler

780b8ea

Attempt to fix flaky sidebar test

58e92f7

focus handler: Fix note list scrolls when toggling a dropdown

4a4da5b

Tests: Rename notebook -> folder

97ec1a9

Commenting: folders/notebooks/tags -> folders/tags

ea48c78

personalizedrefrigerator commented Apr 20, 2024

View reviewed changes

personalizedrefrigerator and others added 12 commits April 22, 2024 06:41

Remove unnecessary toggleblock attribute

14d67ac

Revert focus-outline fix -- should go in a different PR

9ba81d3

Remove unnecessary connect logic in FolderItem

6811579

Add test for all notes section

7c93d4a

Re-add ctrl+a handler

fd3aefe

Trying to fix flaky test

f7bfbc8

The focus handler test is occasionally failing in CI. Hypothesizing that this issue is caused by the note counter updating (and perhaps causing the sidebar to be re-focused), the text selector for folder A is updated

Trying to fix flaky test

2782ffa

Rather than using mainWindow.keyboard.press, uses locator.press, which seems to be the recommended way to test keyboard events (per the docs)

Try to fix flaky tests: Add additional expects to pause until notes are

ac9a992

added to the note list

Merge branch 'dev' into pr/desktop/sidebar-perf-improvement

f2b4fa9

Remove flaky focus handler test

2be5b15

Merge branch 'dev' into pr/desktop/sidebar-perf-improvement

e523d08

Merge branch 'dev' into pr/desktop/sidebar-perf-improvement

495790d

personalizedrefrigerator commented Apr 24, 2024

View reviewed changes

packages/app-desktop/integration-tests/models/Sidebar.tsx Outdated Show resolved Hide resolved

Test support file: Change .tsx -> .ts

e786bd2

personalizedrefrigerator added 2 commits April 25, 2024 06:13

Merge remote-tracking branch 'upstream/dev' into pr/desktop/sidebar-p…

736e6b0

…erf-improvement

Merge remote-tracking branch 'upstream/dev' into pr/desktop/sidebar-p…

4d16387

…erf-improvement

personalizedrefrigerator commented Apr 25, 2024

View reviewed changes

packages/app-desktop/gui/Sidebar/hooks/useOnRenderItem.tsx Show resolved Hide resolved

Update comment

596c4e7

laurent22 merged commit 97b5276 into laurent22:dev Apr 25, 2024
10 checks passed

personalizedrefrigerator mentioned this pull request Apr 26, 2024

Desktop: Fix "new notebook" shown in context menu when right-clicking on the "Tags" header #10378

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desktop: Resolves #4251: Refactor sidebar to better handle thousands of tags and notebooks #10331

Desktop: Resolves #4251: Refactor sidebar to better handle thousands of tags and notebooks #10331

personalizedrefrigerator commented Apr 18, 2024 •

edited

Loading

laurent22 commented Apr 19, 2024

laurent22 left a comment

laurent22 Apr 19, 2024

personalizedrefrigerator Apr 19, 2024

laurent22 Apr 20, 2024

personalizedrefrigerator Apr 20, 2024

personalizedrefrigerator commented Apr 19, 2024

personalizedrefrigerator Apr 19, 2024

laurent22 commented Apr 25, 2024

Desktop: Resolves #4251: Refactor sidebar to better handle thousands of tags and notebooks #10331

Desktop: Resolves #4251: Refactor sidebar to better handle thousands of tags and notebooks #10331

Conversation

personalizedrefrigerator commented Apr 18, 2024 • edited Loading

Summary

Notes

To-do

Manual testing

Screen recordings

Faster notebook switching

Faster tagging

laurent22 commented Apr 19, 2024

laurent22 left a comment

Choose a reason for hiding this comment

laurent22 Apr 19, 2024

Choose a reason for hiding this comment

personalizedrefrigerator Apr 19, 2024

Choose a reason for hiding this comment

laurent22 Apr 20, 2024

Choose a reason for hiding this comment

personalizedrefrigerator Apr 20, 2024

Choose a reason for hiding this comment

personalizedrefrigerator commented Apr 19, 2024

personalizedrefrigerator Apr 19, 2024

Choose a reason for hiding this comment

laurent22 commented Apr 25, 2024

personalizedrefrigerator commented Apr 18, 2024 •

edited

Loading