Desktop: KeymapService: Initial implementation #3252

anjulalk · 2020-05-22T19:43:10Z

This PR includes the initial implementation of the KeymapService service. This service is responsible for abstracting the in-memory keymap.

It's now possible to correctly parse keymap-desktop.json file and update the keymap.

Replaces hardcoded Accelerators of menu items defined in the ElectronClient/app.js and ElectronClient/plugins/goToAnything.jsx files with the getAccelerator() method defined in the service class mentioned above.
Provides a simple setAccelerator() method for updating the in-memory keymap.
Provides method getKeymap() which compares current keymap with default keymap and returns an object with the changes. This object can be converted to a string and saved to the disk.
Provides method setKeymap() which accepts a custom keymap object as the parameter, validates, checks for duplicate Accelerators in the current platform and updates the in-memory keymap.
Uses static method isAccelerator() method for validating Accelerator strings.
Provides methods resetCommand() and resetKeymap() which resets shortcut for a single command and restores the entire keymap to default respectively.

The service is implemented using TypeScript classes and interfaces. All the methods currently implemented have been unit tested.

ElectronClient/keymapHandler.ts

anjulalk · 2020-06-05T20:31:09Z

I've removed label and section properties. I totally agree with your point here. This service should map an Accelerator to a command.

However, we need still to display labels for each command and group them by section in the GUI editor. I'm thinking maybe hard-coding them in the editor's code itself?

For example,
{ command: 'newNoteItem', label: _("New note"), section: 'File' }

I see no other option, at least until we get the actions centralized and sorted out. Would that approach make sense?

laurent22

Thanks for the update Anjula, I've added a few comments to the code.

Also I think the "CommandAndControl" thing is going to cause us troubles down the line. On Windows and Linux "CommandAndControl" is meaningless. And on macOS it also makes things more complicated - I create a CommandAndControl+S shortcut, so does it work when pressing Cmd+S and Ctrl+S or just one and not the other? Then if I create a separate Cmd+S shortcut, I guess that's a conflict? I also can't wrap my head how it's going to work on platforms without Command.

So my thinking is that we should not use this shortcut and only use either Cmd or Ctrl. In practice, I think it means there should be separate default config for Windows/Linux and for macOS. That's a bit of duplicated config but I think that's fine as it won't change that often. That's how Sublime Text handle the config and I think it makes sense. What do you think?

ElectronClient/app.js

laurent22 · 2020-06-20T18:39:51Z

ElectronClient/app.js

+				await KeymapService.instance().setKeymap(customKeymap);
+			} catch (error) {
+				const msg = error.message ? error.message : '';
+				console.error(`Failed to parse keymap ${keymapPath}\n${msg}`);


Error should be displayed to the user with an explanation on how to fix the problem. For example, by giving the line number where the error happens.

If there's an error in the JSON formatting, now it'll be shown in an error dialogue to the user (Standard error message thrown by the JSON.parse() function)

I've also added more helpful error messages.

However it's a bit complicated to find the line number of some object. I think we'll have to write a mini JSON parser to do that. Is that really necessary?

However it's a bit complicated to find the line number of some object. I think we'll have to write a mini JSON parser to do that. Is that really necessary?

No that's fine - we just display the errors the lib gives us. If they aren't very good then so be it. Later we can indeed think about using a more advanced parser with comment support, better error message, etc. but that's later.

ReactNativeClient/lib/services/KeymapService.ts

laurent22 · 2020-06-21T09:01:49Z

I thought about it again and as I see it there's just two cases for the keymap:

Default keymap, that applies to Linux and Windows (we don't explicitely support FreeBSD)
macOS keymap, which is just the default keymap, but with Control changed to Command, plus the Hide Window and Close Window commands.

CommandOrControl modifier should not be present in the keymap.

Please remove the "platforms" property from the keymap. Each platform should have its own separate keymap.json named keymap.macos.json, keymap.windows.json and keymap.linux.json.

Differences like this or this should be removed. All platforms should have the same basic shortcut and it's up to the user to change them to something else if they want.

In general I think your implementation is pretty good. I just wonder about the part where you load the keymap, as you rely on the log to handle errors, but log is just for us developers. We also need to tell the user about any issue, and for that throwing exceptions is best: something is unexpected? Just throw an exception and break the app. That's much better than letting it "work" but with undetected issues. If you throw exceptions from the beginning it will force you to find solutions on how to handle them well - for example by displaying a dialog box or a banner in the app. In some cases logging the error is reasonable too, for example for issues that are very unlikely.

laurent22 · 2020-06-21T09:23:05Z

What's your proposition to retrieve the label for a given action? As you will need this for the config screen. I can't really think of any good way without some refactoring but please let me know what you would suggest.

anjulalk · 2020-06-22T13:41:14Z

Could you clarify this a bit more?

Each platform should have its own separate keymap.json named keymap.macos.json, keymap.windows.json and keymap.linux.json.

Are you referring to the default keymap which is included in the source code?

If yes, rather than storing them as separate files, I think we should just include them in the source code as objects. Then we can dynamically use the appropriate keymap object.

I'll make the changes as soon as possible. Thanks for the review.

anjulalk · 2020-06-22T13:49:06Z

What's your proposition to retrieve the label for a given action? As you will need this for the config screen. I can't really think of any good way without some refactoring but please let me know what you would suggest.

I expect there would be a way to get all labels, and all commands after the refactoring. For the moment, I suppose we can hard-code these two properties in the config screen itself.

Would that be appropriate?

anjulalk · 2020-06-24T15:40:53Z

I've separated the keymap into two: One for macOS and one for all the other platforms.
Appropriate keymap for the current platform will be chosen at runtime.

I had to use synchronous functions for reading the keymap-desktop.json file as it's not possible to use promises in the constructor.
But I don't really think it matters because,

It's only runs at the startup. Therefore it'll not make the app unresponsive, and
For tiny files like this one, synchronous methods are actually faster because of not having the promise overhead. (Refer fs.readFileSync() reads hot tiny files much faster than fs.readFile() )

Menu items are still created even if they're not relevant to the current platform. I've added special checks to prevent calling getAccelerator() with unavailable commands in current platform keymap. Otherwise it'll throw an error.

More helpful errors will be shown to the user in an error dialogue box.

For some reason, the service doesn't seem to be able to access values from Setting model prior to the app initialization. That's why I moved the plugin loading after the app initialization. If GotoAnything plugin creates the service instance first, it cannot access the profile directory and fails to read the file. Hope that's okay.

Please have a look and give your feedback.

laurent22 · 2020-06-27T23:13:50Z

I expect there would be a way to get all labels, and all commands after the refactoring. For the moment, I suppose we can hard-code these two properties in the config screen itself.

Would that be appropriate?

Yes that's reasonable. Please just make sure the labels are exactly the same, so that they don't generate new translation strings (and that of course they are wrapped in _()). That will make it easier to refactor the labels, etc. if it's ever done.

ReactNativeClient/lib/services/KeymapService.ts

ElectronClient/app.js

ReactNativeClient/lib/services/KeymapService.ts

laurent22 · 2020-07-04T10:02:39Z

@anjulalk, please don't force-push your commits because then it makes it hard to know what has changed since the last time I reviewed. Was the only new commit this one? 680428b8e9f602151748a7fac0fb9aab33ccb83e

laurent22 · 2020-07-04T10:12:30Z

@anjulalk, I recently made some important changes to the way the commands work in Joplin, in this commit: c63c637

It will impact your work a bit because the way the accelerators are set is a bit different now, it's done like so: c63c637#diff-bff206348900b3c1ff4cbc14ca186272R509

I expect it shouldn't be too complicated to apply your changes on top though, for example using a regex search and replace, you could replace this:

cmdService.commandToMenuItem('newNote', 'CommandOrControl+N');

to this:

cmdService.commandToMenuItem('newNote', getAccelerator('newNote'));

Perhaps some of the command names will be slightly different too.

Finally, you'll need to change how you get the accelerator label in your Config screen. For that, please add a label(commandName:string):string method to CommandService that will return the label for a command. Have a look at isEnabled to see how it is done.

In any case I would recommend merging master to your branch as soon as possible to avoid diverging too much. And if you have any question about the command service, please let me know.

anjulalk · 2020-07-04T11:53:26Z

@anjulalk, please don't force-push your commits because then it makes it hard to know what has changed since the last time I reviewed. Was the only new commit this one? 680428b

Sorry about that. Yes, it's the only new one. I rebased to the latest commits on master branch and it messes up the history a bit.

I'll make the changes immediately.

anjulalk · 2020-07-05T12:10:43Z

@laurent22 Accelerator for "Print" command seems to be removed in c63c637
Was this intentional? I've re-added it just in case.

laurent22 · 2020-07-06T12:13:16Z

For some reason, the service doesn't seem to be able to access values from Setting model prior to the app initialization. That's why I moved the plugin loading after the app initialization. If GotoAnything plugin creates the service instance first, it cannot access the profile directory and fails to read the file. Hope that's okay.

I think it means that if the GotoAnything shortcut is changed it's never going to be updated, unless you restart the app. I'd say you should put the require statement where it was and make manifest.accelerator a function, so that it always get the latest value from your service.

anjulalk · 2020-07-18T21:20:33Z

Thanks for reviewing. I've made the requested changes. Please have a look if there are any issues.

laurent22

Thanks for the latest changes Anjula, I think we're getting there. Please don't add GUI-related files in here to keep the pull request small (I think the one you've added was by mistake but just want to confirm).

laurent22 · 2020-07-20T13:34:20Z

ReactNativeClient/lib/services/CommandService.ts

@@ -75,6 +76,8 @@ export default class CommandService extends BaseService {
 	private commandPreviousStates_:CommandStates = {};
 	private mapStateToPropsIID_:any = null;

+	private keymapService = KeymapService.instance();


That should be passed as a dependency to initialize() (like the store now). This makes it easier to control the initialization order, or indeed to find bugs in the program when we realize that for example the keymap service is initialized after the command service. Dependency injection also makes it easier to test.

I see. I added an extra parameter for the KeymapService instance to the initialize() function. The instance is now created in the app.js file and passed to CommandService.

laurent22 · 2020-07-20T13:34:41Z

ElectronClient/gui/KeymapConfigScreen.js

@@ -0,0 +1,106 @@
+'use strict';
+Object.defineProperty(exports, '__esModule', { value: true });


Please remove this file

Sorry about that. I removed the file.

laurent22 · 2020-07-20T13:35:13Z

ElectronClient/package-lock.json

@@ -10047,11 +10047,6 @@
        "through2": "^2.0.3"
      }


If package.json hasn't change, this file should not be included so please remove it.

I believe this is from a merge commit which had a new dependency for aws-sdk package. But I rebased on the latest commit on master so that I wouldn't need to commit this file, just to be sure..

laurent22 · 2020-07-24T17:17:09Z

CliClient/tests/services_KeymapService.js

+describe('services_KeymapService', () => {
+	describe('validateAccelerator', () => {
+		it('should identify valid Accelerators', () => {
+			const testCases = shim.isMac()


Tests should not be platform-specific, because if you make changes to one platform code, the tests might pass even though something is broken in another platform. It's best to pass the platform name to the KeymapService constructor for example. You can default the value to shim.platformName() if you want, but it should be possible to set it to something else.

This comment applies to all the uses of shim.isMac in the tests.

Thanks for pointing that out. I faced this exact situation situation after splitting the keymap into macOS and other platforms. What I did was either use the CI to see if tests had been passed, or boot up macOS in a VM and perform tests manually.

Providing a method to change the platform will enable it to test for both platforms at once. I'll come up with a solution to this.

Yes when we build the apps on CI (Travis and Appveyor) we could technically run platform-specific tests, however it's nice too to be able to run them all locally, and for that the easiest way (at least in this case) is to provide the platform to the service.

By the way, rather than changing the keymap based on a boolean (isMac) please make it change based on a string (platformName). So for now, your code would be very similar, just checking for platformName = 'darwin', but later we also have the option to add support for other platform-specific shortcuts.

laurent22 · 2020-08-01T17:23:40Z

ReactNativeClient/lib/services/CommandService.ts

@@ -1,4 +1,5 @@
 const BaseService = require('lib/services/BaseService');
+const KeymapService = require('lib/services/KeymapService.js');


Since KeymapService is a TypeScript file, you should import it with import KeymapService from "lib/services/KeymapService" so as to benefit from type-checking.

laurent22 · 2020-08-01T17:24:27Z

ReactNativeClient/lib/services/CommandService.ts

+	private keymapService:typeof KeymapService = null;
+
+	initialize(store:any, keymapService:typeof KeymapService) {


Once you've updated the import statement, the "typeof KeymapService" should no longer be needed, and you can just do keymapService:KeymapService

Thanks for pointing that out. I'll update it right away.

laurent22 · 2020-08-01T17:26:22Z

ReactNativeClient/lib/services/KeymapService.ts

+			this.modifiersRegExp = modifiersRegExp.default;
+			break;
+		default:
+			throw new Error(`Unsupported platform: ${platform}`);


Why throw an exception here? It should be either "darwin" or "default". There's no need to list all the supported platforms. That way if it's a platform we don't know about it will work instead of crashing for no particular reason.

…te; Remove side-effects from validation method

… validateAccelerator()

Refactor import statements Removed explicit platform support

anjulalk · 2020-08-02T02:52:33Z

I've changed import statements and removed explicit platform checks. Please have a look and give your feedback.

laurent22 · 2020-08-02T11:13:02Z

Thanks @anjulalk, I'm going to check, but I'm wondering about your current git workflow. Since you've used forced push I have no idea what you've changed since last time. It looks like all your commits were done 8 hours ago, even though they've been there for several months. What it means is that I cannot follow your progress - I have to start over and review everything every time.

So I'm wondering why is there a need for force push? Is that because you rebase from dev? Instead of rebasing, could you merge dev in your branch, then do a regular push? Or are there some issues that are difficult to solve without force push?

laurent22 · 2020-08-02T11:26:26Z

Other than the force push issue it's all looking good, so let's merge. Good job Anjula!

anjulalk force-pushed the keymap-handler branch 2 times, most recently from e839134 to 2be3878 Compare May 29, 2020 09:01

laurent22 reviewed Jun 3, 2020

View reviewed changes

ElectronClient/keymapHandler.ts Outdated Show resolved Hide resolved

ElectronClient/keymapHandler.ts Outdated Show resolved Hide resolved

anjulalk force-pushed the keymap-handler branch from f674ca3 to c1e8746 Compare June 5, 2020 20:00

anjulalk changed the title ~~Desktop: Keymap handler module: initial implementation~~ Desktop: KeymapService: Initial implementation Jun 5, 2020

anjulalk force-pushed the keymap-handler branch from c1e8746 to f443723 Compare June 5, 2020 20:29

anjulalk force-pushed the keymap-handler branch 4 times, most recently from a82a0a3 to 5deec3b Compare June 13, 2020 12:18

laurent22 reviewed Jun 20, 2020

View reviewed changes

laurent22 reviewed Jun 21, 2020

View reviewed changes

ReactNativeClient/lib/services/KeymapService.ts Outdated Show resolved Hide resolved

anjulalk force-pushed the keymap-handler branch from 5deec3b to 40c794a Compare June 24, 2020 14:42

anjulalk force-pushed the keymap-handler branch from 90087c1 to 360557b Compare June 25, 2020 11:14

anjulalk marked this pull request as ready for review June 25, 2020 11:14

anjulalk force-pushed the keymap-handler branch from 360557b to 98489c9 Compare June 25, 2020 14:42

laurent22 reviewed Jun 27, 2020

View reviewed changes

anjulalk force-pushed the keymap-handler branch from 2fe0b64 to 680428b Compare July 2, 2020 18:41

tessus added the gsoc-2020 label Jul 18, 2020

anjulalk force-pushed the keymap-handler branch 2 times, most recently from 926b181 to f375718 Compare July 18, 2020 20:59

anjulalk force-pushed the keymap-handler branch from ddb28d7 to ef33dc8 Compare July 20, 2020 02:08

laurent22 reviewed Jul 20, 2020

View reviewed changes

anjulalk force-pushed the keymap-handler branch from ef33dc8 to cb25535 Compare July 24, 2020 11:20

laurent22 reviewed Jul 24, 2020

View reviewed changes

laurent22 changed the base branch from master to dev August 1, 2020 16:59

laurent22 requested changes Aug 1, 2020

View reviewed changes

Anjula Karunarathne added 15 commits August 2, 2020 08:20

KeymapService: Initial implementation

f18fb95

Fix failing tests

461b4e1

Remove redundant logger initialization; Made validation methods priva…

45997b8

…te; Remove side-effects from validation method

Remove multiple-context support

aadd77a

Minor tweaks

b7ab530

Move keymap parsing logic into loadKeymap() function

301cf66

Remove localizeAccelerator() in favor of strict accelerator formatting

4641755

Fix failing tests

549a381

Remove getKeymap()

e19ad56

Use getAccelerator() within CommandService; Rename isAccelerator() to…

d55cc1c

… validateAccelerator()

Remove mistakenly added KeymapConfigScreen.js

9736c36

KeymapService dependency injection

fd00a47

Refactor KeymapService

2b584c9

Fix failing tests

0503576

Run unit tests for all platforms in the current platform

d84cbcb

Refactor import statements Removed explicit platform support

anjulalk force-pushed the keymap-handler branch from d9a549e to d84cbcb Compare August 2, 2020 02:51

laurent22 merged commit 88f22fa into laurent22:dev Aug 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desktop: KeymapService: Initial implementation #3252

Desktop: KeymapService: Initial implementation #3252

anjulalk commented May 22, 2020 •

edited

Loading

anjulalk commented Jun 5, 2020 •

edited

Loading

laurent22 left a comment

laurent22 Jun 20, 2020

anjulalk Jun 24, 2020

laurent22 Jun 27, 2020

laurent22 commented Jun 21, 2020

laurent22 commented Jun 21, 2020

anjulalk commented Jun 22, 2020

anjulalk commented Jun 22, 2020

anjulalk commented Jun 24, 2020 •

edited

Loading

laurent22 commented Jun 27, 2020

laurent22 commented Jul 4, 2020

laurent22 commented Jul 4, 2020

anjulalk commented Jul 4, 2020

anjulalk commented Jul 5, 2020 •

edited

Loading

laurent22 commented Jul 6, 2020

anjulalk commented Jul 18, 2020

laurent22 left a comment

laurent22 Jul 20, 2020

anjulalk Jul 24, 2020

laurent22 Jul 20, 2020

anjulalk Jul 24, 2020

laurent22 Jul 20, 2020

anjulalk Jul 24, 2020 •

edited

Loading

laurent22 Jul 24, 2020

anjulalk Jul 25, 2020

laurent22 Jul 25, 2020

laurent22 Aug 1, 2020 •

edited

Loading

laurent22 Aug 1, 2020

anjulalk Aug 2, 2020

laurent22 Aug 1, 2020

anjulalk commented Aug 2, 2020

laurent22 commented Aug 2, 2020

laurent22 commented Aug 2, 2020

		@@ -0,0 +1,106 @@
		'use strict';
		Object.defineProperty(exports, '__esModule', { value: true });

		@@ -1,4 +1,5 @@
		const BaseService = require('lib/services/BaseService');
		const KeymapService = require('lib/services/KeymapService.js');

		private keymapService:typeof KeymapService = null;

		initialize(store:any, keymapService:typeof KeymapService) {

Desktop: KeymapService: Initial implementation #3252

Desktop: KeymapService: Initial implementation #3252

Conversation

anjulalk commented May 22, 2020 • edited Loading

anjulalk commented Jun 5, 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

Choose a reason for hiding this comment

laurent22 commented Jun 21, 2020

laurent22 commented Jun 21, 2020

anjulalk commented Jun 22, 2020

anjulalk commented Jun 22, 2020

anjulalk commented Jun 24, 2020 • edited Loading

laurent22 commented Jun 27, 2020

laurent22 commented Jul 4, 2020

laurent22 commented Jul 4, 2020

anjulalk commented Jul 4, 2020

anjulalk commented Jul 5, 2020 • edited Loading

laurent22 commented Jul 6, 2020

anjulalk commented Jul 18, 2020

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

Choose a reason for hiding this comment

anjulalk Jul 24, 2020 • edited Loading

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 Aug 1, 2020 • edited Loading

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 2, 2020

laurent22 commented Aug 2, 2020

laurent22 commented Aug 2, 2020

anjulalk commented May 22, 2020 •

edited

Loading

anjulalk commented Jun 5, 2020 •

edited

Loading

anjulalk commented Jun 24, 2020 •

edited

Loading

anjulalk commented Jul 5, 2020 •

edited

Loading

anjulalk Jul 24, 2020 •

edited

Loading

laurent22 Aug 1, 2020 •

edited

Loading