Desktop: Editor commands

[CalebJohn]

Based on the discussion in #4136.

The change in CodeMirror.tsx is to fix a bug that prevented the editor context menu from refreshing.

I also needed to add props.plugins to the useWindowCommand dependencies to ensure that the plugin editor command runtimes are updated properly.

I noticed when working on the example plugin, that the accelerator is bound in the keymap settings, but doesn't actually work until the user manually rebinds the command in the keymap editor.

[laurent22]

Thanks for the update @CalebJohn, but it seems to undo the plugin framework update on the content_script demo. Could you try to update it with the latest version of the framework please? If you run npm install -g generator-joplin && yo joplin --update from the directory that should do it.

[CalebJohn]

Sorry about that, it's fixed now.

[laurent22]

Looks good now, thanks!

[laurent22]

I think the issue is here. You are registering a runtime for the commands in CommandService.instance().editorCommandDeclarations(), but there are some more in editorCommandDeclarations , such as "insertText" or "scrollToHash", for which you are no longer registering a runtime.

I'm not sure the logic of all this is sound. Everything in editorCommandDeclarations.ts should be registered, while CommandService.editorCommandDeclarations is doing some filtering.

Also without looking at the method, it's strange that these two functions with identical names are not doing the same thing, which again makes me think the design is not quite right.

I'm kind of lost with all these recent editor changes and I wonder if we should take a step back. Maybe we don't need keyboard shortcuts right here right now for all editor commands. If it's hard to implement, let's do it later.

[CalebJohn]

Let me take a look, I think the approach is correct, but I may have made a wrong assumption about how the editor commands are loaded. I'll let you know soon if we should just roll this back or if I have a fix.

That said, you mentioned on the forum that you're working on a system for bi-directional data passing between content scripts and the plugin system? Maybe it's possible that this is unnecessary, the primary benefit of this change was to enable plugin commands that interact with the editor.

[laurent22]

That said, you mentioned on the forum that you're working on a system for bi-directional data passing between content scripts and the plugin system? Maybe it's possible that this is unnecessary, the primary benefit of this change was to enable plugin commands that interact with the editor.

Hmm, maybe, I don't know actually. The feature I'm going to add is that you can for example call const noteTags = await postMessage('getNoteTags') from the content script, and in the plugin you'll have this:

joplin.contentScripts.onMessage(message => {
    if (message === 'getNoteTags') // ...
    // Fetch the note tags...
    return tags;
});

So it allows communicating between the content script and plugin. It's also a way to indirectly access the plugin API from a content script. Would that solve the problems that this pull request deals with?

[CalebJohn]

If you allow passing data along with the message then it would. The reason for this pull request is to allow plugin developers to create commands that are attached to the editor runtime.

If you were to allow passing data alongside a message, then the developers can create a message that will register the runtime when the editor is mounted, and unregister it when removed.

I took a look at the this bug and you're exactly right about the cause of the issue. The problem is in the isEditorCommand function in CommandService.ts. It's missing a few commands because I copied it from ToolbarButtonUtils where it was used to define the commands that should focus the editor. I'll make a fix and maybe we can discuss more once you're looking at it.

[laurent22]

If you allow passing data along with the message then it would. The reason for this pull request is to allow plugin developers to create commands that are attached to the editor runtime.

Indeed it's possible to pass any data. The message can be a string as in my example, but also a JavaScript object, so any arbitrary data (except for functions) can be posted.

In general postMessage() should be the only way for an external script, such as a content script or webview script, to interact with the host. It keeps things a bit simpler by not having the plugin API appears in many different contexts and I expect it's also safer, as business logic will have to restricted to the plugin instance, which runs in a different process.

With that in mind, is there something in this pull request that we can simplify?

I've looked a few times at this PR and I still don't understand what it does, but that's probably because I'm not familiar with CodeMirror or its plugins. If you think something can be improved let's do it, or otherwise let's leave it as it is, it's up to you.

[CalebJohn]

The core of this pull requests was to useWindowCommandHandler.ts pull editor command declarations from the CommandService rather than from the declaration file. This allows any command prefixed with editor. to have access to an editor instance at runtime.

Using the message functionality we could actually rollback all these changes and add have users instead pass the editor instance to the plugin.
e.g.

// contentScript.js
// ...
await postMessage({ action: 'registerCommand',
                    name: 'editor.codeMirrorCommand',
                    execute:  async () => {
		 	cm.execCommand('anything');
                    }});
// ...

// index.ts
// ...
joplin.contentScripts.onMessage(message => {
    if (message.action && message.action === 'registerCommand') {
    		await joplin.commands.register({
		 	name: message.name,
		 	execute: message.execute,
		 });
    }
});
// ...

[CalebJohn]

This won't get rid of the need for the hardcoding in ToolbarUtils.ts but it will remove the changes from CommandService.
Let me know what you think. I can go ahead and undo the changes from this pull if you think the above is the right approach (I think it is).

[laurent22]

The core of this pull requests was to useWindowCommandHandler.ts pull editor command declarations from the CommandService rather than from the declaration file. This allows any command prefixed with editor. to have access to an editor instance at runtime.

So the plugin might define a command that starts with "editor." and you want the host to pick that up and create a runtime for it, is that correct?

The postMessage example you've shown above wouldn't work because it would require passing functions and instances, which can't be serialized. However, couldn't we just create a command that calls CodeMirror commands, and then use this from the plugin?

We could define a new command called execCodeMirrorCommand. Then for example the content script could define a CM command "deleteParagraph". If you want to expose this to the command system, you could create a new command in the plugin that wraps execCodeMirrorCommand:

So in the content script:

CodeMirror.defineExtension('deleteParagraph', function() {
	// ...
});

In the plugin:

await joplin.commands.register({
	name: 'deleteParagraph',
	execute: () => {
		await joplin.command.execCommand('execCodeMirrorCommand', {
			name: 'deleteParagraph',
			value: '', // Optional value
		});
	},
});

I think that would work and it means we don't need to make the editor commands special. The plugin just explicitely declare what the command.

The execCodeMirrorCommand runtime would be registered when the CodeMirror or TinyMCE components are mounted, so that's where the editor instance will be located.

Do you think that would meet the requirements?

[laurent22]

I had in mind it could work with both TinyMCE and CodeMirror, so I guess the command could be called "execEditorCommand".

[CalebJohn]

Hi @laurent22 sorry for getting back to this so late.

That would work perfectly, and would replace the pull request here. I'll make a PR undoing these changes, and then follow up with adding the new command when I get the chance.