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

Switches for typable commands #5828

Open
the-mikedavis opened this issue Feb 4, 2023 · 18 comments · May be fixed by #12288
Open

Switches for typable commands #5828

the-mikedavis opened this issue Feb 4, 2023 · 18 comments · May be fixed by #12288
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements E-hard Call for participation: Experience needed to fix: Hard / a lot

Comments

@the-mikedavis
Copy link
Member

Some typable commands might benefit from taking switches to control small parts of their behavior. A few examples of commands that could take advantage of switches:

  • :write and friends could take a --no-format switch which prevents auto-formatting (#4853, #4909 (comment))
  • :sort and :rsort could be merged into one command with reverse-sorting accomplished by :sort --reverse (#1288 (comment))
  • :clear-registers (#5695) could take an --all switch to clear all registers (#5695 (comment))

Kakoune has switches and it has a nice way of showing the available switches in the documentation popup:

kakoune-switches-demo

A few items worth some discussion:

  • Call these "switches" or "flags"?
  • Support shorthand switches? For example -a would be a shorthand for --all in the :clear-registers example.
@the-mikedavis the-mikedavis added the C-enhancement Category: Improvements label Feb 4, 2023
@kirawi kirawi added the A-helix-term Area: Helix term improvements label Feb 5, 2023
@goyalyashpal
Copy link
Contributor

:config-reload and :config-open can also merged?

@eduardorittner
Copy link
Contributor

Would support for this be added by just changing the command function implementations (fun field of TypableCommand), or would something else be needed? Like adding an extra field Option<Vec<Flag>> to TypableCommand, for example.

@the-mikedavis the-mikedavis added the E-hard Call for participation: Experience needed to fix: Hard / a lot label Dec 16, 2024
@the-mikedavis
Copy link
Member Author

I think this will be a fair amount of work so I've added the "hard" tag. The work should basically all be scoped to helix-term/src/commands/typed.rs though. (Maybe with some changes to the shellwords module if necessary, since that's where parsing will live after #11149 is merged.) We'll need some extra information on the TypeableCommand struct for each switch that includes the switch name, any information attached to the switch for example a path, and documentation about the switch.

Whatever Kakoune is doing for their switches may be good inspiration and also existing crates like xflags or clap. Though I don't think we would accept a patch that pulls in a dependency like clap - it should be possible to build something specific to our usecase without any extra deps.

@RoloEdits
Copy link
Contributor

This could be something I can start looking into once #11149 is finished. switch to me coveys a binary state, where as flag can have some extra information attached to it. So if we must handle flaglike behavior(as defined), I think "Flags" makes more sense.

I'll do some initial scouting to see what the scope of this would look like before I start asking questions on specific features/behavior.

@RoloEdits RoloEdits linked a pull request Dec 18, 2024 that will close this issue
@NikitaRevenco
Copy link
Contributor

NikitaRevenco commented Dec 18, 2024

This could be something I can start looking into once #11149 is finished. switch to me coveys a binary state, where as flag can have some extra information attached to it. So if we must handle flaglike behavior(as defined), I think "Flags" makes more sense.

I'll do some initial scouting to see what the scope of this would look like before I start asking questions on specific features/behavior.

I think it's worth to remove all these commands:

  • clipboard-yank
  • yank-join
  • clipboard-yank-join
  • primary-clipboard-yank
  • primary-clipboard-yank-join
  • clipboard-paste-after
  • primary-clipboard-paste-after
    ... etc

Instead we can have just two:

yank
flags:
  -j, --join        join selections into a single yank
  -r, --register    choose register to yank into
paste
flags:
  -a, --after       paste after
  -b, --before      paste before
  -r, --register    choose register to paste from

Not only does it clean up the mess, it also gives more flexibility. Now we can use any register

Note: Instead of using --register flag like this:

yank --register=* --join

What if it was like this instead:

yank * --join

I think making it as if we are passing an argument rather than a flag in this case provides a better experience

@RoloEdits
Copy link
Contributor

I hadn't thought much of alternate ways the order could be, I built around the idea of flags first.

With the current implimentation I have in #12288 both

yank --register=* --join

and

yank * --join

would be possible (though I am not handling = assignment yet, but just having a space would work though). The changes in #11149 make the arguments an iterator, so in argument first case, its just a normal next and then you would be able to call args.flags and match on --join. I do think it should provide a substantial improvement to UX if deciding to deviate from what other commands may be doing. Consistency should be a higher priority imo. With a lot of branch handling, theoretically both could be handled at the same time, but this would be a big no from me.

@RoloEdits
Copy link
Contributor

RoloEdits commented Dec 18, 2024

I do know quite a few commands could be combined, but that feels like it would be something that would hold up the base implimentation getting merged for longer than needed, especially when its so easily atomized into separate prs. I did mark that the PR would close this on merger, but perhaps I shouldn't, and then we can add commands here that need to be converted.

I can do a few examples (already doing sort), to show how to handle some common flag use.

Before that though I am going to wait for some feedback if what I am doing is what is wanted.

@NikitaRevenco
Copy link
Contributor

I do know quite a few commands could be combined, but that feels like it would be one something that would hold up the base implimentation getting merged for longer than needed, especially when its so easily atomized into separate prs. I did mark that the PR would close this on merger, but perhaps I shouldn't, and then we can add commands here that need to be converted.

I can do a few examples (already doing sort), to show how to handle some common flag use.

Before that though I am going to wait for some feedback if what I am doing is what is wanted.

oh yeah, definitely. There's a lot of design decision as to which commands should be removed exactly. so I personally think that you shouldn't worry about thinking about those for your PR. it would be easier to have separate PRs that replace existing commands with switches

@RoloEdits
Copy link
Contributor

Thinking on some combinations being collapsed, :write presents a good example of some issues with how the commands currently are. For example, this is a command where aliases reign supreme. Being able to do :wq and :w! with the shorthand is such a common occurrence. And merging means the aliases wouldn't work out of the box like now.

Something like #4423 could be done to add aliases ourselves, but that feature is its own can of worms. Like, is there default aliases provided for these? How do we get the prompt to propagate? etc.

@NikitaRevenco
Copy link
Contributor

Thinking on some combinations being collapsed, :write presents a good example of some issues with how the commands currently are. For example, this is a command where aliases reign supreme. Being able to do :wq and :w! with the shorthand is such a common occurrence. And merging means the aliases wouldn't work out of the box like now.

Something like #4423 could be done to add aliases ourselves, but that feature is its own can of worms. Like, is there default aliases provided for these? How do we get the prompt to propagate? etc.

I don't think that aliases like :w! and :wq should be removed. This would be a huge breaking change especially since people have decades of muscle memory for those

So the alternative is to keep those aliases but don't have aliases show up in the command prompt. E.g. a command like :write will show up in the autocomplete when you enter command mode. But it's multiple aliases such as :wq! => write --quit --force and :w => :write won't show up.

Instead you'll be able to see the aliases in a sub-menu. Like this:

write
flags:
  -q, --quit
  -a, --all
  -f, --force
aliases:
  wq! -> write --quit --force
  x -> write --quit

please let me know what you think about this idea


and #4423 may make more sense as a plugin, see this comment by mikedavis. I don't think it's worth worrying about that for now

@RoloEdits
Copy link
Contributor

RoloEdits commented Dec 18, 2024

For those not familiar with what a typeable command looks like:

    TypableCommand {
        name: "write-all",
        aliases: &["wa"],
        flags: &[],
        doc: "Write changes from all buffers to disk.",
        fun: write_all,
        signature: CommandSignature::none(),
    },

The issue is that the aliases are per command. If you wanted to collapse all write to a single typeable command, using as small a change as possible, you would need to have access to the exact command used to initiate the function. One option would entail Shellwords being passed in to the callback, instead of Args, and then in the write function, match on the command just for aliases:

let command = shellwords.command();

match command {
    "w" => ...
    "w!" => ...
}

Then, you would have to have to also handle the case where its written with flags. For write, I don't think there is a need for any other kind of flag beside a switch flag, but for other commands, you might have to also handle when a flag might have something assigned to it, and as Args is an iterator, you'd be consuming the args as they come, and this can lead to a lot of roundabout logic that can muddy up the code quality in the code base.

@RoloEdits
Copy link
Contributor

RoloEdits commented Dec 18, 2024

With the docs in mind, though, an Alias struct could be in order. A macro could be made to make quick handling of multiple aliases:

    TypableCommand {
        name: "write",
        aliases: alias!["w" => "write", "wa" => "write --all", "w!" => "write --force"],
        flags: &[flag!{ long: "force", short: "f", "desc: "forcefully saves buffer" }],
        doc: "Write changes from buffer to disk.",
        fun: write,
        signature: CommandSignature::none(),
    },

If the custom commands ever went anywhere, it could potentially use the Alias as a starting point.

@darshanCommits
Copy link

darshanCommits commented Dec 19, 2024

Clap like attribute syntax would be cool here.

#[alias("w")]
#[alias("wa" = "% --all")]
#[alias("w!" = "% --force")]
TypableCommamd {
....
 }

% being placeholder for self cmd

@RoloEdits
Copy link
Contributor

RoloEdits commented Dec 21, 2024

An update on progress in #12288, have been able to collapse all write commands down to one main one, and map aliases in a way that uses the provided flags to achieve the same behavior as before:
image

@RoloEdits
Copy link
Contributor

RoloEdits commented Dec 21, 2024

For some other commands worth looking at to combine, where, If done for all of them, it would bring 31 commands to 7.:

append-output, insert-output

Could combine to one single output command:

output [<flags>] <cmd>: ...

flags:
    --append, -a        appends the output after each selection
    --insert, -i        inserts the output before each selection

config-open, config-reload

Could combine to one single config command:

config [<flags>]: opens config file

flags:
    --reload, -r        reloads the config instead of opening

hsplit-new, hsplit, vsplit-new, vsplit

Could combine to one single split command:

split [<flags>]: ...

flags:
    --vertical, -v             splits vertical 
    --horizontal, -h           splits horizontal
    --new, -n                  opens a new buffer when splitting

reload, reload-all

Could combine to one single reload command:

reload [<flags>]: ...

flags:
    --all, -a             reloads all buffers
    --force, -f           forces a reload of buffers

primary-clipboard-paste-replace, primary-clipboard-paste-replace, primary-clipboard-paste-after, clipboard-paste-replace, clipboard-paste-before, clipboard-paste-after

Could combine to one single paste command:

paste [<flags>]: ...

flags:
    --register -r                register to paste from
    --before, -b                 paste before cursor
    --after, -a                  paste after cursor
    --clipboard, -c              paste from primary clipboard
    --system, -s                 paste from system clipboard

cquit!, cquit, quit-all, quit-all!, quit, quit!

Could combine to one single quit command:

quit [<flags>]: ...

flags:
    --code, -c <code>        exit code helix should exist with
    --force, -f              forcefully exit, ignoring unsaved changes
    --all, -a                close all views

buffer-previous, buffer-next, buffer-close-all!, buffer-close-all, buffer-close-others!, buffer-close-others, buffer-close!, buffer-close

Could combine to one single buffer command:

buffer [<flags>]: ...

flags:
    --next, -n               operate on next buffer
    --previous, -p           operate in previous buffer
    --other, -o              operate on all but current buffer
    --close, -c              close buffer
    --all, -a                operate on all buffers
    --force, -f              forcefully operate

@NikitaRevenco
Copy link
Contributor

For some other commands worth looking at to combine, where, If done for all of them, it would bring 31 commands to 7.:

append-output, insert-output

Could combine to one single output command:

output [<flags>] <cmd>: ...

flags:
    --append, -a        appends the output after each selection
    --insert, -i        inserts the output before each selection

config-open, config-reload

Could combine to one single config command:

config [<flags>]: opens config file

flags:
    --reload, -r        reloads the config instead of opening

hsplit-new, hsplit, vsplit-new, vsplit

Could combine to one single split command:

split [<flags>]: ...

flags:
    --vertical, -v             splits vertical 
    --horizontal, -h           splits horizontal
    --new, -n                  opens a new buffer when splitting

reload, reload-all

Could combine to one single reload command:

reload [<flags>]: ...

flags:
    --all, -a             reloads all buffers
    --force, -f           forces a reload of buffers

primary-clipboard-paste-replace, primary-clipboard-paste-replace, primary-clipboard-paste-after, clipboard-paste-replace, clipboard-paste-before, clipboard-paste-after

Could combine to one single paste command:

paste [<flags>]: ...

flags:
    --register -r                register to paste from
    --before, -b                 paste before cursor
    --after, -a                  paste after cursor
    --clipboard, -c              paste from primary clipboard
    --system, -s                 paste from system clipboard

cquit!, cquit, quit-all, quit-all!, quit, quit!

Could combine to one single quit command:

quit [<flags>]: ...

flags:
    --code, -c <code>        exit code helix should exist with
    --force, -f              forcefully exit, ignoring unsaved changes
    --all, -a                close all views

buffer-previous, buffer-next, buffer-close-all!, buffer-close-all, buffer-close-others!, buffer-close-others, buffer-close!, buffer-close

Could combine to one single buffer command:

buffer [<flags>]: ...

flags:
    --next, -n               operate on next buffer
    --previous, -p           operate in previous buffer
    --other, -o              operate on all but current buffer
    --close, -c              close buffer
    --all, -a                operate on all buffers
    --force, -f              forcefully operate

awesome! This is really good because users won't be overwhelmed with 100 commands in the completion prompt when they use :

@RoloEdits
Copy link
Contributor

Seeing how large the :write command prompt has gotten, though this is the worst case I can think of, I wonder if when #4423 gets implemented, we end up removing "cross-cutting" command options, treating the implemented typable commands as a sort of primitive to compose together larger actions. This would also mean the implementations would only be worried about its one job and doing it correctly.

:write, for example, could remove the --quit and --close-buffer options, and then in the config they could have:

":wq" = [":write", ":quit"]
":x" = [":write", ":quit"]
":wbc" = [":write", "buffer --kill"]

Then the remaining aliases on write would be just w, w!, wa, wa!, and u.

@RoloEdits
Copy link
Contributor

RoloEdits commented Dec 23, 2024

sh:

shell [<flags>]: ...

flags:
    --no-popup                       disables popup
    --register, -r <register>        saves output to register

move:

move [<flags>]: ...

flags:
    --rename, -r               renames the current file in-place
    --force, -f                forcefully moves, creating directories if needed

open:

open [<flags>]: ...

flags:
    --register, -r <register>              register to read in the path from

In conjunction with :sh --register +, for example, it could open up some more streamlined workflows when dealing with external programs, such as the yazi file manager. You could setup something like:

[keys.normal.space]
e = [":sh --no-popup --register + ...", ":open --register +"]

This would do mux + yazi things, eventually getting back the path of the file you wish to open, and then open it from the register, streamlining a file tree without the need for a plugin system or any glue scripts.

Can also add an --env to :open that would read from the environment variable. This would be useful in scenarios where stdout is too dirty to work with. For example, wezterm outputs an id for the spawned pane.

As an example of usage, helix would launch a floating pane of yazi with the current buffers file as the starting point. From there, a yazi plugin could write the "opened" file path to the environment variable, and then exit. :open would then run and read from the variable.

[keys.normal.space]
e = [":sh --no-popup wezterm sli spawn --floating-pane yazi %{path}", ":open --env YAZI_LAST_OPENED_FILE"]

register:

register [<flags>]: ...

flags:
    --set, -s <register>                              ...
    --copy, -c <register>                             ...
    --clear                                           ...
    --all, -a                                         ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements E-hard Call for participation: Experience needed to fix: Hard / a lot
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants