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

[Juri Linkov]

>> I'm thinking about adding 3 customizable options for
>> next-error-find-buffer-function:
>>
>> 1. buffer-local - new default
>> 2. window-local - useful for one window per each navigation buffer
>
> I'm not sure either can be congruent to all next-error-function
> applications. Some next-error source buffers contain their own errors (so
> buffer-local is natural), and others point to errors in other buffers
> (supposing they can learn to open those in the same window, window-local
> might be fitting). But both kinds are there, and all users deal with both.

They can learn to open those in the same window by the patch below.

>> 3. frame-local - old implicit default now separated into its own function
>
> That doesn't sound like a sufficient description: the old default also
> includes visibility-based logic. So it's not just one value per frame.

Do you think we should use the real frame-parameter?

>>> In general, next-error can jump between windows, so window-local is not
>>> a good fit for my mental model.
>>
>> It's bad when next-error unpredictably jumps between windows.
>> I hope we could find a way to fix this erratic behavior.
>
> It basically a rule now that Grep opens errors in a different windows (so
> that the Grep buffer stays visible). So erratic or not, multiple windows
> are used routinely.

This is improved by the same patch.

>>> Do we need the buffer-local-ity changes to next-error-last-buffer for that?
>>> Because if we do, that's okay, let's commit it and everything, but
>>> otherwise I'd rather wait and look for a more elegant approach (for other
>>> issues aside from this one).
>>
>> In the last previous patch, next-error visits next-error-find-buffer,
>> calls next-error-function that possibly navigates to another buffer,
>> then sets both global and buffer-local of next-error-last-buffer.
>> This seems quite logical.  And it works in all my tests.
>
> That... doesn't answer my question, I think. Or I didn't understand
> the answer.
>
> So let me try again: is the new next-error-find-buffer stuff necessary to
> fix the current bug? Or is suppressing the change to next-error-last-buffer
> during next-error-function calls enough for that?

The key point is (setq next-error-last-buffer) after
(funcall next-error-function), not before.

>>>> +      (message "Showing another error from %s" next-error-last-buffer)
>>>
>>> Is it really "another"? Or maybe it's "current", "last" or "closest" error?
>>
>> Maybe just "Showing error from %s"?  Or simply "Error from %s".
>> Then we could simplify the above messages as well: "%s error from %s"
>> for e.g. "Next error from %s", "Previous error from %s", ...
>
> Why not use "current" here as well? After all, we pass 0 to
> next-error-function here.

OK, here is the next patch:

diff --git a/lisp/simple.el b/lisp/simple.el
index 12d65e5..a741bf8 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-last-buffer)
 
 (defvar next-error-function nil
   "Function to use to find the next error in the current buffer.
@@ -191,6 +192,13 @@ 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 "No default" nil)
+                 (function :tag "Other function"))
+  :group 'next-error
+  :version "27.1")
+
 (defun next-error-find-buffer (&optional avoid-current
                                         extra-test-inclusive
                                         extra-test-exclusive)
@@ -207,18 +215,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 +280,48 @@ 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 "%s error from %s"
+                 (cond (reset                             "First")
+                       ((eq (prefix-numeric-value arg) 0) "Current")
+                       ((< (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 "Current 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)

diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index e4b77ab..8496cc9 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -2624,6 +2624,23 @@ compilation-set-window
 
 (defvar next-error-highlight-timer)
 
+(defun display-buffer-in-next-error-last-window (buffer alist)
+  "Return a window used to display the last next-error buffer."
+  (let* ((window (car (delq nil
+          (mapcar (lambda (w) (when (and (local-variable-p 
'next-error-last-buffer
+                                                           (window-buffer w))
+                                         (eq (current-buffer)
+                                             (buffer-local-value
+                                              'next-error-last-buffer
+                                              (window-buffer w))))
+                                w))
+                  (window-list-1 nil 'nomini))))))
+    (when (window-live-p window)
+      window
+      (prog1 (window--display-buffer buffer window 'reuse alist)
+        (unless (cdr (assq 'inhibit-switch-frame alist))
+          (window--maybe-raise-frame (window-frame window)))))))
+
 (defun compilation-goto-locus (msg mk end-mk)
   "Jump to an error corresponding to MSG at MK.
 All arguments are markers.  If END-MK is non-nil, mark is set there
@@ -2654,7 +2671,8 @@ compilation-goto-locus
         ;; keep the compilation buffer in this window;
         ;; display the source in another window.
         (let ((pop-up-windows t))
-          (pop-to-buffer (marker-buffer mk) 'other-window))
+          (pop-to-buffer (marker-buffer mk) 
'((display-buffer-in-next-error-last-window)
+                                              (inhibit-same-window . t))))
       (switch-to-buffer (marker-buffer mk)))
     (unless (eq (goto-char mk) (point))
       ;; If narrowing gets in the way of going to the right place, widen.