Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Desktop: Hiding the note preview pane is much slower than showing it for large notes #9890

Closed
personalizedrefrigerator opened this issue Feb 8, 2024 · 7 comments · Fixed by #10006
Labels
bug It's a bug desktop All desktop platforms medium Medium priority issues

Comments

@personalizedrefrigerator
Copy link
Collaborator

personalizedrefrigerator commented Feb 8, 2024

Operating system

Linux

Joplin version

2.14.12

Desktop version info

Joplin 2.14.12 (dev, linux)

Client ID: 50a95f7d97414363a84473b388d1916d
Sync Version: 3
Profile Version: 45
Keychain Supported: No

Revision: 6b5c60b (pr/fix-mermaid-save-button-under-graph)

CodeMirror 6 snippets: 0.0.4
Simple Backup: 1.3.5

Current behaviour

  1. Create a new note in markdown mode.
  2. Download the Project Gutenberg Ebook of Frankenstein and paste it ten times into a note.
  3. Click "Toggle editor layout" to cycle through different layouts

Currently, hiding the markdown preview pane is much slower than any of the other layout transitions.

Screen recording:

untitled.mp4

I can reproduce this both with the legacy CodeMirror 5 editor and the beta CodeMirror 6 editor.

Expected behaviour

Hiding the markdown preview pane should be at least as fast as showing it.

Logs

08:32:52: CommandService::execute: toggleVisiblePanes
11[Violation] 'requestIdleCallback' handler took <N>ms
main-html.js:54 08:32:53: models/Setting: Saving settings...
main-html.js:54 08:32:54: models/Setting: Settings have been saved.
main-html.js:54 08:32:54: CommandService::execute: toggleVisiblePanes
main-html.js:54 08:32:55: models/Setting: Saving settings...
main-html.js:54 08:32:55: models/Setting: Settings have been saved.
main-html.js:54 08:32:55: CommandService::execute: toggleVisiblePanes
main-html.js:54 08:33:02: models/Setting: Saving settings...
main-html.js:54 08:33:02: models/Setting: Settings have been saved.
@manu-r12
Copy link

i would love to work on this problem, since i am new here can you assist me with where can i find code file?

@personalizedrefrigerator
Copy link
Collaborator Author

i would love to work on this problem, since i am new here can you assist me with where can i find code file?

You might find this pull request that fixed a related issue to be helpful. I suspect that the fix for this issue will probably involve similar files.

@manu-r12
Copy link

manu-r12 commented Feb 24, 2024

Screenshot 2024-02-24 at 11 47 24 PM

while i am working on this issue. I wanna know when we click this "toggle editor layout" , what function this triggers.?
Any idea or hint would be so helpful

@personalizedrefrigerator
Copy link
Collaborator Author

personalizedrefrigerator commented Feb 24, 2024

I wanna know when we click this "toggle editor layout" , what function this triggers.?

Hi!

The "toggle editor layout" button updates noteVisiblePanes in Joplin's app state, this causes props.visiblePanes to change in the markdown editors.

The markdown editor components can be found here:

The issue seems to be present in both.

How the above information might be discovered.

Here's a process that I find helpful when trying to debug issues like this:

  1. Find a UI string related to the action.
    • In this case, hovering over the button shows "toggle editor layout": screenshot: toggle editor layout shown as a tooltip
  2. Search for that UI string (or part of it) in Joplin's codebase.

In this case, I see that it dispatches a NOTE_VISIBLE_PANES_TOGGLE event. Searching for NOTE_VISIBLE_PANES_TOGGLE brings me to the Redux reducer in app.ts.

In the case statement, I see that the noteVisiblePanes property of the app's state is updated:

newState.noteVisiblePanes = getNextLayout(panes);

Searching for noteVisiblePanes gives a list of results. One is in NoteEditor.tsx. Because this is an editor-related bug, this seems to be likely related:

visiblePanes: props.noteVisiblePanes || ['editor', 'viewer'],

Searching again for visiblePanes in the NoteEditor folder brings me to lines in v6/CodeMirror.tsx. For example,

if (!props.visiblePanes.includes('editor')) {

Note: v6/CodeMirror.tsx is the beta markdown editor. v5/CodeMirror.tsx is the legacy markdown editor.

Adding console.log statements or breakpoints in Joplin's development tools can then be used to verify that visiblePanes does change when clicking on the toggle button.

Additional suggestions:

  • The Performance tab in Joplin's developer tools can be useful.
    • Note that a long-running Layout task might suggest that changes made to the DOM by Joplin are causing a large amount of work for the browser.
    • In contrast, long-running JavaScript tasks might suggest that the issue is related to rendering.
  • console.timeLog and console.time could be helpful here (for example, to determine which useEffects are taking a long time).

@cagnusmarlsen
Copy link
Contributor

The Performance tab shows a long running task (takes around 10 seconds) which leads to the measureForScrollbars function inside the CodeMirror package (node_modules) itself. It seems like the main problem is that calculating offsetWidth, offsetHeight and other DOM properties is slowing it down for some reason. I wonder what can be done because the long running task is inside the CodeMirror package itself.

@personalizedrefrigerator
Copy link
Collaborator Author

At least in the CM6 beta editor, the layout performance issue was caused by hiding the note viewer by setting its width to 1px. Using display = "none" instead seems to fix the issue for both editors.

@personalizedrefrigerator
Copy link
Collaborator Author

I've opened a similar performance issue related to a similar part of the codebase as this issue: #10008.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug It's a bug desktop All desktop platforms medium Medium priority issues
Projects
None yet
4 participants