Expose $key_sequences

[kidonng]

Congratulations on the 7.0 release! I love the new fzf_configure_bindings, it's friendly and idempotent, just like fish_add_path. However, aside from using fish_add_path, fish also allows directly using the universal variable $fish_user_paths, but there's no similar way in fzf.fish.

I know one of the reason for the overhauling is because universal variables are not easy to understand and use. But I'm not a fan of calling fzf_configure_bindings twice either. We can have a more flexible way to configure the keybindings.

I propose to expose $key_sequences, more precisely, move this line to conf.d/fzf.fish:

fzf.fish/functions/fzf_configure_bindings.fish

Line 16 in 21dd7eb

set key_sequences \e\cf \e\cl \e\cs \cr \cv # \c = control, \e = escape

It may be become:

set -q fzf_key_sequences || set -g fzf_key_sequences \e\cf \e\cl \e\cs \cr \cv

And if a user wants to customize, say, history shortcut to \er they can do so via:

set -U fzf_key_sequences $fzf_key_sequences[1..3] \er $fzf_key_sequences[5]

[PatrickF1]

Hi, thanks for the note! I appreciate your idea and it sounds pretty simple. However, here are my reasons against it

• It'll make fzf_configure_bindings a little more complicated. That means more tests, more documentation, another way for fzf_configure_bindings to break.
• The interface for it has a major abstraction leak in fzf_key_sequences being exposed.
• Modifying fzf_key_sequences as an array is not intuitive to use.

Sorry. I hope at least fzf_configure_bindings is an upgrade from the DIY custom bindings method so you benefit as well. If not, please let me know why not.

Also, what about calling fzf_configure_bindings don't you like? I used to not like this idea. It wasn't elegant at all. Initially, I was going to not set any default bindings and make everyone just execute the command themselves in their config.fish. But I realized that a lot of other programs/apps/CLIs have defaults that require some code to be executed only for it to be undone when the user customizes it. So after realizing that, I became okay with the idea.

[kidonng]

The idea is basically the same as that fish_add_path is a front end for $fish_user_paths.

• It'll make fzf_configure_bindings a little more complicated. That means more tests, more documentation, another way for fzf_configure_bindings to break.

That's true for every feature, not going to argue. However it doesn't fundamentally change how fzf_configure_bindings work, and there is little to be added given the existing excellent documentation.

• The interface for it has a major abstraction leak in fzf_key_sequences being exposed.
• Modifying fzf_key_sequences as an array is not intuitive to use.

I would argue a major abstraction leak is not true. "bind first to do thing A, second to do things B..." is just as plain simple as it looks. Plus, this is intended for those who seeks for a more flexible configuration.

Sorry. I hope at least fzf_configure_bindings is an upgrade from the DIY custom bindings method so you benefit as well. If not, please let me know why not.

My current setup:

if functions -q fzf_configure_bindings
	  fzf_configure_bindings --directory=\cf --history=\e\cr
	  # These are custom functions
	  bind \cr _fzf_search_history_no_timestamp
	  bind \e\cf _fzf_search_directory_all
end

I feel like overriding fzf_configure_bindings directly as it's more straightforward for me.

Also, what about calling fzf_configure_bindings don't you like?

Not that I dislike it, but "I'm not a fan of calling fzf_configure_bindings twice either".

I recently switched to Nix package manager and my fish start up time increased noticeably, so I profiled my startup time and tries to cut down any unnecessary calls. fzf_configure_bindings isn't slow at all but I still want to save the time.

[PatrickF1]

I'm loathe to do this currently but I'll wait to see if there's more interest before deciding.

fzf_configure_bindings isn't slow at all but I still want to save the time.

If it's slow, that would be a compelling reason for me. What's the run time on it?