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: Multiple window support #11181

Draft
wants to merge 48 commits into
base: dev
Choose a base branch
from

Conversation

personalizedrefrigerator
Copy link
Collaborator

@personalizedrefrigerator personalizedrefrigerator commented Oct 5, 2024

Summary

This pull request allows opening Joplin's editor in a new window using React's portal API. It still has a few issues and is thus a draft.

Known issues

  • (Not an issue?) Secondary windows are just the editor. While they can show dialogs, they don't include panels and can't show other screens.
  • Notes need to be reloaded to show updates. (Fixed)
  • Editor commands (e.g. bold with ctrl-b) are always applied to the last-created editor. (Fixed)
  • All dialogs (e.g. go to anything, note info) open in the main window. (Fixed)
  • The editor sometimes incorrectly reloads content — it seems to detect changes from recent saves as edits made by a different editor.
    • This should be fixed by fad84d4. The fix can be improved, though. Currently, it involves storing recent note versions in memory.
  • If editing in a secondary window, dialogs (e.g. note statistics) will report information for the note in the main window.
  • Commands that rely on selectedNoteId always act on the note displayed in the main window.
    • For example, clicking "Note", then "Permanently delete note" in a secondary window prompts to delete whichever note is open in the main Joplin window.
  • Commands that rely on selectedNoteId for their enabled condition can be incorrectly enabled/disabled in new windows.
  • Toolbar buttons are enabled/disabled based on the current window, rather than the window that they're in.
  • The tags list always reflects the tags associated with the currently focused window (even if the list is displayed in a background window).
  • state.notes is refreshed unnecessarily when a window gains focus and it isn't a just-created window.
  • Context menus only work in the main window.
  • All windows must have the same editor (Markdown/Rich Text).
  • All windows have the same layout.
    all-windows-same-layout.webm
  • Opening settings closes all secondary windows.
  • Drag-and-drop doesn't work between windows.
  • Plugin dialogs are shown on all windows, and (depending on the plugin) may only work correctly in one of the windows.
    • Plugin dialogs are now shown only on one window by default. However, if a different window is focused, it's possible for two plugin dialogs with the same ID to be opened. This can cause issues for certain plugins.
  • Enabling the Rich Text Editor in one of the windows disables certain commands (e.g. header, checklist) for all windows.
  • Adding a new tag in a new window causes the new window to navigate to a different note.
  • When notes are deleted and in the navigation history, they are only deleted from the navigation history for the last focused window.
  • Plugin API global CSS applies only to the main window.
  • The legacy Markdown editor is styled incorrectly (fixed size, Markdown preview hidden) in secondary windows.
  • Warning banners (e.g. "the synchronization password is missing") are only shown on the main window.

Screen recording

multi-window-demo-oct-15-2024.webm

Notes

const parentNode = loaded ? doc?.body : null;

// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Needed to allow adding the portal to the DOM
const contentPortal = parentNode && createPortal(props.children, parentNode) as any;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related documentation/explanation:

…en and into separate directory

This pull request previously moved the main window command registration logic
from MainScreen.tsx to a different component that could manage showing
dialogs, etc for different windows. This, however, could cause errors
when closing settings (attempts to use commands with unregistered
runtimes). This pull request ensures that command runtimes are
registered by loading them in Root.tsx, rather than in MainScreen.tsx.
return <>
<div ref={setDependencyStyleContainer}></div>
<StyleSheetManager target={dependencyStyleContainer}>
<CacheProvider value={cache}>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code uses CacheProvider from @emotion/react to cause react-select to insert its styles into the new window (see comment). @emotion/react is a dependency of react-select and, at present, is not a direct dependency of Joplin.

Adding @emotion/react to app-desktop/package.json could be problematic if react-select is updated without also updating the @emotion/react version — having multiple versions of @emotion/react may cause styling issues (or perhaps break styling just in new windows).

See this unmerged upstream pull request for a possible future solution that doesn't involve importing transitive dependencies.

@personalizedrefrigerator personalizedrefrigerator marked this pull request as draft October 11, 2024 21:06
@personalizedrefrigerator personalizedrefrigerator changed the title Proof of concept: Desktop: Multiple window support Desktop: Multiple window support Oct 14, 2024
@personalizedrefrigerator personalizedrefrigerator added enhancement Feature requests and code enhancements desktop All desktop platforms labels Oct 16, 2024
This commit attempts to address what may be a race condition -- if
commands aren't loaded by the time certain parts of the UI attempt to
render, a "crashed" screen can be shown. This may be what is happening
in CI.
background windows

Previously, these buttons were enabled/disabled based on the state of
whichever window had focus.
Previously, only the focused window's note history was updated.
Creates titles for background windows based on the current note ID.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NoteEditor.tsx is a new file. The old NoteEditor.tsx was renamed to NoteEditorContent.tsx.

  • To-do: Use a different name for the note editor wrapper so that it's marked as a rename by git?

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

Successfully merging this pull request may close these issues.

1 participant