bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Sean Whitton]

Hello,

The recent change to fix #19032 has broken my fido-mode workflow in a
way which I think reveals a bug.

When starting from an empty minibuffer, icomplete-fido-exit now selects
a different match depending on whether the completions have appeared
yet.  So if you hit keys fast enough then you get different behaviour
than if you hit them a bit slower.

For example:

1. emacs -q
2. M-x fido-mode RET
3. C-x b foo M-j (type slowly)
4. C-x b bar M-j (type slowly)
5. C-x b M-j (type quickly)

This will take you to *GNU Emacs* but it should take you to foo (the
minibuffer prompt is "Switch to buffer (default foo)").  If you wait for
the completions to appear before hitting M-j, you go to the right place.

I think the fix would be to clear completion-content-when-empty when the
minibuffer exits, instead of leaving data from the last completion
there.  Or possibly M-j should call icomplete-completions to popular
completion-content-when-empty with the correct information.

-- 
Sean Whitton

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[João Távora]

Sean Whitton <spwhitton@spwhitton.name> writes:

> For example:
>
> 1. emacs -q
> 2. M-x fido-mode RET
> 3. C-x b foo M-j (type slowly)
> 4. C-x b bar M-j (type slowly)
> 5. C-x b M-j (type quickly)

This sounds like a bug, indeed.  I dealt with this kind of problem in
the past and worked hard to mitigate it.  I was unware of this recent
patch to icomplete/fido-mode, sorry.

I will note that the problem doesn't seem to happen if step 5 ends with
RET instead of M-j.  M-j in the fido-mode minibuffer is bound to
icomplete-fido-exit which does have have an optional FORCE arg, which
could maybe justify the behaviour you're observing.  But anyway it's nil
by default, so it doesn't.

> I think the fix would be to clear completion-content-when-empty when the
> minibuffer exits, instead of leaving data from the last completion
> there.  Or possibly M-j should call icomplete-completions to popular
> completion-content-when-empty with the correct information.

I don't understand the reason of being for the patch.  I didn't read the
bug, but I do read this in NEWS:

    +*** 'icomplete-show-matches-on-no-input' behavior change
    +Previously, choosing a different completion with commands like 'C-.'
    +and then hitting enter would choose the default completion.  Doung
    +this will now choose the completion under point.

Now, I have never observed this reported behaviour in fido-mode even
before the patch.

João

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Sean Whitton]

Hello João,

On Mon 31 Aug 2020 at 12:50AM +01, João Távora wrote:

> Sean Whitton <spwhitton@spwhitton.name> writes:
>
>> For example:
>>
>> 1. emacs -q
>> 2. M-x fido-mode RET
>> 3. C-x b foo M-j (type slowly)
>> 4. C-x b bar M-j (type slowly)
>> 5. C-x b M-j (type quickly)
>
> This sounds like a bug, indeed.  I dealt with this kind of problem in
> the past and worked hard to mitigate it.  I was unware of this recent
> patch to icomplete/fido-mode, sorry.
>
> I will note that the problem doesn't seem to happen if step 5 ends with
> RET instead of M-j.  M-j in the fido-mode minibuffer is bound to
> icomplete-fido-exit which does have have an optional FORCE arg, which
> could maybe justify the behaviour you're observing.  But anyway it's nil
> by default, so it doesn't.

Right.  I actually have icomplete-fido-exit bound to RET because that
means that RET in any minibuffer always exits with the current input,
which I find makes completion less obstrusive.  But this bug is breaking
that atm.

-- 
Sean Whitton

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Lars Ingebrigtsen]

João Távora <joaotavora@gmail.com> writes:

> I don't understand the reason of being for the patch.  I didn't read the
> bug, but I do read this in NEWS:
>
>     +*** 'icomplete-show-matches-on-no-input' behavior change
>     +Previously, choosing a different completion with commands like 'C-.'
>     +and then hitting enter would choose the default completion.  Doung
>     +this will now choose the completion under point.
>
> Now, I have never observed this reported behaviour in fido-mode even
> before the patch.

It was reported as a problem in icomplete, but I don't know how fido
relates to icomplete.  

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[João Távora]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> João Távora <joaotavora@gmail.com> writes:
>
>> I don't understand the reason of being for the patch.  I didn't read the
>> bug, but I do read this in NEWS:
>>
>>     +*** 'icomplete-show-matches-on-no-input' behavior change
>>     +Previously, choosing a different completion with commands like 'C-.'
>>     +and then hitting enter would choose the default completion.  Doung
>>     +this will now choose the completion under point.
>>
>> Now, I have never observed this reported behaviour in fido-mode even
>> before the patch.
>
> It was reported as a problem in icomplete, but I don't know how fido
> relates to icomplete.

This makes sense then.  fido-mode is basically icomplete-mode with
different defaults and slightly different bindings, resulting in a
visual experience that looks vaguely like Ido (Fido = "fake Ido").

However, the description of this icomplete problem sounds like something
where fido-mode and icomplete-mode should exhibit exactly the same
behaviour, i.e. work along the very same code.  So I'm baffled as to why
that problem affected icomplete and not fido-mode.  Almost equally
baffled as to why it now breaks fido-mode.

I see in the side thread there seems to be a straightforward and logical
fix.  If that makes sense globally and also fixes it for Sean, I'm fine
with it, otherwise I'd suggest having a second look at the original
problem that prompted the problematic patch, specifically at why
fido-mode didn't suffer from it.

João

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[João Távora]

João Távora <joaotavora@gmail.com> writes:

> I see in the side thread there seems to be a straightforward and logical
> fix.  If that makes sense globally and also fixes it for Sean, I'm fine
> with it, otherwise I'd suggest having a second look at the original
> problem that prompted the problematic patch, specifically at why
> fido-mode didn't suffer from it.

It didn't, but it sure does now.

I just noticed that this affects bare-bones fido-mode as well, i.e. even
without using M-j we get some akward and very  highly annoying cache.

In an Emacs -Q with fido-mode enabled:

   (bound-and-true-p [press C-h f RET here, slowly])

now move the point to the "setq" in, say

   (setq blabla )

And type C-h f RET here, quickly.  You'll be presented with
`bound-and-true-p`'s doc, not `setq`'s.

This is a regression: the bug should be fixed ASAP.  Maybe some
"clearing" as was suggested is in order.  I'm not very confortable
writing that patch right now.  Can anyone else do it?  Or should we
revert the original fix instead in the meantime?

João

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Lars Ingebrigtsen]

João Távora <joaotavora@gmail.com> writes:

> This is a regression: the bug should be fixed ASAP.  Maybe some
> "clearing" as was suggested is in order.  I'm not very confortable
> writing that patch right now.  Can anyone else do it?  Or should we
> revert the original fix instead in the meantime?

I think the fix should be pretty trivial.  I don't have time to test it
today, but tomorrow should be open.

Reverting the problematic patch in the meantime is fine by me, but on
the other hand, it'll get reapplied again tomorrowish, I guess, so that
seems like unnecessary churn...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[João Távora]

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

On Thu, Sep 3, 2020 at 2:11 PM Lars Ingebrigtsen <larsi@gnus.org> wrote:

> João Távora <joaotavora@gmail.com> writes:
>
> > This is a regression: the bug should be fixed ASAP.  Maybe some
> > "clearing" as was suggested is in order.  I'm not very confortable
> > writing that patch right now.  Can anyone else do it?  Or should we
> > revert the original fix instead in the meantime?
>
> I think the fix should be pretty trivial.  I don't have time to test it
> today, but tomorrow should be open.
>
> Reverting the problematic patch in the meantime is fine by me, but on
> the other hand, it'll get reapplied again tomorrowish, I guess, so that
> seems like unnecessary churn...


No problem then, but can you give me an idea of the patch, so
I can hotfix my Emacs master where this is bothering me a bit?

João

[-- Attachment #2: Type: text/html, Size: 1289 bytes --]

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Lars Ingebrigtsen]

João Távora <joaotavora@gmail.com> writes:

> No problem then, but can you give me an idea of the patch, so
> I can hotfix my Emacs master where this is bothering me a bit?

I think setting the variable to nil in completion--complete-and-exit is
probably the correct fix, but I haven't tested it.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[João Távora]

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

Ok, thanks. I'll try that.

On Thu, Sep 3, 2020 at 2:51 PM Lars Ingebrigtsen <larsi@gnus.org> wrote:

> João Távora <joaotavora@gmail.com> writes:
>
> > No problem then, but can you give me an idea of the patch, so
> > I can hotfix my Emacs master where this is bothering me a bit?
>
> I think setting the variable to nil in completion--complete-and-exit is
> probably the correct fix, but I haven't tested it.
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no
>


-- 
João Távora

[-- Attachment #2: Type: text/html, Size: 1085 bytes --]

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Lars Ingebrigtsen]

Sean Whitton <spwhitton@spwhitton.name> writes:

> I think the fix would be to clear completion-content-when-empty when the
> minibuffer exits, instead of leaving data from the last completion
> there.

Oh yeah, definitely -- it should be cleared on minibuffer exit.  I'm
guessing that should go in...  `completion--complete-and-exit'?  Would
it be possible for you to propose a patch that does that?  I don't use
fido-mode myself, and it's probably easier for you to test whether
that's the right fix.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Sean Whitton]

Hello Lars,

On Tue 01 Sep 2020 at 04:19PM +02, Lars Ingebrigtsen wrote:

> Sean Whitton <spwhitton@spwhitton.name> writes:
>
>> I think the fix would be to clear completion-content-when-empty when the
>> minibuffer exits, instead of leaving data from the last completion
>> there.
>
> Oh yeah, definitely -- it should be cleared on minibuffer exit.  I'm
> guessing that should go in...  `completion--complete-and-exit'?  Would
> it be possible for you to propose a patch that does that?  I don't use
> fido-mode myself, and it's probably easier for you to test whether
> that's the right fix.

I'm certainly happy to test a patch but I don't think that can be the
right place to clear the variable.

The docstring for that function says it exits from a require-match
minibuffer.  But completion gets used in other minibuffers too.  So we
need to clear the variable somewhere which gets called for every
minibuffer exit.

-- 
Sean Whitton

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Lars Ingebrigtsen]

Sean Whitton <spwhitton@spwhitton.name> writes:

> The docstring for that function says it exits from a require-match
> minibuffer.  But completion gets used in other minibuffers too.  So we
> need to clear the variable somewhere which gets called for every
> minibuffer exit.

Yeah, following the exit logic (from the multiple number of code paths)
isn't trivial here.  But as far as I can tell, the exit-minibuffer
function is called in all the relevant code paths?

Does this fix the problem for you?

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 864726e3cc..8984440576 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -2062,6 +2062,9 @@ exit-minibuffer
   ;; (or to turn it into a list of buffers, ...), but in the mean time,
   ;; this should do the trick in most cases.
   (setq deactivate-mark nil)
+  ;; Clear any computed default values (so that they're not used on
+  ;; the next invocation).
+  (setq completion-content-when-empty nil)
   (throw 'exit nil))
 
 (defun self-insert-and-exit ()

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Sean Whitton]

Hello,

On Fri 04 Sep 2020 at 04:11AM +02, Lars Ingebrigtsen wrote:

> Sean Whitton <spwhitton@spwhitton.name> writes:
>
>> The docstring for that function says it exits from a require-match
>> minibuffer.  But completion gets used in other minibuffers too.  So we
>> need to clear the variable somewhere which gets called for every
>> minibuffer exit.
>
> Yeah, following the exit logic (from the multiple number of code paths)
> isn't trivial here.  But as far as I can tell, the exit-minibuffer
> function is called in all the relevant code paths?
>
> Does this fix the problem for you?

Unfortunately it does not, so I guess icomplete-fido-exit is not exiting
the minibuffer through that path.

-- 
Sean Whitton

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Lars Ingebrigtsen]

Sean Whitton <spwhitton@spwhitton.name> writes:

> Unfortunately it does not, so I guess icomplete-fido-exit is not exiting
> the minibuffer through that path.

Hm.  Am I misreading the code here?  So the else branch definitely calls
exit-minibuffer... 

(defun icomplete-fido-exit (force)
[...]
  (if (and (not force) minibuffer--require-match)
      (minibuffer-complete-and-exit)
    (exit-minibuffer)))

And the "then" branch ends up here:

(defun minibuffer-complete-and-exit ()
[...]
  (completion-complete-and-exit (minibuffer-prompt-end) (point-max)
                                #'exit-minibuffer))

Hm...  which then calls completion--complete-and-exit, which should then
end up calling the exit function in...  hm.  Perhaps not all the
branches?  There's a bunch of callbacks, but I thought I followed them
all to the end and they all ended up calling the exit function, but
perhaps not?  Would it be possible for you to edebug through
completion-complete-and-exit?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Sean Whitton]

Hello Lars,

On Fri 04 Sep 2020 at 05:27AM +02, Lars Ingebrigtsen wrote:

> Sean Whitton <spwhitton@spwhitton.name> writes:
>
>> Unfortunately it does not, so I guess icomplete-fido-exit is not exiting
>> the minibuffer through that path.
>
> Hm.  Am I misreading the code here?  So the else branch definitely calls
> exit-minibuffer...
>
> (defun icomplete-fido-exit (force)
> [...]
>   (if (and (not force) minibuffer--require-match)
>       (minibuffer-complete-and-exit)
>     (exit-minibuffer)))
>
> And the "then" branch ends up here:
>
> (defun minibuffer-complete-and-exit ()
> [...]
>   (completion-complete-and-exit (minibuffer-prompt-end) (point-max)
>                                 #'exit-minibuffer))
>
> Hm...  which then calls completion--complete-and-exit, which should then
> end up calling the exit function in...  hm.  Perhaps not all the
> branches?  There's a bunch of callbacks, but I thought I followed them
> all to the end and they all ended up calling the exit function, but
> perhaps not?  Would it be possible for you to edebug through
> completion-complete-and-exit?

I think I see what the problem is.  I was doing the following to
generate a test case: C-h f comp [wait for completions to appear] C-g

This leaves completion-content-when-empty populated to interfere with
the next run, as getting out of the minibuffer that way does not clear
the variable, but it should.

Might it work to set the variable buffer-local to the minibuffer?  Then
we can be sure it would always be cleared.

-- 
Sean Whitton

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Lars Ingebrigtsen]

Sean Whitton <spwhitton@spwhitton.name> writes:

> I think I see what the problem is.  I was doing the following to
> generate a test case: C-h f comp [wait for completions to appear] C-g

Oh, right; didn't think that through properly...

> This leaves completion-content-when-empty populated to interfere with
> the next run, as getting out of the minibuffer that way does not clear
> the variable, but it should.
>
> Might it work to set the variable buffer-local to the minibuffer?  Then
> we can be sure it would always be cleared.

All this time I thought the minibuffer was reused, but poking around a
bit now, it seems like it's not?

In which case -- does the following fix this problem?

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index 8a68df876c..ba266cfbfe 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -715,7 +715,7 @@ icomplete-completions
 	(setq prospects (nreverse prospects))
         ;; Return the first match if the user hits enter.
         (when icomplete-show-matches-on-no-input
-          (setq completion-content-when-empty (car prospects)))
+          (setq-local completion-content-when-empty (car prospects)))
         ;; Decorate first of the prospects.
 	(when prospects
 	  (let ((first (copy-sequence (pop prospects))))


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Sean Whitton]

Hello,

On Sat 05 Sep 2020 at 02:25PM +02, Lars Ingebrigtsen wrote:

> In which case -- does the following fix this problem?

It does indeed.  Hope this patch can be applied.

-- 
Sean Whitton

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Lars Ingebrigtsen]

Sean Whitton <spwhitton@spwhitton.name> writes:

> On Sat 05 Sep 2020 at 02:25PM +02, Lars Ingebrigtsen wrote:
>
>> In which case -- does the following fix this problem?
>
> It does indeed.  Hope this patch can be applied.

Thanks for testing; I've now applied the patch.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#19032: bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[João Távora]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Sean Whitton <spwhitton@spwhitton.name> writes:
>
>> On Sat 05 Sep 2020 at 02:25PM +02, Lars Ingebrigtsen wrote:
>>
>>> In which case -- does the following fix this problem?
>>
>> It does indeed.  Hope this patch can be applied.
>
> Thanks for testing; I've now applied the patch.

I've had a look at the original problem that triggered this, and I
wonder if this much simpler patch wouldn't be preferable.  For one, it
doesn't touch the minibuffer.el machinery (which is complicated as it
is) or has any kind of complicated caching semantics.  It just binds a
different command to RET in icomplete-minibuffer-map, presumably solving
19032 (in my limited testing).  It's also guaranteed not to affect
fido-mode.

I think something like this is the way to go for a behaviour change such
as this.

João


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Change-icomplete-show-matches-on-no-input-behaviour-.patch --]
[-- Type: text/x-diff, Size: 3013 bytes --]

From f4dc81e0c7be75ace3766ca16e2be8bdcc8f0627 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Sun, 6 Sep 2020 19:03:52 +0100
Subject: [PATCH] Change icomplete-show-matches-on-no-input behaviour for
 Icomplete only

Fixes: bug#19032, bug#43120

Previous fixes to bug#19032 introduced bugs in Fido mode.  This fix
relies on a new command bound to RET.

* etc/NEWS (Miscellaneous): Mention icomplete-show-matches-on-no-input.

* lisp/icomplete.el (icomplete-show-matches-on-no-input): Add comment.
(icomplete-minibuffer-map): Bind icomplete-ret.
(icomplete-ret): New command.
---
 etc/NEWS          |  6 ++++++
 lisp/icomplete.el | 16 +++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/etc/NEWS b/etc/NEWS
index 749b28ac3f..d40a4807ec 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -938,6 +938,12 @@ window after starting).  This variable defaults to nil.
 
 ** Miscellaneous
 
+---
+*** 'icomplete-show-matches-on-no-input' behavior change
+Previously, choosing a different completion with commands like 'C-.'
+and then hitting enter would choose the default completion.  Doing
+this will now choose the completion under point.
+
 +++
 *** The user can now customize how "default" values are prompted for.
 The new utility function 'format-prompt' has been added which uses the
diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index f76ab28fb8..c4d5012af9 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -75,7 +75,11 @@ icomplete-tidy-shadowed-file-names
 selection process starts again from the user's $HOME.")
 
 (defcustom icomplete-show-matches-on-no-input nil
-  "When non-nil, show completions when first prompting for input."
+  "When non-nil, show completions when first prompting for input.
+This also means that if you traverse the list of completions with
+commands like `C-.' and just hit RET without typing any
+characters, the match under point will be chosen instead of the
+default."
   :type 'boolean
   :version "24.4")
 
@@ -153,12 +157,22 @@ icomplete-post-command-hook
 (defvar icomplete-minibuffer-map
   (let ((map (make-sparse-keymap)))
     (define-key map [?\M-\t] 'icomplete-force-complete)
+    (define-key map (kbd "RET") 'icomplete-ret)
     (define-key map [?\C-j]  'icomplete-force-complete-and-exit)
     (define-key map [?\C-.]  'icomplete-forward-completions)
     (define-key map [?\C-,]  'icomplete-backward-completions)
     map)
   "Keymap used by `icomplete-mode' in the minibuffer.")
 
+(defun icomplete-ret ()
+  "Exit minibuffer for icomplete."
+  (interactive)
+  (if (and icomplete-show-matches-on-no-input
+           (car completion-all-sorted-completions)
+           (eql (icomplete--field-end) (icomplete--field-beg)))
+      (icomplete-force-complete-and-exit)
+    (exit-minibuffer)))
+
 (defun icomplete-force-complete-and-exit ()
   "Complete the minibuffer with the longest possible match and exit.
 Use the first of the matches if there are any displayed, and use
-- 
2.25.1

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Lars Ingebrigtsen]

João Távora <joaotavora@gmail.com> writes:

> I've had a look at the original problem that triggered this, and I
> wonder if this much simpler patch wouldn't be preferable.  For one, it
> doesn't touch the minibuffer.el machinery (which is complicated as it
> is) or has any kind of complicated caching semantics.  It just binds a
> different command to RET in icomplete-minibuffer-map, presumably solving
> 19032 (in my limited testing).  It's also guaranteed not to affect
> fido-mode.

That does look like a much simpler and less invasive way to implement
this; yes.  (And you'd presumably remove the stuff that was added for
19032 already?)

But I was wondering whether there were any other use cases where the
newly added stuff would be useful...  but perhaps not?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#19032: bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[João Távora]

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

On Mon, Sep 7, 2020 at 11:30 AM Lars Ingebrigtsen <larsi@gnus.org> wrote:

> That does look like a much simpler and less invasive way to implement
> this; yes.  (And you'd presumably remove the stuff that was added for
> 19032 already?)

Yes, I'd revert both the original patch and the fixup.

> But I was wondering whether there were any other use cases where the
> newly added stuff would be useful...  but perhaps not?

This is for Stefan to say: he's the "owner" of minibuffer.el.  Sorry Stefan
if
you're not, I just nominated you.  Anyway, I kind of screech at any
added complexity there, hence this simple suggestion.

Anyway, I'd risk committing my patch to master while we evaluate
this. Or maybe let's hear from Stefan first.

João

[Also I'm moving this to 19032's thread exclusively, since 43120 is fixed]

[-- Attachment #2: Type: text/html, Size: 1104 bytes --]

bug#19032: bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Lars Ingebrigtsen]

João Távora <joaotavora@gmail.com> writes:

> Anyway, I'd risk committing my patch to master while we evaluate
> this. Or maybe let's hear from Stefan first.

Let's wait for Stefan to chime in.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Stefan Monnier]

> I think something like this is the way to go for a behaviour change such
> as this.

Looks fine to me.

> +    (define-key map (kbd "RET") 'icomplete-ret)

Maybe use a `remap`ping instead?


        Stefan

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[João Távora]

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

>> I think something like this is the way to go for a behaviour change such
>> as this.
>
> Looks fine to me.
>
>> +    (define-key map (kbd "RET") 'icomplete-ret)
>
> Maybe use a `remap`ping instead?

Yes, maybe makes sense.

bug#19032: bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[João Távora]

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

>> I think something like this is the way to go for a behaviour change such
>> as this.
>
> Looks fine to me.

Thanks, pushed.

>> +    (define-key map (kbd "RET") 'icomplete-ret)
>
> Maybe use a `remap`ping instead?

I did that, too.

João

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[OGAWA Hirofumi]

João Távora <joaotavora@gmail.com> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> I think something like this is the way to go for a behaviour change such
>>> as this.
>>
>> Looks fine to me.
>
> Thanks, pushed.
>
>>> +    (define-key map (kbd "RET") 'icomplete-ret)
>>
>> Maybe use a `remap`ping instead?
>
> I did that, too.

> +(defun icomplete-ret ()
> +  "Exit minibuffer for icomplete."
> +  (interactive)
> +  (if (and icomplete-show-matches-on-no-input
> +           (car completion-all-sorted-completions)
> +           (eql (icomplete--field-end) (icomplete--field-beg)))
> +      (icomplete-force-complete-and-exit)
> +    (exit-minibuffer)))

This changed the behavior of RET from `minibuffer-complete-and-exit' to
`exit-minibuffer'. Was that intention? What I noticed is the following.

[before]
	emacs -Q
        M-x icomplete-mode
        C-xd /usr
        C-xk u		;; shows "Kill buffer (default usr): u(sr)"
        RET
        killed "usr" buffer

[after]
	emacs -Q
        M-x icomplete-mode
        C-xd /usr
        C-xk u		;; shows "Kill buffer (default usr): u(sr)"
        RET
        No buffer named u

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

bug#19032: bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[João Távora]

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

On Wed, Sep 9, 2020 at 3:01 PM OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
wrote:

> João Távora <joaotavora@gmail.com> writes:
>
> > Stefan Monnier <monnier@iro.umontreal.ca> writes:
> >
> >>> I think something like this is the way to go for a behaviour change
> such
> >>> as this.
> >>
> >> Looks fine to me.
> >
> > Thanks, pushed.
> >
> >>> +    (define-key map (kbd "RET") 'icomplete-ret)
> >>
> >> Maybe use a `remap`ping instead?
> >
> > I did that, too.
>
> > +(defun icomplete-ret ()
> > +  "Exit minibuffer for icomplete."
> > +  (interactive)
> > +  (if (and icomplete-show-matches-on-no-input
> > +           (car completion-all-sorted-completions)
> > +           (eql (icomplete--field-end) (icomplete--field-beg)))
> > +      (icomplete-force-complete-and-exit)
> > +    (exit-minibuffer)))
>
> This changed the behavior of RET from `minibuffer-complete-and-exit' to
> `exit-minibuffer'. Was that intention?
>

Nope, sorry. You're right. It should read minibuffer-complete-and-exit there
of course.

João

[-- Attachment #2: Type: text/html, Size: 1740 bytes --]

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Stefan Monnier]

> Nope, sorry. You're right. It should read minibuffer-complete-and-exit there
> of course.

IIRC this depends on whether the completion is `require-match` or not.


        Stefan

bug#19032: bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[João Távora]

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

>> Nope, sorry. You're right. It should read minibuffer-complete-and-exit there
>> of course.
>
> IIRC this depends on whether the completion is `require-match` or not.

If so, shouldn't minibuffer-complete-and-exit take care of that?  I
mean, I've remapped the binding to _that_ command, so unless it's making
some "(interactive)" magic, which it is not, calling it from lisp in the
normal case as it should be completely equivalent.

I've gone ahead and commited the fix, as it fixes Hirofumi's problem, as
he described it.

João

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Stefan Monnier]

>>> Nope, sorry. You're right. It should read minibuffer-complete-and-exit there
>>> of course.
>> IIRC this depends on whether the completion is `require-match` or not.
> If so, shouldn't minibuffer-complete-and-exit take care of that?

Yes and no: IIRC depending on `require-match`, RET is bound either
to `minibuffer-complete-and-exit` or to `exit-minibuffer`.


        Stefan

bug#19032: bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[João Távora]

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

On Wed, Sep 9, 2020 at 8:52 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> >>> Nope, sorry. You're right. It should read minibuffer-complete-and-exit
> there
> >>> of course.
> >> IIRC this depends on whether the completion is `require-match` or not.
> > If so, shouldn't minibuffer-complete-and-exit take care of that?
>
> Yes and no: IIRC depending on `require-match`, RET is bound either
> to `minibuffer-complete-and-exit` or to `exit-minibuffer`.


Bah, so the remap you suggested wouldn't work anyway.  What to do?
The good 'ol :filter trick? How does it go again?

João

[-- Attachment #2: Type: text/html, Size: 992 bytes --]

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[João Távora]

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

On Wed, Sep 9, 2020 at 8:54 PM João Távora <joaotavora@gmail.com> wrote:

> On Wed, Sep 9, 2020 at 8:52 PM Stefan Monnier <monnier@iro.umontreal.ca>
> wrote:
>
>> >>> Nope, sorry. You're right. It should read
>> minibuffer-complete-and-exit there
>> >>> of course.
>> >> IIRC this depends on whether the completion is `require-match` or not.
>> > If so, shouldn't minibuffer-complete-and-exit take care of that?
>>
>> Yes and no: IIRC depending on `require-match`, RET is bound either
>> to `minibuffer-complete-and-exit` or to `exit-minibuffer`.
>
>
> Bah, so the remap you suggested wouldn't work anyway.  What to do?
> The good 'ol :filter trick? How does it go again?
>

Alternatively (and a bit sillily), two remaps for two different commands:
one for exit-minibuffer and one for minibuffer-complete-and-exit.  Or
check minibuffer-require-match which was recently added. Pick your
poison.

João

[-- Attachment #2: Type: text/html, Size: 1649 bytes --]

bug#19032: bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Stefan Monnier]

> Alternatively (and a bit sillily), two remaps for two different commands:
> one for exit-minibuffer and one for minibuffer-complete-and-exit.

I'd go with that, yes,

Also, because it will handle the case where the user has added a binding
to `minibuffer-complete-and-exit` to the keymap where RET is bound to
`exit-minibuffer`.


        Stefan

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[João Távora]

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

>> Alternatively (and a bit sillily), two remaps for two different commands:
>> one for exit-minibuffer and one for minibuffer-complete-and-exit.
>
> I'd go with that, yes,
>
> Also, because it will handle the case where the user has added a binding
> to `minibuffer-complete-and-exit` to the keymap where RET is bound to
> `exit-minibuffer`.

OK, how's this look?

Though I'm starting to think that when require-match is nil, an
icomplete user wouldn't want the new icomplete-show-matches-on-no-input
behaviour anyway.  But I'm not one of those.  Else, if she does, doesn't
it mean she wants fido-mode instead?

The question is thus: remap exit-minibuffer or not?  It means usually:
exit with whatever has been input, which may well be the empty string.

João

diff --git a/lisp/icomplete.el b/lisp/icomplete.el
index 4e546807b7..6d48aa84d4 100644
--- a/lisp/icomplete.el
+++ b/lisp/icomplete.el
@@ -157,21 +157,31 @@ icomplete-post-command-hook
 (defvar icomplete-minibuffer-map
   (let ((map (make-sparse-keymap)))
     (define-key map [?\M-\t] 'icomplete-force-complete)
-    (define-key map [remap minibuffer-complete-and-exit] 'icomplete-ret)
+    (define-key map [remap minibuffer-complete-and-exit] 'icomplete-complete-and-exit)
+    (define-key map [remap exit-minibuffer] 'icomplete-exit)
     (define-key map [?\C-j]  'icomplete-force-complete-and-exit)
     (define-key map [?\C-.]  'icomplete-forward-completions)
     (define-key map [?\C-,]  'icomplete-backward-completions)
     map)
   "Keymap used by `icomplete-mode' in the minibuffer.")
 
-(defun icomplete-ret ()
-  "Exit minibuffer for icomplete."
-  (interactive)
+(defun icomplete--maybe-force (fallback)
+  "Helper for `icomplete-complete-and-exit' and `icomplete-exit'."
   (if (and icomplete-show-matches-on-no-input
            (car completion-all-sorted-completions)
            (eql (icomplete--field-end) (icomplete--field-beg)))
       (icomplete-force-complete-and-exit)
-    (minibuffer-complete-and-exit)))
+    (funcall fallback)))
+
+(defun icomplete-complete-and-exit ()
+  "Complete, then exit minibuffer for icomplete."
+  (interactive)
+  (icomplete--maybe-force #'minibuffer-complete-and-exit))
+
+(defun icomplete-exit ()
+  "Exit minibuffer for icomplete."
+  (interactive)
+  (icomplete--maybe-force #'exit-minibuffer))
 
 (defun icomplete-force-complete-and-exit ()
   "Complete the minibuffer with the longest possible match and exit.

bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Stefan Monnier]

> The question is thus: remap exit-minibuffer or not?

Good question.  I think if there is a default value (i.e. if
exit-minibuffer would return that non-nil default when the minibuffer
is empty), then I think it makes sense to use the new
icomplete-show-matches-on-no-input, but if not, indeed we probably
should return "" (otherwise we're making it impossible to return "").


        Stefan

bug#19032: bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Juri Linkov]

> Though I'm starting to think that when require-match is nil, an
> icomplete user wouldn't want the new icomplete-show-matches-on-no-input
> behaviour anyway.

This is confusing, I don't understand why behaviour of
icomplete-show-matches-on-no-input should depend on require-match.

Here are two examples that produce a different result.

The first example is from hi-lock-read-face-name:

(defvar hi-history nil)
(icomplete-mode)
(let ((icomplete-show-matches-on-no-input t)
      (defaults '("hi-yellow" "hi-green"))
      (hi-history '("hi-blue")))
  (completing-read
   (format-prompt "Highlight using face" (car defaults))
   obarray 'facep t nil 'hi-history defaults))

displays this prompt:

  Highlight using face (default hi-yellow): {link | menu | bold ...

Typing RET returns "link" (and sometimes returns "hi-blue" from the history),
but never returns the expected default value "hi-yellow".

Whereas the second example from tab-bar-switch-to-tab
works correctly since its arg require-match is nil:

(let ((icomplete-show-matches-on-no-input t)
      (defaults '("yellow" "green"))
      (hi-history '("blue")))
  (completing-read
   (format-prompt "Switch to tab by name" (car defaults))
   defaults nil nil nil 'hi-history defaults))

displays this prompt:

  Switch to tab by name (default yellow): {green | yellow}

Typing RET returns the default value "yellow", not the first candidate "green".

This makes the behaviour of icomplete-show-matches-on-no-input
unpredictable, and thus in some cases dangerous.

bug#19032: bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[João Távora]

Juri Linkov <juri@linkov.net> writes:

>> Though I'm starting to think that when require-match is nil, an
>> icomplete user wouldn't want the new icomplete-show-matches-on-no-input
>> behaviour anyway.
>
> This is confusing, I don't understand why behaviour of
> icomplete-show-matches-on-no-input should depend on require-match.
>
> Here are two examples that produce a different result.



>
> The first example is from hi-lock-read-face-name:
>
> (defvar hi-history nil)
> (icomplete-mode)
> (let ((icomplete-show-matches-on-no-input t)
>       (defaults '("hi-yellow" "hi-green"))
>       (hi-history '("hi-blue")))
>   (completing-read
>    (format-prompt "Highlight using face" (car defaults))
>    obarray 'facep t nil 'hi-history defaults))
>
> displays this prompt:
>
>   Highlight using face (default hi-yellow): {link | menu | bold ...
>
> Typing RET returns "link" (and sometimes returns "hi-blue" from the history),
> but never returns the expected default value "hi-yellow".
>
> Whereas the second example from tab-bar-switch-to-tab
> works correctly since its arg require-match is nil:
>
> (let ((icomplete-show-matches-on-no-input t)
>       (defaults '("yellow" "green"))
>       (hi-history '("blue")))
>   (completing-read
>    (format-prompt "Switch to tab by name" (car defaults))
>    defaults nil nil nil 'hi-history defaults))
>
> displays this prompt:
>
>   Switch to tab by name (default yellow): {green | yellow}
>
> Typing RET returns the default value "yellow", not the first candidate "green".
>
> This makes the behaviour of icomplete-show-matches-on-no-input
> unpredictable, and thus in some cases dangerous.

I think I agree, but I've just tested this with the version of
icomplete.el before I started messing with this stuff (commit
c8472cc69d4bce7f53c9a62966245a4de3d99fbd) and I get exactly the same
results as you.  So I'd leave my work here for someone else to pick up
on: To be clear, I just wanted to simplify/refactor the code to be less
intrusive on minibuffer.el.

I'm not much of an icomplete-mode user, more of a fido-mode user where
these discrepancies are "fixed" by copying ido-mode's behaviour.

João

bug#19032: bug#43120: 28.0.50; fido-mode: M-j before completions appear selects wrong choice

[Juri Linkov]

>> +(defun icomplete-ret ()
>> +  "Exit minibuffer for icomplete."
>> +  (interactive)
>> +  (if (and icomplete-show-matches-on-no-input
>> +           (car completion-all-sorted-completions)
>> +           (eql (icomplete--field-end) (icomplete--field-beg)))
>> +      (icomplete-force-complete-and-exit)
>> +    (exit-minibuffer)))
>
> This changed the behavior of RET from `minibuffer-complete-and-exit' to
> `exit-minibuffer'. Was that intention? What I noticed is the following.

I confirm that this change broke icomplete-mode.  Here is a test case
that shows regression:

M-x icomplete-mode RET
M-x rgr RET

  Lisp error: (error "‘rgr’ is not a valid command name")

Until yesterday it used to run 'rgrep'.