9:27 a.m.
On 7/15/19 1:07 PM, Erik Skultety wrote:
On Wed, Jun 19, 2019 at 01:50:14PM +0200, Michal Privoznik wrote:
> On 6/19/19 12:59 PM, Erik Skultety wrote:
>> On Tue, Jun 18, 2019 at 03:46:15PM +0200, Michal Privoznik wrote:
>>> There are more arguments than 'shutdown --mode' that accept a
>>> list of strings separated by commas. 'nodedev-list --cap' is one
>>> of them. To avoid duplicating code, let's separate interesting
>>> bits of virshDomainShutdownModeCompleter() into a function that
>>> can then be reused.
>>>
>>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>>> ---
>>> tools/virsh-completer.c | 120 ++++++++++++++++++++++++++--------------
>>> 1 file changed, 77 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
>>> index 7d5cf8cb90..ef2f39320e 100644
>>> --- a/tools/virsh-completer.c
>>> +++ b/tools/virsh-completer.c
>>> @@ -69,6 +69,79 @@
>>> */
>>>
>>>
>>> +/**
>>> + * virshCommaStringListComplete:
>>> + * @input: user input so far
>>> + * @options: ALL options available for argument
>>> + *
>>> + * Some arguments to our commands accept the following form:
>>> + *
>>> + * virsh command --arg str1,str2,str3
>>> + *
>>> + * This does not play nicely with our completer funtions, because
>>> + * they have to return strings prepended with user's input. For
>>> + * instance:
>>> + *
>>> + * str1,str2,str3,strA
>>> + * str1,str2,str3,strB
>>> + * str1,str2,str3,strC
>>
>> ^This sounds rather sub-optimal. I wouldn't even insist on making the
>> suggestions contextual like it is now, IOW not suggesting options which have
>> already been specified and would rather return the same list of possible
>> options than a string with the user input prepended.
>
> So IIUC, for 'shutdown --mode <TAB><TAB>' you want to see:
>
> "acpi", "agent", "initctl", "signal",
"paravirt"
>
> and for 'shutdown --mode acpi,agent,<TAB><TAB>' you want to see
the same
> list again (optionally with already specified strings removed)? Yep, that
> would be great but I don't think that is how readline works. At least, I
> don't know how to achieve that. Do you perhaps have an idea?
It very well may be the case that it doesn't work the way we'd like to and I
don't understand how it actually works, but why does readline even matter here?
Readline calls our completers which generate the output presented to the user,
so we should be in charge what is returned, so why do we need to prepend the
user input then? In fact, I found there's a function called vshCompleterFilter
which removes the whole output list if the items are not prepended with the
original user input, why is that? When I commented out the bit dropping items
from the list and stopped pre-pending the user input, I achieved what I
suggested in my original response to this series, a context-based list without
unnecessary prefixes.
This very likely did not work and only gave impression it is working.
I've just tried what you suggest here and find it not working. The
reason is that if we return only one option to complete it replaces the
whole argument with that string. Or, if we return more strings then the
argument is replaced with their longest shared prefix. For instance, if
our completer would return only {"testA", "testB", NULL}, then the
following input:
virsh # start t<TAB><TAB>
would be overwritten to:
virsh # start test
testA testB
This is expected and in fact desired. But things get tricky when we
start dealing with out argument lists:
virsh # shutdown --mode <TAB><TAB>
gets you:
virsh # shutdown --mode a
acpi agent
So far so good. But then you introduce comma:
virsh # shutdown --mode agent,a<TAB><TAB>
Now, there is only one possible completion = "acpi". So readline saves
you some typing and turns that into:
virsh # shutdown --mode acpi
Problem is that readline does not handle comma as a separator. Okay, we
can fix that. It's easy to put comma at the end of @break_characters in
vshReadlineInit(). But that breaks our option lookup because then @text
== "a" in vshReadlineParse(). On one hand we want @text == "a" because
that means that readline split user's input at the comma, on the other
hand we can't now properly identify which --option is user trying to
autocomplete because neither --option has "a" as its value (--mode has
"agent,a").
I also tried a few other random completions to see
whether I didn't break something by stripping some code from
vshCompleterFilter and it looks like it worked, so the question is, what was
the reason for that function in the first place, since I haven't experienced
the effects described by commit d4e63aff5d0 which introduced it?
The reason for existence of vshCompleterFilter() is to filter out
non-relevant options. For instance, in aforementioned shutdown mode
completer - we want, I want completers to be as simple as possible.
Therefore, the shutdown mode completer returns all five strings,
regardless of user's input. Then the filter function prunes out those
strings (=options) which do not share prefix with user's input. For
instance, if user's input is "a" then "initctl",
"signal" and "paravirt"
will be filtered out. If they weren't, and they would be returned back
to readline, it would present them to the user for complete and it would
look like this:
virsh # shutdown --mode a<TAB><TAB>
acpi agent initctl paravirt signal
And I believe we can both agree that this is bad behaviour.
Maybe solution is to not call rl_completion_matches() in
vshReadlineCompletion() and utilize @start and @end arguments which
point at the beginning and end of the word that user is trying to
complete (although how would we use them to find the corresponding
--option is something I do not know). But that's something I haven't tried.
Michal