Re: [libvirt] [PATCH 1/2] virsh-completer: Separate comma list construction into a function

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(a)redhat.com&gt; > > > > > --- > > > > > 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.