search

autozimu / LanguageClient-neovim

Language Server Protocol (LSP) support for vim and neovim.
MIT License
3.55k stars 273 forks source link

Attempt to fix autodetection of fzf #1122

Closed otaconix closed 3 years ago

otaconix commented 3 years ago

Prior to this commit, even if fzf was available on the PATH, it wasn't used for the pleasant UI. Setting g:LanguageClient_selectionUI to FZF definitely worked, but the documentation says this isn't necessary.

Please note that I've not tested this extensively. Removing fzf from the PATH doesn't result in any problems, but I haven't checked if there are more places where this was/is broken.

martskins commented 3 years ago

Nice catch!

Your fix sadly breaks the other options for selection UI (clap for example, or anything that uses funcref). I think what we really need to do is change the default v:null for FZF on this line: https://github.com/autozimu/LanguageClient-neovim/blob/d6d6d0804d3372ce2c67b092aa19d615d96ce9e3/autoload/LanguageClient.vim#L1469

If you are happy to make that change I can give it a try and merge this.

otaconix commented 3 years ago

@martskins I pushed your suggested change. Are you sure though? It looked as though the call to s:selectionUI_funcref eventually does the heavy lifting of handling funcref, though I may be misunderstanding :)

martskins commented 3 years ago

Yeah maybe this could be simplified a little but if you what you want to fix is the default being FZF when it's loaded then that should do it. WIth the v:null we had there if you didn't specifically set FZF it wouldn't matter if it was loaded or not.

I'll give this a try but I think it should be enough to solve this.

I also forgot to mention, PRs should be opened pointing the dev branch, could you please change that? You can do that without closing this PR by pressing the edit button next to the title of the PR.

otaconix commented 3 years ago

Just in case you don't get notified of edits to PR's, here's a message that should result in a notification.

martskins commented 3 years ago

This seems to be working for me. I think it's safe to merge to dev.

Remember that to see this change you'll need to switch to branch dev on your plugin definition and also build the plugin from source. Otherwise you can just manually edit that file until we cut a new release (surely next week).

Thanks for contributing!