bug#28864: 25.3.50; next-error-no-select does select

[Juri Linkov <juri@linkov.net>]

>> We need the global value as well to get the last next-error buffer
>> when there is no buffer-local value yet in the current buffer.
>
> Doesn't sound very elegant: we accumulate hidden state as the time passes,
> and the user calls 'next-error' with compilations, grep, and so on. But
> this is better than nothing, of course.
>
> Does it also mean that the effect of next-error-select-buffer will also be
> buffer-local?

Yes, next-error-select-buffer sets both buffer-local and global values,
exactly like in next-error.

>>>> 4. moves window-on-frame-visibility code to separate function
>>>>      that can be used to customize for backward-compatibility
>>>
>>> Do we get a built-in alternative to this value? If we just provide
>>> a customization point, that nice, but not a significant improvement.
>>
>> You mean to remove next-error-buffer-on-selected-frame?
>> Or maybe to do the other way: add more predefined functions like
>> a function to use window-local navigation as an alternative.
>
> Now I see it: you have moved the visibility-based search to the custom
> variable and disabled it by default. So the default behavior is changed
> (for the better, IMO), and I withdraw the question, thanks. :)

We could also add an alternative function based on window-local values.
At least, when I tried this code, it works as expected:

(setq next-error-find-buffer-function
      (lambda (&optional avoid-current extra-test-inclusive extra-test-exclusive)
	(window-parameter nil 'next-error-buffer)))

(add-hook 'next-error-hook
	  (lambda ()
	    (set-window-parameter
	     nil 'next-error-buffer next-error-last-buffer)))

>> When the user visits a ChangeLog buffer from *grep*, then
>> next-error continues the *grep* navigation.  When the user
>> visit a ChangeLog file by any other command, then next-error
>> navigates entries in that ChangeLog buffer.
>
> Does this also work with next-error, not just next-error-no-select?
>
> I'm guessing this part helps with that:
>
> +  (let ((next-error-buffer (next-error-find-buffer)))
> +    (when next-error-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)
> +        ;; next-error-function might change the value of
> +        ;; next-error-last-buffer, so set it later
> +        (setq next-error-last-buffer next-error-buffer)
> +        (setq-default next-error-last-buffer next-error-last-buffer)
>
> So we ignore the next-error-last-buffer change during a call to
> next-error-last-function, but not in any other circumstances? Like visiting
> a ChangeLog file manually. Maybe that's okay...

Oh, sorry, there was a typo: it should be (with-current-buffer next-error-buffer
Fixed in the next version of the patch below.

>>>> 6. message to show which next-error buffer is currently used
>>>
>>> Every time we call `next-error'?
>>
>> That's right.
>
> That might be a bit excessive. But more importantly, opaque to an
> average user.
>
> How about a message like this:
>
> "Showing next/last/previous error from %s"
>
> ?

Yes, it's good to show its direction as well:

diff --git a/lisp/simple.el b/lisp/simple.el
index 12d65e5..ce5c858 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -143,6 +143,7 @@ next-error-last-buffer
 A buffer becomes most recent when its compilation, grep, or
 similar mode is started, or when it is used with \\[next-error]
 or \\[compile-goto-error].")
+(make-variable-buffer-local 'next-error-function)
 
 (defvar next-error-function nil
   "Function to use to find the next error in the current buffer.
@@ -191,6 +192,30 @@ next-error-buffer-p
 	   (and extra-test-inclusive
 		(funcall extra-test-inclusive))))))
 
+(defcustom next-error-find-buffer-function nil
+  "Function called to find a `next-error' capable buffer."
+  :type '(choice (const :tag "Buffer on selected frame" next-error-buffer-on-selected-frame)
+                 (const :tag "No default" nil)
+                 (function :tag "Other function"))
+  :group 'next-error
+  :version "27.1")
+
+(defun next-error-buffer-on-selected-frame (&optional avoid-current
+                                                      extra-test-inclusive
+                                                      extra-test-exclusive)
+  "Return a single visible next-error buffer on the selected frame."
+  (let ((window-buffers
+         (delete-dups
+          (delq nil (mapcar (lambda (w)
+                              (if (next-error-buffer-p
+        			   (window-buffer w)
+                                   avoid-current
+                                   extra-test-inclusive extra-test-exclusive)
+                                  (window-buffer w)))
+                            (window-list))))))
+    (if (eq (length window-buffers) 1)
+        (car window-buffers))))
+
 (defun next-error-find-buffer (&optional avoid-current
 					 extra-test-inclusive
 					 extra-test-exclusive)
@@ -207,18 +232,11 @@ next-error-find-buffer
 that would normally be considered usable.  If it returns nil,
 that buffer is rejected."
   (or
-   ;; 1. If one window on the selected frame displays such buffer, return it.
-   (let ((window-buffers
-          (delete-dups
-           (delq nil (mapcar (lambda (w)
-                               (if (next-error-buffer-p
-				    (window-buffer w)
-                                    avoid-current
-                                    extra-test-inclusive extra-test-exclusive)
-                                   (window-buffer w)))
-                             (window-list))))))
-     (if (eq (length window-buffers) 1)
-         (car window-buffers)))
+   ;; 1. If a customizable function returns a buffer, use it.
+   (when next-error-find-buffer-function
+     (funcall next-error-find-buffer-function avoid-current
+                                              extra-test-inclusive
+                                              extra-test-exclusive))
    ;; 2. If next-error-last-buffer is an acceptable buffer, use that.
    (if (and next-error-last-buffer
             (next-error-buffer-p next-error-last-buffer avoid-current
@@ -279,23 +297,47 @@ next-error
 `compilation-error-regexp-alist'."
   (interactive "P")
   (if (consp arg) (setq reset t arg nil))
-  (when (setq next-error-last-buffer (next-error-find-buffer))
+  (let ((next-error-buffer (next-error-find-buffer)))
+    (when next-error-buffer
+      ;; we know here that next-error-function is a valid symbol we can funcall
+      (with-current-buffer next-error-buffer
+        (funcall next-error-function (prefix-numeric-value arg) reset)
+        ;; next-error-function might change the value of
+        ;; next-error-last-buffer, so set it later
+        (setq next-error-last-buffer next-error-buffer)
+        (setq-default next-error-last-buffer next-error-last-buffer)
+        (when next-error-recenter
+          (recenter next-error-recenter))
+        (message "Showing %s error from %s"
+                 (cond (reset                            "first")
+                       ((< (prefix-numeric-value arg) 0) "previous")
+                       (t                                "next"))
+                 next-error-last-buffer)
+        (run-hooks 'next-error-hook)))))
+
+(defun next-error-internal ()
+  "Visit the source code corresponding to the `next-error' message at point."
+  (let ((next-error-buffer (current-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)
+    (with-current-buffer next-error-buffer
+      (funcall next-error-function 0 nil)
+      ;; next-error-function might change the value of
+      ;; next-error-last-buffer, so set it later
+      (setq next-error-last-buffer next-error-buffer)
+      (setq-default next-error-last-buffer next-error-last-buffer)
       (when next-error-recenter
         (recenter next-error-recenter))
+      (message "Showing another error from %s" next-error-last-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
-    (funcall next-error-function 0 nil)
-    (when next-error-recenter
-      (recenter next-error-recenter))
-    (run-hooks 'next-error-hook)))
+(defun next-error-select-buffer (next-error-buffer)
+  "Select a `next-error' capable buffer and set it as the last used."
+  (interactive
+   (list (get-buffer
+          (read-buffer "Select next-error buffer: " nil nil
+                       (lambda (b) (next-error-buffer-p (cdr b)))))))
+  (setq next-error-last-buffer next-error-buffer)
+  (setq-default next-error-last-buffer next-error-last-buffer))
 
 (defalias 'goto-next-locus 'next-error)
 (defalias 'next-match 'next-error)