bug#12081: 24.1; buffer-predicate often not called

[Alp Aker]

On Sun, Jul 29, 2012 at 1:08 PM, martin rudalics <rudalics@gmx.at> wrote:
> I see.  But `kill-buffer' calls `replace-buffer-in-windows' which
> doesn't call `other-buffer'.  Only if the buffer to be killed is still
> current after that, `kill-buffer' calls `other-buffer'.  In the scenario
> above it is not called.
>
> Why is showing the buffer visiting /tmp/xx bad in your scenario?  Can
> you give a scenario where the present behavior really hurts you?  In
> that case we can try to ignore such a buffer in `switch-to-prev-buffer'.

Well, note that it's a regression:  replace-buffer-in-windows used to
call other-window (via window-loop).

As for how it hurts not to check buffer predicates in
swtich-to-prev-buffer:  Buffer predicates are supposed to provide a
way of exercising some control over what buffers are automatically
selected for display.  But if kill-buffer doesn't respect buffer
predicates, then there's not much point to setting up a buffer
predicate at all:  why bother filtering buffers chosen for display, if
the filter isn't respected by one of the most common ways in which a
buffer is chosen for the user?

It seems like something like the following would restore the old functionality:

=== modified file 'lisp/window.el'
--- lisp/window.el      2012-07-18 10:02:54 +0000
+++ lisp/window.el      2012-07-29 16:59:18 +0000
@@ -2679,10 +2679,12 @@
         (old-buffer (window-buffer window))
         ;; Save this since it's destroyed by `set-window-buffer'.
         (next-buffers (window-next-buffers window))
+         (pred (frame-parameter frame 'buffer-predicate))
         entry new-buffer killed-buffers visible)
     (when (window-dedicated-p window)
       (error "Window %s is dedicated to buffer %s" window old-buffer))

+    (unless
     (catch 'found
       ;; Scan WINDOW's previous buffers first, skipping entries of next
       ;; buffers.
@@ -2692,6 +2694,7 @@
                       (not (setq killed-buffers
                                  (cons new-buffer killed-buffers))))
                   (not (eq new-buffer old-buffer))
+                   (or (null pred) (funcall pred new-buffer))
                    (or bury-or-kill
                       (not (memq new-buffer next-buffers))))
          (if (and (not switch-to-visible-buffer)
@@ -2713,6 +2716,7 @@
        (when (and (buffer-live-p buffer)
                   (not (eq buffer old-buffer))
                   (not (eq (aref (buffer-name buffer) 0) ?\s))
+                   (or (null pred) (funcall pred buffer))
                   (or bury-or-kill (not (memq buffer next-buffers))))
          (if (get-buffer-window buffer frame)
              ;; Try to avoid showing a buffer visible in some other window.
@@ -2731,16 +2735,22 @@
                         (not (setq killed-buffers
                                    (cons buffer killed-buffers))))
                     (not (eq buffer old-buffer))
+                     (or (null pred) (funcall pred buffer))
                     (setq entry (assq buffer (window-prev-buffers window))))
            (setq new-buffer buffer)
            (set-window-buffer-start-and-point
             window new-buffer (nth 1 entry) (nth 2 entry))
-           (throw 'found t))))
-
-      ;; Show a buffer visible in another window.
-      (when visible
-       (setq new-buffer visible)
-       (set-window-buffer-start-and-point window new-buffer)))
+           (throw 'found t)))))
+
+      ;; If we reach this, then either: (1) we have a
+      ;; candidate buffer that was skipped because it was already visible on
+      ;; the frame, in which case we switch to it now, or (2) no candidate
+      ;; was found, in which case we switch to *scratch*.
+      (if visible
+          (setq new-buffer visible)
+        (setq new-buffer (get-buffer-create "*scratch*")))
+      (set-window-buffer-start-and-point window new-buffer))
+

     (if bury-or-kill
        ;; Remove `old-buffer' from WINDOW's previous and (restored list
@@ -2773,10 +2783,12 @@
         (frame (window-frame window))
         (old-buffer (window-buffer window))
         (next-buffers (window-next-buffers window))
+         (pred (frame-parameter frame 'buffer-predicate))
         new-buffer entry killed-buffers visible)
     (when (window-dedicated-p window)
       (error "Window %s is dedicated to buffer %s" window old-buffer))

+    (unless
     (catch 'found
       ;; Scan WINDOW's next buffers first.
       (dolist (buffer next-buffers)
@@ -2784,6 +2796,7 @@
                       (not (setq killed-buffers
                                  (cons buffer killed-buffers))))
                   (not (eq buffer old-buffer))
+                   (or (null pred) (funcall pred buffer))
                   (setq entry (assq buffer (window-prev-buffers window))))
          (setq new-buffer buffer)
          (set-window-buffer-start-and-point
@@ -2794,6 +2807,7 @@
       (dolist (buffer (buffer-list frame))
        (when (and (buffer-live-p buffer) (not (eq buffer old-buffer))
                   (not (eq (aref (buffer-name buffer) 0) ?\s))
+                   (or (null pred) (funcall pred buffer))
                   (not (assq buffer (window-prev-buffers window))))
          (if (get-buffer-window buffer frame)
              ;; Try to avoid showing a buffer visible in some other window.
@@ -2808,6 +2822,7 @@
                   (or (buffer-live-p new-buffer)
                       (not (setq killed-buffers
                                  (cons new-buffer killed-buffers))))
+                   (or (null pred) (funcall pred new-buffer))
                   (not (eq new-buffer old-buffer)))
          (if (and (not switch-to-visible-buffer)
                   (get-buffer-window new-buffer frame))
@@ -2816,12 +2831,17 @@
                (setq visible new-buffer))
            (set-window-buffer-start-and-point
             window new-buffer (nth 1 entry) (nth 2 entry))
-           (throw 'found t))))
-
-      ;; Show a buffer visible in another window.
-      (when visible
-       (setq new-buffer visible)
-       (set-window-buffer-start-and-point window new-buffer)))
+           (throw 'found t)))))
+
+      ;; If we reach this, then either: (1) we have a candidate buffer that
+      ;; was skipped because it was already visible on the frame, in which
+      ;; case we switch to it now, or (2) no candidate was found, in which
+      ;; case we switch to *scratch*.
+      (if visible
+          (setq new-buffer visible)
+        (setq new-buffer (get-buffer-create "*scratch*")))
+      (set-window-buffer-start-and-point window new-buffer))
+

     ;; Remove `new-buffer' from and restore WINDOW's next buffers.
     (set-window-next-buffers window (delq new-buffer next-buffers))