next-error-find-buffer (was: kill-compilation failing when there are several compilation buffers)

[Stefan Monnier]

>>> !   (if (and (compilation-buffer-internal-p (current-buffer))
>>> !      (not avoid-current))
>>> !       (current-buffer)
>>> !     (next-error-find-buffer avoid-current 
>>> 'compilation-buffer-internal-p)))
>> 
>> Curiously, next-error-find-buffer only checks current-buffer as its
>> 3rd choice.  This function either needs to be changed to try the current
>> buffer as its first choice, or it needs a clear comment explaining why.
>> 
>> It looks like this was changed by:
>> 
>> revision 1.655
>> date: 2004-09-01 13:05:59 -0400;  author: jurta;  state: Exp;  lines: +45 
>> -45;
>> * simple.el (next-error-find-buffer): Move the rule
>> "if current buffer is a next-error capable buffer" after the
>> rule "if next-error-last-buffer is set to a live buffer".
>> Simplify to test all rules in one `or'.
>> (next-error): Doc fix.
>> 
>> where it is not explained.  Juri, do you remember what was the motivation
>> for this change?

> This change was based on the conclusion of the very long discussion started
> from http://lists.gnu.org/archive/html/emacs-devel/2004-05/msg00476.html

> Any change in current rules may break test cases mentioned on that thread.

Actually, that thread did not conclude that the current set of rules is The
Right Way.  It ignored my suggestion (which was to keep the
`current-buffer' as the first rule, but to disable it if current-buffer was
the last source buffer visited by next-error).

Interestingly, I found out in the mean time that my suggestion has another
advantage: not only it ignores the current-buffer less often but it also
correctly ignores it in some cases where the current rules don't.

To remind people of the context, C-x ` has two problems:

1 - if you have two projects, do C-x ` in the first, then do C-x ` in the
    second, and then come back to the first and do C-x ` again, you'd want
    this C-x ` to use the compilation buffer of the first project, not of
    the second one.  So you'd want to sometimes ignore
    next-error-last-buffer.  This is the reason why "next-error capable
    buffer in the current frame" currently takes precedence over
    next-error-last-buffer.

2 - if you do C-x ` on a compilation buffer and that jumps to a patch file,
    and then do C-x ` from that patch file's buffer, it should look for the
    next error in the compilation buffer rather than the next hunk in the
    patch file.
    Currently this is done by giving precedence to next-error-last-buffer
    over current-buffer (i.e. basically this always ignores
    current-buffer.  In practice current-buffer is still often used because
    of the first rule "next-error capable buffer in current frame").

The way I use Emacs, I have one frame per buffer.  So the first rule
basically means that current-buffer is always selected, so for me problem
number 2 is not corrected.

My original suggestion to fix number 2 was to introduce a new variable
next-error-last-source-buffer which is set to the last source buffer visited
by next-error.  Then C-x ` would ignore current-buffer (and obey
next-error-last-buffer instead) if the current buffer is equal to
next-error-last-source-buffer: in the example problem, the C-x ` that puts
me in the patch file would set next-error-last-source-buffer to that patch
file buffer so hitting C-x ` in that patch file buffer would correctly
ignore the patch file buffer and use the compilation buffer instead.

BTW, it looks like the `avoid-current' argument is *never* used.

Any objection to the patch below?


        Stefan


--- simple.el   28 jui 2007 16:22:16 -0400      1.859.2.3
+++ simple.el   02 aoû 2007 12:17:20 -0400      
@@ -173,6 +173,9 @@
 similar mode is started, or when it is used with \\[next-error]
 or \\[compile-goto-error].")
 
+(defvar next-error-last-source-buffer nil
+  "The most recent buffer to which `next-error' jumped.")
+
 (defvar next-error-function nil
   "Function to use to find the next error in the current buffer.
 The function is called with 2 parameters:
@@ -228,14 +231,33 @@
 The function EXTRA-TEST-EXCLUSIVE, if non-nil, is called in each buffer
 that would normally be considered usable.  If it returns nil,
 that buffer is rejected."
+  ;; FIXME: It looks like the `avoid-current' argument is never used.
+  (let ((minor-avoid-current
+         (or avoid-current
+             ;; When a C-x ` in an occur or compilation buffer jumps to
+             ;; a patch file, we want the next C-x ` to keep visiting the
+             ;; next errors or occurrences, rather than jump to the
+             ;; next hunk.
+             (eq (current-buffer) next-error-last-source-buffer))))
   (or
+     ;; 0. If the current buffer is acceptable, choose it, unless it is the
+     ;;    buffer to which the last next-error jumped.
+     (if (next-error-buffer-p (current-buffer) minor-avoid-current
+                              extra-test-inclusive extra-test-exclusive)
+         (current-buffer))
    ;; 1. If one window on the selected frame displays such buffer, return it.
+     ;;    This takes precedence to the next-error-last-buffer so as to
+     ;;    try and address the situation where the user works on 2 projects
+     ;;    within the same Emacs session, does a C-x ` on a compilation in the
+     ;;    first project, then C-x ` on an occur buffer in the second, then
+     ;;    comes back to the first project and hits C-x ` expecting to jump to
+     ;;    the next error of the first project's compilation.
    (let ((window-buffers
           (delete-dups
            (delq nil (mapcar (lambda (w)
                                (if (next-error-buffer-p
                                    (window-buffer w)
-                                    avoid-current
+                                      minor-avoid-current
                                     extra-test-inclusive extra-test-exclusive)
                                    (window-buffer w)))
                              (window-list))))))
@@ -246,7 +268,9 @@
             (next-error-buffer-p next-error-last-buffer avoid-current
                                  extra-test-inclusive extra-test-exclusive))
        next-error-last-buffer)
-   ;; 3. If the current buffer is acceptable, choose it.
+     ;; 3. If the current buffer is acceptable, choose it.  This is only
+     ;;    useful if the current-buffer is the buffer to which the last
+     ;;    next-error jumped (otherwise rule 0 applied already).
    (if (next-error-buffer-p (current-buffer) avoid-current
                            extra-test-inclusive extra-test-exclusive)
        (current-buffer))
@@ -267,7 +291,7 @@
          (message "This is the only buffer with error message locations")
          (current-buffer)))
    ;; 6. Give up.
-   (error "No buffers contain error message locations")))
+     (error "No buffers contain error message locations"))))
 
 (defun next-error (&optional arg reset)
   "Visit next `next-error' message and corresponding source code.
@@ -301,18 +325,21 @@
 \`compilation-error-regexp-alist' for customization ideas."
   (interactive "P")
   (if (consp arg) (setq reset t arg nil))
+  ;; This `when' is unneeded because next-error-find-buffer never returns nil.
   (when (setq next-error-last-buffer (next-error-find-buffer))
     ;; we know here that next-error-function is a valid symbol we can funcall
     (with-current-buffer next-error-last-buffer
       (funcall next-error-function (prefix-numeric-value arg) reset)
+      (setq next-error-last-source-buffer (current-buffer))
       (run-hooks 'next-error-hook))))
 
 (defun next-error-internal ()
   "Visit the source code corresponding to the `next-error' message at point."
   (setq next-error-last-buffer (current-buffer))
-  ;; we know here that next-error-function is a valid symbol we can funcall
-  (with-current-buffer next-error-last-buffer
+  ;; We know here that next-error-function is a valid symbol we can funcall.
+  (save-current-buffer
     (funcall next-error-function 0 nil)
+    (setq next-error-last-source-buffer (current-buffer))
     (run-hooks 'next-error-hook)))
 
 (defalias 'goto-next-locus 'next-error)