jethrokuan / fzf

Ef-🐟-ient fish keybindings for fzf
MIT License
863 stars 65 forks source link

Key bindings not loaded in omf #137

Closed StealthyCoder closed 3 years ago

StealthyCoder commented 4 years ago
Oh My Fish version:   6
OS type:              Linux
Fish version:         fish, version 3.0.2
Git version:          git version 2.20.1
Git core.autocrlf:    no

The only one thing missing is a file called key_bindings.fish somewhere in the package structure. This one is called fzf_key_bindings.fish. I looked over at the OMF repo and they state that the packages individually should be patched instead of solving it in the omf core.

Something can be said for that for sure. I will make a PR and just fix it by adding a simple file in the conf.d that references the other file if not already loaded.

StealthyCoder commented 4 years ago

138 Should hopefully contain the fix with backwards compatibility and complying with omf spec.

jethrokuan commented 4 years ago

@StealthyCoder can you take a look at https://github.com/oh-my-fish/oh-my-fish/issues/646

Everything in the conf.d folder should be sourced, so the patch shouldn't be necessary.

StealthyCoder commented 4 years ago

I checked the PR and the issue and the conf.d is not sourced :disappointed:

The problem is in the line I specified, the * does not go through all dirs recursively just the top ones. So in this case fzf. Therefore it will not source conf.d.

StealthyCoder commented 4 years ago

Also I updated my PR because when you install the package fzf so the other one, via apt in my case being on Ubuntu I see that it installs the bindings in vendor_functions.d folder and by default loads those instead. So even then it did not work for me. I want this package's key bindings.

StealthyCoder commented 4 years ago

My mistake, I retract my earlier statement. It loads the configurations from another file. In require.fish.That one will source all the conf.d files. Okay learned something today.

Do you still want to keep this PR because the fzf_key_bindings in vendor_functions.d still could interfere. I could change it to just making sure these keybindings get loaded instead of the other one? What do you think @jethrokuan ?

StealthyCoder commented 4 years ago

I just tested with dev channel and sadly the other vendor_functions.d one gets loaded instead of this package's one. Even with my PR it seems the wrong bindings get loaded when in dev channel. :disappointed:

StealthyCoder commented 4 years ago

OMG, it has been a long day and I set the FZF_LEGACY_KEYBINDINGS to 1.. therefore the bindings did not show up correctly. I stop now. :joy:

So to conclude in the end, my PR does set the correct bindings even though the package maintainer fzf provides the key bindings as well.

jorgebucaran commented 4 years ago

@StealthyCoder Is OMF behavior correct? The conf.d directory should be sourced by default since fish 3.0.2. I don't think there's anything we should change in fzf.

WIZARDELF commented 4 years ago

@StealthyCoder have you tried this workaround? $ source ~/.config/fish/conf.d/fzf_key_bindings.fish

StealthyCoder commented 4 years ago

Sorry to get back to you so late. @jorgebucaran you are correct in the so called dev channel of OMF the conf.d gets sourced. I ran into the problem that you also have to install fzf binary which I did through APT on Ubuntu. So that package also supplies key bindings and somehow I had legacy mode on.

@Kejia That workaround will work indeed. It was late and a long day :D I think I will close this issue now and my PR as well. I am not sure it is needed. Unless this function can be used to always make sure this particular keybinding should be loaded automatically. In OMF there are two places where things get sourced, once in init.fish and once in require.fish. I am not sure if that is the way to go but that is what OMF fixed :smile:

jorgebucaran commented 3 years ago

@jethrokuan conf.d should be sourced, and there's nothing we can do from here. I suggest closing.