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 #10230: Fix new note and to-do buttons greyed when initial selection is "all notes" or a tag #10434

Conversation

personalizedrefrigerator
Copy link
Collaborator

@personalizedrefrigerator personalizedrefrigerator commented May 14, 2024

Summary

This pull request fixes #10230 by setting selectedFolderId in the application state on startup, even if the selected note parent is not a folder.

Notes

At present, selectedFolderId determines whether the new note buttons are enabled and the parent of new notes and to-dos.

Currently, if a user navigates from a folder to a smart filter (e.g. "All Notes") or a tag, selectedFolderId maintains its previous value. This causes the "New Note" and "New To-Do" buttons to stay enabled, despite a folder not being selected in the sidebar.

However, when the app starts (before this pull request), selectedFolderId would be null, unless a folder is initially selected. This caused "New Note" and "New To-Do" to be disabled on desktop and extra logic to determine whether the "new note" FAB.Group should be enabled on mobile.

On startup, making selectedFolderId default to the last selected folder ID, rather than null, thus both fixes #10230 and is consistent with the last application state, before the app was closed.

Testing plan

  1. Select "All Notes"
  2. Quit Joplin
  3. Re-open Joplin
  4. Verify that the "new note" button can be used to create a new note.
  5. Click on a tag in the sidebar.
  6. Quit Joplin
  7. Re-open Joplin and verify that the "new to-do" button can be used to create a note.
    • Existing issue: New notes/to-dos created when a tag is selected aren't given that tag.
    • On mobile, the last open notebook should be shown when Joplin is re-opened. On desktop, the last-selected tag should still be selected.

This has been tested successfully on desktop/Ubuntu 24.04 and on mobile/iOS 17.4.

Note

There are a few things I don't like about this approach:

  • For it to work on mobile, the NAV_GO route needed to be changed. Otherwise, the default route (all notes) would clear activeFolderId. Changing this makes mobile more consistent with desktop, but I think it makes navigation logic more confusing.
  • Conceptually, it could more sense to have selectedFolderId be null when no folder is selected. A new state variable or settings.activeFolderId could then be used to determine the parent folder for new notes.
    • Drawbacks:
      • This would require more refactoring.
      • whenClause.oneFolderSelected is currently determined by selectedFolderId. If plugin commands have enabledConditions similar to the enabled condition for newNote, this could break plugins.

Comment on lines 886 to 890
case 'INITIAL_SELECTION_SET':
// To allow creating notes when opening the app with all notes and/or tags,
// we also need a "last selected folder ID".
draft.selectedFolderId ??= draft.settings.activeFolderId;
break;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It could be simpler to use existing events:

Suggested change
case 'INITIAL_SELECTION_SET':
// To allow creating notes when opening the app with all notes and/or tags,
// we also need a "last selected folder ID".
draft.selectedFolderId ??= draft.settings.activeFolderId;
break;
case 'SMART_FILTER_SELECT':
case 'TAG_SELECT':
// To allow creating notes when opening the app with all notes and/or tags,
// we also need a "last selected folder ID".
draft.selectedFolderId ??= draft.settings.activeFolderId;
break;

This would, however, require moving the logic to a separate switch statement.

Copy link
Owner

Choose a reason for hiding this comment

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

In general, the reducer actions are essentially setters - you provides some value and this is used to update the state. While in this case it's more like an event that has some side effect and I think we should avoid this pattern if possible (although it's present for a few actions).

If the goal here is to set the selected folder, isn't it possible to call FOLDER_SELECT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FOLDER_SELECT also sets notesParentType to 'Folder' and clears selectedNoteIds. However, this could be fine if FOLDER_SELECT is called before the initial selection is set.

An alternative could be to handle this in SETTING_UPDATE_ONE or SETTING_UPDATE_ALL, which would remove the need for dispatch({ type: 'SOME_TYPE_TO_SET_THE_INITIAL_SELECTION_HERE' }) in the initialization logic for mobile and desktop.

packages/app-mobile/root.tsx Outdated Show resolved Hide resolved
@personalizedrefrigerator personalizedrefrigerator marked this pull request as draft May 23, 2024 14:29
@personalizedrefrigerator personalizedrefrigerator marked this pull request as ready for review May 23, 2024 15:04
@personalizedrefrigerator personalizedrefrigerator added the desktop All desktop platforms label May 24, 2024
@laurent22 laurent22 merged commit 4e3326b into laurent22:dev May 28, 2024
10 checks passed
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.

Cannot create a new note or to-do after Joplin shutdown with All Notes or a tag selected in the sidebar
2 participants