bug#52491: 28.0.90; Regression in window deletion with minibuffer

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[Juri Linkov]

emacs-28 -Q

 M-x	    ;; execute-extended-command
 <tab>	    ;; minibuffer-complete
 <kp-prior> ;; switch-to-completions
 q	    ;; quit-window

Point moves to the *scratch* buffer.  This is a regression.
In emacs-27 it moved to the minibuffer.

The problem is that delete-window now uses get-mru-window
to get the window to select after deleting the completions window.
But get-mru-window ignores the minibuffer window.
This could be fixed by:

diff --git a/lisp/window.el b/lisp/window.el
index 0f17bb28b4..507eaab705 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -2556,7 +2556,7 @@ get-mru-window
 Any other value of ALL-FRAMES means consider all windows on the
 selected frame and no others."
    (let (best-window best-time time)
-    (dolist (window (window-list-1 nil 'nomini all-frames))
+    (dolist (window (window-list-1 nil nil all-frames))
       (setq time (window-use-time window))
       (when (and (or dedicated (not (window-dedicated-p window)))
 		 (or (not not-selected) (not (eq window (selected-window))))

Maybe better to add a new argument to get-mru-window
and pass it down to window-list-1.

This problem exists for the default value 'mru' of
delete-window-choose-selected.  When its value is customized
to 'pos' then the *scratch* buffer is selected too,
but for a different reason.  In this case, the *scratch* buffer
is selected instead of the minibuffer because frame coordinates
of point that was previously in the completions window
now belongs to the window of the *scratch* buffer
that now takes screen space previously used by completions.
To prevent this, maybe completion-setup-function should set
buffer-local delete-window-choose-selected to 'mru'
in the completions buffer:

diff --git a/lisp/simple.el b/lisp/simple.el
index 0a58297bd2..08932c689a 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9256,6 +9278,7 @@ completion-setup-function
         (setq-local completion-base-position base-position)
         (setq-local completion-list-insert-choice-function insert-fun))
       (setq-local completion-reference-buffer mainbuf)
+      (setq-local delete-window-choose-selected 'mru)
       (if base-dir (setq default-directory base-dir))
       (when completion-tab-width
         (setq tab-width completion-tab-width))
diff --git a/lisp/window.el b/lisp/window.el
index 0f17bb28b4..507eaab705 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -4205,6 +4205,7 @@ delete-window
   (let* ((frame (window-frame window))
 	 (function (window-parameter window 'delete-window))
 	 (parent (window-parent window))
+	 (choose-selected delete-window-choose-selected)
 	 atom-root)
     (window--check frame)
     (catch 'done
@@ -4255,7 +4256,7 @@ delete-window
 	  ;; Can't do without resizing fixed-size windows.
 	  (window--resize-siblings window (- size) horizontal t)))
 
-        (when (eq delete-window-choose-selected 'pos)
+        (when (eq choose-selected 'pos)
           ;; Remember edges and position of point of the selected window
           ;; of WINDOW'S frame.
           (setq frame-selected-window-edges
@@ -4289,7 +4290,7 @@ delete-window
                       ;; Select window at WINDOW's position at point.
 	              (set-frame-selected-window
                        frame new-frame-selected-window)))))
-         ((and (eq delete-window-choose-selected 'mru)
+         ((and (eq choose-selected 'mru)
                ;; Try to use the most recently used window.
                (let ((mru-window (get-mru-window frame nil nil t)))
                  (and mru-window
-- 

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[Juri Linkov]

>  M-x	    ;; execute-extended-command
>  <tab>	    ;; minibuffer-complete
>  <kp-prior> ;; switch-to-completions
>  q	    ;; quit-window
>
> Point moves to the *scratch* buffer.  This is a regression.
> In emacs-27 it moved to the minibuffer.
>
> The problem is that delete-window now uses get-mru-window
> to get the window to select after deleting the completions window.
> But get-mru-window ignores the minibuffer window.

There is another problem:

 M-x	    ;; execute-extended-command
 <kp-prior> ;; switch-to-completions
 C--	    ;; negative-argument
 C-x o	    ;; other-window
 C-x o	    ;; other-window
 q	    ;; quit-window

After switching to the *scratch* buffer with ‘C-- C-x o’, and back to
the *Completions* buffer with ‘C-x o’, then ‘q’ (quit-window) selects
the *scratch* window because it's the most-recently-used window.
But in 27.2 it selected the minibuffer window.

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Thu, 16 Dec 2021 19:28:30 +0200
> 
> There is another problem:
> 
>  M-x	    ;; execute-extended-command
>  <kp-prior> ;; switch-to-completions
>  C--	    ;; negative-argument
>  C-x o	    ;; other-window
>  C-x o	    ;; other-window
>  q	    ;; quit-window
> 
> After switching to the *scratch* buffer with ‘C-- C-x o’, and back to
> the *Completions* buffer with ‘C-x o’, then ‘q’ (quit-window) selects
> the *scratch* window because it's the most-recently-used window.
> But in 27.2 it selected the minibuffer window.

And the patch you proposed earlier doesn't fix this other problem?

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[Juri Linkov]

>> There is another problem:
>> 
>>  M-x	    ;; execute-extended-command
>>  <kp-prior> ;; switch-to-completions
>>  C--	    ;; negative-argument
>>  C-x o	    ;; other-window
>>  C-x o	    ;; other-window
>>  q	    ;; quit-window
>> 
>> After switching to the *scratch* buffer with ‘C-- C-x o’, and back to
>> the *Completions* buffer with ‘C-x o’, then ‘q’ (quit-window) selects
>> the *scratch* window because it's the most-recently-used window.
>> But in 27.2 it selected the minibuffer window.
>
> And the patch you proposed earlier doesn't fix this other problem?

Unfortunately, it doesn't fix this, since 28.0.90 uses mru,
but 27.2 didn't.

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 52491@debbugs.gnu.org
> Date: Thu, 16 Dec 2021 21:02:05 +0200
> 
> >> There is another problem:
> >> 
> >>  M-x	    ;; execute-extended-command
> >>  <kp-prior> ;; switch-to-completions
> >>  C--	    ;; negative-argument
> >>  C-x o	    ;; other-window
> >>  C-x o	    ;; other-window
> >>  q	    ;; quit-window
> >> 
> >> After switching to the *scratch* buffer with ‘C-- C-x o’, and back to
> >> the *Completions* buffer with ‘C-x o’, then ‘q’ (quit-window) selects
> >> the *scratch* window because it's the most-recently-used window.
> >> But in 27.2 it selected the minibuffer window.
> >
> > And the patch you proposed earlier doesn't fix this other problem?
> 
> Unfortunately, it doesn't fix this, since 28.0.90 uses mru,
> but 27.2 didn't.

FWIW, I don't see any catastrophe here: whoever plays such games with
switching windows during completion should know what they are doing,
and how to switch back to the minibuffer.

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[Juri Linkov]

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

>> > And the patch you proposed earlier doesn't fix this other problem?
>> 
>> Unfortunately, it doesn't fix this, since 28.0.90 uses mru,
>> but 27.2 didn't.
>
> FWIW, I don't see any catastrophe here: whoever plays such games with
> switching windows during completion should know what they are doing,
> and how to switch back to the minibuffer.

Anyway, here is the patch for the release branch
that fixes the previous regression:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: delete-window.patch --]
[-- Type: text/x-diff, Size: 3708 bytes --]

diff --git a/lisp/window.el b/lisp/window.el
index 0f17bb28b4..0a422b4305 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -2531,14 +2531,15 @@ get-lru-window
 	    (setq best-window window)))))
     (or best-window second-best-window)))
 
-(defun get-mru-window (&optional all-frames dedicated not-selected no-other)
+(defun get-mru-window (&optional all-frames dedicated not-selected no-other minibuf)
    "Return the most recently used window on frames specified by ALL-FRAMES.
-A minibuffer window is never a candidate.  A dedicated window is
-never a candidate unless DEDICATED is non-nil, so if all windows
-are dedicated, the value is nil.  Optional argument NOT-SELECTED
-non-nil means never return the selected window.  Optional
+A dedicated window is never a candidate unless DEDICATED is non-nil,
+so if all windows are dedicated, the value is nil.  Optional argument
+NOT-SELECTED non-nil means never return the selected window.  Optional
 argument NO-OTHER non-nil means to never return a window whose
-'no-other-window' parameter is non-nil.
+'no-other-window' parameter is non-nil.  A minibuffer window is never
+a candidate when MINIBUF is nil or omitted.  MINIBUF t means consider
+the minibuffer window only if the minibuffer is active.
 
 The following non-nil values of the optional argument ALL-FRAMES
 have special meanings:
@@ -2556,7 +2557,7 @@ get-mru-window
 Any other value of ALL-FRAMES means consider all windows on the
 selected frame and no others."
    (let (best-window best-time time)
-    (dolist (window (window-list-1 nil 'nomini all-frames))
+    (dolist (window (window-list-1 nil (unless minibuf 'nomini) all-frames))
       (setq time (window-use-time window))
       (when (and (or dedicated (not (window-dedicated-p window)))
 		 (or (not not-selected) (not (eq window (selected-window))))
@@ -4205,6 +4206,7 @@ delete-window
   (let* ((frame (window-frame window))
 	 (function (window-parameter window 'delete-window))
 	 (parent (window-parent window))
+	 (choose-selected delete-window-choose-selected)
 	 atom-root)
     (window--check frame)
     (catch 'done
@@ -4255,7 +4257,7 @@ delete-window
 	  ;; Can't do without resizing fixed-size windows.
 	  (window--resize-siblings window (- size) horizontal t)))
 
-        (when (eq delete-window-choose-selected 'pos)
+        (when (eq choose-selected 'pos)
           ;; Remember edges and position of point of the selected window
           ;; of WINDOW'S frame.
           (setq frame-selected-window-edges
@@ -4289,9 +4291,9 @@ delete-window
                       ;; Select window at WINDOW's position at point.
 	              (set-frame-selected-window
                        frame new-frame-selected-window)))))
-         ((and (eq delete-window-choose-selected 'mru)
+         ((and (eq choose-selected 'mru)
                ;; Try to use the most recently used window.
-               (let ((mru-window (get-mru-window frame nil nil t)))
+               (let ((mru-window (get-mru-window frame nil nil t t)))
                  (and mru-window
 	              (set-frame-selected-window frame mru-window)))))
          ((and (window-parameter
diff --git a/lisp/simple.el b/lisp/simple.el
index 84928caa31..73c5840e02 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -9252,6 +9278,7 @@ completion-setup-function
         (setq-local completion-base-position base-position)
         (setq-local completion-list-insert-choice-function insert-fun))
       (setq-local completion-reference-buffer mainbuf)
+      (setq-local delete-window-choose-selected 'mru)
       (if base-dir (setq default-directory base-dir))
       (when completion-tab-width
         (setq tab-width completion-tab-width))

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 52491@debbugs.gnu.org
> Date: Fri, 17 Dec 2021 10:25:52 +0200
> 
> Anyway, here is the patch for the release branch
> that fixes the previous regression:

Thanks.

Martin, any comments on this for the release branch?  It looks
non-trivial to me, and delete-window is a low-level API.

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[martin rudalics]

 > Martin, any comments on this for the release branch?  It looks
 > non-trivial to me, and delete-window is a low-level API.

The bug is a regression so it should be fixed.  Whether and how Juri's
fix can affect other operations that use the minibuffer before deleting
some window is beyond my imagination.

I do not understand two things: It seems that this part of the
doc-string

   A minibuffer window is never
   a candidate when MINIBUF is nil or omitted.  MINIBUF t means consider
   the minibuffer window only if the minibuffer is active.

does not match well this part of the code

     (dolist (window (window-list-1 nil (unless minibuf 'nomini) all-frames))

And this (are we sure that we always want to do it when "setting up"
completions?)

       (setq-local delete-window-choose-selected 'mru)

apparently makes this binding

	 (choose-selected delete-window-choose-selected)

necessary.  But this would imply that at the time 'delete-window' is
called, its buffer is current which is by no means guaranteed.  So the
latter form should probably become

        (choose-selected
	(buffer-local-value
	 'delete-window-choose-selected (window-buffer window))))

martin

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[Juri Linkov]

> The bug is a regression so it should be fixed.  Whether and how Juri's
> fix can affect other operations that use the minibuffer before deleting
> some window is beyond my imagination.

Maybe it would be possible to change the default behavior of delete-window
to do what it did in Emacs 27?  But honestly, I don't understand how
this old code managed to select the minibuffer after deleting the
completions buffer and why its behavior differs after it was moved to Lisp:

-	  /* Now look whether `get-mru-window' gets us something.  */
-	  mru_window = call1 (Qget_mru_window, frame);
-	  if (WINDOW_LIVE_P (mru_window)
-	      && EQ (XWINDOW (mru_window)->frame, frame))
-	    new_selected_window = mru_window;
-
-	  /* If all ended up well, we now promote the mru window.  */
-	  if (EQ (FRAME_SELECTED_WINDOW (f), selected_window))
-	    Fselect_window (new_selected_window, Qnil);
-	  else
-	    fset_selected_window (f, new_selected_window);

> I do not understand two things: It seems that this part of the
> doc-string
>
>   A minibuffer window is never
>   a candidate when MINIBUF is nil or omitted.  MINIBUF t means consider
>   the minibuffer window only if the minibuffer is active.
>
> does not match well this part of the code
>
>     (dolist (window (window-list-1 nil (unless minibuf 'nomini) all-frames))

When MINIBUF is nil, ‘(unless minibuf 'nomini)’ is ‘'nomini’,
when MINIBUF is non-nil, ‘(unless minibuf 'nomini)’ is ‘nil’,
but the third possible value MINIBUF=‘t’ of window-list-1
is not used here.  This value means consider the minibuffer window
even if the minibuffer is not active.  It seems this value is useless
in get-mru-window for the purposes of delete-window?

> And this (are we sure that we always want to do it when "setting up"
> completions?)

Another place would be completion-list-mode, but it's empty.

>       (setq-local delete-window-choose-selected 'mru)
>
> apparently makes this binding
>
> 	 (choose-selected delete-window-choose-selected)
>
> necessary.  But this would imply that at the time 'delete-window' is
> called, its buffer is current which is by no means guaranteed.  So the
> latter form should probably become
>
>        (choose-selected
> 	(buffer-local-value
> 	 'delete-window-choose-selected (window-buffer window))))

‘buffer-local-value’ would be more correct, thanks.

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[martin rudalics]

 > Maybe it would be possible to change the default behavior of delete-window
 > to do what it did in Emacs 27?  But honestly, I don't understand how
 > this old code managed to select the minibuffer after deleting the
 > completions buffer and why its behavior differs after it was moved to Lisp:
 >
 > -	  /* Now look whether `get-mru-window' gets us something.  */
 > -	  mru_window = call1 (Qget_mru_window, frame);
 > -	  if (WINDOW_LIVE_P (mru_window)
 > -	      && EQ (XWINDOW (mru_window)->frame, frame))
 > -	    new_selected_window = mru_window;
 > -
 > -	  /* If all ended up well, we now promote the mru window.  */
 > -	  if (EQ (FRAME_SELECTED_WINDOW (f), selected_window))
 > -	    Fselect_window (new_selected_window, Qnil);
 > -	  else
 > -	    fset_selected_window (f, new_selected_window);

I doubt that anything has changed here - the Emacs 27 code didn't select
the minibuffer window either.  What has changed, however, is the value
of the 'quit-restore' parameter of the *Completions* window.  With Emacs
27 it was a a quadruple whose third element was the minibuffer window
which subsequently got selected.  With Emacs 28 it is nil.

martin

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[Juri Linkov]

>> Maybe it would be possible to change the default behavior of delete-window
>> to do what it did in Emacs 27?  But honestly, I don't understand how
>> this old code managed to select the minibuffer after deleting the
>> completions buffer and why its behavior differs after it was moved to Lisp:
>>
>> -	  /* Now look whether `get-mru-window' gets us something.  */
>> -	  mru_window = call1 (Qget_mru_window, frame);
>> -	  if (WINDOW_LIVE_P (mru_window)
>> -	      && EQ (XWINDOW (mru_window)->frame, frame))
>> -	    new_selected_window = mru_window;
>
> I doubt that anything has changed here - the Emacs 27 code didn't select
> the minibuffer window either.  What has changed, however, is the value
> of the 'quit-restore' parameter of the *Completions* window.  With Emacs
> 27 it was a a quadruple whose third element was the minibuffer window
> which subsequently got selected.  With Emacs 28 it is nil.

I checked that the value is the same in 27 and 28.  But while comparing
quit-restore-window in 27 and 28 indeed there are many differences.

In 27, quit-restore-window calls window--delete, and then later does
`(select-window (nth 2 quit-restore))'.

But in 28, for the dedicated *Completions* window it calls
window--delete, but doesn't call `(select-window (nth 2 quit-restore))'
afterwards, that is another `cond' branch.

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[martin rudalics]

 > I checked that the value is the same in 27 and 28.

Not here.  If with

(setq enable-recursive-minibuffers t)

followed by M-x, TAB, kp-prior and

M-: (window-parameter nil 'quit-restore)

Emacs 27.2.50 gets me

(window window #<window 4 on  *Minibuf-1*> #<buffer *Completions*>)

while Emacs 28.0.90 gets me nil.

 > But while comparing
 > quit-restore-window in 27 and 28 indeed there are many differences.
 >
 > In 27, quit-restore-window calls window--delete, and then later does
 > `(select-window (nth 2 quit-restore))'.
 >
 > But in 28, for the dedicated *Completions* window it calls
 > window--delete, but doesn't call `(select-window (nth 2 quit-restore))'
 > afterwards, that is another `cond' branch.

All these are easily explained by my observation above.  And, with Emacs
27 the *Completions* window is softly dedicated to its buffer here while
with Emacs 28 it is not dedicated to its buffer.  Please recheck.

martin

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[Juri Linkov]

>> I checked that the value is the same in 27 and 28.
>
> Not here.  If with
>
> (setq enable-recursive-minibuffers t)
>
> followed by M-x, TAB, kp-prior and
>
> M-: (window-parameter nil 'quit-restore)
>
> Emacs 27.2.50 gets me
>
> (window window #<window 4 on  *Minibuf-1*> #<buffer *Completions*>)
>
> while Emacs 28.0.90 gets me nil.

I get nil too, but I don't believe this value.
Maybe because of some strange window interactions,
the displayed value is wrong.  When I tried to print
the real value with 'message' in the *Messages* value,
it was the same as in 27.2.90.

>> But while comparing
>> quit-restore-window in 27 and 28 indeed there are many differences.
>>
>> In 27, quit-restore-window calls window--delete, and then later does
>> `(select-window (nth 2 quit-restore))'.
>>
>> But in 28, for the dedicated *Completions* window it calls
>> window--delete, but doesn't call `(select-window (nth 2 quit-restore))'
>> afterwards, that is another `cond' branch.
>
> All these are easily explained by my observation above.  And, with Emacs
> 27 the *Completions* window is softly dedicated to its buffer here while
> with Emacs 28 it is not dedicated to its buffer.  Please recheck.

I checked that in 28 in 'quit-restore-window', only these 2 lines are used:

     ((and dedicated (not (eq dedicated 'side))
           (window--delete window 'dedicated (eq bury-or-kill 'kill))))

but these lines are not used in 28:

      (when (window-live-p (nth 2 quit-restore))
	(select-window (nth 2 quit-restore)))

But in 27 they correctly selected the minibuffer window.

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[martin rudalics]

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

 > I get nil too, but I don't believe this value.
 > Maybe because of some strange window interactions,
 > the displayed value is wrong.  When I tried to print
 > the real value with 'message' in the *Messages* value,
 > it was the same as in 27.2.90.

Well then ...

 > I checked that in 28 in 'quit-restore-window', only these 2 lines are used:
 >
 >       ((and dedicated (not (eq dedicated 'side))
 >             (window--delete window 'dedicated (eq bury-or-kill 'kill))))
 >
 > but these lines are not used in 28:
 >
 >        (when (window-live-p (nth 2 quit-restore))
 > 	(select-window (nth 2 quit-restore)))
 >
 > But in 27 they correctly selected the minibuffer window.

... the attached patch should cure it (I suppose this got broken in
commit d0c7d8bc22a935f2a79747a96b4043f0b449a212 but didn't check).

martin

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: quit-restore.diff --]
[-- Type: text/x-patch; name="quit-restore.diff", Size: 2368 bytes --]

diff --git a/lisp/window.el b/lisp/window.el
index d12232641e..0db2dcbb90 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -5171,10 +5171,7 @@ quit-restore-window
      ((and (not prev-buffer)
 	   (eq (nth 1 quit-restore) 'tab)
 	   (eq (nth 3 quit-restore) buffer))
-      (tab-bar-close-tab)
-      ;; If the previously selected window is still alive, select it.
-      (when (window-live-p (nth 2 quit-restore))
-	(select-window (nth 2 quit-restore))))
+      (tab-bar-close-tab))
      ((and (not prev-buffer)
 	   (or (eq (nth 1 quit-restore) 'frame)
 	       (and (eq (nth 1 quit-restore) 'window)
@@ -5184,10 +5181,7 @@ quit-restore-window
 		    (not (eq window (frame-root-window window)))))
 	   (eq (nth 3 quit-restore) buffer)
 	   ;; Delete WINDOW if possible.
-	   (window--delete window nil (eq bury-or-kill 'kill)))
-      ;; If the previously selected window is still alive, select it.
-      (when (window-live-p (nth 2 quit-restore))
-	(select-window (nth 2 quit-restore))))
+	   (window--delete window nil (eq bury-or-kill 'kill))))
      ((and (listp (setq quad (nth 1 quit-restore)))
 	   (buffer-live-p (car quad))
 	   (eq (nth 3 quit-restore) buffer))
@@ -5229,10 +5223,7 @@ quit-restore-window
 	(set-window-prev-buffers
 	 window (append (window-prev-buffers window) (list entry))))
       ;; Reset the quit-restore parameter.
-      (set-window-parameter window 'quit-restore nil)
-      ;; Select old window.
-      (when (window-live-p (nth 2 quit-restore))
-	(select-window (nth 2 quit-restore))))
+      (set-window-parameter window 'quit-restore nil))
      (t
       ;; Show some other buffer in WINDOW and reset the quit-restore
       ;; parameter.
@@ -5244,9 +5235,11 @@ quit-restore-window
       (if (switch-to-prev-buffer window bury-or-kill)
           (when (eq dedicated 'side)
             (set-window-dedicated-p window 'side))
-        (window--delete window nil (eq bury-or-kill 'kill))
-        (when (window-live-p (nth 2 quit-restore))
-          (select-window (nth 2 quit-restore))))))
+        (window--delete window nil (eq bury-or-kill 'kill)))))
+
+    ;; When the previously selected window is still live, select it.
+    (when (window-live-p (nth 2 quit-restore))
+      (select-window (nth 2 quit-restore)))

     ;; Deal with the buffer.
     (cond

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[Eli Zaretskii]

> Cc: Eli Zaretskii <eliz@gnu.org>, 52491@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Mon, 20 Dec 2021 10:18:53 +0100
> 
>  > I checked that in 28 in 'quit-restore-window', only these 2 lines are used:
>  >
>  >       ((and dedicated (not (eq dedicated 'side))
>  >             (window--delete window 'dedicated (eq bury-or-kill 'kill))))
>  >
>  > but these lines are not used in 28:
>  >
>  >        (when (window-live-p (nth 2 quit-restore))
>  > 	(select-window (nth 2 quit-restore)))
>  >
>  > But in 27 they correctly selected the minibuffer window.
> 
> ... the attached patch should cure it (I suppose this got broken in
> commit d0c7d8bc22a935f2a79747a96b4043f0b449a212 but didn't check).

Thanks.

Do you think this is safe enough to install on the release branch?

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[martin rudalics]

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

 > Do you think this is safe enough to install on the release branch?

Probably not.  I'm confused as to which posts made it today to the list.
A correct patch is attached now and should be safe for Emacs 28.

martin


[-- Attachment #2: quit-restore.diff --]
[-- Type: text/x-patch, Size: 680 bytes --]

diff --git a/lisp/window.el b/lisp/window.el
index d12232641e..901bc0fc4d 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -5167,7 +5167,10 @@ quit-restore-window
     (cond
      ;; First try to delete dedicated windows that are not side windows.
      ((and dedicated (not (eq dedicated 'side))
-           (window--delete window 'dedicated (eq bury-or-kill 'kill))))
+           (window--delete window 'dedicated (eq bury-or-kill 'kill))
+           (or (and (window-live-p (nth 2 quit-restore))
+	            (select-window (nth 2 quit-restore)))
+               t)))
      ((and (not prev-buffer)
 	   (eq (nth 1 quit-restore) 'tab)
 	   (eq (nth 3 quit-restore) buffer))

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[Eli Zaretskii]

> Cc: juri@linkov.net, 52491@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> Date: Mon, 20 Dec 2021 19:08:40 +0100
> 
>  > Do you think this is safe enough to install on the release branch?
> 
> Probably not.  I'm confused as to which posts made it today to the list.

Yes, the list was in disarray today, because of connectivity problems
gnu.org machines had a few hours ago.

> A correct patch is attached now and should be safe for Emacs 28.

Thanks, please install on the release branch.

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[Juri Linkov]

>> Do you think this is safe enough to install on the release branch?
>
> Probably not.  I'm confused as to which posts made it today to the list.
> A correct patch is attached now and should be safe for Emacs 28.
>
> @@ -5167,7 +5167,10 @@ quit-restore-window
>      (cond
>       ;; First try to delete dedicated windows that are not side windows.
>       ((and dedicated (not (eq dedicated 'side))
> -           (window--delete window 'dedicated (eq bury-or-kill 'kill))))
> +           (window--delete window 'dedicated (eq bury-or-kill 'kill))
> +           (or (and (window-live-p (nth 2 quit-restore))
> +	            (select-window (nth 2 quit-restore)))
> +               t)))

Or maybe simply:

     ((and dedicated (not (eq dedicated 'side))
           (window--delete window 'dedicated (eq bury-or-kill 'kill)))
      ;; If the previously selected window is still alive, select it.
      (when (window-live-p (nth 2 quit-restore))
        (select-window (nth 2 quit-restore))))

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[martin rudalics]

 > Or maybe simply:
 >
 >       ((and dedicated (not (eq dedicated 'side))
 >             (window--delete window 'dedicated (eq bury-or-kill 'kill)))
 >        ;; If the previously selected window is still alive, select it.
 >        (when (window-live-p (nth 2 quit-restore))
 >          (select-window (nth 2 quit-restore))))

Right.  Please install that.

martin

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[Juri Linkov]

close 52491 28.0.90
thanks

>> Or maybe simply:
>>
>>       ((and dedicated (not (eq dedicated 'side))
>>             (window--delete window 'dedicated (eq bury-or-kill 'kill)))
>>        ;; If the previously selected window is still alive, select it.
>>        (when (window-live-p (nth 2 quit-restore))
>>          (select-window (nth 2 quit-restore))))
>
> Right.  Please install that.

Thanks, now pushed to emacs-28.

BTW, could you suggest where to look to fix such problem: for example,
create a grep/occur buffer, then start visiting occurrences from it
one by one, e.g. type RET that shows the occurrence in the second window,
then select the first window again, then type RET again to visit the
next file in the second window, etc.  Then invoke 'quit-window' in the
last visited buffer - it selects the first window.  Then select the
previous buffer in the second window, and again invoke 'quit-window'.
Now it removes the buffer, but doesn't select the first window.

Such inconsistency causes a lot of trouble because of this
unpredictable behavior.  It reselects another window only
on the first invocation of 'quit-window' in the same window.

Would it be possible either to always select another window
on every 'quit-window', or to never select another window
on 'quit-window'?

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[martin rudalics]

 > BTW, could you suggest where to look to fix such problem: for example,
 > create a grep/occur buffer, then start visiting occurrences from it
 > one by one, e.g. type RET that shows the occurrence in the second window,
 > then select the first window again, then type RET again to visit the
 > next file in the second window, etc.  Then invoke 'quit-window' in the
 > last visited buffer - it selects the first window.  Then select the
 > previous buffer in the second window, and again invoke 'quit-window'.
 > Now it removes the buffer, but doesn't select the first window.
 >
 > Such inconsistency causes a lot of trouble because of this
 > unpredictable behavior.  It reselects another window only
 > on the first invocation of 'quit-window' in the same window.
 >
 > Would it be possible either to always select another window
 > on every 'quit-window', or to never select another window
 > on 'quit-window'?

I would need a precise recipe with Emacs -Q to look into this.  I never
use *grep* buffers and when trying it in a one-window frame showing
*grep*, it pops up a new window for the first occurrence, reuses that
window for further occurrences and deletes that window when invoking
'quit-window'.

martin

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[Juri Linkov]

>> I get nil too, but I don't believe this value.
>> Maybe because of some strange window interactions,
>> the displayed value is wrong.  When I tried to print
>> the real value with 'message' in the *Messages* value,
>> it was the same as in 27.2.90.
>
> Well then ...

It's very strange that evaluating in the *Completions* buffer with

  M-: (selected-window)

shows that the selected window is the minibuffer window.

Do you have an idea what is going on?

> ... the attached patch should cure it (I suppose this got broken in
> commit d0c7d8bc22a935f2a79747a96b4043f0b449a212 but didn't check).

Thanks, then this patch should be pushed to master,
and another shorter patch to the release branch?

bug#52491: 28.0.90; Regression in window deletion with minibuffer

[martin rudalics]

 > It's very strange that evaluating in the *Completions* buffer with
 >
 >    M-: (selected-window)
 >
 > shows that the selected window is the minibuffer window.
 >
 > Do you have an idea what is going on?

Not yet.  I suspect select_mini_window_flag is somehow involved here but
I cannot see how it could possibly affect 'selected-window'.

 >> ... the attached patch should cure it (I suppose this got broken in
 >> commit d0c7d8bc22a935f2a79747a96b4043f0b449a212 but didn't check).
 >
 > Thanks, then this patch should be pushed to master,
 > and another shorter patch to the release branch?

No.  This patch might get the "t" case wrong - there we do not
necessarily want to select the previous window.

martin