* bug#58343: 29.0.50; ELisp code run in "inconsitent" selected-window state @ 2022-10-07 0:06 Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2022-10-12 18:08 ` Alan Mackenzie 0 siblings, 1 reply; 5+ messages in thread From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-10-07 0:06 UTC (permalink / raw) To: 58343; +Cc: Alan Mackenzie Package: Emacs Version: 29.0.50 Hi Alan and friends, In commit dfa3e6f424b20fe27d9041b2ce7d69811df5d8cd, Alan added the following code to do_switch_frame: diff --git a/src/frame.c b/src/frame.c index ccac18d23c2..dc8045f41e6 100644 --- a/src/frame.c +++ b/src/frame.c @@ -1564,6 +1564,13 @@ do_switch_frame (Lisp_Object frame, int track, int for_deletion, Lisp_Object nor if (! FRAME_MINIBUF_ONLY_P (XFRAME (selected_frame))) last_nonminibuf_frame = XFRAME (selected_frame); + /* If the selected window in the target frame is its mini-window, we move + 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) + && NILP (Fminibufferp (XWINDOW (f->minibuffer_window)->contents, Qt))) + Fset_frame_selected_window (frame, call1 (Qget_mru_window, frame), Qnil); + Fselect_window (f->selected_window, norecord); /* We want to make sure that the next event generates a frame-switch the problem with this is that it calls `Qget_mru_window` which is ELisp code, and that we're in the very middle of changing the selected frame, so we have already changed the `selected-frame` variable a few lines earlier, but the select-window has not yet been adjusted accordingly. Running ELisp code in a state where (selected-window) and (frame-selected-window) aren't equal is a recipe for problems. I have already wasted many hours in the past tracking down bugs linked to this kind of situation (back when the mode-line was processed in such an inconsistent state, for example), and I really don't want to go there. So we should arrange to run this `get-mru-window` function at some other time, for example a few lines earlier before we set `selected-frame`. I don't understand this code nearly enough to know how to move the code, because it probably interacts with the other statements in non-trivial ways, so the patch below is just a naive suggestion (it seems to work here without triggering my many sprinkled assertions checking that `EQ (XFRAME (selected_frame)->selected_window, selected_window)`, but it's a far cry from a confirmation that it's right). Hopefully someone here is aware of some of the potential pitfalls. Stefan diff --git a/src/frame.c b/src/frame.c index 91b9bec82c3..58b6ee50d23 100644 --- a/src/frame.c +++ b/src/frame.c @@ -1503,18 +1503,8 @@ do_switch_frame (Lisp_Object frame, int for_deletion, Lisp_Object norecord) sf->select_mini_window_flag = MINI_WINDOW_P (XWINDOW (sf->selected_window)); - selected_frame = frame; - move_minibuffers_onto_frame (sf, for_deletion); - if (f->select_mini_window_flag - && !NILP (Fminibufferp (XWINDOW (f->minibuffer_window)->contents, Qt))) - f->selected_window = f->minibuffer_window; - f->select_mini_window_flag = false; - - if (! FRAME_MINIBUF_ONLY_P (XFRAME (selected_frame))) - last_nonminibuf_frame = XFRAME (selected_frame); - /* If the selected window in the target frame is its mini-window, we move to a different window, the most recently used one, unless there is a valid active minibuffer in the mini-window. */ @@ -1528,6 +1518,16 @@ do_switch_frame (Lisp_Object frame, int for_deletion, Lisp_Object norecord) Fset_frame_selected_window (frame, w, Qnil); } + selected_frame = frame; + + if (f->select_mini_window_flag + && !NILP (Fminibufferp (XWINDOW (f->minibuffer_window)->contents, Qt))) + f->selected_window = f->minibuffer_window; + f->select_mini_window_flag = false; + + if (! FRAME_MINIBUF_ONLY_P (XFRAME (selected_frame))) + last_nonminibuf_frame = XFRAME (selected_frame); + Fselect_window (f->selected_window, norecord); /* We want to make sure that the next event generates a frame-switch ^ permalink raw reply related [flat|nested] 5+ messages in thread
* bug#58343: 29.0.50; ELisp code run in "inconsitent" selected-window state 2022-10-07 0:06 bug#58343: 29.0.50; ELisp code run in "inconsitent" selected-window state Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-10-12 18:08 ` Alan Mackenzie 2022-10-12 18:35 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 5+ messages in thread From: Alan Mackenzie @ 2022-10-12 18:08 UTC (permalink / raw) To: Stefan Monnier; +Cc: 58343 Hello, Stefan. On Thu, Oct 06, 2022 at 20:06:25 -0400, Stefan Monnier wrote: > Package: Emacs > Version: 29.0.50 > Hi Alan and friends, > In commit dfa3e6f424b20fe27d9041b2ce7d69811df5d8cd, Alan added the > following code to do_switch_frame: > diff --git a/src/frame.c b/src/frame.c > index ccac18d23c2..dc8045f41e6 100644 > --- a/src/frame.c > +++ b/src/frame.c > @@ -1564,6 +1564,13 @@ do_switch_frame (Lisp_Object frame, int track, int for_deletion, Lisp_Object nor > if (! FRAME_MINIBUF_ONLY_P (XFRAME (selected_frame))) > last_nonminibuf_frame = XFRAME (selected_frame); > + /* If the selected window in the target frame is its mini-window, we move > + 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) > + && NILP (Fminibufferp (XWINDOW (f->minibuffer_window)->contents, Qt))) > + Fset_frame_selected_window (frame, call1 (Qget_mru_window, frame), Qnil); > + > Fselect_window (f->selected_window, norecord); > /* We want to make sure that the next event generates a frame-switch > the problem with this is that it calls `Qget_mru_window` which is ELisp > code, and that we're in the very middle of changing the selected frame, > so we have already changed the `selected-frame` variable a few lines > earlier, but the select-window has not yet been adjusted accordingly. > Running ELisp code in a state where (selected-window) and > (frame-selected-window) aren't equal is a recipe for problems. I have > already wasted many hours in the past tracking down bugs linked to this > kind of situation (back when the mode-line was processed in such an > inconsistent state, for example), and I really don't want to go there. > So we should arrange to run this `get-mru-window` function at some other > time, for example a few lines earlier before we set `selected-frame`. > I don't understand this code nearly enough to know how to move the code, > because it probably interacts with the other statements in non-trivial > ways, so the patch below is just a naive suggestion (it seems to work > here without triggering my many sprinkled assertions checking that > `EQ (XFRAME (selected_frame)->selected_window, selected_window)`, > but it's a far cry from a confirmation that it's right). > Hopefully someone here is aware of some of the potential pitfalls. 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. Is there any reason this function call was left where it was? Or could it just be moved, too? Otherwise, its calling convention will need to be adapted. Other than that, I haven't seen any problems as yet, though I admit I haven't tried it out. > Stefan [ .... ] -- Alan Mackenzie (Nuremberg, Germany). ^ permalink raw reply [flat|nested] 5+ messages in thread
* bug#58343: 29.0.50; ELisp code run in "inconsitent" selected-window state 2022-10-12 18:08 ` Alan Mackenzie @ 2022-10-12 18:35 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2022-10-12 19:13 ` Alan Mackenzie 0 siblings, 1 reply; 5+ messages in thread From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-10-12 18:35 UTC (permalink / raw) To: Alan Mackenzie; +Cc: 58343 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`. > 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. Stefan diff --git a/src/frame.c b/src/frame.c index 91b9bec82c3..24de0701d13 100644 --- a/src/frame.c +++ b/src/frame.c @@ -1503,17 +1503,7 @@ do_switch_frame (Lisp_Object frame, int for_deletion, Lisp_Object norecord) sf->select_mini_window_flag = MINI_WINDOW_P (XWINDOW (sf->selected_window)); - selected_frame = frame; - - move_minibuffers_onto_frame (sf, for_deletion); - - if (f->select_mini_window_flag - && !NILP (Fminibufferp (XWINDOW (f->minibuffer_window)->contents, Qt))) - f->selected_window = f->minibuffer_window; - f->select_mini_window_flag = false; - - if (! FRAME_MINIBUF_ONLY_P (XFRAME (selected_frame))) - last_nonminibuf_frame = XFRAME (selected_frame); + move_minibuffers_onto_frame (sf, frame, for_deletion); /* If the selected window in the target frame is its mini-window, we move to a different window, the most recently used one, unless there is a @@ -1528,6 +1518,16 @@ do_switch_frame (Lisp_Object frame, int for_deletion, Lisp_Object norecord) Fset_frame_selected_window (frame, w, Qnil); } + selected_frame = frame; + + if (f->select_mini_window_flag + && !NILP (Fminibufferp (XWINDOW (f->minibuffer_window)->contents, Qt))) + f->selected_window = f->minibuffer_window; + f->select_mini_window_flag = false; + + if (! FRAME_MINIBUF_ONLY_P (XFRAME (selected_frame))) + last_nonminibuf_frame = XFRAME (selected_frame); + Fselect_window (f->selected_window, norecord); /* We want to make sure that the next event generates a frame-switch @@ -2110,7 +2110,7 @@ delete_frame (Lisp_Object frame, Lisp_Object force) else /* Ensure any minibuffers on FRAME are moved onto the selected frame. */ - move_minibuffers_onto_frame (f, true); + move_minibuffers_onto_frame (f, selected_frame, true); /* Don't let echo_area_window to remain on a deleted frame. */ if (EQ (f->minibuffer_window, echo_area_window)) diff --git a/src/lisp.h b/src/lisp.h index 56f24d82810..5f6721595c0 100644 --- a/src/lisp.h +++ b/src/lisp.h @@ -4792,7 +4792,7 @@ fast_string_match_ignore_case (Lisp_Object regexp, Lisp_Object string) extern Lisp_Object Vminibuffer_list; extern Lisp_Object last_minibuf_string; -extern void move_minibuffers_onto_frame (struct frame *, bool); +extern void move_minibuffers_onto_frame (struct frame *, Lisp_Object, bool); extern bool is_minibuffer (EMACS_INT, Lisp_Object); extern EMACS_INT this_minibuffer_depth (Lisp_Object); extern EMACS_INT minibuf_level; diff --git a/src/minibuf.c b/src/minibuf.c index bedc5644807..aebd17c0b76 100644 --- a/src/minibuf.c +++ b/src/minibuf.c @@ -187,13 +187,15 @@ zip_minibuffer_stacks (Lisp_Object dest_window, Lisp_Object source_window) /* If `minibuffer_follows_selected_frame' is t, or we're about to delete a frame which potentially "contains" minibuffers, move them - from the old frame to the selected frame. This function is + from the old frame to the to-be-selected frame. This function is intended to be called from `do_switch_frame' in frame.c. OF is the - old frame, FOR_DELETION is true if OF is about to be deleted. */ + old frame, SF is the to-be-selected frame, and FOR_DELETION is true + if OF is about to be deleted. */ void -move_minibuffers_onto_frame (struct frame *of, bool for_deletion) +move_minibuffers_onto_frame (struct frame *of, Lisp_Object sf, + bool for_deletion) { - struct frame *f = XFRAME (selected_frame); + struct frame *f = XFRAME (sf); minibuf_window = f->minibuffer_window; if (!(minibuf_level @@ -206,7 +208,7 @@ move_minibuffers_onto_frame (struct frame *of, bool for_deletion) { zip_minibuffer_stacks (f->minibuffer_window, of->minibuffer_window); if (for_deletion && XFRAME (MB_frame) != of) - MB_frame = selected_frame; + MB_frame = sf; } } ^ permalink raw reply related [flat|nested] 5+ messages in thread
* bug#58343: 29.0.50; ELisp code run in "inconsitent" selected-window state 2022-10-12 18:35 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-10-12 19:13 ` Alan Mackenzie 2022-10-12 21:07 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 5+ messages in thread From: Alan Mackenzie @ 2022-10-12 19:13 UTC (permalink / raw) To: Stefan Monnier; +Cc: acm, 58343 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). ^ permalink raw reply [flat|nested] 5+ messages in thread
* bug#58343: 29.0.50; ELisp code run in "inconsitent" selected-window state 2022-10-12 19:13 ` Alan Mackenzie @ 2022-10-12 21:07 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 0 replies; 5+ messages in thread From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2022-10-12 21:07 UTC (permalink / raw) To: Alan Mackenzie; +Cc: 58343 > Just one small thing: the name sf is a bad name for a Lisp_Object - Very good point, thanks. > 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. ;-) Did you install a camera in my office? I pushed the updated code, thank you, Stefan ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-10-12 21:07 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-07 0:06 bug#58343: 29.0.50; ELisp code run in "inconsitent" selected-window state Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2022-10-12 18:08 ` Alan Mackenzie 2022-10-12 18:35 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2022-10-12 19:13 ` Alan Mackenzie 2022-10-12 21:07 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.