Desktop: Fixes #10685: Disable permanentlyDeleteNote command when the…

… note list is not focused

This is one option for fixing #10685. It has a few drawbacks:
- Users might expect a cusom keyboard shortcut for permanentlyDeleteNote to be
  activatable when other parts of the app are focused.
- This disables the note > permanently delete note toolbar action when
  the note list doesn't have focus. Ideally, it would still be possible
  to permanently delete notes from this menu without first focusing the
  note list.

packages/app-desktop/gui/MainScreen/commands/permanentlyDeleteNote.ts

@@ -25,6 +25,6 @@ export const runtime = (): CommandRuntime => {
 				await Note.batchDelete(noteIds, { toTrash: false, sourceDescription: 'permanentlyDeleteNote command' });
 			}
 		},
-		enabledCondition: '(!noteIsReadOnly || inTrash) && someNotesSelected',
+		enabledCondition: '(!noteIsReadOnly || inTrash) && (noteListFocused || inTrash) && someNotesSelected',
 	};
 };

packages/app-desktop/gui/NoteList/NoteList2.tsx

@@ -1,5 +1,5 @@
 import * as React from 'react';
-import { useMemo, useRef, useEffect } from 'react';
+import { useMemo, useRef, useEffect, useCallback } from 'react';
 import { AppState } from '../../app.reducer';
 import BaseModel, { ModelType } from '@joplin/lib/BaseModel';
 import { Props } from './utils/types';
@@ -262,6 +262,20 @@ const NoteList = (props: Props) => {
 		return output;
 	}, [listRenderer.flow]);
 
+	const onFocus = useCallback(() => {
+		props.dispatch({
+			type: 'FOCUS_SET',
+			field: 'noteList',
+		});
+	}, [props.dispatch]);
+
+	const onBlur = useCallback(() => {
+		props.dispatch({
+			type: 'FOCUS_CLEAR',
+			field: 'noteList',
+		});
+	}, [props.dispatch]);
+
 	return (
 		<div
 			className="note-list"
@@ -270,6 +284,8 @@ const NoteList = (props: Props) => {
 			onScroll={onScroll}
 			onKeyDown={onKeyDown}
 			onDrop={onDrop}
+			onFocus={onFocus}
+			onBlur={onBlur}
 		>
 			{renderEmptyList()}
 			{renderFiller('top', topFillerStyle)}

packages/app-desktop/gui/NoteList/utils/useOnKeyDown.ts

@@ -8,6 +8,7 @@ import { Dispatch } from 'redux';
 import { FocusNote } from './useFocusNote';
 import { ItemFlow } from '@joplin/lib/services/plugins/api/noteListType';
 import { KeyboardEventKey } from '@joplin/lib/dom';
+import KeymapService from '@joplin/lib/services/KeymapService';
 
 const useOnKeyDown = (
 	selectedNoteIds: string[],
@@ -104,7 +105,14 @@ const useOnKeyDown = (
 			event.preventDefault();
 		}
 
-		if (noteIds.length && (key === 'Delete' || (key === 'Backspace' && event.metaKey))) {
+		if (
+			noteIds.length &&
+			CommandService.instance().isEnabled('permanentlyDeleteNote')
+			&& KeymapService.instance().eventMatchesCommandAccelerator(event, 'permanentlyDeleteNote')
+		) {
+			event.preventDefault();
+			void CommandService.instance().execute('permanentlyDeleteNote', noteIds);
+		} else if (noteIds.length && (key === 'Delete' || (key === 'Backspace' && event.metaKey))) {
 			event.preventDefault();
 			if (CommandService.instance().isEnabled('deleteNote')) {
 				void CommandService.instance().execute('deleteNote', noteIds);

packages/app-desktop/integration-tests/noteList.spec.ts

@@ -1,5 +1,7 @@
 import { test, expect } from './util/test';
 import MainScreen from './models/MainScreen';
+import activateMainMenuItem from './util/activateMainMenuItem';
+import setMessageBoxResponse from './util/setMessageBoxResponse';
 
 test.describe('noteList', () => {
 	test('should be possible to edit notes in a different notebook when searching', async ({ mainWindow }) => {
@@ -35,4 +37,42 @@ test.describe('noteList', () => {
 		// Updating the title should force the sidebar to update sooner
 		await expect(editor.noteTitleInput).toHaveValue('note-1');
 	});
+
+	test('shift-delete should ask to permanently delete notes, but only when the note list is focused', async ({ electronApp, mainWindow }) => {
+		const mainScreen = new MainScreen(mainWindow);
+		const sidebar = mainScreen.sidebar;
+
+		const folderBHeader = await sidebar.createNewFolder('Folder B');
+		const folderAHeader = await sidebar.createNewFolder('Folder A');
+		await expect(folderAHeader).toBeVisible();
+
+		await mainScreen.createNewNote('test note 1');
+		await mainScreen.createNewNote('test note 2');
+
+		await activateMainMenuItem(electronApp, 'Note list', 'Focus');
+		await expect(mainScreen.noteListContainer.getByText('test note 1')).toBeVisible();
+
+		await setMessageBoxResponse(electronApp, /^Delete/i);
+
+		const pressShiftDelete = async () => {
+			await mainWindow.keyboard.press('Shift');
+			await mainWindow.keyboard.press('Delete');
+			await mainWindow.keyboard.up('Delete');
+			await mainWindow.keyboard.up('Shift');
+		};
+		await pressShiftDelete();
+
+		await folderBHeader.click();
+		await folderAHeader.click();
+		await expect(mainScreen.noteListContainer.getByText('test note 2')).not.toBeVisible();
+
+		// Should not delete when the editor is focused
+		await mainScreen.noteEditor.focusCodeMirrorEditor();
+		await mainWindow.keyboard.type('test');
+		await pressShiftDelete();
+
+		await folderBHeader.click();
+		await folderAHeader.click();
+		await expect(mainScreen.noteListContainer.getByText('test note 1')).toBeVisible();
+	});
 });

packages/app-desktop/services/commands/stateToWhenClauseContext.ts

@@ -22,6 +22,8 @@ export default function stateToWhenClauseContext(state: AppState, options: WhenC
 		sidebarVisible: !!state.mainLayout && layoutItemProp(state.mainLayout, 'sideBar', 'visible'),
 		noteListHasNotes: !!state.notes.length,
 
+		noteListFocused: state.focusedField === 'noteList',
+
 		// Deprecated
 		sideBarVisible: !!state.mainLayout && layoutItemProp(state.mainLayout, 'sideBar', 'visible'),
 	};

packages/lib/services/KeymapService.test.js

@@ -337,4 +337,12 @@ describe('services_KeymapService', () => {
 			}
 		});
 	});
+
+	it('should check whether a DOM event matches an accelerator', () => {
+		keymapService.initialize();
+		keymapService.overrideKeymap([{ command: 'print', accelerator: 'Ctrl+Alt+Shift+A' }]);
+		const mockEvent = { code: 'KeyA', keyCode: 65, key: 'a', shiftKey: true, ctrlKey: true, altKey: true };
+		expect(keymapService.eventMatchesCommandAccelerator(mockEvent, 'print')).toBe(true);
+		expect(keymapService.eventMatchesCommandAccelerator(mockEvent, 'gotoAnything')).toBe(false);
+	});
 });

packages/lib/services/KeymapService.ts

@@ -3,6 +3,7 @@ import shim from '../shim';
 import { _ } from '../locale';
 import keysRegExp from './KeymapService_keysRegExp';
 import keycodeToElectronMap from './KeymapService_keycodeToElectronMap';
+import type * as React from 'react';
 
 import BaseService from './BaseService';
 
@@ -416,6 +417,11 @@ export default class KeymapService extends BaseService {
 		return parts.join('+');
 	}
 
+	public eventMatchesCommandAccelerator(event: KeyboardEvent|React.KeyboardEvent, command: string) {
+		const accelerator = this.domToElectronAccelerator(event);
+		return this.getAccelerator(command) === accelerator;
+	}
+
 	public on<Name extends EventName>(eventName: Name, callback: EventListenerCallback<Name>) {
 		eventManager.on(eventName, callback);
 	}