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

Add tab completion for neovim #9543

Merged
merged 10 commits into from
Feb 8, 2023

Conversation

bagohart
Copy link
Contributor

@bagohart bagohart commented Feb 5, 2023

Description

Added tab completion for neovim, mostly based on the existing vim completions.
In the process of reading the docs, I noticed that two options (startuptime and clean) are also available for vim, so I added them there, too.

Fixes issue #9535

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

complete -c nvim -l serverlist -d 'List all Vim servers that can be found'
complete -c nvim -l servername -d 'Set server name'

# Options not included in vim.fish, also available in vim
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... not included in vim.fish ...

After this PR, aren't these in vim.fish as well? So this comment is obsolete.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's right. I will remove them if everything else is okay.
I wasn't sure how to structure the nvim.fish file since it shares a lot of code with the vim.fish file, so I decided to add a few maybe unnecessary comments so it's more comprehensible for the reviewers where the respective entries came from. (Forgot to mention that in the original post.)

Comment on lines 39 to 46
complete -c nvim -l remote -d 'Edit files on NVim server'
complete -c nvim -l remote-expr -d 'Evaluate expr on NVim server'
complete -c nvim -l remote-send -d 'Send keys to NVim server'
complete -c nvim -l remote-silent -d 'Edit files on NVim server'
complete -c nvim -l remote-wait -d 'Edit files on NVim server'
complete -c nvim -l remote-wait-silent -d 'Edit files on Vim server'
complete -c nvim -l serverlist -d 'List all Vim servers that can be found'
complete -c nvim -l servername -d 'Set server name'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove the options that aren't yet supported by any official neovim release. e.g. nvim --serverlist returns the singularly unhelpful message nvim: Garbage after option argument: "--serverlist" and would mislead a user into thinking they aren't using it right (instead of printing a "not implemented" message).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay. I think it's helpful to leave them in (but commented out) so it's easy to add them once they're supported and so the next person looking at this understands why they were omitted in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commenting them out and leaving a note is fine.

Comment on lines 6 to 8
complete -c nvim -s o -r -d 'Open horizontally split windows for each file'
complete -c nvim -s O -r -d 'Open vertically split windows for each file'
complete -c nvim -s p -r -d 'Open tab pages for each file'
Copy link
Contributor

@mqudsi mqudsi Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These completions are not exactly correct (for both vim and neovim).

  -o[N]                 Open N windows (default: one per file)
  -O[N]                 Open N vertical windows (default: one per file)
  -p[N]                 Open N tab pages (default: one per file)

complete -r means the current switch requires an argument. -[oOp] don't take any arguments, but they (say that they) require that vim/nvim be executed with at least one file argument (except in practice they don't because not supplying an argument just opens N windows/tabs). So using -r here is wrong because it will suppress another option afterwards (or at least it's supposed to).

Aside from that, I don't know if there's any meaningful way for us to complete the -[oOp]N version of the switches. One option would be to add a second complete line with (maybe!) an old-style completion like -o2 and a description "open two horizontally-split windows" and the user can take that as a hint to modify the 2 for themselves as needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the -o2 thing, implemented.

Copy link
Contributor

@mqudsi mqudsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. I've added some more review feedback. No harshness is intended; I applaud you for contributing better neovim completions and just want to make sure they are the best they can be.

complete -c nvim -s p -r -d 'Open tab pages for each file'
complete -c nvim -s q -r -d 'Start in quickFix mode'
complete -c nvim -s r -r -d 'Use swap files for recovery'
complete -c nvim -s t -xa '(__fish_vim_tags)' -d 'Set the cursor to tag'
Copy link
Contributor

@mqudsi mqudsi Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't use __fish_vim_tags here because there is no guarantee the function is loaded (and it probably is not). A function is automatically loaded from a file if the file name matches the function name (and the file containing the function is in the function search path). But __fish_vim_tags is written in vim.fish.

You would need to either move __fish_vim_tags to its own file, copy it (with a new name) to this file, or (best option, imho) add the following at the top of nvim.fish:

type -q __fish_vim_tags || source (status dirname)/vim.fish

to load the functions defined in vim.fish before running the completion script (if the vim completions script hasn't yet been loaded because the user tried to complete vim).

Comment on lines 36 to 39
complete -c nvim -l remote -d 'Edit files on NVim server'
complete -c nvim -l remote-expr -d 'Evaluate expr on NVim server'
complete -c nvim -l remote-send -d 'Send keys to NVim server'
complete -c nvim -l remote-silent -d 'Edit files on NVim server'
Copy link
Contributor

@mqudsi mqudsi Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change "on NVim server" to "on server specified with --server" as it is required to use --server <server> with these commands or an error message will be printed to the terminal (and not visible because a new neovim instance will launch in the alternate buffer).

Also, please do not capitalize nvim as NVim anywhere. It is either Neovim to refer to it as a proper noun or neovim colloquially (preferred as this is how the name is stylized on the official neovim website) or nvim to refer to the executable.

@mqudsi mqudsi merged commit ef07e21 into fish-shell:master Feb 8, 2023
@mqudsi
Copy link
Contributor

mqudsi commented Feb 8, 2023

@bagohart thanks for putting in all the hard work and research on these! I've merged it.

@bagohart
Copy link
Contributor Author

bagohart commented Feb 8, 2023

Nice! Thanks for your improvements, too. tbh didn't anticipate getting that much feedback for a bunch of mundane tab completions :)

@mqudsi
Copy link
Contributor

mqudsi commented Feb 9, 2023

We try!

@zanchey zanchey added this to the fish 3.6.1 milestone Feb 21, 2023
zanchey pushed a commit that referenced this pull request Feb 27, 2023
Separate the neovim completions from the vim ones, as their supported
options have diverged considerably.

Some documented options are not yet implemented, these are added but
commented out.

Closes #9535.

---------

Co-authored-by: Mahmoud Al-Qudsi <mqudsi@neosmart.net>
(cherry picked from commit ef07e21)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants