bug#34070: 27.0.50; icomplete-mode candidate cycling broken for C-x C-f

bug#34070: 27.0.50; icomplete-mode candidate cycling broken for C-x C-f

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1268 bytes --]

Hi maintainers,

   Emacs -Q
   M-x icomplete-mode
   C-x C-f
   C-.

Expected candidate list to rotate once to the left, immediately.
Instead if only rotates if I press C-. two additional times.  This is
because "."  and ".." are still taking part in the rotation under the
hood (but the user doesn't see them, by default, because of
completion-ignored-extensions).

Seems to have been introduced by

   commit 65797b1d75e9f608ffd50fd88be47a854b143bb1
   Author: Drew Adams <drew.adams@oracle.com>
   Date:   Thu Apr 28 19:31:43 2016 +0200
    
       Make icomplete respect `completion-ignored-extensions'
       
       * lisp/icomplete.el (icomplete-completions): Heed
       `completion-ignored-extensions' (bug#12939).

Naive patch attached that seems to fix it for me.

Here's another problem that might be related (should I open a new bug?)

   When using substring completion and navigating deeper in directories
   using successive C-M-i's (sometimes the directories don't make sense)
    
   Try it with:
    
     Emacs -Q
     M-: (add-to-list 'completion-styles 'substring)
     M-x icomplete-mode
     C-x C-f p a t h / t o / e m a c s /
     s r c C-M-i
    
   Expected to see the same as if I had typed "s r c /", instead I see
   "{src | lib-src}"


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-icomplete-s-cycling-when-filename-filtering-kick.patch --]
[-- Type: text/x-patch, Size: 3046 bytes --]

From 7ec9a12f2e22b846c2635a045a8c1a13b4573eba Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Mon, 14 Jan 2019 12:46:34 +0000
Subject: [PATCH] Fix icomplete's cycling when filename filtering kicks in

To reproduce:

   Emacs -Q
   M-x icomplete-mode
   C-x C-f
   C-.

Expected candidate list to rotate once to the left.  Instead if only
rotates if I press C-. twice more.  This is because "." and ".." are
still taking part in the rotation under the hood (but one doesn't see
them by default because of completion-ignored-extensions)

* lisp/icomplete.el (icomplete--filtered-completions): New variable.
(icomplete-forward-completions, icomplete-backward-completions):
Use it.
(icomplete-completions): Set it.
---
 lisp/icomplete.el | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index 8bed46cb3b..82e2728487 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -162,6 +162,9 @@ icomplete-force-complete-and-exit
       (minibuffer-force-complete-and-exit)
     (minibuffer-complete-and-exit)))
 
+(defvar icomplete--filtered-completions nil
+  "If non-nil completions as filtered by `icomplete-completions'")
+
 (defun icomplete-forward-completions ()
   "Step forward completions by one entry.
 Second entry becomes the first and can be selected with
@@ -169,7 +172,8 @@ icomplete-forward-completions
   (interactive)
   (let* ((beg (icomplete--field-beg))
          (end (icomplete--field-end))
-         (comps (completion-all-sorted-completions beg end))
+         (comps (or icomplete--filtered-completions
+                    (completion-all-sorted-completions beg end)))
 	 (last (last comps)))
     (when comps
       (setcdr last (cons (car comps) (cdr last)))
@@ -182,7 +186,8 @@ icomplete-backward-completions
   (interactive)
   (let* ((beg (icomplete--field-beg))
          (end (icomplete--field-end))
-         (comps (completion-all-sorted-completions beg end))
+         (comps (or icomplete--filtered-completions
+                    (completion-all-sorted-completions beg end)))
 	 (last-but-one (last comps 2))
 	 (last (cdr last-but-one)))
     (when (consp last)		      ; At least two elements in comps
@@ -382,9 +387,11 @@ icomplete-completions
 	(progn ;;(debug (format "Candidates=%S field=%S" candidates name))
 	       (format " %sNo matches%s" open-bracket close-bracket))
       (if last (setcdr last nil))
-      (when (and minibuffer-completing-file-name
-                 icomplete-with-completion-tables)
-        (setq comps (completion-pcm--filename-try-filter comps)))
+      (if (and minibuffer-completing-file-name
+               icomplete-with-completion-tables)
+          (setq comps (completion-pcm--filename-try-filter comps)
+                icomplete--filtered-completions comps)
+        (setq icomplete--filtered-completions nil))
       (let* ((most-try
               (if (and base-size (> base-size 0))
                   (completion-try-completion
-- 
2.19.2

bug#34070: 27.0.50; icomplete-mode candidate cycling broken for C-x C-f

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Mon, 14 Jan 2019 13:47:48 +0000
> Cc: monnier@iro.umontreal.ca
> 
>    Emacs -Q
>    M-x icomplete-mode
>    C-x C-f
>    C-.
> 
> Expected candidate list to rotate once to the left, immediately.
> Instead if only rotates if I press C-. two additional times.  This is
> because "."  and ".." are still taking part in the rotation under the
> hood (but the user doesn't see them, by default, because of
> completion-ignored-extensions).
> 
> Seems to have been introduced by
> 
>    commit 65797b1d75e9f608ffd50fd88be47a854b143bb1
>    Author: Drew Adams <drew.adams@oracle.com>
>    Date:   Thu Apr 28 19:31:43 2016 +0200
>     
>        Make icomplete respect `completion-ignored-extensions'
>        
>        * lisp/icomplete.el (icomplete-completions): Heed
>        `completion-ignored-extensions' (bug#12939).
> 
> Naive patch attached that seems to fix it for me.

Please add a test for this, if possible, so that we don't screw this
up again in the future.

> Here's another problem that might be related (should I open a new bug?)

I think so, yes.

bug#34070: 27.0.50; icomplete-mode candidate cycling broken for C-x C-f

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> Seems to have been introduced by
>> 
>>    commit 65797b1d75e9f608ffd50fd88be47a854b143bb1
>>    Author: Drew Adams <drew.adams@oracle.com>
>>    Date:   Thu Apr 28 19:31:43 2016 +0200
>>     
>>        Make icomplete respect `completion-ignored-extensions'
>>        
>>        * lisp/icomplete.el (icomplete-completions): Heed
>>        `completion-ignored-extensions' (bug#12939).
>> 
>> Naive patch attached that seems to fix it for me.
>
> Please add a test for this, if possible, so that we don't screw this
> up again in the future.

I'd love to, but it's not exactly simple to write tests that observe
user interaction the minibuffer with ert.  At least for me.  How would I
go about doing that?  Any pointers/prior art?

João

bug#34070: 27.0.50; icomplete-mode candidate cycling broken for C-x C-f

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: Drew Adams <drew.adams@oracle.com>,  34070@debbugs.gnu.org,  monnier@iro.umontreal.ca
> Date: Mon, 14 Jan 2019 16:25:41 +0000
> 
> > Please add a test for this, if possible, so that we don't screw this
> > up again in the future.
> 
> I'd love to, but it's not exactly simple to write tests that observe
> user interaction the minibuffer with ert.  At least for me.  How would I
> go about doing that?  Any pointers/prior art?

(A stab in the dark) invoke the function bound to TAB and examine what
it produces?

bug#34070: 27.0.50; icomplete-mode candidate cycling broken for C-x C-f

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Cc: Drew Adams <drew.adams@oracle.com>,  34070@debbugs.gnu.org,  monnier@iro.umontreal.ca
>> Date: Mon, 14 Jan 2019 16:25:41 +0000
>> 
>> > Please add a test for this, if possible, so that we don't screw this
>> > up again in the future.
>> 
>> I'd love to, but it's not exactly simple to write tests that observe
>> user interaction the minibuffer with ert.  At least for me.  How would I
>> go about doing that?  Any pointers/prior art?
>
> (A stab in the dark) invoke the function bound to TAB and examine what
> it produces?

:-)

Where do you mean TAB?  It's not even in the recipe.  Well I though
about it a bit and can probalby use minibuffer-setup-hook

(let ((minibuffer-setup-hook
       (append minibuffer-setup-hook
               (list (lambda ()
                       ;; commands and observations
                       ))))
      (default-directory source-directory))
  (find-file-read-args "Find file: " (confirm-nonexistent-file-or-buffer)))

Don't know how to observe the icomplete candidates though, but this
strategy is probably enough to make a test.  I'd like to push the fix
before that maybe, any objections?

João

bug#34070: 27.0.50; icomplete-mode candidate cycling broken for C-x C-f

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: drew.adams@oracle.com,  34070@debbugs.gnu.org,  monnier@iro.umontreal.ca
> Date: Mon, 14 Jan 2019 17:11:58 +0000
> 
> I'd like to push the fix before that maybe, any objections?

Please wait for Drew to respond, before you do.

bug#34070: 27.0.50; icomplete-mode candidate cycling broken for C-x C-f

[João Távora]

On Mon, Jan 14, 2019 at 5:40 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: João Távora <joaotavora@gmail.com>
> > Cc: drew.adams@oracle.com,  34070@debbugs.gnu.org,  monnier@iro.umontreal.ca
> > Date: Mon, 14 Jan 2019 17:11:58 +0000
> >
> > I'd like to push the fix before that maybe, any objections?
>
> Please wait for Drew to respond, before you do.

Fair enough.

João

bug#34070: 27.0.50; icomplete-mode candidate cycling broken for C-x C-f

[Drew Adams]

> > I'd like to push the fix before that maybe, any objections?
> 
> Please wait for Drew to respond, before you do.

I took a look at the bug description, reproduced it, and took a look at the proposed patch.  It looks OK to me.  Thx.

bug#34070: 27.0.50; icomplete-mode candidate cycling broken for C-x C-f

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]

tag 34070 patch

Drew Adams <drew.adams@oracle.com> writes:

>> > I'd like to push the fix before that maybe, any objections?
>> 
>> Please wait for Drew to respond, before you do.
>
> I took a look at the bug description, reproduced it, and took a look
> at the proposed patch.  It looks OK to me.  Thx.

I've already pushed the proposed patch to master, but there's a much
less intrusive way using a new patch attached after my sig.

Significantly, while still honouring the original intention of Drew's
change:

   65797b1d7 "Make icomplete respect `completion-ignored-extensions'"

the new patch does two things:

   1. Still fixes the candidate cycling (i.e. this bug)
   
   2. Leaves the current directory as a candidate, i.e. "./" is *not*
      filtered from the prospects list (but "../" is).

Number 2 can be seen as "new" behaviour, but then Drew's patch also
silently introduced new behaviour by filtering out "./" and "../", which
are *not* in completion-ignored-extensions.  Reading bug#12939
(https://debbugs.gnu.org/12939) this seems to have gone unnoticed.

If someone thinks this is a problem we can make this configurable
(though I think the default should be what I suggest, since it makes C-x
C-f'ing directories much easier).

João


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-Simplify-ignored-extensions-filtering-in-Icomplete-b.patch --]
[-- Type: text/x-patch, Size: 4839 bytes --]

From 99c712809fca46648a02451c4eaa8196e915207b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Tue, 15 Jan 2019 12:10:23 +0000
Subject: [PATCH 2/5] Simplify ignored extensions filtering in Icomplete
 (bug#34070)

* lisp/icomplete.el: Use lexical binding.
(icomplete--filtered-completions): Remove.
(icomplete-forward-completions, icomplete-backward-completions):
Revert last change.
(icomplete-completions): Use minibuffer-completion-predicate
to filter out completion-ignored-extensions.
---
 lisp/icomplete.el | 45 ++++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index 82e2728487..6d77c0649a 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -1,4 +1,4 @@
-;;; icomplete.el --- minibuffer completion incremental feedback
+;;; icomplete.el --- minibuffer completion incremental feedback -*- lexical-binding: t -*-
 
 ;; Copyright (C) 1992-1994, 1997, 1999, 2001-2019 Free Software
 ;; Foundation, Inc.
@@ -162,9 +162,6 @@ icomplete-force-complete-and-exit
       (minibuffer-force-complete-and-exit)
     (minibuffer-complete-and-exit)))
 
-(defvar icomplete--filtered-completions nil
-  "If non-nil completions as filtered by `icomplete-completions'")
-
 (defun icomplete-forward-completions ()
   "Step forward completions by one entry.
 Second entry becomes the first and can be selected with
@@ -172,8 +169,7 @@ icomplete-forward-completions
   (interactive)
   (let* ((beg (icomplete--field-beg))
          (end (icomplete--field-end))
-         (comps (or icomplete--filtered-completions
-                    (completion-all-sorted-completions beg end)))
+         (comps (completion-all-sorted-completions beg end))
 	 (last (last comps)))
     (when comps
       (setcdr last (cons (car comps) (cdr last)))
@@ -186,8 +182,7 @@ icomplete-backward-completions
   (interactive)
   (let* ((beg (icomplete--field-beg))
          (end (icomplete--field-end))
-         (comps (or icomplete--filtered-completions
-                    (completion-all-sorted-completions beg end)))
+         (comps (completion-all-sorted-completions beg end))
 	 (last-but-one (last comps 2))
 	 (last (cdr last-but-one)))
     (when (consp last)		      ; At least two elements in comps
@@ -373,8 +368,21 @@ icomplete-completions
 The displays for unambiguous matches have ` [Matched]' appended
 \(whether complete or not), or ` [No matches]', if no eligible
 matches exist."
-  (let* ((minibuffer-completion-table candidates)
-	 (minibuffer-completion-predicate predicate)
+  (let* ((ignored-extension-re
+          (and minibuffer-completing-file-name
+               icomplete-with-completion-tables
+               completion-ignored-extensions
+               (concat "\\(?:\\`\\.\\./\\|"
+                       (regexp-opt completion-ignored-extensions)
+                       "\\)\\'")))
+         (minibuffer-completion-table candidates)
+	 (minibuffer-completion-predicate
+          (if ignored-extension-re
+              (lambda (cand)
+                (and (not (string-match ignored-extension-re cand))
+                     (or (null predicate)
+                         (funcall predicate cand))))
+            predicate))
 	 (md (completion--field-metadata (icomplete--field-beg)))
 	 (comps (completion-all-sorted-completions
                  (icomplete--field-beg) (icomplete--field-end)))
@@ -385,13 +393,8 @@ icomplete-completions
     ;; `concat'/`mapconcat' is the slow part.
     (if (not (consp comps))
 	(progn ;;(debug (format "Candidates=%S field=%S" candidates name))
-	       (format " %sNo matches%s" open-bracket close-bracket))
+	  (format " %sNo matches%s" open-bracket close-bracket))
       (if last (setcdr last nil))
-      (if (and minibuffer-completing-file-name
-               icomplete-with-completion-tables)
-          (setq comps (completion-pcm--filename-try-filter comps)
-                icomplete--filtered-completions comps)
-        (setq icomplete--filtered-completions nil))
       (let* ((most-try
               (if (and base-size (> base-size 0))
                   (completion-try-completion
@@ -477,11 +480,11 @@ icomplete-completions
 		  (if prefix-len (substring (car comps) prefix-len) (car comps))
 		  comps (cdr comps))
 	    (setq prospects-len
-                           (+ (string-width comp)
-			      (string-width icomplete-separator)
-			      prospects-len))
-		     (if (< prospects-len prospects-max)
-			 (push comp prospects)
+                  (+ (string-width comp)
+		     (string-width icomplete-separator)
+		     prospects-len))
+	    (if (< prospects-len prospects-max)
+		(push comp prospects)
 	      (setq limit t))))
 	(setq prospects (nreverse prospects))
 	;; Decorate first of the prospects.
-- 
2.19.2

bug#34070: 27.0.50; icomplete-mode candidate cycling broken for C-x C-f

[Stefan Monnier]

> I've already pushed the proposed patch to master, but there's a much
> less intrusive way using a new patch attached after my sig.

Sounds good.  If you install it, could you install it as 2 separate
patches: one that undoes the previous one and another that performs the
new changes?


        Stefan

bug#34070: 27.0.50; icomplete-mode candidate cycling broken for C-x C-f

[João Távora]

On Thu, Jan 17, 2019 at 3:00 PM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > I've already pushed the proposed patch to master, but there's a much
> > less intrusive way using a new patch attached after my sig.
>
> Sounds good.  If you install it, could you install it as 2 separate
> patches: one that undoes the previous one and another that performs the
> new changes?

Done.

commit 5a6df06494f9ba6df53af82cfdf81f1d3708edc3
Author: João Távora
Date:   Tue Jan 15 12:10:23 2019 +0000

    Simplify ignored extensions filtering in Icomplete (bug#34070)

    * lisp/icomplete.el: Use lexical binding.
    (icomplete-completions): Use minibuffer-completion-predicate
    to filter out completion-ignored-extensions.

commit 7560ef7de925b56f367df168befc9b748b6237c1
Author: João Távora
Date:   Thu Jan 17 15:11:21 2019 +0000

    Revert "Fix icomplete's cycling when filename filtering kicks in"

    This reverts commit cdb082322d4209c5104bc1a98b21bf3dd75e8f17,
which
    was a fix for bug#34070.  A much better fix to be added soon.

bug#34070: 27.0.50; icomplete-mode candidate cycling broken for C-x C-f

[Drew Adams]

> while still honouring the original intention of Drew's change:
> 65797b1d7 "Make icomplete respect `completion-ignored-extensions'"
> the new patch does two things:
>    1. Still fixes the candidate cycling (i.e. this bug)
>    2. Leaves the current directory as a candidate, i.e. "./" is *not*
>       filtered from the prospects list (but "../" is).
> 
> Number 2 can be seen as "new" behaviour, but then Drew's patch also
> silently introduced new behaviour by filtering out "./" and "../",
> which are *not* in completion-ignored-extensions.  Reading bug
> #12939... this seems to have gone unnoticed.
> 
> If someone thinks this is a problem we can make this configurable
> (though I think the default should be what I suggest, since it
> makes C-x C-f'ing directories much easier).

I can't speak to whether `.' or `..' should (always?
sometimes?) be filtered out for Icomplete completion of
file-name candidates.

I think you're right that the intention of my patch was
just to respect `completion-ignored-extensions'.  That was
the "new" behavior to be introduced, and not silently.

But why is it that `completion-pcm--filename-try-filter'
adds `.' and `..' to its filter, so they too are excluded?
I guess it's because they are not candidates returned by
`try'?

Assuming there's a good reason why `c-p--f-t-f' does that,
and if that's not appropriate for Icomplete in all or most
cases, then I guess `c-p--f-t-f' wasn't a perfect match for
Icomplete. ;-)

But maybe someone should take a look at `c-p--f-t-f' more
generally?  If `.' or `..' is appropriate for file-name
completions sometimes (e.g. in Icomplete), then do some of
the current uses of `c-p--f-t-f' also manifest the same
bug that you are adding here?

E.g., is removal of `.' and `..' always appropriate for
`completion-basic-try-completion',
`completion-pcm-try-completion' and
`completion-substring-try-completion'?

(Note too that you are adding to this bug, which was
purportedly about broken cycling. Is this additional
change necessary to fix the cycling problem, or should
it be the subject of a new bug report?)

BTW, see also bug #13322, companion to bug #12939.
It should never have been closed, IMHO.

BTW2, as stated in bug #12939,
`completion-pcm--filename-try-filter' is the wrong name.
It should not use `--'.  It's not "internal" in any way,
or at least it should not be considered so.  Internal to
what?  It's not internal to `completion-pcm' (whatever
that might be/mean now) or to `minibuffer.el'.

It's as general and useful as any other Lisp function -
no reason to try to signal to users that it is (also)
used to build some basic Emacs behavior.  Users can
well make use of such a function.  Misuse of the label
"internal" is akin to Trump trying to "build that wall".

bug#34070: 27.0.50; icomplete-mode candidate cycling broken for C-x C-f

[Stefan Monnier]

> Assuming there's a good reason why `c-p--f-t-f' does that,
> and if that's not appropriate for Icomplete in all or most
> cases, then I guess `c-p--f-t-f' wasn't a perfect match for
> Icomplete. ;-)

I think stripping . and .. was fine, and that not stripping them is also
acceptable (or only stripping one of the two): it doesn't matter that
much.

it really matters for c-p--f-t-f, tho, because otherwise TAB in a dir
with a single file won't directly complete to that file's name.


        Stefan


> well make use of such a function.  Misuse of the label
> "internal" is akin to Trump trying to "build that wall".

Should Trump also count for Godwin's law?

bug#34070: 27.0.50; icomplete-mode candidate cycling broken for C-x C-f

[João Távora]

On Thu, Jan 17, 2019 at 3:46 PM Drew Adams <drew.adams@oracle.com> wrote:

> I think you're right that the intention of my patch was
> just to respect `completion-ignored-extensions'.  That was
> the "new" behavior to be introduced, and not silently.

Right. That was the "declared" new behaviour (for your definition
of "new").  But there was some "undeclared", undiscussed
new behaviour: that was my point.

> (Note too that you are adding to this bug, which was
> purportedly about broken cycling. Is this additional
> change necessary to fix the cycling problem, or should
> it be the subject of a new bug report?)

Right. I put emphasis on this myself, so I don't have to
"note" it.  If you believe it should be the subject of a new
bug, go ahead and create it.  I don't think it's worth it.

--
João Távora