-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Add cmdline tab-completion for setting string options #13182
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
Conversation
Add tab-completion for setting string options on the cmdline using `:set=` (along with `:set+=` and `:set-=`). The existing tab completion for setting options currently only works when nothing is typed yet, and it only fills in with the existing value, e.g. when the user does `:set diffopt=<Tab>` it will be completed to `set diffopt=internal,filler,closeoff` and nothing else. This isn't too useful as a user usually wants auto-complete to suggest all the possible values, such as 'iblank', or 'algorithm:patience'. For set= and set+=, this adds a new optional callback function for each option that can be invoked when doing completion. This allows for each option to have control over how completion works. For example, in 'diffopt', it will suggest the default enumeration, but if `algorithm:` is selected, it will further suggest different algorithm types like 'meyers' and 'patience'. When using set=, the existing option value will be filled in as the first choice to preserve the existing behavior. When using set+= this won't happen as it doesn't make sense. For flag list options (e.g. 'mouse' and 'guioptions'), completion will take into account existing typed values (and in the case of set+=, the existing option value) to make sure it doesn't suggest duplicates. For set-=, there is a new `ExpandSettingSubtract` function which will handle flag list and comma-separated options smartly, by only suggesting values that currently exist in the option. Note that Vim has some existing code that adds special handling for 'filetype', 'syntax', and misc dir options like 'backupdir'. This change preserves them as they already work, instead of converting to the new callback API for each option.
Marking this as draft to gather some feedback first. I still need to do a couple things but would like to the basic feedback before proceeding. The couple things are:
For the API design, I decided to add a generic function to each option (similar to what we did with I also decided not to touch 'filetype', 'syntax', 'backupdir' etc for now. Those have special handling and I left them be. There are also some interesting behaviors I found with commas. E.g. if you have a file with a comma in it (e.g. |
Oh, I really like this |
Awesome! Now I wonder if it’s possible to do something like Probably would only be enabled for |
That's a good idea! It's definitely possible within this framework to be added later. I think the callback can return some info on the The more annoying problem is actually figuring out what to show. The 'shortmess' option has individiual help tags for each flag (e.g. I do think if we want to do this (in a later commit), we may as well go all the way. Make it so that we could add popup docs for completing option names (e.g. Edit: (this makes me envious of Emacs… they have nice builtin documentation for a lot of builtin functions) |
Yep, agreed with all you said, esp. the bits about later :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very nice. Thanks for working on this. This will require quite a few new tests to make sure all the expansion cases are covered.
src/option.c
Outdated
if ((nextchar == '-' || nextchar == '+' || nextchar == '^') && p[1] == '=') | ||
{ | ||
if (nextchar == '-') | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent the style followed for the next if statement, can you remove the braces for this if statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can fix up the misc code styles issues. I do think it will be nice if at some point Vim can add a proper code formatter/linter that we can just run instead of having to manually conform. It's always a fair bit of work for the initial setup and code cleanups though so not suggesting right now.
src/option.c
Outdated
*numMatches = 1; | ||
return OK; | ||
} | ||
|
||
// Expansion handler for :set=/:set+= when the option has a custom expansion handler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For function headers, the style is to use the three part "/* */" comments.
'whichwrap': This option is quite weird as it's a comma-separated flag list (the commas are actually optional). Fix up the logic for set-= to handle it properly, and remove the ',' from WW_ALL so auto-complete works properly. 'concealcursor': This option is a flag list option but wasn't tagged correctly. Make sure to tag it as a P_FLAGLIST to make sure it follows the same logic as other similar options (e.g. 'mouse').
Some of these require keeping in sync with the parsing code, as the expansion code is using a separate char* array. List of options: - breakindentopt - clipboard - complete - cursorlineopt - If we change the parsing code to use `opt_strings_flags()` instead, we wouldn't have to duplicate the array. But it works for now. - eventignore - keyprotocol - Only the protocol (the second part after the colon) is matched. The first part (term name) is not matched, since there is no good weay to do so. - lispoptions - printoptions - rightleftcmd - spelloptions - wildmode Also, make the options that use ExpandGeneric not sort the results to give us more control.
Just pushed an update. I have now implemented all the options that I intended to. Among them, most of them are simple enumeration type options, or flags, and just consist of a few lines of additions. Some other options are a little more involved, and I tried to split the commits up to make them easier to review. If it's too much, I could split them into separate PRs to do later. Among them, Among the remaining options, I think some of them could have auto-complete added to them but I don't feel like adding them for now:
|
33390a5
to
be30d74
Compare
- Fix optiondef to make sure it works on tiny / gui builds - Fix misc warnings
be30d74
to
388570f
Compare
Make sure to clean up the match array if we found no matches because the caller doesn't clean up returned array if the number of matches is 0. Also, fix a bug in immediately returning instead of just setting a ret variable.
Deal with Windows escaping slashes differently on cmdline Also fix 'bg' could either be dark or light.
Codecov Report
@@ Coverage Diff @@
## master #13182 +/- ##
==========================================
+ Coverage 82.12% 82.14% +0.01%
==========================================
Files 160 160
Lines 195360 196007 +647
Branches 43823 43921 +98
==========================================
+ Hits 160446 161014 +568
- Misses 22076 22133 +57
- Partials 12838 12860 +22
Flags with carried forward coverage won't be shown. Click here to find out more.
|
3a12c75
to
a446b8b
Compare
a446b8b
to
12b6b18
Compare
Ok, I think this PR is ready to be reviewed. It should work, with the tests implemented, docs updated, etc. |
" Expand value for 'key' | ||
set key=abcd | ||
call feedkeys(":set key=\<Tab>\<C-B>\"\<CR>", 'xt') | ||
call assert_equal('"set key=*****', @:) | ||
call feedkeys(":set key-=\<Tab>\<C-B>\"\<CR>", 'xt') | ||
call assert_equal('"set key-=*****', @:) | ||
set key= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to also need if has('+key')
. 'key' option doesn't work if FEAT_CRYPT
isn't enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the whole completion for set key
(I mean what sense does it make to autocomplete *****
) and I also don't want to accidentally leak the crypt-key. ah this already seems to happen with the current vim. So probably fine. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually going to suggest that in a separate PR! Since I was looking into cmdline completion, I think key
needs some hardening.
I was going to propose removing completion for set key
, and also remove set-=
/ set+=
/ set^=
. Those have some security ramifications since they allow you to guess a substring of the key. We can just keep set=
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, yes makes sense!
@@ -382,6 +386,278 @@ func Test_set_completion() | |||
call assert_equal('"set syntax=' .. getcompletion('a*', 'syntax')->join(), @:) | |||
endfunc | |||
|
|||
" Test handling of expanding individual string option values | |||
func Test_set_completion_string_values() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the assert_equal()
calls in this function have wrong order of arguments. This will make error message confusing if test fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right… the first item is the expected value. I internalized the weirdness when I was debugging this but didn't correct it. I could fix it in a follow-up commit.
/* | ||
* Function given to ExpandGeneric() to obtain possible arguments of the | ||
* 'listchars' option. | ||
*/ | ||
char_u * | ||
get_listchars_name(expand_T *xp UNUSED, int idx) | ||
{ | ||
if (idx >= (int)(sizeof(lcstab) / sizeof(lcstab[0]))) | ||
return NULL; | ||
|
||
return (char_u*)lcstab[idx].name; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These don't include multispace
and leadmultispace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created PR #13225 to fix this.
@@ -1837,7 +1837,7 @@ stropt_get_newval( | |||
&(options[opt_idx]), OPT_GLOBAL)); | |||
else | |||
{ | |||
++arg; // jump to after the '=' or ':' | |||
++arg; // joption_value2stringump to after the '=' or ':' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like an accidental mistake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it was pointed out in 900894b#r128808467 as well. I have a follow-up PR that will fix this.
while (s > xp->xp_pattern && *(s - 1) == '\\') | ||
--s; | ||
if ((*p == ' ' && (xp->xp_backslash == XP_BS_THREE && (p - s) < 3)) | ||
|| (*p == ',' && (flags & P_COMMA) && ((p - s) % 1) == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (p - s) % 1
looks strange. Isn't any number module 1 always 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are absolutely right. I only moved this code around and added the P_COLON
check below it but I didn't change the logic for handling commas here. As you said, the logic here is wrong. I didn't want the scope of PR to grow any more which is why I didn't fix it.
I think the correct logic here is actually (p - s) >= 2
. Currently, if you have a file/folder of the name foo,bar
, the correct way to specify it in an option is do so something like :set backupdir=dir1,foo\\,bar,dir2
(note that this is equivalent to :set backupdir=dir1,foo\\\,bar,dir2
). Note that you will see that in the test cases that I added, they actually check that set-=
will correctly escape the commas to insert those "\" when doing set path-=<Tab>
.
Note: there's another cmd auto-complete bug (that's why I wanted to fix this in another PR instead of growing in scope). In the above example, if you do auto-complete as follows: set backupdir=fo<Tab>
, if will complete to set backupdir=foo,bar
, forgetting to insert the \\
there. In general the option setting syntax is a little weird regarding commas, as I believe the syntax is incomplete and doesn't allow certain types of names to be entered (you cannot enter a folder/file that ends with "", because that slash will end up escaping the comma instead).
tldr: Yes it's broken, but it's broken before my change and I could submit another PR to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commas will be fixed in #13303
The following warning messages are displayed in the Appveyor CI build (introduced by this PR):
|
Ok, I will fix them up in #13237. Found a couple other places that trigger warnings as well if I turn on |
Problem: cannot complete option values Solution: Add completion functions for several options Add cmdline tab-completion for setting string options Add tab-completion for setting string options on the cmdline using `:set=` (along with `:set+=` and `:set-=`). The existing tab completion for setting options currently only works when nothing is typed yet, and it only fills in with the existing value, e.g. when the user does `:set diffopt=<Tab>` it will be completed to `set diffopt=internal,filler,closeoff` and nothing else. This isn't too useful as a user usually wants auto-complete to suggest all the possible values, such as 'iblank', or 'algorithm:patience'. For set= and set+=, this adds a new optional callback function for each option that can be invoked when doing completion. This allows for each option to have control over how completion works. For example, in 'diffopt', it will suggest the default enumeration, but if `algorithm:` is selected, it will further suggest different algorithm types like 'meyers' and 'patience'. When using set=, the existing option value will be filled in as the first choice to preserve the existing behavior. When using set+= this won't happen as it doesn't make sense. For flag list options (e.g. 'mouse' and 'guioptions'), completion will take into account existing typed values (and in the case of set+=, the existing option value) to make sure it doesn't suggest duplicates. For set-=, there is a new `ExpandSettingSubtract` function which will handle flag list and comma-separated options smartly, by only suggesting values that currently exist in the option. Note that Vim has some existing code that adds special handling for 'filetype', 'syntax', and misc dir options like 'backupdir'. This change preserves them as they already work, instead of converting to the new callback API for each option. closes: vim/vim#13182 vim/vim@900894b Co-authored-by: Yee Cheng Chin <ychin.git@gmail.com>
Problem: cannot complete option values Solution: Add completion functions for several options Add cmdline tab-completion for setting string options Add tab-completion for setting string options on the cmdline using `:set=` (along with `:set+=` and `:set-=`). The existing tab completion for setting options currently only works when nothing is typed yet, and it only fills in with the existing value, e.g. when the user does `:set diffopt=<Tab>` it will be completed to `set diffopt=internal,filler,closeoff` and nothing else. This isn't too useful as a user usually wants auto-complete to suggest all the possible values, such as 'iblank', or 'algorithm:patience'. For set= and set+=, this adds a new optional callback function for each option that can be invoked when doing completion. This allows for each option to have control over how completion works. For example, in 'diffopt', it will suggest the default enumeration, but if `algorithm:` is selected, it will further suggest different algorithm types like 'meyers' and 'patience'. When using set=, the existing option value will be filled in as the first choice to preserve the existing behavior. When using set+= this won't happen as it doesn't make sense. For flag list options (e.g. 'mouse' and 'guioptions'), completion will take into account existing typed values (and in the case of set+=, the existing option value) to make sure it doesn't suggest duplicates. For set-=, there is a new `ExpandSettingSubtract` function which will handle flag list and comma-separated options smartly, by only suggesting values that currently exist in the option. Note that Vim has some existing code that adds special handling for 'filetype', 'syntax', and misc dir options like 'backupdir'. This change preserves them as they already work, instead of converting to the new callback API for each option. closes: vim/vim#13182 vim/vim@900894b Co-authored-by: Yee Cheng Chin <ychin.git@gmail.com>
Problem: cannot complete option values Solution: Add completion functions for several options Add cmdline tab-completion for setting string options Add tab-completion for setting string options on the cmdline using `:set=` (along with `:set+=` and `:set-=`). The existing tab completion for setting options currently only works when nothing is typed yet, and it only fills in with the existing value, e.g. when the user does `:set diffopt=<Tab>` it will be completed to `set diffopt=internal,filler,closeoff` and nothing else. This isn't too useful as a user usually wants auto-complete to suggest all the possible values, such as 'iblank', or 'algorithm:patience'. For set= and set+=, this adds a new optional callback function for each option that can be invoked when doing completion. This allows for each option to have control over how completion works. For example, in 'diffopt', it will suggest the default enumeration, but if `algorithm:` is selected, it will further suggest different algorithm types like 'meyers' and 'patience'. When using set=, the existing option value will be filled in as the first choice to preserve the existing behavior. When using set+= this won't happen as it doesn't make sense. For flag list options (e.g. 'mouse' and 'guioptions'), completion will take into account existing typed values (and in the case of set+=, the existing option value) to make sure it doesn't suggest duplicates. For set-=, there is a new `ExpandSettingSubtract` function which will handle flag list and comma-separated options smartly, by only suggesting values that currently exist in the option. Note that Vim has some existing code that adds special handling for 'filetype', 'syntax', and misc dir options like 'backupdir'. This change preserves them as they already work, instead of converting to the new callback API for each option. closes: vim/vim#13182 vim/vim@900894b Co-authored-by: Yee Cheng Chin <ychin.git@gmail.com>
Problem: cannot complete option values Solution: Add completion functions for several options Add cmdline tab-completion for setting string options Add tab-completion for setting string options on the cmdline using `:set=` (along with `:set+=` and `:set-=`). The existing tab completion for setting options currently only works when nothing is typed yet, and it only fills in with the existing value, e.g. when the user does `:set diffopt=<Tab>` it will be completed to `set diffopt=internal,filler,closeoff` and nothing else. This isn't too useful as a user usually wants auto-complete to suggest all the possible values, such as 'iblank', or 'algorithm:patience'. For set= and set+=, this adds a new optional callback function for each option that can be invoked when doing completion. This allows for each option to have control over how completion works. For example, in 'diffopt', it will suggest the default enumeration, but if `algorithm:` is selected, it will further suggest different algorithm types like 'meyers' and 'patience'. When using set=, the existing option value will be filled in as the first choice to preserve the existing behavior. When using set+= this won't happen as it doesn't make sense. For flag list options (e.g. 'mouse' and 'guioptions'), completion will take into account existing typed values (and in the case of set+=, the existing option value) to make sure it doesn't suggest duplicates. For set-=, there is a new `ExpandSettingSubtract` function which will handle flag list and comma-separated options smartly, by only suggesting values that currently exist in the option. Note that Vim has some existing code that adds special handling for 'filetype', 'syntax', and misc dir options like 'backupdir'. This change preserves them as they already work, instead of converting to the new callback API for each option. closes: vim/vim#13182 vim/vim@900894b Co-authored-by: Yee Cheng Chin <ychin.git@gmail.com>
Currently cmdline expansion for setting option values that take a list of file names (as a comma-separated list) isn't accounting for file names with commas. For such options, the commas need to be escaped, as a reverse of `copy_option_part()` which will unescape it. Fix as follows: - Cmdline expansion for options with comma-separated file/folder names will not start a new match when seeing `\\,` and will instead consider it as one value. - File/folder matching code will strip the `\\` when seeing `\\,` to make sure it can find the correct files/folders. - The expanded value will escape `,` with `\\,`, similar to how spaces are escaped. Also fix up documentation to be clearer. The existing docs are misleading in how it discusses triple slashes for 'tags'. Also, see: vim#13182 (comment)
Add tab-completion for setting string options on the cmdline using
:set=
(along with:set+=
and:set-=
).The existing tab completion for setting options currently only works when nothing is typed yet, and it only fills in with the existing value, e.g. when the user does
:set diffopt=<Tab>
it will be completed toset diffopt=internal,filler,closeoff
and nothing else. This isn't too useful as a user usually wants auto-complete to suggest all the possible values, such as 'iblank', or 'algorithm:patience'.For set= and set+=, this adds a new optional callback function for each option that can be invoked when doing completion. This allows for each option to have control over how completion works. For example, in 'diffopt', it will suggest the default enumeration, but if
algorithm:
is selected, it will further suggest different algorithm types like 'meyers' and 'patience'. When using set=, the existing option value will be filled in as the first choice to preserve the existing behavior (this helps doing small edits to the option value). When using set+= this won't happen as it doesn't make sense to append an option by its own value in general.For flag list options (e.g. 'mouse' and 'guioptions'), completion will take into account existing typed values (and in the case of set+=, the existing option value) to make sure it doesn't suggest duplicates.
For set-=, there is a new
ExpandSettingSubtract
function which will handle flag list and comma-separated options smartly, by only suggesting values that currently exist in the option so you won't try to subtract the wrong value.Note that Vim has some existing code that adds special handling for 'filetype', 'syntax', and misc dir options like 'backupdir'. This change preserves them as they already work, instead of converting to the new callback API for each option.