search

jethrokuan / fzf

Ef-🐟-ient fish keybindings for fzf
MIT License
861 stars 66 forks source link

Updating FZF via Fisher resets user settings #107

Closed rbpatt2019 closed 5 years ago

rbpatt2019 commented 5 years ago

Sorry to raise two issues back-to-back!

I went to update FZF using fisher - as I installed your package that way - only to find that updating reset all my settings for FZF (FZF_DEFAULT_OPTS, eg). This is particularly odd because, as far as I can see, everything in conf.d/fzf.fish and conf.d/fzf_key_bindings.fish is already coded to prevent this, using if test and set -q.

Honestly not sure if this is a bug with FZF or fisher. If it's fisher, I'll close this and move the issue to their repo.

Thanks for the help!

jethrokuan commented 5 years ago

I can see how this happens. What probably happens on updates is that the __fzf_uninstall function is called which unsets the variables you have set.

@jorgebucaran thoughts? maybe not call the __uninstall function in fisher if it's just an update?

jorgebucaran commented 5 years ago

@jethrokuan Instead of erasing all the variables that begin with FZF_, erase only the variables you set. In other words, don't touch variables set by the user.

jethrokuan commented 5 years ago

unless I add some machinery, it's impossible to know if the user overrode the defaults of those variables, and hence whether to erase them or not. Think I'll just disable erasing of variables for now.

jorgebucaran commented 5 years ago

Nevermind, I see they're the same variables. A temporary solution is not erasing the variables.

jethrokuan commented 5 years ago

@rbpatt2019 the issue was auto-closed with the PR, let me know if you still face any issues. Sorry for the trouble!

rbpatt2019 commented 5 years ago

@jethrokuan No worries! I appreciate all the work. Pulled in the changes. Quick fix works great! As way of a solution, would having the user set any custom variables in their config.fish file get around this? That way, any defaults could be set by the plugin, but the shell instance would overwrite those that are customized with the values in config.fish.

jethrokuan commented 5 years ago

that works for sure, but it's not ideal and would still lead to surprising behaviour (that you experienced). Remnant env variables aren't that big of a deal so I think that's fine.

jorgebucaran commented 5 years ago

@jethrokuan Correct, but using global variables would be a bit nicer, generally speaking. Just my two cents.