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

Modularize user key bindings #5191

Closed
jorgebucaran opened this issue Sep 17, 2018 · 6 comments
Closed

Modularize user key bindings #5191

jorgebucaran opened this issue Sep 17, 2018 · 6 comments

Comments

@jorgebucaran
Copy link

@jorgebucaran jorgebucaran commented Sep 17, 2018

Would you consider making fish aware of a new keybindings directory inside $__fish_datadir, XDG_CONFIG_HOME, etc.?

Then, during startup, copy the user's fish_user_key_bindings function (if there is one) and create a new fish_user_key_bindings function that when invoked, will source every file in each keybindings/ directory in $__fish_datadir and XDG_CONFIG_HOME.

Basically this:

if functions -q fish_user_key_bindings
  functions -c fish_user_key_bindings __fish_user_key_bindings
end

function fish_user_key_bindings
  for file in ~/.config/fish/keybindings/*.fish
    source $file
  end
  if functions -q __fish_user_key_bindings
    __fish_user_key_bindings
  end
end

I can emulate this behavior using conf.d and distribute that script as a package, but it would be nice to have this out of the box.

In fisher v2.x (since I depend exclusively on fish autoloading feature) I go to great lengths to generate a fish_user_key_bindings function on the fly that concatenates the keybindings exported by each package with the user's key bindings defined inside their fish_user_key_bindings, but it's highly impractical (and buggy). In fisher v3.x I'll work around this more cleanly with a package as suggested above, but it would be nice if I didn't need to.

Oh-my-fish (here) and fundle (here) emulate this behavior through the user's config.fish using the same (or similar) technique described above.

@faho
Copy link
Member

@faho faho commented Sep 17, 2018

I've actually been thinking about doing away with the need for fish_user_key_bindings entirely.

Really, the only reason that function is needed is because, when the user switches binding sets (e.g. from emacs to vi) we want to erase everything belonging to the old set. So what we do now is to erase everything and rerun that function.

What I've been thinking about is to add a "--default" or "--system" switch to bind. Any binding set would then always call bind --default, and the user wouldn't, so we could then just erase bindings defined with that switch (bind --erase --all --default).

So what fisher/omf/fundle would then do is simply call bind - I'm just not quite sure if with --default or not, yet.

@faho faho added this to the fish-future milestone Sep 17, 2018
@jorgebucaran
Copy link
Author

@jorgebucaran jorgebucaran commented Sep 17, 2018

@faho Do you mean that it would work similarly to complete?

So, with complete we can already complete -ec $funcname which makes uninstalling completions easy. Installing them is easy as well, just put them in the $fish_complete_path, and if you want them in the current shell, just source the $path/completions/$funcname.fish file.

@faho
Copy link
Member

@faho faho commented Sep 17, 2018

Do you mean that it would work similarly to complete?

Not really, no. Just if you call bind, we keep that binding, even if you switch from vi to emacs or the other way around. In fish_vi_key_bindings and such we'd use bind --default, and we'd do bind --erase --all --default to clear all previously defined default bindings, but not bindings defined without --default.

If you do bind \t something-else, we still keep the default (bind --default \t complete) in the background, so if you then delete yours, that becomes active again.

I don't think we would need any special files to support this - you'd do bind statements in conf.d files as part of your setup.

@jorgebucaran
Copy link
Author

@jorgebucaran jorgebucaran commented Sep 17, 2018

@faho I don't think we would need any special files to support this - you'd do bind statements in conf.d files as part of your setup.

Wouldn't that run into the same problem of using bind in config.fish (you can't). 🤔

@jorgebucaran
Copy link
Author

@jorgebucaran jorgebucaran commented Sep 18, 2018

@faho ...you'd do bind statements in conf.d files as part of your setup.

I haven't run any benchmarks, but I suspect this wouldn't be scalable as it would force every script that wants to add its own bindings to copy any existing fish_user_key_bindings.

echo "...initializing my script"

if functions -q fish_user_key_bindings
    functions --copy fish_user_key_bindings _my_script_fish_user_key_bindings
end

function fish_user_key_bindings
    bind ...
    bind ...
    bind ...

    if functions -q _my_script_fish_user_key_bindings
        _my_script_fish_user_key_bindings
    end
end

If I have 10 my_script-like files in conf.d, because that's the point of this issue—to be able to modularize key bindings—then the most recently defined fish_user_key_bindings function would be copied over 10 times.

A more sensible approach would be to copy the function only once and source a bunch of files with the bind statements as described above.

Now, perhaps I still don't understand what "doing away with the need for fish_user_key_bindings entirely" means. Do you mean removing this feature?

That would be a breaking change, and IMO, a less optimal plan of action, as it would force plugin manager authors to go to great lengths if they intend to support at least fish >=2.3, as I intend to do.

@faho
Copy link
Member

@faho faho commented Sep 18, 2018

Now, perhaps I still don't understand what "doing away with the need for fish_user_key_bindings entirely" means.

Correct.

Do you mean removing this feature?

What I mean, in short, is this: Making bind statements in config.fish work.

Wouldn't that run into the same problem of using bind in config.fish (you can't). thinking

Ever wondered why that is?

It's because we only set up your bindings after config.fish has run (via a function triggered --on-event fish_prompt), because your config.fish might contain changes to $fish_key_bindings. So we run the binding function which, as we've established, erases all bindings to get back to a clean slate [0], and that kills any bindings you've set up previously. Then we run fish_user_key_bindings.

What I want is to erase all bindings that have been set up as part of a binding set (which won't include any bindings you've set up previously). Then we can still run fish_user_key_bindings for backwards-compatibility and nice grouping. It just won't be necessary anymore.

Which means:

this wouldn't be scalable as it would force every script that wants to add its own bindings to copy any existing fish_user_key_bindings.

You wouldn't have to! You'd just do bind in your script.

And also:

That would be a breaking change

It wouldn't, because we'd still run fish_user_key_bindings. Only if you had bind statements in config.fish, they'd suddenly start to work!


[0]: Of course we could also stop erasing all bindings the first time the bindings are set up. That would make bind calls in config.fish work, unless you switched bindings later. Which is a bit dirty.

@faho faho mentioned this issue Sep 19, 2018
3 of 3 tasks complete
@faho faho closed this in 444f9f8 Sep 30, 2018
@faho faho modified the milestones: fish-future, fish-3.0 Sep 30, 2018
ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Nov 24, 2018
This allows for marking certain bindings as part of a preset, which allows us to

- only erase those when switching presets
- go back to the preset binding when erasing a user binding
- only show user customization if requested
- make bare bind statements in config.fish work (!!!11elf!!!)

Fixes fish-shell#5191.
Fixes fish-shell#3699.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants