GNU bug report logs - #69056
30.0.50; history-add-new-input and recursive minibuffers

Previous Next

To reply to this bug, email your comments to 69056 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; history-add-new-input and recursive minibuffers
Date: Sun, 11 Feb 2024 16:54:43 +0100

1. emacs -Q
2. (setq enable-recursive-minibuffers t)
3. M-y
4. In the minibuffer (with the prompt "Yank from kill-ring: "),
   type M-x calendar RET (or any other command).
5. M-x M-p
   Expected: "calendar" is inserted in the minibuffer.
   Observed: error saying "Beginning of history; no preceding item".

The problem is that the minibuffer history of M-x isn't recorded when
you invoke M-x from within the minibuffer of read-from-kill-ring (M-y).
The reason is that read-from-kill-ring let binds history-add-new-input,
and that affects all recursive minibuffers as well, so no minibuffer
history is recorded until you exit the first (non-recursive) minibuffer.

AFAICT This issue affects all uses history-add-new-input, unfortunately,
not only read-from-kill-ring, since it's always used via let-bindings.

Message #8 received at 69056 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 69056 <at> debbugs.gnu.org
Subject: Re: bug#69056: 30.0.50;
 history-add-new-input and recursive minibuffers
Date: Sun, 11 Feb 2024 18:47:13 +0200

> Date: Sun, 11 Feb 2024 16:54:43 +0100
> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> 
> 1. emacs -Q
> 2. (setq enable-recursive-minibuffers t)
> 3. M-y
> 4. In the minibuffer (with the prompt "Yank from kill-ring: "),
>    type M-x calendar RET (or any other command).
> 5. M-x M-p
>    Expected: "calendar" is inserted in the minibuffer.
>    Observed: error saying "Beginning of history; no preceding item".
> 
> The problem is that the minibuffer history of M-x isn't recorded when
> you invoke M-x from within the minibuffer of read-from-kill-ring (M-y).
> The reason is that read-from-kill-ring let binds history-add-new-input,
> and that affects all recursive minibuffers as well, so no minibuffer
> history is recorded until you exit the first (non-recursive) minibuffer.
> 
> AFAICT This issue affects all uses history-add-new-input, unfortunately,
> not only read-from-kill-ring, since it's always used via let-bindings.

I'm not sure we should be interested in fixing this.  Recursive
minibuffers are not supposed to start a completely new command loop
unaffected by whatever was before it, so we shouldn't try.  Even if
this particular case is solved (which I'm not sure we can), there are
a legion of other similar situations, where something let-bound by a
command entering the minibuffer affects all the recursive minibuffers.
Let-binding in commands that prompt users is ubiquitous in Emacs.

It's easy enough to work around the problem: C-g (perhaps more than
once), then start afresh.

So I tend to close this as wontfix.

Message #11 received at 69056 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 69056 <at> debbugs.gnu.org
Subject: Re: bug#69056: 30.0.50; history-add-new-input and recursive
 minibuffers
Date: Sun, 11 Feb 2024 19:50:21 +0200

> From: Eshel Yaron <me <at> eshelyaron.com>
> Cc: 69056 <at> debbugs.gnu.org
> Date: Sun, 11 Feb 2024 18:42:49 +0100
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > I'm not sure we should be interested in fixing this.  Recursive
> > minibuffers are not supposed to start a completely new command loop
> > unaffected by whatever was before it, so we shouldn't try.
> 
> I see that, but the problem, IMO, is that there's nothing telling you
> that you're in this state of not recording minibuffer history.  You
> likely won't know that you're using a command that let-binds
> history-add-new-input when you enter a recursive minibuffer, and losing
> all minibuffer history from commands you invoked in the recursive edit
> may come as an unpleasant surprise.

We should probably document this caveat.  enable-recursive-minibuffers
is an advanced feature, not recommended to newbies.

Message #14 received at 69056 <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 69056 <at> debbugs.gnu.org
Subject: Re: bug#69056: 30.0.50; history-add-new-input and recursive
 minibuffers
Date: Sun, 11 Feb 2024 18:42:49 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Date: Sun, 11 Feb 2024 16:54:43 +0100
>> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>>
>>
>> 1. emacs -Q
>> 2. (setq enable-recursive-minibuffers t)
>> 3. M-y
>> 4. In the minibuffer (with the prompt "Yank from kill-ring: "),
>>    type M-x calendar RET (or any other command).
>> 5. M-x M-p
>>    Expected: "calendar" is inserted in the minibuffer.
>>    Observed: error saying "Beginning of history; no preceding item".
>>
>> The problem is that the minibuffer history of M-x isn't recorded when
>> you invoke M-x from within the minibuffer of read-from-kill-ring (M-y).
>> The reason is that read-from-kill-ring let binds history-add-new-input,
>> and that affects all recursive minibuffers as well, so no minibuffer
>> history is recorded until you exit the first (non-recursive) minibuffer.
>>
>> AFAICT This issue affects all uses history-add-new-input, unfortunately,
>> not only read-from-kill-ring, since it's always used via let-bindings.
>
> I'm not sure we should be interested in fixing this.  Recursive
> minibuffers are not supposed to start a completely new command loop
> unaffected by whatever was before it, so we shouldn't try.

I see that, but the problem, IMO, is that there's nothing telling you
that you're in this state of not recording minibuffer history.  You
likely won't know that you're using a command that let-binds
history-add-new-input when you enter a recursive minibuffer, and losing
all minibuffer history from commands you invoked in the recursive edit
may come as an unpleasant surprise.

> Even if this particular case is solved (which I'm not sure we can),
> there are a legion of other similar situations, where something
> let-bound by a command entering the minibuffer affects all the
> recursive minibuffers.  Let-binding in commands that prompt users is
> ubiquitous in Emacs.

Indeed, this issue is possibly broader.  Often the solution is to use
minibuffer-setup-hook to bind a variable buffer-locally in a minibuffer,
rather than let-binding it (affecting all recursive minibuffers).  For
history-add-new-input this is slightly trickier since read_minibuf
checks the value of this variable only after the minibuffer is exited.
I'm experimenting with a possible solution where we change read_minibuf
to grab the buffer-local value of this variable from the minibuffer, and
change all users of history-add-new-input to set it buffer-locally
instead of let-binding it.  Works pretty well, but it doesn't cover
third party code that uses this variable, naturally.

> It's easy enough to work around the problem: C-g (perhaps more than
> once), then start afresh.
>
> So I tend to close this as wontfix.

All right, fair enough.

Message #17 received at 69056 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefankangas <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 69056 <at> debbugs.gnu.org, me <at> eshelyaron.com
Subject: Re: bug#69056: 30.0.50;
 history-add-new-input and recursive minibuffers
Date: Thu, 15 Feb 2024 10:19:18 +0200

> Cc: 69056 <at> debbugs.gnu.org
> Date: Sun, 11 Feb 2024 19:50:21 +0200
> From: Eli Zaretskii <eliz <at> gnu.org>
> 
> > From: Eshel Yaron <me <at> eshelyaron.com>
> > Cc: 69056 <at> debbugs.gnu.org
> > Date: Sun, 11 Feb 2024 18:42:49 +0100
> > 
> > Eli Zaretskii <eliz <at> gnu.org> writes:
> > 
> > > I'm not sure we should be interested in fixing this.  Recursive
> > > minibuffers are not supposed to start a completely new command loop
> > > unaffected by whatever was before it, so we shouldn't try.
> > 
> > I see that, but the problem, IMO, is that there's nothing telling you
> > that you're in this state of not recording minibuffer history.  You
> > likely won't know that you're using a command that let-binds
> > history-add-new-input when you enter a recursive minibuffer, and losing
> > all minibuffer history from commands you invoked in the recursive edit
> > may come as an unpleasant surprise.
> 
> We should probably document this caveat.  enable-recursive-minibuffers
> is an advanced feature, not recommended to newbies.

Stefan & Stefan, any comments or suggestions, beyond documenting this
caveat?

Message #20 received at 69056 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 69056 <at> debbugs.gnu.org
Subject: Re: bug#69056: 30.0.50; history-add-new-input and recursive
 minibuffers
Date: Thu, 15 Feb 2024 10:24:45 -0500

> 1. emacs -Q
> 2. (setq enable-recursive-minibuffers t)
> 3. M-y
> 4. In the minibuffer (with the prompt "Yank from kill-ring: "),
>    type M-x calendar RET (or any other command).
> 5. M-x M-p
>    Expected: "calendar" is inserted in the minibuffer.
>    Observed: error saying "Beginning of history; no preceding item".
>
> The problem is that the minibuffer history of M-x isn't recorded when
> you invoke M-x from within the minibuffer of read-from-kill-ring (M-y).
> The reason is that read-from-kill-ring let binds history-add-new-input,
> and that affects all recursive minibuffers as well, so no minibuffer
> history is recorded until you exit the first (non-recursive) minibuffer.

I suspect this can bite in more cases, including cases which don't
involve setting `enable-recursive-minibuffers` to t.
We should probably change the way `history-add-new-input` works so it's
attached to a particular minibuffer rather than being dynbound.


        Stefan

Message #23 received at 69056 <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 69056 <at> debbugs.gnu.org
Subject: Re: bug#69056: 30.0.50; history-add-new-input and recursive
 minibuffers
Date: Thu, 15 Feb 2024 17:17:26 +0100

[Message part 1 (text/plain, inline)]

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

>> 1. emacs -Q
>> 2. (setq enable-recursive-minibuffers t)
>> 3. M-y
>> 4. In the minibuffer (with the prompt "Yank from kill-ring: "),
>>    type M-x calendar RET (or any other command).
>> 5. M-x M-p
>>    Expected: "calendar" is inserted in the minibuffer.
>>    Observed: error saying "Beginning of history; no preceding item".
>>
>> The problem is that the minibuffer history of M-x isn't recorded when
>> you invoke M-x from within the minibuffer of read-from-kill-ring (M-y).
>> The reason is that read-from-kill-ring let binds history-add-new-input,
>> and that affects all recursive minibuffers as well, so no minibuffer
>> history is recorded until you exit the first (non-recursive) minibuffer.
>
> I suspect this can bite in more cases, including cases which don't
> involve setting `enable-recursive-minibuffers` to t.
> We should probably change the way `history-add-new-input` works so it's
> attached to a particular minibuffer rather than being dynbound.

Thanks, that's what I thought too.  Here's an attempt do just that:

[0001-Use-buffer-local-value-of-history-add-new-input-in-m.patch (text/x-patch, attachment)]

Message #26 received at 69056 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 69056 <at> debbugs.gnu.org
Subject: Re: bug#69056: 30.0.50; history-add-new-input and recursive
 minibuffers
Date: Thu, 15 Feb 2024 12:56:43 -0500

> Thanks, that's what I thought too.  Here's an attempt do just that:

Looks pretty good.  I do have some comments/questions:

> @@ -902,6 +903,9 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
>    /* Don't allow the user to undo past this point.  */
>    bset_undo_list (current_buffer, Qnil);
>
> +  /* Cache the buffer-local value. */
> +  nohist = NILP (find_symbol_value (Qhistory_add_new_input));

Why not use `Vhistory_add_new_input`?
[ Also, it's not really "cache" (which implies it impacts only
  performance).  More like "remember".  ]

> @@ -965,7 +969,7 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
>    /* Add the value to the appropriate history list, if any.  This is
>       done after the previous buffer has been made current again, in
>       case the history variable is buffer-local.  */
> -  if (! (NILP (Vhistory_add_new_input) || NILP (histstring)))
> +  if (! (nohist || NILP (histstring)))
>      call2 (Qadd_to_history, histvar, histstring);
>  
>    /* If Lisp form desired instead of string, parse it.  */

IIUC this change is needed because by the time we get here the
buffer-local value of `history-add-new-input` has been flushed by
`minibuffer-inactive-mode` called by `read_minibuf_unwind`,
itself run by the `unbind_to` a few lines above.
So maybe we can simplify this by just moving the above 2 lines before
the `unbind_to`, WDYT?


        Stefan

Message #29 received at 69056 <at> debbugs.gnu.org (full text, mbox):

From: Eshel Yaron <me <at> eshelyaron.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 69056 <at> debbugs.gnu.org
Subject: Re: bug#69056: 30.0.50; history-add-new-input and recursive
 minibuffers
Date: Thu, 15 Feb 2024 19:40:29 +0100

[Message part 1 (text/plain, inline)]

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

>> Thanks, that's what I thought too.  Here's an attempt do just that:
>
> Looks pretty good.  I do have some comments/questions:
>
>> @@ -902,6 +903,9 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
>>    /* Don't allow the user to undo past this point.  */
>>    bset_undo_list (current_buffer, Qnil);
>>
>> +  /* Cache the buffer-local value. */
>> +  nohist = NILP (find_symbol_value (Qhistory_add_new_input));
>
> Why not use `Vhistory_add_new_input`?

Good question, I guess for some reason I assumed that `NILP (Vfoo)`
doesn't check the buffer-local value like `find_symbol_value (Qfoo)`
does...

> [ Also, it's not really "cache" (which implies it impacts only
>   performance).  More like "remember".  ]

>> @@ -965,7 +969,7 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
>>    /* Add the value to the appropriate history list, if any.  This is
>>       done after the previous buffer has been made current again, in
>>       case the history variable is buffer-local.  */
>> -  if (! (NILP (Vhistory_add_new_input) || NILP (histstring)))
>> +  if (! (nohist || NILP (histstring)))
>>      call2 (Qadd_to_history, histvar, histstring);
>>
>>    /* If Lisp form desired instead of string, parse it.  */
>
> IIUC this change is needed because by the time we get here the
> buffer-local value of `history-add-new-input` has been flushed by
> `minibuffer-inactive-mode` called by `read_minibuf_unwind`,
> itself run by the `unbind_to` a few lines above.
> So maybe we can simplify this by just moving the above 2 lines before
> the `unbind_to`, WDYT?

Oh, that's much simpler indeed.  And it seems to work just as well.
Here's an updated patch (v2):

[v2-0001-Use-buffer-local-value-of-history-add-new-input-i.patch (text/x-patch, attachment)]

Message #32 received at 69056 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eshel Yaron <me <at> eshelyaron.com>
Cc: 69056 <at> debbugs.gnu.org
Subject: Re: bug#69056: 30.0.50; history-add-new-input and recursive
 minibuffers
Date: Thu, 15 Feb 2024 14:20:08 -0500

>> Why not use `Vhistory_add_new_input`?
> Good question, I guess for some reason I assumed that `NILP (Vfoo)`
> doesn't check the buffer-local value like `find_symbol_value (Qfoo)`
> does...

The handling of Vfoo is quite delicate, but it does give you the value
in the current-buffer (i.e. they're changed as needed whenever we go
through `set_buffer`).

> Oh, that's much simpler indeed.  And it seems to work just as well.
> Here's an updated patch (v2):

LGTM.
Eli&Stefan, any objection?


        Stefan

Message #35 received at 69056 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 69056 <at> debbugs.gnu.org, me <at> eshelyaron.com
Subject: Re: bug#69056: 30.0.50;
 history-add-new-input and recursive minibuffers
Date: Thu, 15 Feb 2024 21:27:44 +0200

> Cc: 69056 <at> debbugs.gnu.org
> Date: Thu, 15 Feb 2024 12:56:43 -0500
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> > Thanks, that's what I thought too.  Here's an attempt do just that:
> 
> Looks pretty good.

Is this really worthwhile?  It might solve some problems with commands
invoked from the recursive edit, but it doesn't solve all of them,
because the value of history-add-new-input is still set in that
minibuffer.  And it introduces tricky effects due to the variable
being buffer-local for any code that let-binds history-add-new-input,
and could potentially break something because of that.

I'm afraid I don't like this change, for those reasons.

Message #38 received at 69056 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 69056 <at> debbugs.gnu.org, me <at> eshelyaron.com
Subject: Re: bug#69056: 30.0.50;
 history-add-new-input and recursive minibuffers
Date: Thu, 15 Feb 2024 21:34:09 +0200

> Cc: 69056 <at> debbugs.gnu.org
> Date: Thu, 15 Feb 2024 14:20:08 -0500
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> >> Why not use `Vhistory_add_new_input`?
> > Good question, I guess for some reason I assumed that `NILP (Vfoo)`
> > doesn't check the buffer-local value like `find_symbol_value (Qfoo)`
> > does...
> 
> The handling of Vfoo is quite delicate, but it does give you the value
> in the current-buffer (i.e. they're changed as needed whenever we go
> through `set_buffer`).
> 
> > Oh, that's much simpler indeed.  And it seems to work just as well.
> > Here's an updated patch (v2):
> 
> LGTM.
> Eli&Stefan, any objection?

Yes, see my other message.  I feel like we are making an effort to
change the internals, which runs the usual risk of breaking things,
for very little gain.  The more general issue with let-binding
variables around APIs that enter the minibuffer stays, so I see little
sense to fix just this one problem in an incomplete way that could on
top of that break existing code.

Message #41 received at 69056 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 69056 <at> debbugs.gnu.org, me <at> eshelyaron.com
Subject: Re: bug#69056: 30.0.50; history-add-new-input and recursive
 minibuffers
Date: Thu, 15 Feb 2024 14:54:07 -0500

> for very little gain.  The more general issue with let-binding
> variables around APIs that enter the minibuffer stays, so I see little

I agree that it would be good to tackle this more general problem.

Basically, should we treat recursive-edits as if they were run in a sort
of separate thread (with the original thread blocked until the new
thread exits)?

I think we can't do that in general, since I think we sometimes do want
let-bindings performed around `read-from-minibuffer` to affect the
command executed inside the minibuffer.  But we should maybe experiment
with it to get a clearer idea of where we do want/need it and where
we don't.


        Stefan

Message #44 received at 69056 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 69056 <at> debbugs.gnu.org, me <at> eshelyaron.com
Subject: Re: bug#69056: 30.0.50; history-add-new-input and recursive
 minibuffers
Date: Fri, 16 Feb 2024 09:08:52 +0200

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: me <at> eshelyaron.com,  69056 <at> debbugs.gnu.org
> Date: Thu, 15 Feb 2024 14:54:07 -0500
> 
> > for very little gain.  The more general issue with let-binding
> > variables around APIs that enter the minibuffer stays, so I see little
> 
> I agree that it would be good to tackle this more general problem.
> 
> Basically, should we treat recursive-edits as if they were run in a sort
> of separate thread (with the original thread blocked until the new
> thread exits)?

I don't think we can do that by default.  We need some evidence that
this is the intent.

> I think we can't do that in general, since I think we sometimes do want
> let-bindings performed around `read-from-minibuffer` to affect the
> command executed inside the minibuffer.

Exactly.  And doing so is a very wide-spread paradigm in Emacs.  Which
is one reason why enable-recursive-minibuffers is not turned on by
default, and should be considered an advanced feature for users who
"know what they are doing" and are ready to sustain the consequences.

> But we should maybe experiment with it to get a clearer idea of
> where we do want/need it and where we don't.

Perhaps.  But I'd like to hear concrete ideas for such experiments
before I have an opinion on their value.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.