bug#55684: 29.0.50; wrong-type-argument in minibuffer-only frames

bug#55684: 29.0.50; wrong-type-argument in minibuffer-only frames

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Package: Emacs
Version: 29.0.50


    src/emacs -Q --eval '(setq default-frame-alist `((minibuffer . nil)))'

Stays "frozen" because of a wrong-type-argument error in
`Fset_frame_selected_window` on `CHECK_LIVE_WINDOW (window);`.

The patch below seems to "fix" it, but I must admit that I don't really
understand this code (see for example the FIXME comment in the patch).
Does someone know what is the user-level behavior we're trying to
obtain here?

It seems that this comes from Alan's commit dfa3e6f424b20fe27d904.


        Stefan


diff --git a/src/frame.c b/src/frame.c
index 252dc591bfa..dc55004b193 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -1576,8 +1576,14 @@ do_switch_frame (Lisp_Object frame, int track, int for_deletion, Lisp_Object nor
      to a different window, the most recently used one, unless there is a
      valid active minibuffer in the mini-window.  */
   if (EQ (f->selected_window, f->minibuffer_window)
+         /* FIXME: Can this test ever fail?  I.e. can 'minibuffer_window'
+            ever contain a non-mini-buffer (and if so, should we care here)? */
       && NILP (Fminibufferp (XWINDOW (f->minibuffer_window)->contents, Qt)))
-    Fset_frame_selected_window (frame, call1 (Qget_mru_window, frame), Qnil);
+    {
+      Lisp_Object w = call1 (Qget_mru_window, frame);
+      if (!NILP (w))            /* Can be nil in minibuffer-only frames.  */
+        Fset_frame_selected_window (frame, w, Qnil);
+    }
 
   Fselect_window (f->selected_window, norecord);
 

bug#55684: 29.0.50; wrong-type-argument in minibuffer-only frames

[Eli Zaretskii]

> Cc: Alan Mackenzie <acm@muc.de>
> Date: Fri, 27 May 2022 16:09:13 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> Package: Emacs
> Version: 29.0.50

Doesn't the same problem exist on the emacs-28 branch?  I can
reproduce it there.

> -    Fset_frame_selected_window (frame, call1 (Qget_mru_window, frame), Qnil);
> +    {
> +      Lisp_Object w = call1 (Qget_mru_window, frame);
> +      if (!NILP (w))            /* Can be nil in minibuffer-only frames.  */
> +        Fset_frame_selected_window (frame, w, Qnil);

The NILP(w) test should probably be WINDOW_LIVE_P(w).  Martin, do you
agree?

bug#55684: 29.0.50; wrong-type-argument in minibuffer-only frames

[Alan Mackenzie]

Hello, Stefan.

On Fri, May 27, 2022 at 16:09:13 -0400, Stefan Monnier wrote:
> Package: Emacs
> Version: 29.0.50


>     src/emacs -Q --eval '(setq default-frame-alist `((minibuffer . nil)))'

> Stays "frozen" because of a wrong-type-argument error in
> `Fset_frame_selected_window` on `CHECK_LIVE_WINDOW (window);`.

Yes.  This is unfortunate.  Sorry.

> The patch below seems to "fix" it, but I must admit that I don't really
> understand this code (see for example the FIXME comment in the patch).
> Does someone know what is the user-level behavior we're trying to
> obtain here?

When the mini-window in frame F1 is the selected window, and we switch
to a different frame F2, F1 remembers the mini-window as its selected
window.  When we later switch back to F1, we check whether there is
still a minibuffer in the mini-window, and if not select a different
window.

There are several reasons why there might not be a minibuffer on
returning to F1.  The MB in F1 might have moved to F2 on the frame
switch, and been terminated there, for example.

> It seems that this comes from Alan's commit dfa3e6f424b20fe27d904.


>         Stefan


> diff --git a/src/frame.c b/src/frame.c
> index 252dc591bfa..dc55004b193 100644
> --- a/src/frame.c
> +++ b/src/frame.c
> @@ -1576,8 +1576,14 @@ do_switch_frame (Lisp_Object frame, int track, int for_deletion, Lisp_Object nor
>       to a different window, the most recently used one, unless there is a
>       valid active minibuffer in the mini-window.  */
>    if (EQ (f->selected_window, f->minibuffer_window)
> +         /* FIXME: Can this test ever fail?  I.e. can 'minibuffer_window'
> +            ever contain a non-mini-buffer (and if so, should we care here)? */

Yes, the test can fail.  The buffer in the mini-window might be (the
non-active) minibuffer 0.  The meaning of the argument Qt is to return
non-nil only for active minibuffers.

>        && NILP (Fminibufferp (XWINDOW (f->minibuffer_window)->contents, Qt)))
> -    Fset_frame_selected_window (frame, call1 (Qget_mru_window, frame), Qnil);
> +    {
> +      Lisp_Object w = call1 (Qget_mru_window, frame);
> +      if (!NILP (w))            /* Can be nil in minibuffer-only frames.  */
> +        Fset_frame_selected_window (frame, w, Qnil);
> +    }
 
>    Fselect_window (f->selected_window, norecord);

I think the patch looks OK.  I'm going to comment on Eli's suggestion.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#55684: 29.0.50; wrong-type-argument in minibuffer-only frames

[Alan Mackenzie]

Hello, Eli.

On Sat, May 28, 2022 at 08:56:04 +0300, Eli Zaretskii wrote:
> > Cc: Alan Mackenzie <acm@muc.de>
> > Date: Fri, 27 May 2022 16:09:13 -0400
> > From:  Stefan Monnier via "Bug reports for GNU Emacs,
> >  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>

> > Package: Emacs
> > Version: 29.0.50

> Doesn't the same problem exist on the emacs-28 branch?  I can
> reproduce it there.

> > -    Fset_frame_selected_window (frame, call1 (Qget_mru_window, frame), Qnil);
> > +    {
> > +      Lisp_Object w = call1 (Qget_mru_window, frame);
> > +      if (!NILP (w))            /* Can be nil in minibuffer-only frames.  */
> > +        Fset_frame_selected_window (frame, w, Qnil);

> The NILP(w) test should probably be WINDOW_LIVE_P(w).  Martin, do you
> agree?

I'm not Martin, but I think WINDOW_LIVE is safe, but might not be
needed.  I think get-mru-window will only return a live window or nil.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#55684: 29.0.50; wrong-type-argument in minibuffer-only frames

[Eli Zaretskii]

> Date: Sat, 28 May 2022 10:51:25 +0000
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
>   martin rudalics <rudalics@gmx.at>, 55684@debbugs.gnu.org, acm@muc.de
> From: Alan Mackenzie <acm@muc.de>
> 
> Hello, Eli.
> 
> On Sat, May 28, 2022 at 08:56:04 +0300, Eli Zaretskii wrote:
> > > Cc: Alan Mackenzie <acm@muc.de>
> > > Date: Fri, 27 May 2022 16:09:13 -0400
> > > From:  Stefan Monnier via "Bug reports for GNU Emacs,
> > >  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> > > Package: Emacs
> > > Version: 29.0.50
> 
> > Doesn't the same problem exist on the emacs-28 branch?  I can
> > reproduce it there.
> 
> > > -    Fset_frame_selected_window (frame, call1 (Qget_mru_window, frame), Qnil);
> > > +    {
> > > +      Lisp_Object w = call1 (Qget_mru_window, frame);
> > > +      if (!NILP (w))            /* Can be nil in minibuffer-only frames.  */
> > > +        Fset_frame_selected_window (frame, w, Qnil);
> 
> > The NILP(w) test should probably be WINDOW_LIVE_P(w).  Martin, do you
> > agree?
> 
> I'm not Martin, but I think WINDOW_LIVE is safe, but might not be
> needed.  I think get-mru-window will only return a live window or nil.

It starts with a list of live windows, that's true, but who will
guarantee that while it's processing some window cannot be deleted?
WINDOW_LIVE_P is not expensive enough to make such assumptions.

In any case, even if we are sure a window returned by get-mru-window
I'd prefer to use WINDOWP instead of NILP here.

bug#55684: 29.0.50; wrong-type-argument in minibuffer-only frames

[Alan Mackenzie]

Hello, Eli.

On Sat, May 28, 2022 at 13:57:25 +0300, Eli Zaretskii wrote:
> > Date: Sat, 28 May 2022 10:51:25 +0000
> > Cc: Stefan Monnier <monnier@iro.umontreal.ca>,
> >   martin rudalics <rudalics@gmx.at>, 55684@debbugs.gnu.org, acm@muc.de
> > From: Alan Mackenzie <acm@muc.de>

> > Hello, Eli.

> > On Sat, May 28, 2022 at 08:56:04 +0300, Eli Zaretskii wrote:
> > > > Cc: Alan Mackenzie <acm@muc.de>
> > > > Date: Fri, 27 May 2022 16:09:13 -0400
> > > > From:  Stefan Monnier via "Bug reports for GNU Emacs,
> > > >  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>

> > > > Package: Emacs
> > > > Version: 29.0.50

> > > Doesn't the same problem exist on the emacs-28 branch?  I can
> > > reproduce it there.

> > > > -    Fset_frame_selected_window (frame, call1 (Qget_mru_window, frame), Qnil);
> > > > +    {
> > > > +      Lisp_Object w = call1 (Qget_mru_window, frame);
> > > > +      if (!NILP (w))            /* Can be nil in minibuffer-only frames.  */
> > > > +        Fset_frame_selected_window (frame, w, Qnil);

> > > The NILP(w) test should probably be WINDOW_LIVE_P(w).  Martin, do you
> > > agree?

> > I'm not Martin, but I think WINDOW_LIVE is safe, but might not be
> > needed.  I think get-mru-window will only return a live window or nil.

> It starts with a list of live windows, that's true, but who will
> guarantee that while it's processing some window cannot be deleted?
> WINDOW_LIVE_P is not expensive enough to make such assumptions.

That makes sense.

> In any case, even if we are sure a window returned by get-mru-window
> I'd prefer to use WINDOWP instead of NILP here.

I've committed Stefan's patch, amended with WINDOW_LIVE_P and an extra
comment to the emacs-28 branch.

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#55684: 29.0.50; wrong-type-argument in minibuffer-only frames

[Eli Zaretskii]

> Date: Sat, 28 May 2022 13:09:17 +0000
> Cc: monnier@iro.umontreal.ca, rudalics@gmx.at, 55684@debbugs.gnu.org,
>   acm@muc.de
> From: Alan Mackenzie <acm@muc.de>
> 
> > It starts with a list of live windows, that's true, but who will
> > guarantee that while it's processing some window cannot be deleted?
> > WINDOW_LIVE_P is not expensive enough to make such assumptions.
> 
> That makes sense.
> 
> > In any case, even if we are sure a window returned by get-mru-window
> > I'd prefer to use WINDOWP instead of NILP here.
> 
> I've committed Stefan's patch, amended with WINDOW_LIVE_P and an extra
> comment to the emacs-28 branch.

Thanks.

bug#55684: 29.0.50; wrong-type-argument in minibuffer-only frames

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> @@ -1576,8 +1576,14 @@ do_switch_frame (Lisp_Object frame, int track, int for_deletion, Lisp_Object nor
>>       to a different window, the most recently used one, unless there is a
>>       valid active minibuffer in the mini-window.  */
>>    if (EQ (f->selected_window, f->minibuffer_window)
>> +         /* FIXME: Can this test ever fail?  I.e. can 'minibuffer_window'
>> +            ever contain a non-mini-buffer (and if so, should we care here)? */
>>        && NILP (Fminibufferp (XWINDOW (f->minibuffer_window)->contents, Qt)))
>
> Yes, the test can fail.  The buffer in the mini-window might be (the
> non-active) minibuffer 0.  The meaning of the argument Qt is to return
> non-nil only for active minibuffers.

Ah, indeed I missed this detail.  I guess that mini window can
also hold the echo area.  Thanks for the explanation,


        Stefan