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: Fixes #6416: Switching a note using Sidebar is slow and grayed out #6430

Merged
merged 2 commits into from Nov 14, 2022

Conversation

ken1kob
Copy link
Contributor

@ken1kob ken1kob commented Apr 19, 2022

This PR fixes issue #6416.

Features:

  • Makes note switching using SideBar faster by preventing scripts from reloaded
  • Prevents NoteEditor from grayed out when note switches using SideBar

Description:

  • When a notebook is changed without any selected note, no note is selected for a moment, and then a new note gets selected.
  • In this short transient period with no note, NoteEditor.renderNoNote() is called, and then its body (Editor/Viewer) is unmounted. So, NoteEditor becomes grayed out, and scripts in Editor/Viewer gets reloaded.
  • To prevent them, in the transient period, the id of the last displayed note will be temporarily kept until the id of a new note arrives.

@laurent22
Copy link
Owner

When a notebook is changed without any selected note, no note is selected for a moment, and then a new note gets selected.

But I'm wondering why no note is selected for a moment? Wasn't that some kind of hack to reset the editor state for instance? And by fixing it we may bring back some old bug?

Or is it not possible to fix the state handling so that it doesn't have a gap with no selected note?

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 20, 2022

But I'm wondering why no note is selected for a moment? Wasn't that some kind of hack to reset the editor state for instance? And by fixing it we may bring back some old bug?

I thought the possibility, if it were intentional. But it seems no to be intentional. The reason why no note is selected is just that there is no way to decide the first note in the new notebook when FOLDER_SELECT action is performed. The first note becomes known when and after BaseApplication.ts/refreshNotes() is called. To erase the no-note state, some part of Baseapplication.ts/refreshNotes() needs to be ported into reducer's FOLDER_SELECT action. So, it is more natural to assume that it was simply implemented that way, rather than that the no-note state was intentionally created.

Speaking of resetting, it is unreasonable that resetting scripts relies on SideBar. Anyway, initializing scripts are performed when a new note is loaded. And, if reloading scripts is needed, it should be implemented in a more reliable way than clicking SideBar.

If resetting (reloading) is needed explicitly by a user's operation, other operations such as showing options and selecting an empty notebook delivers the same effect.

Or is it not possible to fix the state handling so that it doesn't have a gap with no selected note?

As stated in this comment, a modification in reducer will have a heavy impact on wide areas.

In the GUI part, no no-note-state can be accepted. But I don't know if it is OK elsewhere. So I think it is safe not to erase the no-note-state.

@laurent22
Copy link
Owner

Thanks for clarifying but I feel it's risky to change this, and I'm not keen having certain parts of the code loading an old note, and other parts loading no note. Right now, it's not ideal but at least it's consistent with the application state.

Does it make a big performance improvement to remove this temporary gray screen?

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 20, 2022

I'm not keen having certain parts of the code loading an old note, and other parts loading no note.

This time gap causes no inconsistency, because it is transient and has no timing room for user's interaction. Practically speaking, this PR only ignores a transient state which is perfectly acceptable not to be recognized.

Of course, fixing reducer is ideal.

Does it make a big performance improvement to remove this temporary gray screen?

Yes. This PR removes "Reloading script" part of image (B) and (C) in this comment.
And, its improvement is shown in this discourse post's chart.
Besides, reloading script time is cumulative for each script.

@laurent22 laurent22 merged commit 5d49fcf into laurent22:dev Nov 14, 2022
@laurent22
Copy link
Owner

Thanks for the fix @ken1kob!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop All desktop platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants