Order of eshell/pcomplete completions when cycling

Order of eshell/pcomplete completions when cycling

[Tassilo Horn]

Hi all,

assuming I have the directories test, test1, test2, and test11, when I
do `cd tes<TAB>' in eshell, it'll cycle the possible completions in the
order test2/, test1/, test/, test11/.

In a typical shell like ZSH or BASH, the order would be test/, test1/,
test11/, test2/, i.e., the completions come in lexicographic order.

Now I've found `pcomplete-compare-entry-function' which is
`string-lessp' by default and suggests alphabetic sorting.  That
variable is set buffer-locally to `file-newer-than-file-p' in eshell
buffers, so it seems there sorting according to creation (or
modification?) time is intended which is a very weird default value,
IMHO.

Anyway, I've created the directories in the order stated above so it
doesn't conform with the actual cycling order I get when cycling thru
the completions.

Now I've set `eshell-cmpl-compare-entry-function' to `string-lessp'.
When I fire up a new eshell buffer, `pcomplete-compare-entry-function'
is initialized to `string-lessp' in there.  But the completion order
stays unaffected.  It's still test2/, test1/, test/, test11/.

So `pcomplete-compare-entry-function' and
`eshell-cmpl-compare-entry-function' are probably the wrong knobs.
Unfortunately, I don't find another option which seems relevant.

Bye,
Tassilo

Re: Order of eshell/pcomplete completions when cycling

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Date: Thu, 09 Apr 2015 17:20:48 +0200
> 
> assuming I have the directories test, test1, test2, and test11, when I
> do `cd tes<TAB>' in eshell, it'll cycle the possible completions in the
> order test2/, test1/, test/, test11/.
> 
> In a typical shell like ZSH or BASH, the order would be test/, test1/,
> test11/, test2/, i.e., the completions come in lexicographic order.
> 
> Now I've found `pcomplete-compare-entry-function' which is
> `string-lessp' by default and suggests alphabetic sorting.  That
> variable is set buffer-locally to `file-newer-than-file-p' in eshell
> buffers, so it seems there sorting according to creation (or
> modification?) time is intended which is a very weird default value,
> IMHO.

I think you want string-collate-lessp, not string-lessp.  AFAIK,
that's what the shells use.

Re: Order of eshell/pcomplete completions when cycling

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> assuming I have the directories test, test1, test2, and test11, when
>> I do `cd tes<TAB>' in eshell, it'll cycle the possible completions in
>> the order test2/, test1/, test/, test11/.
>> 
>> In a typical shell like ZSH or BASH, the order would be test/,
>> test1/, test11/, test2/, i.e., the completions come in lexicographic
>> order.
>> 
>> Now I've found `pcomplete-compare-entry-function' which is
>> `string-lessp' by default and suggests alphabetic sorting.  That
>> variable is set buffer-locally to `file-newer-than-file-p' in eshell
>> buffers, so it seems there sorting according to creation (or
>> modification?) time is intended which is a very weird default value,
>> IMHO.
>
> I think you want string-collate-lessp, not string-lessp.  AFAIK,
> that's what the shells use.

Yes, you are right.  Unfortunately, it seems the function used for
`eshell-cmpl-compare-entry-function' (which is the default value for
`pcomplete-compare-entry-function' in eshell buffers) has no effect on
the order in which completions are cycled when hitting TAB repeatedly.

Bye,
Tassilo

Re: Order of eshell/pcomplete completions when cycling

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: help-gnu-emacs@gnu.org
> Date: Fri, 10 Apr 2015 13:08:01 +0200
> 
> > I think you want string-collate-lessp, not string-lessp.  AFAIK,
> > that's what the shells use.
> 
> Yes, you are right.  Unfortunately, it seems the function used for
> `eshell-cmpl-compare-entry-function' (which is the default value for
> `pcomplete-compare-entry-function' in eshell buffers) has no effect on
> the order in which completions are cycled when hitting TAB repeatedly.

Well, then maybe they should, at least as an option?

Re: Order of eshell/pcomplete completions when cycling

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

>> > I think you want string-collate-lessp, not string-lessp.  AFAIK,
>> > that's what the shells use.
>> 
>> Yes, you are right.  Unfortunately, it seems the function used for
>> `eshell-cmpl-compare-entry-function' (which is the default value for
>> `pcomplete-compare-entry-function' in eshell buffers) has no effect on
>> the order in which completions are cycled when hitting TAB repeatedly.
>
> Well, then maybe they should, at least as an option?

Yes, of course they should. :-)

I have no deep understanding of the emacs completion stuff.  But in
pcomplete.el there is

--8<---------------cut here---------------start------------->8---
(defun pcomplete--entries (&optional regexp predicate)
  "Like `pcomplete-entries' but without env-var handling."
  (let* ((ign-pred
[...snip...]
    (lambda (s p a)
      (if (and (eq a 'metadata) pcomplete-compare-entry-function)
          `(metadata (cycle-sort-function
                      . ,(lambda (comps)
                           (sort comps pcomplete-compare-entry-function)))
                     ,@(cdr (completion-file-name-table s p a)))
        (let ((completion-ignored-extensions nil)
              (completion-ignore-case pcomplete-ignore-case))
          (completion-table-with-predicate
           #'comint-completion-file-name-table pred 'strict s p a))))))
--8<---------------cut here---------------end--------------->8---

where the lambda returns a cycle-sort-function using
`pcomplete-compare-entry-function' if asked for completion metadata.
The docs say

,----[ (info "(elisp)Programmed Completion") ]
| ‘cycle-sort-function’
|      The value should be a function for sorting completions, when
|      ‘completion-cycle-threshold’ is non-‘nil’ and the user is cycling
|      through completion alternatives.  *Note (emacs)Completion
|      Options::.  Its argument list and return value are the same as for
|      ‘display-sort-function’.
`----

so that seems to be what I am looking for and eshell/pcomplete
initialize it correctly.

The returned lambda is only called via

   (complete-with-action action table newstring pred))

where table is that lambda.

I've verified that the lambda is never called with the a(ction) argument
bound to 'metadata, so the cycle-sort-function which would sort using
`pcomplete-compare-entry-function' is never asked for.

Grepping the sources, the only user of cycle-sort-function is
`completion-all-sorted-completions' which is never called by eshell or
pcomplete.  So the `pcomplete-compare-entry-function' will never every
be called.

I suspect that there has been a time where that used to work, and in the
meantime the completion stuff has changed and forgotten that some
completion users want sorted completions also without calling
`completion-all-sorted-completions'.

Bye,
Tassilo

Re: Order of eshell/pcomplete completions when cycling

[Stefan Monnier]

> I suspect that there has been a time where that used to work, and in the
> meantime the completion stuff has changed and forgotten that some
> completion users want sorted completions also without calling
> `completion-all-sorted-completions'.

Indeed, it seems that the pcomplete completion UI code was somehow
broken when its innards were separated to provide
pcomplete-completion-at-point.

The right fix moving forward (which I've been meaning to do but haven't
found time for yet) is to change Eshell to not use the `pcomplete'
command any more, but to use `completion-at-point' instead and to
finally obsolete the `pcomplete' command.


        Stefan

Re: Order of eshell/pcomplete completions when cycling

[Tassilo Horn]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I suspect that there has been a time where that used to work, and in
>> the meantime the completion stuff has changed and forgotten that some
>> completion users want sorted completions also without calling
>> `completion-all-sorted-completions'.
>
> Indeed, it seems that the pcomplete completion UI code was somehow
> broken when its innards were separated to provide
> pcomplete-completion-at-point.
>
> The right fix moving forward (which I've been meaning to do but
> haven't found time for yet) is to change Eshell to not use the
> `pcomplete' command any more, but to use `completion-at-point' instead
> and to finally obsolete the `pcomplete' command.

I've just tried that:

--8<---------------cut here---------------start------------->8---
diff -u --label /home/horn/Repos/el/emacs/lisp/eshell/em-cmpl.el --label \#\<buffer\ em-cmpl.el\> /home/horn/Repos/el/emacs/lisp/eshell/em-cmpl.el /tmp/buffer-content-2696WcN
--- /home/horn/Repos/el/emacs/lisp/eshell/em-cmpl.el
+++ #<buffer em-cmpl.el>
@@ -463,11 +463,7 @@
   (interactive "p")
   ;; Pretend to be pcomplete so that cycling works (bug#13293).
   (setq this-command 'pcomplete)
-  (condition-case nil
-      (if interactively
-	  (call-interactively 'pcomplete)
-	(pcomplete))
-    (text-read-only (completion-at-point)))) ; Workaround for bug#12838.
+  (completion-at-point))
 
 (provide 'em-cmpl)
--8<---------------cut here---------------end--------------->8---

As far as I can tell, I don't see any difference to before, i.e., all
pcomplete/<command> functions are still considered, and also the order
when cycling completions is now test/, test1/, test11/, test2/ so my
value of `eshell-cmpl-compare-entry-function' seems to take effect.

It seems I can also remove the this-command workaround for bug#13293
without any negative effect.  (But I didn't test too hard.)

Bye,
Tassilo

Re: Order of eshell/pcomplete completions when cycling

[Stefan Monnier]

> As far as I can tell, I don't see any difference to before, i.e., all
> pcomplete/<command> functions are still considered, and also the order
> when cycling completions is now test/, test1/, test11/, test2/ so my
> value of `eshell-cmpl-compare-entry-function' seems to take effect.

Yes, it still uses the pcomplete machinery, but the UI is implemented by
the generic completion code.

> It seems I can also remove the this-command workaround for bug#13293
> without any negative effect.  (But I didn't test too hard.)

Yes, there's some clean that could take place.
But first we have to address things like the M-? and backtab bindings
which currently are bound to pcomplete-list and pcomplete-reverse (or
something like that) and would need to be changed to a corresponding
command in minibuffer.el.

If someone's interested, here's a kind of "todo" list in this regard, in
the form of a patch with FIXMEs.


        Stefan


diff --git a/lisp/pcomplete.el b/lisp/pcomplete.el
index 7bcf2c1..7ec73aa 100644
--- a/lisp/pcomplete.el
+++ b/lisp/pcomplete.el
@@ -477,6 +477,7 @@ Same as `pcomplete' but using the standard completion UI."
   "Support extensible programmable completion.
 To use this function, just bind the TAB key to it, or add it to your
 completion functions list (it should occur fairly early in the list)."
+  (declare (obsolete completion-at-point "25.1"))
   (interactive "p")
   (if (and interactively
 	   pcomplete-cycle-completions
@@ -519,6 +520,7 @@ completion functions list (it should occur fairly early in the list)."
 ;;;###autoload
 (defun pcomplete-reverse ()
   "If cycling completion is in use, cycle backwards."
+  (declare (obsolete ?? "25.1"))        ;FIXME!
   (interactive)
   (call-interactively 'pcomplete))
 
@@ -527,12 +529,15 @@ completion functions list (it should occur fairly early in the list)."
   "Expand the textual value of the current argument.
 This will modify the current buffer."
   (interactive)
+  ;; FIXME: Either make obsolete or make it use completion-at-point.
   (let ((pcomplete-expand-before-complete t))
     (pcomplete)))
 
 ;;;###autoload
 (defun pcomplete-continue ()
   "Complete without reference to any cycling completions."
+  ;; It doesn't seem to be used, so it's OK if we don't have a substitute.
+  (declare (obsolete nil "25.1"))
   (interactive)
   (setq pcomplete-current-completions nil
 	pcomplete-last-completion-raw nil)
@@ -560,6 +565,7 @@ This will modify the current buffer."
 ;;;###autoload
 (defun pcomplete-help ()
   "Display any help information relative to the current argument."
+  (declare (obsolete ?? "25.1"))        ;FIXME!
   (interactive)
   (let ((pcomplete-show-help t))
     (pcomplete)))
@@ -567,6 +573,7 @@ This will modify the current buffer."
 ;;;###autoload
 (defun pcomplete-list ()
   "Show the list of possible completions for the current argument."
+  (declare (obsolete completion-help-at-point "25.1"))
   (interactive)
   (when (and pcomplete-cycle-completions
 	     pcomplete-current-completions