bug#48493: 28.0.50; quit-window doesn't work

[pillule <pillule@riseup.net>]

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


martin rudalics <rudalics@gmx.at> writes:

> The dedicated flag in a side window serves to prevent "normal" buffer
> display to "avoid" that window.  A side window may be reused for showing
> another buffer only by `display-buffer-in-side-window'.  This sense of
> dedication is somewhat different from the normal sense where killing the
> buffer always attempts to delete its dedicated windows (and maybe their
> frames too) first.
>
> Hence it is perfectly valid to switch to the previous buffer here - any
> such buffer was meant to be displayed in that side window.  It would be,
> however, invalid to switch to some buffer that was never shown in that
> window here.  Note in this context that we can always delete a side
> window, a side window can never be alone on its frame.

Yes.

> Tell me whether this works with DOOM's `kill-buffer-hook'.

I can test the changes against a version of DOOM, yes. For the draft below
it seems to be ok, but keep in mind that their library bypass these parts
window.el

> If you feel that it's more natural to delete the window in the case at
> hand, we can consider that too.

Not at all. For me it is ok with switch-to-prev-buffer, if users want to delete
the window and/or buffer explicitly, they have commands for that. In the case of
DOOM it is implemented as a workaround against some bugs, it is
explicated in :

(+popup/quit-window)
Documentation
The regular quit-window sometimes kills the popup buffer and switches to a
buffer that shouldn't be in a popup. We prevent that by remapping quit-window
to this commmand.


So here is the *draft* that pass the previous cases of this thread.
`replace-buffer-in-windows' take care of killing buffers.
I restore the dedication of side-window because :
1. it seems to me it is the right think to do, and it prevents 2.
2. I lost the trace when we kill a buffer and no previous-buffer is found
but  still an undesirable buffer replace the current,
is it in the C part ? I can't inspect C...


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: better handling of side windows --]
[-- Type: text/x-diff, Size: 5414 bytes --]

From 8305210da9e2215a8b38cd5cf88cd4c6c856fa0b Mon Sep 17 00:00:00 2001
From: Trust me I am a doctor <pillule@riseup.net>
Date: Wed, 26 May 2021 15:40:39 +0200
Subject: [PATCH] Better handling of side-windows

* lisp/window.el
   (switch-to-prev-buffer) : Return nil when prev-buffer unavailable
   (switch-to-next-buffer) : Return nil when next-buffer unavailable
   (replace-buffer-in-windows) : Does not systematically delete
side-windows, instead try to `switch-to-prev-buffer' and delete the
window only if prev-buffer was unavailable
   (quit-restore-window) : Add exceptions for side-windows, so it
try to to `switch-to-prev-buffer' and delete the window only if
prev-buffer was unavailable (bug#48493)
---
 lisp/window.el | 38 +++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/lisp/window.el b/lisp/window.el
index fd1c617d6b..33f4409fcf 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -4559,11 +4559,11 @@ This function is called by `prev-buffer'."
       ;; Scan WINDOW's previous buffers first, skipping entries of next
       ;; buffers.
       (dolist (entry (window-prev-buffers window))
-	(when (and (setq new-buffer (car entry))
+	(when (and (not (eq (car entry) old-buffer))
+                   (setq new-buffer (car entry))
 		   (or (buffer-live-p new-buffer)
 		       (not (setq killed-buffers
 				  (cons new-buffer killed-buffers))))
-		   (not (eq new-buffer old-buffer))
                    (or (null pred) (funcall pred new-buffer))
 		   ;; When BURY-OR-KILL is nil, avoid switching to a
 		   ;; buffer in WINDOW's next buffers list.
@@ -4726,11 +4726,11 @@ This function is called by `next-buffer'."
       ;; Scan WINDOW's reverted previous buffers last (must not use
       ;; nreverse here!)
       (dolist (entry (reverse (window-prev-buffers window)))
-	(when (and (setq new-buffer (car entry))
+	(when (and (not (eq (car entry) old-buffer))
+                   (setq new-buffer (car entry))
 		   (or (buffer-live-p new-buffer)
 		       (not (setq killed-buffers
 				  (cons new-buffer killed-buffers))))
-		   (not (eq new-buffer old-buffer))
                    (or (null pred) (funcall pred new-buffer)))
           (if (switch-to-prev-buffer-skip-p skip window new-buffer)
 	      (setq skipped (or skipped new-buffer))
@@ -4989,10 +4989,14 @@ all window-local buffer lists."
   (let ((buffer (window-normalize-buffer buffer-or-name)))
     (dolist (window (window-list-1 nil nil t))
       (if (eq (window-buffer window) buffer)
-	  (unless (window--delete window t t)
-	    ;; Switch to another buffer in window.
-	    (set-window-dedicated-p window nil)
-	    (switch-to-prev-buffer window 'kill))
+	  (let ((dedicated-side (eq (window-dedicated-p window) 'side)))
+            ;; Try to delete the window, with the exception of side-windows
+            (when (or dedicated-side (not (window--delete window t t)))
+              ;; Switch to another buffer in window.
+              (set-window-dedicated-p window nil)
+              (if (switch-to-prev-buffer window 'kill)
+                  (and dedicated-side (set-window-dedicated-p window 'side))
+                (window--delete window nil 'kill))))
 	;; Unrecord BUFFER in WINDOW.
 	(unrecord-window-buffer window buffer)))))
 
@@ -5040,6 +5044,7 @@ nil means to not handle the buffer in a particular way.  This
                         (dolist (buf (window-prev-buffers window))
                           (unless (eq (car buf) buffer)
                             (throw 'prev-buffer (car buf))))))
+         (dedicated (window-dedicated-p window))
 	 quad entry)
     (cond
      ((and (not prev-buffer)
@@ -5084,6 +5089,8 @@ nil means to not handle the buffer in a particular way.  This
       ;; Restore WINDOW's previous buffer, start and point position.
       (set-window-buffer-start-and-point
        window (nth 0 quad) (nth 1 quad) (nth 2 quad))
+      ;; Preserve the side-window dedication
+      (when (eq dedicated 'side) (set-window-dedicated-p window 'side))
       ;; Deal with the buffer we just removed from WINDOW.
       (setq entry (and (eq bury-or-kill 'append)
 		       (assq buffer (window-prev-buffers window))))
@@ -5110,9 +5117,18 @@ nil means to not handle the buffer in a particular way.  This
       (set-window-parameter window 'quit-restore nil)
       ;; Make sure that WINDOW is no more dedicated.
       (set-window-dedicated-p window nil)
-      (unless (switch-to-prev-buffer window bury-or-kill)
-        ;; Delete WINDOW if there is no previous buffer (Bug#48367).
-        (window--delete window nil (eq bury-or-kill 'kill)))))
+      (if (and prev-buffer (eq dedicated 'side))
+          ;; If a previous buffer exists, try to switch to it and
+          ;; to preserve the side-window dedication. If that
+          ;; fails for whatever reason, try to delete the window.
+          (if (switch-to-prev-buffer window bury-or-kill)
+              (set-dedicated-window-p window 'side)
+            (window--delete window nil (eq bury-or-kill 'kill)))
+          ;; If no previous buffer exists, try to delete the window.  If
+          ;; that fails for whatever reason, try to switch to some other
+          ;; buffer.
+          (unless (window--delete window nil (eq bury-or-kill 'kill))
+            (switch-to-prev-buffer window bury-or-kill)))))
 
     ;; Deal with the buffer.
     (cond
-- 
2.20.1


[-- Attachment #3: Type: text/plain, Size: 174 bytes --]


I will pass the rest of they day to look at ERT to see if I can learn
how to write reusable tests and see if this discussion needs to be reflected
in the documentation.

--