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 #10685: Fix shift-delete asks to permanently delete the current note, rather than cut text, when the editor is selected. #10687

Conversation

personalizedrefrigerator
Copy link
Collaborator

@personalizedrefrigerator personalizedrefrigerator commented Jul 5, 2024

Summary

Fixes #10685 by:

  1. Handling shift-delete in the note list component, rather than with the global KeymapService.
  2. Removing the default shift-delete modifier for permanentlyDeleteNote from the global keyboard shortcuts.

This also fixes an issue where shift-delete deleted notes to the trash, rather than permanently, when the note list was focused.

Note

This pull request targets release-3.0 because I consider shift-delete no longer cutting text to be a regression.

See 8c4aa7a for an alternate fix.

Comparison with 8c4aa7a

  • Benefit: It's possible to permanently delete the current note using Note > Permanently Delete Note, as before.
  • Benefit: Shift-Delete still works, but only when the note list is focused.
  • Benefit: It's possible to assign a custom keyboard shortcut to permanentlyDeleteNote that works regardless of what has focus.
  • Drawback: The Shift-Delete default keyboard shortcut is now more difficult to discover -- it is no longer shown as the keyboard shortcut for "permanently delete note" by the Electron menu UI.

Testing plan

This pull request contains an automated Playwright test that 1) delete a note permanently with shift-delete and 2) verifies that pressing shift-delete while the markdown editor is focused does not delete the current note.

It has also been verified (on Ubuntu 24.04) that:

  • Note > "Permanently delete note" can be used to delete the focused note, even if the note list is not focused.
  • Right-click > "Permanently delete note" can be used to permanently delete a note from the trash folder.
  • Opening the Rich Text Editor, selecting text, then pressing shift-delete cuts the selected text to the clipboard.

…ermanentlyDeleteNote, activate it when pressing shift-delete and focusing the note list.

This commit is an alternate fix for laurent22#10685.

Comparison to 8c4aa7a:
- Benefit: It's possible to permanently delete the current note using
  Note > Permanently Delete Note, as before.
- Benefit: Shift-Delete still works, but only when the note list is
  focused.
- Benefit: It's possible to assign a custom keyboard shortcut to
  permanentlyDeleteNote that works regardless of what has focus.
- Drawback: The Shift-Delete default keyboard shortcut is now more
  difficult to discover -- it is no longer suggested by the Electron
  menu UI.
@laurent22
Copy link
Owner

I'm not a fan of the bespoke code to enable/disable the shortcut but as you mentioned there are drawbacks to all solutions. Let's merge for now since that fixes the regression.

But if the main drawback of 8c4aa7a is that the menu item doesn't work, maybe we can somehow make it work?

@laurent22 laurent22 merged commit 2fd6a3a into laurent22:release-3.0 Jul 6, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants