PATCH: was Re: Closing quotes in completion

PATCH: was Re: Closing quotes in completion

[Sven Wischnowsky]

Peter Stephenson wrote:

> I wrote the following completer called _closequotes to stick before
> _complete; if it finds the completion is in quotes, it adds the appropriate
> closing quote as an ignored suffix.  It works fine for simple cases, but
> for anything more complicated (such as file paths with a slash) the closing
> quote appears too far back in the string (and in that case you want to get
> rid of it anyway).  Any ideas?  I suppose using a `compctl -M' doesn't work
> because it doesn't match against $ISUFFIX anyway.

[ Sorry for the late response... ]

Well, I hadn't envisioned this kind of usage, but let's have a look...

First, this showed a problem in the code, where the automatically
inserted suffixes (e.g. a slash for directories) were inserted after
the ignored suffix, which was wrong.

Then, I would change the test to:

  if [[ -n $compstate[quote] && $RBUFFER != *${compstate[quote]}* ]]; then

so that we don't insert another quote if there already is one.

Also, in your mail (above) you said `too far back', while in the
function you say `too early'. Playing a bit with this, I haven't found 
an example for either of these, could you tell me... (but then again,
you probably just meant the bug fixed by the patch).

I have, however, found another problem. Without menucompletion, but
automenu, if you do:

  ls '/f/b<TAB>

(with multiple possible completions for the `f'), the cursor is moved
after the `f', and a closing quote is inserted. Immediatly tabbing
again, to start automenu will remove the quotes. The problem is the
fix from message 5483. The code remembers that the first completion
attempt started with the cursor after the word. Because of that it
moves the cursor back to the end again (after the inserted quote),
before starting menucompletion. Then the code in `get_comp_string()'
removes the quotes because it thinks both quotes are `in the word'.

Currently I have no idea, how to solve this. When an unambiguous
string is inserted, we can't easily get the position where the ignored 
suffix starts which would be needed to make the code move the cursor
before it when starting automenu. Also, this would be the right thing
in this case, but I have the impression that it would be wrong in
other cases (without being able to come up with examples just now).

Well, otherwise I haven't found any problems with this, apart from the
fact that we can't easily make it auto-removable. For that we would
have to use the same trick _approximate uses: temporarily define wrapper
functions for compgen and compadd and there fiddle with the argument
list (it would be more complicated than in _approximate if you want to 
append the quote to the suffix given by the user, if any, and if none
was given, just use the quote as an auto-removable suffix -- we would
need quite a bit os argument list parsing here).

But since this isn't the first place where such a wrapper would be
useful, we might want to think about some extension to the C-code to
simplify such things. Any suggestions? (Some kind of associative array 
that holds default values? Hm, for some of the above this wouldn't be
enough. An associative array that holds values that are to be added to 
those given to comp{add,gen}? This may look weird to some users,
though.)

This also reminds me of the last thing I'm still thinking about for
the new style completion stuff: a way to access and probably modify
the completions added.
Currently I think about a builtin, say `compdata', that gets the name
of a parameter and the number of a match or group as arguments and
then stores information about the match/group in a associative array
with the given name. Options would say if we want to access the old
list (if any) or the new one, and if we want to access a group or a
match. Then we could either add an option that allows to modify the
existing completions from such an associative array or we allow
options a la compadd to modify the various components directly.

There are several problems, though. For example: the numbering of the
matches, which is really done after the sorting, not while other
matches can still be added. Then we have the problem that some of the
matches may have went into the alternate set instead of the normal set 
(the fignore-thing). Finally, some of the strings stored with the
matches can't easily be changed, including the -P-prefix and ISUFFIX,
but not including the -S-suffix, because the first two are also
contained in the unambiguous string build while adding completions
(which would then become invalid if we allow to modify them).

Because of these problems I haven't implemented anything of this yet,
so I'd like to hear comments/suggestions about all this.

Bye
 Sven

P.S.: What has become of the `re-use-old-list'-completer/patch?


diff -u os/Zle/compctl.c Src/Zle/compctl.c
--- os/Zle/compctl.c	Mon Apr 26 15:41:27 1999
+++ Src/Zle/compctl.c	Tue Apr 27 09:02:12 1999
@@ -1692,7 +1692,7 @@
 bin_compadd(char *name, char **argv, char *ops, int func)
 {
     struct cadata dat;
-    char *p, **sp, *e, *m;
+    char *p, **sp, *e, *m = NULL;
     int dm;
     Cmatcher match = NULL;
 
@@ -1839,7 +1839,7 @@
     if (!*argv)
 	return 1;
 
-    match = cpcmatcher(match);
+    dat.match = match = cpcmatcher(match);
     dm = addmatchesptr(&dat, argv);
     freecmatcher(match);
 
diff -u os/Zle/zle_tricky.c Src/Zle/zle_tricky.c
--- os/Zle/zle_tricky.c	Mon Apr 26 15:41:29 1999
+++ Src/Zle/zle_tricky.c	Tue Apr 27 09:51:02 1999
@@ -6730,11 +6730,13 @@
     return scache;
 }
 
-/* Insert the given match. This returns the number of characters inserted.*/
+/* Insert the given match. This returns the number of characters inserted.
+ * scs is used to return the position where a automatically created suffix
+ * has to be inserted. */
 
 /**/
 static int
-instmatch(Cmatch m)
+instmatch(Cmatch m, int *scs)
 {
     int l, r = 0, ocs, a = cs;
 
@@ -6784,6 +6786,7 @@
     } else
 	brscs = -1;
     /* -S suffix */
+    *scs = cs;
     if (m->suf) {
 	inststrlen(m->suf, 1, (l = strlen(m->suf)));
 	r += l;
@@ -6855,13 +6858,13 @@
 
 	/* If REC_EXACT and AUTO_MENU are set and what we inserted is an  *
 	 * exact match, we want menu completion the next time round       *
-	 * so we set fromcomp,to ensure that the word on the line is not  *
+	 * so we set fromcomp, to ensure that the word on the line is not *
 	 * taken as an exact match. Also we remember if we just moved the *
 	 * cursor into the word.                                          */
 	fromcomp = ((isset(AUTOMENU) ? FC_LINE : 0) |
 		    ((atend && cs != lastend) ? FC_INWORD : 0));
 
-	/* Probably move the cursor to then end. */
+	/* Probably move the cursor to the end. */
 	if (movetoend == 3)
 	    cs = lastend;
 
@@ -6920,7 +6923,7 @@
 static void
 do_single(Cmatch m)
 {
-    int l, sr = 0;
+    int l, sr = 0, scs;
     int havesuff = 0;
     char *str = m->str, *ppre = m->ppre, *psuf = m->psuf, *prpre = m->prpre;
 
@@ -6950,7 +6953,7 @@
     foredel(l);
 
     /* And then we insert the new string. */
-    menulen = instmatch(m);
+    menulen = instmatch(m, &scs);
     menuend = cs;
     cs = menupos + menulen;
 
@@ -6969,6 +6972,7 @@
     } else {
 	/* There is no user-specified suffix, *
 	 * so generate one automagically.     */
+	cs = scs;
 	if (m->ripre && (m->flags & CMF_PARBR)) {
 	    /*{{*/
 	    /* Completing a parameter in braces.  Add a removable `}' suffix. */
@@ -7019,6 +7023,8 @@
 		}
 	    }
 	}
+	if (!menuinsc)
+	    cs = menupos + menulen;
     }
     /* If completing in a brace expansion... */
     if (brbeg) {
@@ -7031,7 +7037,7 @@
 	} else if (!menucmp) {
 	    /*{{*/
 	    /* Otherwise, add a `,' suffix, and let `}' remove it. */
-	    cs = menuend;
+	    cs = scs;
 	    havesuff = 1;
 	    inststrlen(",", 1, 1);
 	    menuinsc++;
@@ -7043,6 +7049,7 @@
 	/* If we didn't add a suffix, add a space, unless we are *
 	 * doing menu completion or we are completing files and  *
 	 * the string doesn't name an existing file.             */
+	cs = scs;
 	inststrlen(" ", 1, 1);
 	menuinsc++;
 	if (menuwe)

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: PATCH: was Re: Closing quotes in completion

[Peter Stephenson]

Sven Wischnowsky wrote:
> [ Sorry for the late response... ]

Oh, yes, just compare it with Microsoft... as soon as anybody makes a minor
suggestion about Windows, the new program is in the shops in a couple of
hours.  Compared with that, this is distinctly sloppy.  Not!

> First, this showed a problem in the code, where the automatically
> inserted suffixes (e.g. a slash for directories) were inserted after
> the ignored suffix, which was wrong.

That seems to fix the problem I was worried about.  One difficulty is you
get a (removable) space between the end of the completion and the quote
(where the cursor ends up) when there's a unique match.  That's not so bad,
but it is it really correct deliberately to insert a space *before* a
suffix?  (There's also the old problems with completing in quotes where you
expect "filename wi<TAB> to complete to "filename with spaces", but if I
remember right that's made difficult by the lexer.)

(Or is it `an unique match'?)

> This also reminds me of the last thing I'm still thinking about for
> the new style completion stuff: a way to access and probably modify
> the completions added.
> 
> Because of these problems I haven't implemented anything of this yet,
> so I'd like to hear comments/suggestions about all this.

The only thing that strikes me is the idea of having a fairly raw access to
the structures, i.e. if you really know what you're doing there's some
option for you to modify pretty much anything.  But I don't know either the
uses for or the difficulties with that.

> P.S.: What has become of the `re-use-old-list'-completer/patch?

Sorry, I lost interest when I discovered it worked :-).  Here are the
_oldlist and _closequotes completers.  I'm sure there are obvious
extensions I've missed.  (I've put them in Core with all the other
completers, although they're rather optional.)

--- Completion/Core/_closequotes.olcq	Tue Apr 27 11:37:29 1999
+++ Completion/Core/_closequotes	Tue Apr 27 11:37:48 1999
@@ -0,0 +1,11 @@
+#autoload
+
+# If the current completion is in quotes, add the closing quote.
+# This can clash with some of the more sophisticated forms of completion
+
+if [[ -n $compstate[quote] && $RBUFFER != *${compstate[quote]}* ]]; then
+  compstate[restore]=''
+  ISUFFIX="$ISUFFIX$compstate[quote]"
+fi
+
+return 1
--- Completion/Core/_oldlist.olcq	Tue Apr 27 11:37:29 1999
+++ Completion/Core/_oldlist	Tue Apr 27 11:37:48 1999
@@ -0,0 +1,29 @@
+#autoload
+
+# If this is a listing widget and there is already an old list,
+# and either the compconfig key oldlist_list is `always', or it is not `never'
+# and the list is not already shown, then use the existing list for listing
+# (even if it was generated by another widget).
+if [[ -n $compstate[old_list] && $compconfig[oldlist_list] != never &&
+  $WIDGET = *list* &&
+  ( $compconfig[oldlist_list] = always || $compstate[old_list] != shown ) ]]
+then
+  compstate[old_list]=keep
+  return 0
+fi
+
+# If this is a completion widget, and we have a completion inserted already,
+# and the compconfig key oldlist_menu is not never, then we cycle through the
+# existing list (even if it was generated by another widget).
+if [[ -n $compstate[old_insert] && $WIDGET = *complete(|-prefix) &&
+  $compconfig[oldlist_menu] != never ]]; then
+  compstate[old_list]=keep
+  if [[ $WIDGET = *reverse* ]]; then
+    compstate[insert]=$(( compstate[old_insert] - 1 ))
+  else
+    compstate[insert]=$(( compstate[old_insert] + 1 ))
+  fi
+  return 0
+fi
+
+return 1
--- Doc/Zsh/compsys.yo.olcq	Tue Apr 27 11:16:04 1999
+++ Doc/Zsh/compsys.yo	Tue Apr 27 11:36:30 1999
@@ -528,6 +528,53 @@
 menucompletion widgets such as tt(reverse-menu-complete), or
 tt(accept-and-menu-complete).
 )
+item(tt(_oldlist))(
+This completer controls how the standard completion widgets behave when
+there is an existing list of completions which may have been generated by a
+special completion (i.e. a separately-bound completion command).  It should
+appear in the list of completers before any of the widgets which generate
+matches.  It understands two configuration keys:
+
+startitem()
+item(tt(oldlist_list))(
+If this is set to tt(always), then standard widgets which perform listing
+will retain the current list of matches, however they were generated.  If
+it is set to tt(never), this will not be done (the behaviour without the
+tt(_oldlist) completer).  If it is unset, or any other value, then the
+existing list of completions will be displayed if it is not already;
+otherwise, the standard completion list will be generated:  this is the
+default behaviour of tt(_oldlist).
+
+For example, suppose you type tt(^Xc) to use the tt(_correct_word)
+widget, which generates a list of corrections for the word under the
+cursor.  Usually, typing tt(^D) would generate a standard list of
+completions for the word on the command line, and show that.  With
+tt(_oldlist), it will instead show the list of corrections already
+generated.
+)
+item(tt(oldlist_menu))(
+Controls how menu completion behaves when a completion has already been
+inserted and the user types a standard completion key type as tt(TAB).
+The default behaviour of tt(_oldlist) is that menu completion always
+continues with the existing list of completions.  If this key is set to
+tt(never), however, a new completion is started if the old list was
+generated by a different completion command (the behaviour without the
+tt(_oldlist) completer).
+For example, suppose you type tt(^Xc) to generate a list of corrections,
+and menu completion is started in one of the usual ways.  Usually, typing
+tt(TAB) at this point would start trying to complete the line as it now
+appears.  With tt(_oldlist), it will instead continue to cycle through the
+list of completions. 
+)
+enditem()
+)
+item(tt(_closequotes))(
+When a completion starts in quotes, this completer will recognise the fact
+and insert a matching closing quote if there is not already one later on
+the line.  It should come earlier in the list of completers than any of the
+completers which generate matches.  It can be confused by some of the more
+sophisticated forms of completion.
+)
 enditem()
 
 texinode(Completion Functions)()(Control Functions)(Completion System)

-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 050 844536
WWW:  http://www.ifh.de/~pws/
Dipartimento di Fisica, Via Buonarroti 2, 56127 Pisa, Italy

Re: PATCH: was Re: Closing quotes in completion

[Sven Wischnowsky]

Peter Stephenson wrote:

> That seems to fix the problem I was worried about.  One difficulty is you
> get a (removable) space between the end of the completion and the quote
> (where the cursor ends up) when there's a unique match.  That's not so bad,
> but it is it really correct deliberately to insert a space *before* a
> suffix?

Err, right. Since the space is considered to be the automatically
inserted separator for the next word and is not a part of the
completed string, we shouldn't insert it before the ISUFFIX.

>  (There's also the old problems with completing in quotes where you
> expect "filename wi<TAB> to complete to "filename with spaces", but if I
> remember right that's made difficult by the lexer.)

Which it does. The problem is that you don't get the backslashes if
you want them even inside quotes (`zsh -c "ls f<TAB>'). And there is
no way how the completion code could detect what you want.

> > This also reminds me of the last thing I'm still thinking about for
> > the new style completion stuff: a way to access and probably modify
> > the completions added.
> > 
> > Because of these problems I haven't implemented anything of this yet,
> > so I'd like to hear comments/suggestions about all this.
> 
> The only thing that strikes me is the idea of having a fairly raw access to
> the structures, i.e. if you really know what you're doing there's some
> option for you to modify pretty much anything.  But I don't know either the
> uses for or the difficulties with that.

The only uses I can think of are:

  - Getting information about an old list or about a new list after
    all completions have been added to find out what to insert.
    Some time ago, we had some people here asking if it would be
    possible to give different compctl-flag-lists for a command and
    then have the completion code first insert only matches from the
    first list, then invoke some other widget to skip over to the next 
    list and so on. If at all I would like to do such things with
    groups of matches. In fact, this is the reason why I made
    inserting the n'th match from the m'th group so easy with the
    compstate[insert] key. For that it would be nice to be able to get 
    information at least about the different groups (name, how many
    matches, etc.).
  - compadd allows more detailed control over some of the things then
    compgen (the is-a-file-flag, the remove-characters/function). Being
    able to set these fields in matches from compgen is currently the
    only thing I can think of where modifying the generated list of
    matches might be useful. And have to do this by hand is ugly, of
    course, options for compgen to give control over this would be
    much better. But we have only one more option-letter (plus some
    non-alphabetic characters) free for comp{ctl,gen}: `h'.
    Now, if we had support for long-options...

Bye
 Sven

diff -u os/Zle/zle_tricky.c Src/Zle/zle_tricky.c
--- os/Zle/zle_tricky.c	Tue Apr 27 11:03:35 1999
+++ Src/Zle/zle_tricky.c	Tue Apr 27 12:45:30 1999
@@ -7049,7 +7049,6 @@
 	/* If we didn't add a suffix, add a space, unless we are *
 	 * doing menu completion or we are completing files and  *
 	 * the string doesn't name an existing file.             */
-	cs = scs;
 	inststrlen(" ", 1, 1);
 	menuinsc++;
 	if (menuwe)

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de