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

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)

Re: next-error-find-buffer

[Ted Zlatanov]

On Thu, 02 Aug 2007 12:17:44 -0400 Stefan Monnier <monnier@iro.umontreal.ca> wrote: 

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

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

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

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

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

I think your fix makes sense.  I've avoided work on next-error in the
past because of the recent release, but this was annoying behavior in
some cases.

I have some other proposals for the next/previous functionality, if
anyone is interested.  Basically I think it should be expanded to other
parts of Emacs, because it's so handy.

Ted

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

[Richard Stallman]

    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).

I can see how, in some situations, that would be better behavior
in next-error.  (My change is still needed for kill-compilation.)

However, it seems to me that there may be other situations were it
might be worse behavior.  This is the sort of issue where no solution
is clearly right, every choice is a heuristic, and there may not be
any best solution.

Since your change makes the code more complex, I think we should not
install it now.  Instead, people should try it, and we can see whether
they generally like it better.  If they do, then please install it.

Re: next-error-find-buffer

[Juri Linkov]

> 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.
>
> Any objection to the patch below?

Perhaps a new variable next-error-last-source-buffer should be buffer-local.
I imagine such a test case: C-x ` on a grep buffer visits the patch file,
then in another frame on another grep buffer C-x ` visits its own patch file,
then I switch back to the first patch file, type C-x `, and it will fail
to visit the next grep hit from the first grep buffer, because the
global next-error-last-source-buffer is set now to the second patch
buffer from the second grep buffer.

Please note that I concluded this outcome only by looking at your patch.
I will try it for a few days to see how well it works in different cases.

-- 
Juri Linkov
http://www.jurta.org/emacs/