Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(commands): add support for custom typable commands #12320

Draft
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

RoloEdits
Copy link
Contributor

@RoloEdits RoloEdits commented Dec 23, 2024

TODO

  • Depends on:
  • Get direction on how should add commands to editor.config
    • Need direction on how to add custom commands to the command validation part of loading the config, currently it tries to parse them into a MappableCommand, but the custom commands cannt be put in that part of the execution path.
  • Figure out MVP for feature set
    • Have an ability to define scopes for the commands?
      • :test in a rust context(buffer?) might have a :sh wezterm cli spawn --floating-pane cargo test
      • :test in a go context might have a :sh wezterm cli spawn --floating-pane go test
      • Default to a global context if not defined under a more specific context?
    • Should custom commands have aliases?

The implimentation needs to build off of #12288 and #11164 which themselves build off of #11149, so still a ways out, but I think I have a good framework going forward.

Concept

Config

Basic

The config will get a [commands] table for which the custom commands will go. At the most basic level of usage, an alias for a single command, it would look like this:

[commands]
"waq" = ":write --all --quit"

This would provide no completions, only showing up in the list of commands. The prompt would be bare, only the name and the mapping, no description or what it accepts:

waq:

maps:
    :write --all --quit

Advanced

The most advanced usage is what I hope to be possible to implement, but as I haven't actually done it, jury is still out on what's actually feasible:

[commands]
"wcd!" = {
commands = [ ":write --force %{arg}", ":cd %sh{ %{arg} | path dirname }" ],
desc= "writes buffer forcefully, then changes to its directory"
completions = "write"
accepts = "<path>"
}

This would show the commands from the list of commands like before, but now offers the ability to run multiple commands in a chain, provide positional arguments, a description, indicate what it accepts, as well as which completer to run when using the command, sourced from the name of an existing typable command.

The prompt now looks like this:

image

%{arg} represents a positional argument that is provided when using the command.

:wcd! parent/sub/sub/file.txt
      ^^^^^^^^^^^^^^^^^^^^^^^

These could be given numbers as well: %{arg:1}. %{arg} would be equal to %{arg:0}. This is able to take advantage of the Args iterator that #11149 introduces, just taking the number provided and pass it to nth.

Nesting

Custom commands can also be used in other custom commands, though with some limitations to be mindful of:

[commands]
# If the above was instead `wcd`, with no `--force`
# and you tried to use it in an `wcd!` impl,
# you would run into an issue with the other commands
# single positional `%{arg}.
#
# Notice even if we tried to pass the flag and the path as
# one argument, using quotes to wrap it, we run into an
# issue with the path dirname evaluation.
# 
# When it all gets expanded write should look fine:
# write --force parent/sub/sub/file.txt
# 
# The issue is with the `:cd`
# :cd  %sh {--force parent/sub/sub/file.txt | path dirname }
# which would error in nushell
"wcd!" = {
commands = ":wcd '--force %{arg}'",
desc= "writes buffer forcefully, then changes to its directory"
completions = "write"
accepts = "<path>"
}

The prompt would look like so, again if the first impl was a wcd, no force.

wcd! <path>: writes buffer forcefully, then changes to its directory
maps:
    :wcd! -> wcd `--force %{arg}`

In short, it works, but the design of the commands must be able to compose.

Implimentation Details

Its is based around a CustomTypableCommand struct:

pub struct CustomTypableCommand {
    pub name: String,
    pub desc: Option<String>,
    pub commands: Vec<String>,
    pub accepts: Option<String>,
    pub completer: Option<String>,
}

The config itself would hold a wrapper of this:

pub struct CustomTypableCommands {
    commands: Vec<CustomTypableCommand>,
}

The usage of this would look roughly like:

            // Checking for custom commands first priotizes custom commands over built-in/
            if let Some(custom) = cx.editor.config().commands.get(command) {
                for command in custom.iter() {
                    // TODO: Expand variables: #11164

                    let shellwords = Shellwords::from(command);

                    if let Some(command) = typed::TYPABLE_COMMAND_MAP.get(shellwords.command()) {
                        if let Err(err) = (command.fun)(cx, shellwords.args(), event) {
                            cx.editor.set_error(format!("{err}"));
                            // Short circuit on error
                            return;
                        }
                    } else if event == PromptEvent::Validate {
                        cx.editor
                            .set_error(format!("no such command: '{}'", shellwords.command()));
                        // Short circuit on error
                        return;
                    }
                }
            }
            // Handle typable commands
            else if let Some(cmd) = typed::TYPABLE_COMMAND_MAP.get(command) {
                if let Err(err) = (cmd.fun)(cx, shellwords.args(), event) {
                    cx.editor.set_error(format!("{err}"));
                }
            } else if event == PromptEvent::Validate {
                cx.editor.set_error(format!("no such command: '{command}'"));
            }

There is more here that would need to be layered in, like the flags and expanding the variables from the other PRs, but this is the gist.

The main points I haven't delved into yet is the parsing from the toml into the struct and add it to the config, which to me is looking like the hardest part (😆). As well as how to provide the command in the list of commands, and be able to show the prompt for it properly.

Currently, this is pretty hacky due to lifetimes, but should work (at least its able to compile currently):

    // PERF: Cheap clone
    let commands = Arc::new(cx.editor.config().commands.clone());

    let mut prompt = Prompt::new(
        ":".into(),
        Some(':'),
        move |editor: &Editor, input: &str| {
            let shellwords = Shellwords::from(input);
            let command = shellwords.command();

            let commands = commands.clone();
            let names = commands.names();

            let items = TYPABLE_COMMAND_LIST
                .iter()
                .map(|command| command.name)
                .chain(names)
                .map(|name| name.to_string())
                .collect::<Vec<String>>();

            if command.is_empty()
                || (shellwords.args().next().is_none() && !shellwords.ends_with_whitespace())
            {
                fuzzy_match(input, items, false)
                    .into_iter()
                    .map(|(name, _)| (0.., name.into()))
                    .collect()
            } else {
                // Otherwise, use the command's completer and the last shellword
                // as completion input.
                let (word, len) = shellwords
                    .args()
                    .last()
                    .map_or(("", 0), |last| (last, last.len()));

                let command = commands.get(command).map_or(command, |c| {
                    c.completer.as_ref().map_or(command, String::as_str)
                });

                TYPABLE_COMMAND_MAP
                    .get(command)
                    .map(|tc| tc.completer_for_argument_number(argument_number_of(&shellwords)))
                    .map_or_else(Vec::new, |completer| {
                        completer(editor, word)
                            .into_iter()
                            .map(|(range, mut file)| {
                                file.content = shellwords::escape(file.content);

                                // offset ranges to input
                                let offset = input.len() - len;
                                let range = (range.start + offset)..;
                                (range, file)
                            })
                            .collect()
                    })
            }
        }, // completion
// ...

The commands have to be cloned into the closure and because it stores Strings, you cant just make a command.name.as_str call, as that references the local function, and the prompt has its own lifetime. Due to this, I had to make all the &'static str on the TYPABLE_COMMAND_LIST String as well, and then collect into a Vec. This is meant to be temporary, but would love some help here to prevent all these allocations. The jank might have to last until the CustomTypableCommand can directly store a TypeableCommand, which needs #5555 to be completed. Otherwise the completers might just have lifetime issues as they cannot be static.

I'd also love some feedback on the TOML design, as well as some example usage, some ultra creative ones, to make sure I can design around as much as possible. Like I said at the start, this is all a ways out, but this still has issues to work out anyways, so hopefully if we get these ironed out, and the dependent pull requests merge in without issue, and I can work on this in the next release cycle as a major feature of the release.


Potentially Common Use-Cases

Floating lazygit pane

":lg" = ":sh --no-popup wezterm cli spawn --floating-pane lazygit"

Floating terminal pane

":t[erm]" = ":sh --no-popup wezterm cli spawn --floating-pane"

Floating yazi pane

":y[azi]" = [":sh --no-popup wezterm cli spawn --floating-pane yazi %{path}", ":open --env HELIX_FILE"]

Removing current buffer file

":rm" = ":sh --no-popup rm %{path}"

Future Considerations

Remove some static aliases

#12288 allows the ability to collapse current separate commands, where behavior can be dictated by flags, into a single command. This is good in a lot of ways, but the side effect is that for a command like write, the prompt is filled with aliases:
image

With the ability to have custom commands, including aliasing, it might be worth it to remove some currently statically defined aliases.

Furthering this idea, is that with the collapsing of commands to a single one, we can adopt a concept where we consider these are now primitives, in which can be composed together:

# remove static alias from :write (which is now a single command)
[commands]
"wq" = ["write %{arg}", "quit"]
"wbc!" = ["write --force %{arg}", "buffer --kill"]

We could also provide a set of defaults, basically what we have now, that crosscut here, away from the "primitive" commands. This would be the blend boundary, and we are just exposing a frontend in the form of a config for the users to also have this ability. Considering they are documented and show the underlying commands, I think it would be enough to inform the user what it happening.

Escaping

Note

Currently this is a feature, but will need to be discussed if it remains or not.

If a user makes a mapping that is the same as a built-in one, should there be a way to escape? For example, we could allow a ^ prefix to escape shadowing:
If we have:

[commands]
"w" = ":write --force"
:^w
# :write

Deterministic Command Execution

Currently, when you list commands, [":sh ...", ":open ..."], :open can happen before :sh is finished. This is done so that there is no blocking that happens. However, this breaks the users assumption that this will work just fine, leading to confusion. This also affects macros as well, for example, preventing their use in sequences. Going forward, non UI blocking, sequential execution order should be strived for. Not only would this line up with expectations, it will also make the command system more powerful and expressive, making it straight forward to compose them together.

Closes: #4423

refactor: no longer special case for bracket lists

refactor: no longer special case end space

This was a hold over from before the `raw` function was added to `Args`.

perf: remove `bytes` field to save 16 bytes

From 56 bytes to 40, saving 16 bytes.

perf: move `in_quotes` field to local variable

perf: move `quote` field to local variable

refactor: remove `is_finished` state from `Args`

test: change example command to `read`

`yank-join` now uses `raw` and thus would not be parsed with the `next`
function so no longer applicable.

refactor: remove unneeded range end for index

refactor: remove backtracking escape check

Instead, it can be tracked as the parser scans through the first time.

refactor: clean up code and add more comments

feat(shellwords): add test for empty values
refactor(commands): ignore unit pattern match

refactor(commands): use `if let` over `matches!`

refactor(commands): ignore unit pattern match

perf(commands):remove unnecessary ref for a &str

refactor(commands): change `MappableCommand` `args` from `Vec<String>` to String

refactor(commands): use `Args::raw` over `fold`ing

refactor: use `rest` in `set_option` command

refactor: use `rest` in `toggle_option` command

chore(dap): add TODOs to switch to `Args`

perf: change `ok_or` to `ok_or_else` to make fn call lazy

feat(shellwords): add `args_count` to `Args`

fix(commands): default to `line_ending` for `yank_join` when args are empty
Unfortunate happening with lifetimes here.

Also supports getting the commands from the custom label and running
them as typable commands.
@RoloEdits
Copy link
Contributor Author

@the-mikedavis I have some questions on the implimentation of how I can get access to the commands in the config.

Context has an Editor, which you can call cx.editor.config.load on to get access to the values. The values themselves are from:

// helix-term/src/config.rs
pub struct Config {
    pub theme: Option<String>,
    pub keys: HashMap<Mode, KeyTrie>,
    pub editor: helix_view::editor::Config,
}

And as helix-term has a dependency on helix-view, where Editor lives, it cannot directly store this config. Would I be adding a commands field to editor.config?

Application::new has access to the config: Config, should I just make this a mut config: Config and add it: config.editor.commands = config.commands?

And I assume there would be another area which handles reloading the config. So If I do this change in new, what other spot do I need to make sure gets updated?

@RoloEdits
Copy link
Contributor Author

Got a functioning backend, at least for basic direct aliasing:

cutstom_commands_alias_demo

More features require the dependent PRs, and is best left for when they are merged into master. I can still work on the frontend part though, so once I get some direction I can start the toml parsing and config propagation.

@RoloEdits
Copy link
Contributor Author

Slightly more applicable demo, spawning lazygit in a floating pane:
lasygit_pane_command_demo

@RoloEdits
Copy link
Contributor Author

RoloEdits commented Dec 23, 2024

Something more difficult than I thought would be is figuring out how best to merge arguments. For now I assume that if the typed command(:lg) has no args, assume that the given command(:sh ...) will provide them. Not sure how to define a set of rules that would be the most intuitive and least brittle.

It needs to work the simplest usage as well as when there are chained commands. Just because the input command has arguments, it should not be assumed that the chained commands wont want to run their own arguments.

This needs to take place even for basic aliases:

[commands]
":w!" = ":write --force %{arg}"

In the input, we can provide a path, but with the current logic, all args for the write command, including the flags, would be disregarded. In this case, only using the provided path, as :write takes a path anyways and so direct forwarding works, but would only write the current buffer to the provided path location as normal, not forcibly so, which is the desire behavior.

We also cant just send the input args to each command, as some commands explicitly take no arguments. And with an %{arg} being expanded, we could end up with a double input where it gets expanded and then appended again.

@RoloEdits
Copy link
Contributor Author

Actually, I think it might be fine, as it will get merged correctly on variable expansion, but will have to wait and see when that gets merged and base off those changes to check. For now there is a validator to decide what to do.

@RoloEdits RoloEdits force-pushed the custom-commands branch 2 times, most recently from 58df78c to b363c57 Compare December 25, 2024 16:21
@RoloEdits RoloEdits force-pushed the custom-commands branch 2 times, most recently from 3f70d97 to 984c447 Compare December 26, 2024 08:31
@RoloEdits
Copy link
Contributor Author

Have the config parsing implemented, at least as a placeholder until I can get more direction.

The command validation is tricker than I expected it to be. They keys are turned into a KeyTrie which stores commands to run as MappableCommand, but MappableCommand cannot know of any custom commands declared in the config.

  • Should we just not allow keybinds to custom commands? They can be built just the same and this retains validation. But the custom commands itself also has no validation if recursive commands are allowed.
  • Should we Just catch all that start with : and make a new MappableCommand::CustomTypable variant as a catch all for typeable commands? This would remove validation.

@RoloEdits
Copy link
Contributor Author

RoloEdits commented Dec 26, 2024

Also not sure if there exists the same issue where MappableCommand are executed all at once, or if this loop will work. This might create an issue with macros, which is why I think it is not allowed iirc.

We also wouldnt be able to sperate multi-operations, like [":write", ":quit"], where the execution needs to be done is a single transaction in a defined order.

nikvoid added a commit to nikvoid/helix that referenced this pull request Dec 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom typable commands
1 participant