lapce / plugin-server-protocol

Defines a common protocol for plugin servers.
https://lapce.github.io/plugin-server-protocol/
Other
9 stars 0 forks source link

RFC: Register Commands #2

Closed MinusGix closed 1 year ago

MinusGix commented 2 years ago

There should be an ability to register commands which when activated (such as from the palette or via a keybind) are sent to the plugin. What VSCode does is have two functions: registerCommand and registerTextEditorCommand. The former can be ran anywhere, but the latter can only be ran when a text editor is open.
This seems like a useful distinction to make. However, this could be instead an extensible ''enum'' on the registration to allow extending it further (without a bunch of separate commands) in the future, and allow nicer custom extensions (ex: Some editor could provide an internal extension for the settings 'editor', which we may or may not want in stable since not all editors would make settings accessible to the palette at all).
VSCode also has so these can be activated by a keybind. We may want to make this the default behavior, and if we really need to allow commands that can't have a keybind set, that can come in the future.
Basic idea:

// Should we allow registering commands after initialization? If not, then this could just go into the initialization structure.
export interface RegisterCommands {
    // Allow multiple to be sent at once
    commands: CommandInfo[],
}
export interface CommandInfo {
    /// A unique command name. We basically want them to do like "lapce-rust.reload_project"/"lapce-rust.reload-project"/"lapce-rust.reload.project" where they include their plugin name as the prefix. This allows it being obvious in keybinds, and for other plugins to potentially send commands like this.
    command: string,
    /// the rendered title "Rust: Reload Project" (or we could automatically prefix them all?)
    title: string,
    when: CommandWhen,
}

// Stringly-typed enum, since that's what lsp typically does for things that don't need to be very compressed
export enum CommandWhen {
    all = 'all', // 'always'? 'anywhere'?
    textEditor = 'textEditor', // this naming style matches with other enums in the lsp docs
    // We can then extend it as need be.
}

Thoughts:

Alternative:
We could do like VSCode does and have so this is in the plugin.toml file. This has benefits in immediately knowing what commands are defined. However, that would require saying that there's some standardized path that the data comes from.

nheuillet commented 2 years ago

I fail to see a case where a plugin would talk to another plugin. Do you have examples in mind from other editors: IMHO inter-plugin communication is not something to aim for

i18n support is something to think about. I would say maybe we could make it an initialize field (language), along with a notification for changed language. In any case, this is something that should be standardized for everything in the PSP spec, and not only to commands, I think.

I'm not entirely sure about handling default keybinds. Maybe a default keybind during registration would be the way to go. We need to keep in mind to create a unique slug for each plugins as to not make plugin A and plugin B have the same command. plugin.toml command registration could be nice, but there definitely is a need to be able to register and remove commands at runtime.

MinusGix commented 2 years ago

I fail to see a case where a plugin would talk to another plugin. Do you have examples in mind from other editors: IMHO inter-plugin communication is not something to aim for

Ex: Jupyter plugin talking to the Python plugin to figure out where the python executable is. I don't think it would be common, but I do think it has some use-cases. Though, it could be put off until some other time.

i18n support is something to think about. I would say maybe we could make it an initialize field (language), along with a notification for changed language. In any case, this is something that should be standardized for everything in the PSP spec, and not only to commands, I think.

There's a locale field in the initialize request of the LSP https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#initialize

nheuillet commented 2 years ago

After a long discussion with bugadani, here is what we came up with:

export interface RegisterCommandParams {
  // Commands to register.
  command: RegisterCommand[];
}

export interface RegisterCommand {
    // Id of the command.
    label: String;
    // Description of the command.
    description: String;
}

export interface AskForInputParams {
    // Id of the command related to this input.
    id: Number;
    // Title of the input.
    title: String;
    // Placeholder to be show.
    placeholder: String;
    // Hints about the way to fill this data.
    hint: String|URI;
}

export interface AskForChoiceParams {
    // Id of the command related to this input.
    id: Number;
    // Title of the input.
    title: String;
    // Choices the user can choose.
    choices: String[];
    // Minimum number of choices the user can choose
    minNumberOfChoices: Number;
    // Maximum number of choices the user can choose
    maxNumberOfChoices: Number;
    // Hints about the way to fill this data.
    hint: String|URI;
}

export interface AskForInputResponse {
    // Id of the command related to this input.
    id: Number;
    // Response of the user
    response: String[];
}

No way to execute commands from one plugin to another yet, but I think it could be implemented afterwards. On the i18n side of things, same thing, based on the locale field, it should work

MinusGix commented 2 years ago

minor: String -> string, it is lowercase in typescript-style interfaces.

command: RegisterCommand[];

commands (plural) would fit better with the typical naming in the LSP

placeholder: String

Could be optional.

id: Number;

This should be specifically id: integer;, since we aren't allowing floating point ids.

// Minimum number of choices the user can choose
minNumberOfChoices: Number;
// Maximum number of choices the user can choose
maxNumberOfChoices: Number;

These should be integer too. I would perhaps suggest making them optional, defaulting to 1 for both?
Should have a comment about what value to use for an unlimited number of choices. (Presumably 0?)

Minor: I'd name them minChoices/maxChoices or minChoiceCount/maxChoiceCount.

hint: String|URI;

Should probably make clear what this is? For inputs, I'm imagining this is intended to be like the grey text you sometimes see in an input when typing. That works for inputs, but is a bit weirder for choices. (Lapce has so you can filter the choices in the palette, so it fits with that I guess)
Why can it be a URI? If the plugin wants to display a path, why not just convert that to whatever string it thinks is best?
It should probably optional in both interfaces.


Other:

bugadani commented 2 years ago

I think we did a passable job since you only seem to have misunderstood a single idea out of that bunch. I call that a win. The hint field is a tooltip or a link to plugin docs, displayed maybe as a (?) icon. The gray text in the input field is the placeholder :)

I think the AskForChoice could be better with a way to specify a choice-id, so that the plugin does not have to do string comparisons

Excellent point, we missed this. Midnight brainstorming has its drawbacks.

nheuillet commented 2 years ago

Those are some very good points! Thank you for your inputs, I'll update accordingly

nheuillet commented 2 years ago

Should we perhaps have an extra structure for AskForChoice choices which includes other information? In Lapce's palette, for example, we have icons, the text, and a hint to the side of it. This could capture some power by allowing MarkedString like the hover documentation does. (of course, the clients can decide to limit what markdown they render in their choice-lists and the like).

This looks like a good idea in theory, but in practice I have trouble imagining a standard structure. I'd appreciate if you could give it a go :)

I think the AskForChoice could be better with a way to specify a choice-id, so that the plugin does not have to do string comparisons. This id could be automatically chosen, based on the index or it could be supplied? That way there's less string copying and less string comparison.

Are you talking about the way a client (Lapce) delivers the chosen data to the server (plugin?) if so, yes I agree that indexes are the easiest way to deliver choices (eg: I choose the first and third option, so result will contain 0 and 3 index)

Minor: I think it would be useful to be able to specify 'how'/'where' the plugin is asking for the data, though I don't think this needs to be in the current RFC

I'm not sure I follow, what exactly do you mean by specifying how / where?

We have the palette for most inputs, but something like vscode's notification mechanism would be nicer for things which don't need to be handled immediately (since you'd to close the palette to continue editing code), or things with only two buttons (like a yes or no choice).

I need to check again but this notification mecanism is already present within PSP as it should have something to do with LSP logMessage

MinusGix commented 2 years ago

This looks like a good idea in theory, but in practice I have trouble imagining a standard structure. I'd appreciate if you could give it a go :)

interface Choice {
    /// The text that should be displayed for the choice
    text: string,
    /// The icon that should be displayed for the choice  
    /// Note that this is a Uri on the filesystem, not to a location on the internet
    icon?: Uri,
    /// Hint which gives a small amount of extra information about the choice
    hint?: string,
}

Just that. Then AskForChoice takes Choice[] (or (string | Choice)[] if we want to make it slightly nicer to use, but I don't think that's notably useful)
There is, however, the open question of how it should implement icons or other media. A relative path (in the plugin folder) for the media? But they might want to display an image in the current workspace as an icon (or in some future image rendering)?

This would let us add more parts to it later if desired. Ex: tooltip on hover; optional sorting priority.

Are you talking about the way a client (Lapce) delivers the chosen data to the server (plugin?) if so, yes I agree that indexes are the easiest way to deliver choices (eg: I choose the first and third option, so result will contain 0 and 3 index)

Yes, I'm saying that we should use indices.
(We could use some other method where they specify the indices themselves, which could be slightly more optimal, but that doesn't matter enough and is easy for plugin authors to use wrong)

The AskForResponse structure should thus be changed to having integer[].

I'm not sure I follow, what exactly do you mean by specifying how / where?

Like palette vs image (not that I have any notifications to show on vscode..)
Looking at the LSP docs, what you're referring to is https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showMessageRequest (and the other one which just shows information, but that's not what the proposed commands do). The api is somewhat similar to what you proposed, though weaker.
So, I guess that works and is enough for now. Though, I do think that we might want a richer representation than that once Lapce gets notifications (ex: just letting you use markdown in the notification would be a benefit), and so we might just want to expand it similar to how we're implementing AskChoice.

nheuillet commented 1 year ago

Closing as registering command has been added as of version 0.1