Desktop: Keyboard shortcut editor #3525

anjulalk · 2020-07-18T18:53:26Z

Contains the implementation of Keyboard shortcut editor GUI. This PR includes,

Integration with KeymapService and CommandService
Display a list of available commands along with their keyboard shortcuts
Allow editing a keyboard shortcut, or reset it to the default shortcut
Synchronize changes with the keymap-desktop.json file in the profile directory
Search for keyboard shortcuts by their command name or label

laurent22

@anjulalk, I've left a few comments, although I realise it's a WIP so maybe you were already planning to do some of what I wrote.

One thing I would note is that you should check existing .tsx components in the codebase - how they are organised, what are the naming conventions, etc. Large effects should be moved to custom hooks in separate file, although you don't have any such hooks yet.

laurent22 · 2020-07-20T17:47:00Z

ElectronClient/gui/KeymapConfigScreen.tsx

+const initialKeymap = commandNames.map(commandName => {
+	return {
+		label: CommandService.instance().label(commandName),
+		accelerator: getAccelerator(commandName),
+		command: commandName,
+		isEditing: false,
+	};
+});


That should be loaded inside the component, using useEffect for example. In general, only constants or functions can be defined outside the component. Any state-related variables should be defined inside the component (or as hooks).

I've passed a state initialization function to useState(). It'll only be initialized once per lifetime.

ElectronClient/gui/KeymapConfigScreen.tsx

ReactNativeClient/lib/KeymapUtils.ts

ReactNativeClient/lib/services/CommandService.ts

anjulalk · 2020-07-25T11:43:30Z

Thanks a lot for the early feedback. It'll certainly help me move in the right direction. I believe I addressed all the issues you've mentioned.

I'll continue to refine the code from examples as I progress. Thanks!

…ectronAccelerator functions in KeymapService itself

laurent22

Hi @anjulalk, I've left a few more comments. One thing that's not present yet is the logic to save keymaps. I think it would be best to get this in relatively soon so that we can get a full picture of how it's going to work.

ElectronClient/gui/KeymapConfig/KeymapConfigScreen.tsx

ElectronClient/gui/KeymapConfig/ShortcutRecorder.tsx

ElectronClient/gui/KeymapConfig/KeymapConfigScreen.tsx

ElectronClient/gui/KeymapConfig/styles/index.ts

ReactNativeClient/lib/components/shared/config-shared.js

…itor

laurent22

Thanks for the update @anjulalk. Overall the design of your component is pretty good and I like how you use array functions like map, reduce and filter to make the intent of your code clear. I’ve left a few comments, which are a bit short as I’m on a tablet, so if something’s unclear please let me know.

ElectronClient/gui/KeymapConfig/KeymapConfigScreen.tsx

laurent22 · 2020-08-15T22:21:52Z

ElectronClient/gui/KeymapConfig/KeymapConfigScreen.tsx

+
+// Some commands aren't actually registered in CommandService
+// These hardcoded labels are used for those commands instead
+const commandLabels: CommandLabels = {


Do the labels get updated when the user changes the language ?

I wrapped this logic in a function. Now it's working even after changing the language.

ElectronClient/gui/KeymapConfig/KeymapConfigScreen.tsx

ReactNativeClient/lib/services/KeymapService.ts

laurent22 · 2020-08-15T22:48:23Z

ElectronClient/gui/KeymapConfig/ShortcutRecorder.tsx

+	toggleEditing: () => void,
+}
+
+export const ShortcutRecorder = ({ setAccelerator, resetAccelerator, toggleEditing, theme }: ShortcutRecorderProps) => {


The child component logic should be decoupled from that of the parent. ShortcutRecorder should only take three event handlers: onSave, onCancel, onReset then on the parent component you handle the business logic to set keymap.

In fact I don’t see anything in this component to cancel input but that should be present

I've updated the logic to use handlers instead. Thanks for pointing that out.

ElectronClient/gui/KeymapConfig/KeymapConfigScreen.tsx

laurent22 · 2020-08-18T12:57:02Z

@anjulalk, any update on your pull request? If there's something tricky about some of my comments, please let me know.

anjulalk · 2020-08-19T16:30:30Z

I'm still working on some issues. I'll be able to push the changes within the next 24 hours. Apologies for the delay.

…itor

ElectronClient/gui/KeymapConfig/KeymapConfigScreen.tsx

laurent22 · 2020-08-23T20:41:55Z

ReactNativeClient/lib/services/KeymapService.ts

+export class KeymapError extends Error {
+	private details_: KeymapErrorDetails;
+
+	constructor(message: string, details: KeymapErrorDetails = {}) {
+		super(message);
+		this.details_ = {
+			missingAccelerator: false,
+			missingCommand: false,
+			invalidAccelerator: false,
+			invalidCommand: false,
+			duplicateAccelerators: false,
+			...details,
+		};
+	}
+
+	get details() {
+		return this.details_;
+	}


Could you extend JoplinError and simply set the code to some string? I think the details object with potentially multiple error causes seem unnecessarily complex. The keymap shortcut should be validated when the user inputs it anyway so we shouldn't have so much complexity going on. Check what they type, throw back an error with a message and that's almost all we need.

I see. In that case I'll add some error handling to the ShortcutRecorder component. I'll update it.

Is it okay to keep just an array of invalid Keymap items, so that we can show the little status icon next to the shortcut?

What could cause the keymap to have multiple invalid items? If an item is invalid it shouldn't be possible to save it anyway, so there could only be one invalid item (the currently active, but unsaved, one), or am I missing something?

No, you're right. We should prevent the user from causing the error by validating the shortcut before the user sets it.
I'm working on it at the moment. I'll push an update in the next 24 hours.

laurent22

@anjulalk, thanks for the recent changes. I found just two remaining issues and I think it will be ready.

laurent22 · 2020-08-29T08:51:07Z

ElectronClient/gui/KeymapConfig/KeymapConfigScreen.tsx

+
+			<div style={styles.container}>
+				<div style={styles.topActions}>
+					<input


Looks like the Search label hasn't been added yet?

laurent22 · 2020-08-29T08:56:21Z

ReactNativeClient/lib/services/KeymapService.ts

+export class KeymapError extends Error {
+	// Translated error message for the GUI
+	private altMessage_: string;
+
+	constructor(message: string, altMessage: string) {
+		super(message);
+		this.altMessage_ = altMessage;
+	}
+
+	get altMessage() {
+		return this.altMessage_;
+	}
+}
+


Please remove the altMessage property - error should only have one message. In fact, as far as I can see there's no actual use for the KeymapError object anymore so please remove KeymapError and use a regular Error object.

To throw the error, please use this:

new Error(_('Accelerator "%s" is already used for command "%s".', item.accelerator, originalItem.command));

Thanks for mentioning the example. I agree that it's unneccessary. My initial idea was give a friendly error message in the GUI.
I'll replace them with one message.

anjulalk · 2020-08-30T18:22:19Z

I've added the label for the search input. Previously I thought you meant the aria-label property, so I added it. Sorry about that.

anjulalk · 2020-08-30T18:24:24Z

I've pushed an update with Import/Export keymap functionality. Please review when you've got the time. I tested every feature and everything seems to be working as expected.

I've also cleaned up some unused code, and refactored most of the code. It should be much clear and easy to follow now.

laurent22

Thanks for the update @anjulalk, I've tried the editor and it's all looking good. I've only left two comments for the import/export feature.

Also please could you add the following tooltips?

When a shortcut is invalid (for example for a duplicate shortcut), you display a small warning icon to the right, which is good, but please could you also add a tooltip on this icon with the complete error message (which would be from the error object that you throw)?
Please could you add a tooltip over the shortcut recorder input field? It should be "Press the shortcut, then press ENTER. Or BACKSPACE to clear the shortcut".

laurent22 · 2020-08-30T19:10:03Z

ElectronClient/gui/KeymapConfig/KeymapConfigScreen.tsx

+		if (filePath) {
+			try {
+				// KeymapService is already synchronized with the in-state keymap
+				await exportCustomKeymap(filePath);


Please call await keymapService.saveCustomKeymap(filePath); directly from here. You're importing KeymapService into this component anyway so the additional layer of indirection is unnecessary. Also it's best to keep React hooks small. So if you really wanted to use a hook here, it would be best to create a new one like "useImportExport" and put the import/export feature there, but it won't be necessary to do it now.

I agree, it's unneccessary. I'll remove it and call the method directly.

ElectronClient/gui/KeymapConfig/KeymapConfigScreen.tsx

…lling the KeymapService method

anjulalk · 2020-09-01T06:55:59Z

I made the requested changes base on your feedback. Please have a look.

After adding the label, it looks a bit odd because the label is "Search" and the search box also says "Search...". Can we change the search box placeholder to something like "Type in here to search...", or something like that?

IMHO, I think it'd look better without the label. But the current layout is fine too.

laurent22 · 2020-09-01T21:46:38Z

Thanks for the update @anjulalk.

After adding the label, it looks a bit odd because the label is "Search" and the search box also says "Search...".

Hmm, now that you say it, it indeed doesn't look right. Could you remove it actually and make it like it was before? Sorry about that!

Also you have some conflicts on the ignore files. Once these small changes are done I think it's all good, and we can merge.

anjulalk · 2020-09-05T19:18:10Z

I've updated the pull request. Please review when you have the time. Thanks!

laurent22 · 2020-09-06T12:00:04Z

That's great, thanks @anjulalk! We'll make the feature part of the next release.

anjulalk marked this pull request as draft July 18, 2020 18:53

tessus added gsoc-2020 desktop All desktop platforms labels Jul 18, 2020

laurent22 reviewed Jul 20, 2020

View reviewed changes

Anjula Karunarathne added 5 commits August 2, 2020 08:08

Blank config screen

f09a896

Basic UI structure

4763157

Initial keyboard shortcut editing workflow

2b8bf90

Remove KeymapUtils.ts in favor of including domToElectronKey, domToEl…

5ba4c90

…ectronAccelerator functions in KeymapService itself

Refactor into subfolders

66c5375

anjulalk force-pushed the keyboard-shortcut-editor branch from e052cd4 to 66c5375 Compare August 2, 2020 02:40

Improve styling

5a46b5d

laurent22 reviewed Aug 3, 2020

View reviewed changes

Anjula Karunarathne added 4 commits August 4, 2020 14:00

Merge remote-tracking branch 'upstream/dev' into keyboard-shortcut-ed…

cd5d574

…itor

Merge remote-tracking branch 'upstream/dev' into keyboard-shortcut-ed…

0f91db7

…itor

Integrate with KeymapService

8440a9c

Merge remote-tracking branch 'upstream/dev' into keyboard-shortcut-ed…

3465af9

…itor

anjulalk changed the base branch from master to dev August 14, 2020 22:53

Anjula Karunarathne and others added 4 commits August 15, 2020 14:35

Make compatible with macOS

fa08c28

Fix Goto Anything shortcut not updating

57a68b9

Simple filtering

de493e6

Fix false warning when disabling a shortcut

51bc13f

laurent22 requested changes Aug 15, 2020

View reviewed changes

Anjula Karunarathne added 4 commits August 20, 2020 15:55

Fix command labels not translating

52dbc6e

Rename property theme to themeId

0b309d9

Use handlers

8ca3aa0

Refresh menu by emitting an event

42c7122

Merge remote-tracking branch 'upstream/dev' into keyboard-shortcut-ed…

3568c70

…itor

laurent22 requested changes Aug 23, 2020

View reviewed changes

Anjula Karunarathne added 2 commits August 25, 2020 21:03

Minor refactoring

43b2bc1

Improve UI/UX

e274acd

anjulalk marked this pull request as ready for review August 27, 2020 21:10

anjulalk changed the title ~~Desktop: Keyboard shortcut editor (WIP)~~ Desktop: Keyboard shortcut editor Aug 27, 2020

Anjula Karunarathne added 2 commits August 28, 2020 16:54

Minor refactoring

bc36748

Fix not being allowed to disable shortcuts

989b3d2

anjulalk force-pushed the keyboard-shortcut-editor branch from 09281ad to 989b3d2 Compare August 28, 2020 12:35

laurent22 requested changes Aug 29, 2020

View reviewed changes

Anjula Karunarathne added 3 commits August 30, 2020 19:36

Import/Export keymap configuration

6ab5ccc

Refactor and code clean-up

471210f

Add label for Search

d5732ff

laurent22 requested changes Aug 30, 2020

View reviewed changes

Anjula Karunarathne added 4 commits August 31, 2020 02:43

Remove unneccessary translated string

8ca20ad

Remove unnecessary method from useKeymap hook in favor of directly ca…

e6643b9

…lling the KeymapService method

Add tooltips

a6d793b

Fix a bug where it failed to find both of duplicate keymap items

80b831d

anjulalk force-pushed the keyboard-shortcut-editor branch 2 times, most recently from 8ade9a2 to b73ad39 Compare September 5, 2020 19:07

Remove search label

88f5300

anjulalk force-pushed the keyboard-shortcut-editor branch from b73ad39 to 88f5300 Compare September 5, 2020 19:13

Merge branch 'dev' into keyboard-shortcut-editor

0bd89c9

laurent22 merged commit a8296e2 into laurent22:dev Sep 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desktop: Keyboard shortcut editor #3525

Desktop: Keyboard shortcut editor #3525

anjulalk commented Jul 18, 2020 •

edited

Loading

laurent22 left a comment

laurent22 Jul 20, 2020

anjulalk Jul 25, 2020

anjulalk commented Jul 25, 2020

laurent22 left a comment

laurent22 left a comment

laurent22 Aug 15, 2020

anjulalk Aug 21, 2020

laurent22 Aug 15, 2020

anjulalk Aug 21, 2020

laurent22 commented Aug 18, 2020

anjulalk commented Aug 19, 2020

laurent22 Aug 23, 2020

anjulalk Aug 24, 2020

anjulalk Aug 24, 2020

laurent22 Aug 24, 2020

anjulalk Aug 25, 2020

laurent22 left a comment

laurent22 Aug 29, 2020

laurent22 Aug 29, 2020

anjulalk Aug 30, 2020

anjulalk commented Aug 30, 2020

anjulalk commented Aug 30, 2020 •

edited

Loading

laurent22 left a comment

laurent22 Aug 30, 2020

anjulalk Aug 30, 2020

anjulalk commented Sep 1, 2020

laurent22 commented Sep 1, 2020

anjulalk commented Sep 5, 2020

laurent22 commented Sep 6, 2020

Desktop: Keyboard shortcut editor #3525

Desktop: Keyboard shortcut editor #3525

Conversation

anjulalk commented Jul 18, 2020 • edited Loading

laurent22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anjulalk commented Jul 25, 2020

laurent22 left a comment

Choose a reason for hiding this comment

laurent22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurent22 commented Aug 18, 2020

anjulalk commented Aug 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurent22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anjulalk commented Aug 30, 2020

anjulalk commented Aug 30, 2020 • edited Loading

laurent22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anjulalk commented Sep 1, 2020

laurent22 commented Sep 1, 2020

anjulalk commented Sep 5, 2020

laurent22 commented Sep 6, 2020

anjulalk commented Jul 18, 2020 •

edited

Loading

anjulalk commented Aug 30, 2020 •

edited

Loading