On Thu, Jul 18, 2019 at 11:36:27AM +0200, Martin Kletzander wrote:
> On Mon, Jul 15, 2019 at 04:27:04PM +0200, Michal Privoznik wrote:
> > 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@xxxxxxxxxx>
> > > > > > ---
> > > > > > 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.
> >
>
> So I understand what Erik wants to have. And yes, it would be a very nice from
> the UX POV, but AFAIK it is nearly impossible to do with readline.
>
> [OK, I'll stop with the acronyms now.]
>
> I think we all agree that rewriting readline does not make sense, and I think we
> might be on the same page (or at least getting there) regarding to working
> around the limitations in readline completion functions. Let me fill up some
> info here that Michal maybe thought is common knowledge or maybe forgot to
> mention it here, just so we (hopefully) get on the same page.
>
> What I use, for example, is completing various things not only based on the
> prefix, let me give you an actual example:
>
> Let's say you are in a directory with files like
>
> my_first_file_hash_cb6a4f64efbc.txt
> my_second_file_hash_1b162a344123.txt
> my_second_file_hash_cacbf987dcb.json
>
> And you want to print second file ending in txt. What you can do is write:
>
> cat second.txt<TAB>
>
> And then you get the only completion that has the characters "second.txt"
> somewhere in the filename in this order, but not necessarily right next to each
> other, which is what you wanted. This is called a fuzzy completion and it is an
> absolutely amazing thing to speed up your workflow when you start using it.
>
> Back to the thing we're talking about here and now. If the readline function
> just accepted the list of strings that can be appended to the current command
> line, then you would not be able to do this. And it's not only needed for this
> fuzzy completion, it might be used to do other tricks as well.
>
> So basically the readline API, as far as I understand, takes the list of
> possible completions that the current string would be transformed to, not
> appended. There is a possibility of telling readline to break words on comma as
> well, but then you wouldn't know whether you are completing an extra value for
> the current parameter or another parameter. There can be ways around it, but
> that gets me to the main two points in this reply:
>
> 1) I do not think it is worth it.
>
> 2) Even if someone wanted to do that, I don't think the discussion should be
> happening in a reply to a patch that just moves some existing code around.
This was a fair enough comment.
Okay then, consider this:
Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
