bug#58343: 29.0.50; ELisp code run in "inconsitent" selected-window state

[Alan Mackenzie]

Hello, Stefan.

On Wed, Oct 12, 2022 at 14:35:33 -0400, Stefan Monnier wrote:
> Hi Alan,

> > One minor problem I see at the moment is that the call to
> > move_minibuffers_onto_frame wasn't moved together with the surrounding
> > code.  With its current calling convention, it needs to be called
> > _after_ selected_frame has been set, because it uses selected_frame.

> Aha!

> > Is there any reason this function call was left where it was?

> I was afraid that swapping `move_minibuffers_onto_frame` and

>   if (EQ (f->selected_window, f->minibuffer_window)
>       /* The following test might fail if the mini-window contains a
>        non-active minibuffer.  */
>       && NILP (Fminibufferp (XWINDOW (f->minibuffer_window)->contents, Qt)))
>     {
>       Lisp_Object w = call1 (Qget_mru_window, frame);
>       if (WINDOW_LIVE_P (w)) /* W can be nil in minibuffer-only frames.  */
>         Fset_frame_selected_window (frame, w, Qnil);
>     }

> would not result in the same final result, e.g. because the name
> "move_minibuffers_onto_frame" suggests it might change the result of
> `XWINDOW (f->minibuffer_window)->contents`.

Yes, I suppose you're right.

> > Or could it just be moved, too?  Otherwise, its calling convention
> > will need to be adapted.

> The patch below refrains from moving it and passes it the to-be-selected
> frame as argument instead.

Yes.  That's a better solution than moving the function call around,
particularly given that there're are only two calls to it, both in
frame.c.

Just one small thing: the name sf is a bad name for a Lisp_Object -
there's a de facto convention in minibuf.c and frame.c that f, sf, and
of, etc are struct *frame's, and selected_frame, frame, etc., are
Lisp_Object's which are frames.  In fact sf is used as a struct *frame
elsewhere in minibuf.c.

Let me guess - originally this sf was a struct *frame, but got changed
to a Lisp_Object when it became clear that it was needed to set MB_frame
with.  ;-)

>         Stefan

[ .... ]

-- 
Alan Mackenzie (Nuremberg, Germany).