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

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

[Tino Calancha]

X-Debbugs-CC: Dmitry Gutov <dgutov@yandex.ru>

;; Start emacs in the emacs root source dir
emacs -Q

M-x grep-find RET
;; Use following command:
find . -type f -name ChangeLog\* -exec grep --color -nH -e baz {} +
C-x o n ; This selects the source (shouldn't)
;; If doesn't select the source, try 'n' a few times more; eventually
;; you will be in the source.

In GNU Emacs 25.3.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2017-09-20
Repository revision: c3ff6712ad24fcf45874dc0665a8606e9b2208a4

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

[Dmitry Gutov]

Hi!

On 10/16/17 4:07 PM, Tino Calancha wrote:
> X-Debbugs-CC: Dmitry Gutov <dgutov@yandex.ru>

I'm happy to commiserate, but it's hard to tell for me which part of 
next-error* is broken for this use case. If not all of it, basically.

For instance, I don't understand why the patch at the end doesn't fix 
the problem, and makes it worse instead (the behavior becomes less 
predictable).

Why doesn't it? If we have to call next-error, and it normally changes 
window configuration, save-selected-window seems like a natural fix.

> ;; Start emacs in the emacs root source dir
> emacs -Q
> 
> M-x grep-find RET
> ;; Use following command:
> find . -type f -name ChangeLog\* -exec grep --color -nH -e baz {} +
> C-x o n ; This selects the source (shouldn't)
> ;; If doesn't select the source, try 'n' a few times more; eventually
> ;; you will be in the source.

It selects the source at the first try here, ever time.

Here's the patch that doesn't work:

diff --git a/lisp/simple.el b/lisp/simple.el
index 5ef511ce0a..16ac4b6788 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -325,8 +325,8 @@ next-error-no-select
  select the source buffer."
    (interactive "p")
    (let ((next-error-highlight next-error-highlight-no-select))
-    (next-error n))
-  (pop-to-buffer next-error-last-buffer))
+    (save-selected-window
+      (next-error n))))

  (defun previous-error-no-select (&optional n)
    "Move point to the previous error in the `next-error' buffer and 
highlight match.

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

[Tino Calancha]

Dmitry Gutov <dgutov@yandex.ru> writes:

> I don't understand why the patch at the end doesn't fix
> the problem, and makes it worse instead (the behavior becomes less
> predictable).
> Why doesn't it? If we have to call next-error, and it normally changes
> window configuration, save-selected-window seems like a natural fix.

Hi Dmitry,

Thank you for the help!
Here your patch fix the issue.  Maybe you just have tested the patch
more carefuly than me; I just run some `grep-find' commands and I keep
always inside my window.

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

[Dmitry Gutov]

On 10/17/17 5:17 PM, Tino Calancha wrote:

> Thank you for the help!
> Here your patch fix the issue.  Maybe you just have tested the patch
> more carefuly than me; I just run some `grep-find' commands and I keep
> always inside my window.

It kind of improves something, but the behavior is still wonky and 
counter-intuitive. Here are my steps:

1. Start emacs -Q, keep the window size it started with (small-ish).

2. Run grep-find with your proposed command line. That splits the 
window, putting the Grep buffer into the bottom window.

3. 'C-x o' to switch to the bottom window. The first match here is:

./lisp/gnus/ChangeLog.3:18140:	 
name*0*=us-ascii''~%2ffoo%2fbar%2fbaz%2fxyzzy%2f;

4. Press 'n' once. That displays ChangeLog.3 in the upper window.

5. Press 'n' again. The fails to move the cursor in the bottom window, 
*and* it shows the definition of nntp-accept-process-output in the top 
window. Most likely because the next entry in ChangeLog.3 refers to that 
function.

6. Press 'n' again, that displays the next Grep result (lisp/ChangeLog.13).

Without step 5, everything would be in order. Unfortunately, it's 
consistent and easy to reproduce. Unfortunately again, if you backtrack 
with 'p' and press 'n' again, you won't get the same behavior, you will 
get the "correct" one instead.

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

[Tino Calancha]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 10/17/17 5:17 PM, Tino Calancha wrote:
>
>> Thank you for the help!
>> Here your patch fix the issue.  Maybe you just have tested the patch
>> more carefuly than me; I just run some `grep-find' commands and I keep
>> always inside my window.
>
> It kind of improves something, but the behavior is still wonky and
> counter-intuitive. Here are my steps:
I think I understand it now:
* change-log-mode sets 'next-error-last-buffer' to the current buffer (the
  ChangeLog source).

We can restrict to not do that when we are calling 'next-error-no-select.
I think some people don't like to use this-command in such
situations; alternatively, we could use `change-log-mode-hook' but the patch
below is shorter and explicit.

Suggestions are very welcome

--8<-----------------------------cut here---------------start------------->8---
commit 76391d208a1a89e2f84c58fb889bc1f19bdd3ce8
Author: Tino Calancha <tino.calancha@gmail.com>
Date:   Fri Oct 20 16:10:16 2017 +0900

    Dont select the source with next-error-no-select
    
    * lisp/vc/add-log.el (change-log-mode): Dont change
    next-error-last-buffer when we are visiting the source with
    next-error-no-select (Bug#28864).

diff --git a/lisp/vc/add-log.el b/lisp/vc/add-log.el
index 392147b14d..e557c49de5 100644
--- a/lisp/vc/add-log.el
+++ b/lisp/vc/add-log.el
@@ -1068,7 +1068,8 @@ change-log-mode
        'change-log-end-of-defun)
   ;; next-error function glue
   (setq next-error-function 'change-log-next-error)
-  (setq next-error-last-buffer (current-buffer)))
+  (unless (eq this-command 'next-error-no-select) ; Bug#28864
+    (setq next-error-last-buffer (current-buffer))))
 
 (defun change-log-next-buffer (&optional buffer wrap)
   "Return the next buffer in the series of ChangeLog file buffers.

--8<-----------------------------cut here---------------end--------------->8---
In GNU Emacs 27.0.50 (build 29, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2017-10-20
Repository revision: 658853aebb0ae2ee243276e04a7672fa7525ec5c

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

[Dmitry Gutov]

On 10/20/17 10:21 AM, Tino Calancha wrote:

>> It kind of improves something, but the behavior is still wonky and
>> counter-intuitive. Here are my steps:
> I think I understand it now:
> * change-log-mode sets 'next-error-last-buffer' to the current buffer (the
>    ChangeLog source).
> 
> We can restrict to not do that when we are calling 'next-error-no-select.
> I think some people don't like to use this-command in such
> situations; alternatively, we could use `change-log-mode-hook' but the patch
> below is shorter and explicit.

It's really a hack though, right? Maybe no major modes should set 
next-error-last-buffer by itself, no matter the current command.

I'd like to know what Juri thinks about this.

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

[Tino Calancha]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 10/20/17 10:21 AM, Tino Calancha wrote:
>
>>> It kind of improves something, but the behavior is still wonky and
>>> counter-intuitive. Here are my steps:
>> I think I understand it now:
>> * change-log-mode sets 'next-error-last-buffer' to the current buffer (the
>>    ChangeLog source).
>>
>> We can restrict to not do that when we are calling 'next-error-no-select.
>> I think some people don't like to use this-command in such
>> situations; alternatively, we could use `change-log-mode-hook' but the patch
>> below is shorter and explicit.
>
> It's really a hack though, right? Maybe no major modes should set
> next-error-last-buffer by itself, no matter the current command.
That sounds better.
> I'd like to know what Juri thinks about this.
In case Juri think that change-log-mode should keep seting
next-error-last-buffer for other reasons we could just
bind next-error-last-buffer:
--8<-----------------------------cut here---------------start------------->8---
commit c7b2ecd19714055b89ce348c07bae1c88a3fdc0a
Author: Tino Calancha <tino.calancha@gmail.com>
Date:   Sat Oct 21 12:49:30 2017 +0900

    Dont select the source with next-error-no-select
    
    * lisp/simple.el (next-error): Bind `next-error-last-buffer'
    before call next-error-function (Bug#28864).

diff --git a/lisp/simple.el b/lisp/simple.el
index 5ef511ce0a..b321d324e7 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -281,7 +281,9 @@ next-error
   (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)
+       ;; next-error-function should not change `next-error-last-buffer' Bug#28864.
+      (let ((next-error-last-buffer next-error-last-buffer))
+        (funcall next-error-function (prefix-numeric-value arg) reset))
       (when next-error-recenter
         (recenter next-error-recenter))
       (run-hooks 'next-error-hook))))

--8<-----------------------------cut here---------------end--------------->8---

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

[Juri Linkov]

>> It's really a hack though, right? Maybe no major modes should set
>> next-error-last-buffer by itself, no matter the current command.
> That sounds better.
>> I'd like to know what Juri thinks about this.
> In case Juri think that change-log-mode should keep seting
> next-error-last-buffer for other reasons we could just
> bind next-error-last-buffer:

Visiting a buffer should not set next-error-last-buffer,
it should be set only by an explicit user request,
like M-x compile/grep/rgrep/occur and other commands do.

So please just remove setting of next-error-last-buffer
from change-log-mode.  After that, next-error will still remain
available in ChangeLog buffers by running ‘next-error’ (when
there are no other next-error capable buffers such as *grep*)
or explicit C-c C-c that will set next-error-last-buffer
and continue visiting matches from the ChangeLog buffer.

We also have other issues with next-error in bug#20489
where I'm waiting when Dmitry will send a comprehensive list
of possible scenarios, so we could find a solution that fits
all of them.

Regarding the ChangeLog buffers, I have a solution for
the following ChangeLog-related scenario:

1. M-x grep-find RET
   find . -type f -name ChangeLog\* -exec grep --color -nH -e baz {} + RET

2. C-x o RET M-n RET

3. M-g M-n should continue visiting next grep hits from the *grep* buffer

4. When the user needs to switch to visiting ChangeLog entries
   from the ChangeLog buffer, this can be achieved by typing
   C-c C-c (change-log-goto-source) once, then all subsequent
   M-g M-n will continue visiting next ChangeLog entries
   in the source files.

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

[Dmitry Gutov]

On 10/22/17 11:32 PM, Juri Linkov wrote:

> Visiting a buffer should not set next-error-last-buffer,
> it should be set only by an explicit user request,
> like M-x compile/grep/rgrep/occur and other commands do.
 >
> So please just remove setting of next-error-last-buffer
> from change-log-mode.

Why not remove it from compilation-start as well? And 
xref--xref-buffer-mode.

After all, the user can visit the results buffer and press M-x 
next-error just as well.

> We also have other issues with next-error in bug#20489
> where I'm waiting when Dmitry will send a comprehensive list
> of possible scenarios, so we could find a solution that fits
> all of them.

I had no idea that someone is waiting for something like that. Is there 
a question in the bug discussion that I'd missed?

My scenario template is basically this:

1. Visit a buffer <A> in some major mode, press M-x next-error or M-x 
previous-error an arbitrary number of times, visiting the appropriate 
error locations, whether they are in the current buffer or in some other 
buffers or files that will have to be opened.

2. The above should happen fine irrespective of the current window 
configuration, and whether the buffer <A> gets hidden for some reason 
during this process (and we do know the possible reasons).

3. Being able to open a different buffer <B>, *somehow* switch to its 
error function, and have M-x next-error and M-x previous-error use it.

4. Being able to go to step 1 without much hassle.

Please ask any further questions.

> 4. When the user needs to switch to visiting ChangeLog entries
>     from the ChangeLog buffer, this can be achieved by typing
>     C-c C-c (change-log-goto-source) once, then all subsequent
>     M-g M-n will continue visiting next ChangeLog entries
>     in the source files.

As solutions go, it's a step forward.

But far from ideal: generally speaking, it requires a separate command 
and a binding in each suitable major mode that will switch to its 
next-error-function.

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

[Juri Linkov]

> My scenario template is basically this:
>
> 1. Visit a buffer <A> in some major mode, press M-x next-error or M-x
> previous-error an arbitrary number of times, visiting the appropriate error
> locations, whether they are in the current buffer or in some other buffers
> or files that will have to be opened.
>
> 2. The above should happen fine irrespective of the current window
> configuration, and whether the buffer <A> gets hidden for some reason
> during this process (and we do know the possible reasons).

It's a step backward: it removes the ability to run several next-error
navigations simultaneously.  Currently it's possible to visit e.g.
compilation results in one frame, and e.g. grep hits in another frame
at the same time without affecting one another.  Undoubtedly, relying
on the frame confines or on the buffer visibility is far from ideal.

One solution that I proposed was to associate a window containing
a navigated buffer with its next-error capable buffer, so that
next-error issued in the same window will continue visiting other matches
from the same next-error buffer (regardless whether it's hidden or not).
This assumes there will be a command to disassociate a window
from its next-error buffer, and switch to another next-error buffer.

Do you have a better idea?

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

[Dmitry Gutov]

On 10/23/17 11:12 PM, Juri Linkov wrote:

> It's a step backward: it removes the ability to run several next-error
> navigations simultaneously.  Currently it's possible to visit e.g.
> compilation results in one frame, and e.g. grep hits in another frame
> at the same time without affecting one another.

Why removes? Each buffer should keep its "current error" state, and 
you'd switch between the navigations by switching to the 
errors-containing buffer (and calling some additional command), probably.

> Undoubtedly, relying
> on the frame confines or on the buffer visibility is far from ideal.

I'm not married to buffer visibility, just figured it would be the 
easiest to implement. And also, for the user to conceptualize.

If you implement switching between navigations even when all 
error-containing buffers are still buried, more power to you.

> One solution that I proposed was to associate a window containing
> a navigated buffer with its next-error capable buffer, so that
> next-error issued in the same window will continue visiting other matches
> from the same next-error buffer (regardless whether it's hidden or not).

It's *a* solution, for sure. But there is little about a window that 
indicates that an error-containing buffer is tied to it somehow, if it's 
not displayed in it.

Implicit state is not so great as a UI.

> This assumes there will be a command to disassociate a window
> from its next-error buffer, and switch to another next-error buffer.
> 
> Do you have a better idea?

Like I said: just adding a command that makes the current buffer the 
next-error buffer. The user will have to switch to it, that may be an 
extra step, but they'd have to specify the target buffer anyway, so 
switching shouldn't make a lot of difference, keystroke-wise.

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

[Juri Linkov]

>> It's a step backward: it removes the ability to run several next-error
>> navigations simultaneously.  Currently it's possible to visit e.g.
>> compilation results in one frame, and e.g. grep hits in another frame
>> at the same time without affecting one another.
>
> Why removes? Each buffer should keep its "current error" state, and you'd
> switch between the navigations by switching to the errors-containing buffer
> (and calling some additional command), probably.

So do you propose to prefer buffer-local next-error-last-buffer
instead of window-local next-error-last-buffer?  This can cause
problems only when the same buffer can be visited from separate
next-error navigations, e.g. from two different *grep* buffers,
next-error from such buffer will use only the latest navigation
that visited this buffer (until it switched manually to another
navigation by a new command) Is this situation frequent enough?

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

[Dmitry Gutov]

On 10/24/17 11:22 PM, Juri Linkov wrote:

> So do you propose to prefer buffer-local next-error-last-buffer
> instead of window-local next-error-last-buffer?

Maybe something like this:

- Make next-error-last-buffer always buffer-local.

- Create the "current next-error source buffer" variable (which we'll 
need anyway), and whenever we need to look up or set 
next-error-last-buffer, read it or set it in that buffer.

> This can cause
> problems only when the same buffer can be visited from separate
> next-error navigations, e.g. from two different *grep* buffers,
> next-error from such buffer will use only the latest navigation
> that visited this buffer (until it switched manually to another
> navigation by a new command) Is this situation frequent enough?

Hopefully the above resolves your concern. But yes, switching to another 
navigation will require calling a certain new command.

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

[Dmitry Gutov]

On 10/25/17 1:23 AM, Dmitry Gutov wrote:
> On 10/24/17 11:22 PM, Juri Linkov wrote:
> 
>> So do you propose to prefer buffer-local next-error-last-buffer
>> instead of window-local next-error-last-buffer?
> 
> Maybe something like this:
> 
> - Make next-error-last-buffer always buffer-local.
> 
> - Create the "current next-error source buffer" variable (which we'll 
> need anyway), and whenever we need to look up or set 
> next-error-last-buffer, read it or set it in that buffer.

Sorry, this is nonsense, since next-error-last-buffer contains the 
reference to the "current next-error source buffer" anyway.

Let me amend it.

I propose to use next-error-last-buffer's global value, and avoid 
changing it too easily. Mostly at the user's request (via a special 
command), or when the last next-error-last-buffer has run out of errors, 
and the current buffer has some (this will require some discussion, 
though; maybe add a user prompt?).

>> This can cause
>> problems only when the same buffer can be visited from separate
>> next-error navigations, e.g. from two different *grep* buffers,
>> next-error from such buffer will use only the latest navigation
>> that visited this buffer (until it switched manually to another
>> navigation by a new command) Is this situation frequent enough?

Is there a reason you thought of buffer-local next-error-last-buffer values?

The "current error" is not stored in this variable. Instead, it's 
"saved" in the value of point in the corresponding error-capable buffer.

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

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 675 bytes --]

>> So do you propose to prefer buffer-local next-error-last-buffer
>> instead of window-local next-error-last-buffer?
>
> Maybe something like this:
>
> - Make next-error-last-buffer always buffer-local.

I believe this patch is a change for the better:

1. makes next-error-function buffer-local

2. sets both buffer-local and global values

3. adds customizable next-error-find-buffer-function

4. moves window-on-frame-visibility code to separate function
   that can be used to customize for backward-compatibility

5. adds next-error-select-buffer to manually switch to another
   next-error capable buffer

6. message to show which next-error buffer is currently used


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: next-error-1.patch --]
[-- Type: text/x-diff, Size: 6497 bytes --]

diff --git a/lisp/simple.el b/lisp/simple.el
index 12d65e5..2371d08 100644
*** a/lisp/simple.el
--- b/lisp/simple.el
***************
*** 143,148 ****
--- 143,149 ----
  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,196 ****
--- 192,221 ----
  	   (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.0")
+
+ (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,224 ****
  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)))
     ;; 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
--- 232,242 ----
  that would normally be considered usable.  If it returns nil,
  that buffer is rejected."
    (or
!    ;; 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,301 ****
  `compilation-error-regexp-alist'."
    (interactive "P")
    (if (consp arg) (setq reset t arg 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)
        (when next-error-recenter
          (recenter next-error-recenter))
        (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)))

  (defalias 'goto-next-locus 'next-error)
  (defalias 'next-match 'next-error)
--- 297,339 ----
  `compilation-error-regexp-alist'."
    (interactive "P")
    (if (consp arg) (setq reset t arg nil))
!   (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)
!         (when next-error-recenter
!           (recenter next-error-recenter))
!         (message "Next error buffer 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."
!   (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-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 "Next error buffer from %s" next-error-last-buffer)
        (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)

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

[Dmitry Gutov]

On 10/29/17 12:07 AM, Juri Linkov wrote:

> I believe this patch is a change for the better:

Could you resend it as unified diff? Or push to a branch. 
diff-context->unified doesn't seem to work on the patch you sent.

Thanks!

> 1. makes next-error-function buffer-local

Interesting.

> 2. sets both buffer-local and global values

Why? If next-error-find-buffer-function can find the appropriate buffer, 
it can use the local value in it.

> 3. adds customizable next-error-find-buffer-function
> 
> 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.

> 5. adds next-error-select-buffer to manually switch to another
>     next-error capable buffer

Not having tried the patch yet... Will the user have to do that after 
semi-accidentally opening a changelog-mode buffer, in order not to 
switch to its error list?

> 6. message to show which next-error buffer is currently used

Every time we call `next-error'?

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

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 1895 bytes --]

>> I believe this patch is a change for the better:
>
> Could you resend it as unified diff? Or push to
> a branch. diff-context->unified doesn't seem to work on the patch you sent.

I thought maybe context diff is easier to read in this case,
but here is unified diff below.

>> 1. makes next-error-function buffer-local
>
> Interesting.
>
>> 2. sets both buffer-local and global values
>
> Why? If next-error-find-buffer-function can find the appropriate buffer, it
> can use the local value in it.

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.

>> 3. adds customizable next-error-find-buffer-function
>>
>> 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.

>> 5. adds next-error-select-buffer to manually switch to another
>>     next-error capable buffer
>
> Not having tried the patch yet... Will the user have to do that after
> semi-accidentally opening a changelog-mode buffer, in order not to switch
> to its error list?

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.

But in case when the user changes mind and decides to switch
from *grep* navigation to ChangeLog navigation, there is
this new command.

>> 6. message to show which next-error buffer is currently used
>
> Every time we call `next-error'?

That's right.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: next-error-2.patch --]
[-- Type: text/x-diff, Size: 5694 bytes --]

diff --git a/lisp/simple.el b/lisp/simple.el
index 12d65e5..55de17d 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,43 @@ 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-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)
+        (when next-error-recenter
+          (recenter next-error-recenter))
+        (message "Next error buffer 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."
+  (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 "Next error buffer 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)

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

[Dmitry Gutov]

On 10/29/17 11:42 PM, Juri Linkov wrote:

> I thought maybe context diff is easier to read in this case,
> but here is unified diff below.

This one is better, thanks. Lots of practice reading unified diffs.

> 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?

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

>>> 5. adds next-error-select-buffer to manually switch to another
>>>      next-error capable buffer
>>
>> Not having tried the patch yet... Will the user have to do that after
>> semi-accidentally opening a changelog-mode buffer, in order not to switch
>> to its error list?
> 
> 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...

>>> 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"

?

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

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 30 Oct 2017 16:59:28 +0200
> Cc: 28864@debbugs.gnu.org, Noam Postavsky <npostavs@gmail.com>,
> 	Tino Calancha <tino.calancha@gmail.com>
> 
> How about a message like this:
> 
> "Showing next/last/previous error from %s"
> 
> ?

That sounds confusing.  How about

  Showing another error from %s

?

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

[Dmitry Gutov]

On 10/30/17 8:30 PM, Eli Zaretskii wrote:

>> How about a message like this:
>>
>> "Showing next/last/previous error from %s"
>>
>> ?
> 
> That sounds confusing

I meant that the choice between "next", "last" and "previous" would be 
made dynamically. No slashes.

> How about
> 
>    Showing another error from %s

Not too bad, but I prefer my suggestion, naturally. :)

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

[Juri Linkov]

>> How about a message like this:
>>
>> "Showing next/last/previous error from %s"
>>
>> ?
>
> That sounds confusing.  How about
>
>   Showing another error from %s
>
> ?

I added your message to next-error-internal where we have no information
about direction of next-error navigation.

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

[Juri Linkov]

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

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

[Dmitry Gutov]

On 10/30/17 11:14 PM, Juri Linkov wrote:
>>> 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.

Then the aforementioned concern stands for it as well.

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

Cool. Not something I've ever asked for, but could be helpful for some 
people.

In general, next-error can jump between windows, so window-local is not 
a good fit for my mental model.

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

Thanks. But if we already ignore the change to next-error-last-buffer 
during a call to next-error-function, that should fix the currently 
discussed bug, right? Maybe even the similar behavior with next-error, too?

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

 > +        (message "Showing %s error from %s"
 > +                 (cond (reset                            "first")
 > +                       ((< (prefix-numeric-value arg) 0) "previous")
 > +                       (t                                "next"))

Can arg be 0? Do we have a word for that? We might use it below, too.

> +(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)

Is it really "another"? Or maybe it's "current", "last" or "closest" error?

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

[Juri Linkov]

>> We could also add an alternative function based on window-local values.
>> At least, when I tried this code, it works as expected:
>
> Cool. Not something I've ever asked for, but could be helpful for
> some people.

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
3. frame-local - old implicit default now separated into its own function

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

>>> 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.
>
> Thanks. But if we already ignore the change to next-error-last-buffer
> during a call to next-error-function, that should fix the currently
> discussed bug, right? Maybe even the similar behavior with next-error, too?
>
> 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.

>> +        (message "Showing %s error from %s"
>> +                 (cond (reset                            "first")
>> +                       ((< (prefix-numeric-value arg) 0) "previous")
>> +                       (t                                "next"))
>
> Can arg be 0? Do we have a word for that? We might use it below, too.

We could use "current" for 0.  Could you also find a value for "last"?

>> +(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)
>
> 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", ...

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

[Dmitry Gutov]

On 10/31/17 11:56 PM, Juri Linkov wrote:

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

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

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

>> 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?

> We could use "current" for 0.

Fine by me.

> Could you also find a value for "last"?

I don't think it's needed. first/previous/current/current is enough.

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

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

[Juri Linkov]

[-- Attachment #1: Type: text/plain, Size: 2790 bytes --]

>> 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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: next-error-4.patch --]
[-- Type: text/x-diff, Size: 5103 bytes --]

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)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: compile-1.patch --]
[-- Type: text/x-diff, Size: 1910 bytes --]

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.

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

[Dmitry Gutov]

On 11/3/17 12:00 AM, Juri Linkov wrote:

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

If 'next-error', from any source, opens its target in the same window, 
does that make window-local storage essentially the same as frame-local 
in practice?

>>> 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?

I almost never use more than one frame, so I wouldn't know. But if you 
use a frame-parameter only, won't that break backward compatibility?

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

Thanks. So maybe we can split your patch into two parts: one that fixes 
the complaint in just this bug report, and the rest that aims to improve 
the general behavior.

We could push the first one right away, and continue discussing the 
second one. I'd like to see some new voices too, not just us two.

>> Why not use "current" here as well? After all, we pass 0 to
>> next-error-function here.
> 
> OK, here is the next patch:

Thanks.

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

[Juri Linkov]

>>> 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.
>
> If 'next-error', from any source, opens its target in the same window, does
> that make window-local storage essentially the same as frame-local
> in practice?

Not necessarily.  It's possible to have several pairs of next-error
source and target windows, each navigating in its own corresponding window,
e.g. *grep* visiting files in one window, and *compilation* in another.

>>>> 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?
>
> I almost never use more than one frame, so I wouldn't know. But if you use
> a frame-parameter only, won't that break backward compatibility?

I don't use more than one frame too, so I can't say how useful it would be.
So let's leave only the previous code as an option.

>>> 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.
>
> Thanks. So maybe we can split your patch into two parts: one that fixes the
> complaint in just this bug report, and the rest that aims to improve the
> general behavior.
>
> We could push the first one right away, and continue discussing the second
> one. I'd like to see some new voices too, not just us two.

Good point.  Closed to be continued in bug#20489.

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

[Juri Linkov]

>>> We can restrict to not do that when we are calling 'next-error-no-select.
>>> I think some people don't like to use this-command in such
>>> situations; alternatively, we could use `change-log-mode-hook' but the patch
>>> below is shorter and explicit.
>>
>> It's really a hack though, right? Maybe no major modes should set
>> next-error-last-buffer by itself, no matter the current command.
> That sounds better.

Please don't remove next-error-last-buffer from change-log-mode.
Actually it is legitimate use according to the documentation
of next-error-last-buffer:
"The most recent ‘next-error’ buffer.
 A buffer becomes most recent when its compilation, grep, or
 similar mode is started, or when it is used with \\[next-error]."

The problem occurred because two next-error buffer windows (*grep* and
next-error capable ChangeLog) were both visible on the selected frame,
but I believe we could fix this in the inner logic of next-error.

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

[Dmitry Gutov]

On 10/28/17 11:54 PM, Juri Linkov wrote:

> Please don't remove next-error-last-buffer from change-log-mode.
> Actually it is legitimate use according to the documentation
> of next-error-last-buffer:
> "The most recent ‘next-error’ buffer.
>   A buffer becomes most recent when its compilation, grep, or
>   similar mode is started, or when it is used with \\[next-error]."

The fact that it's documented doesn't exactly mean that the behavior 
really makes sense or is optimal.

> The problem occurred because two next-error buffer windows (*grep* and
> next-error capable ChangeLog) were both visible on the selected frame,
> but I believe we could fix this in the inner logic of next-error.

We've discussed the visibility-based selection before, I still don't 
think it makes sense, and the present bug report is evidence.