bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Trevor Spiteri]

The next-error-select-buffer documentation states that “the selected
buffer becomes the source of locations for the subsequent invocation of
‘next-error’ or ‘previous-error’.” However, it is not the case for the
following:

1. Go in a fresh next-error capable buffer (not *grep*).
2. Grep for something.
3. M-x next-error-select-buffer *grep*
4. M-x next-error

The buffer of 1 (not *grep*) is the source of locations instead of
the expected *grep*.

This is because although next-error-select-buffer sets the variable
next-error-last-buffer, it is not used in this case: When next-error
calls next-error-find-buffer, next-error-buffer has no buffer-local
value yet, so condition 2. in next-error-find-buffer (that
next-error-buffer has no buffer-local value and the current buffer is a
next-error capable buffer) is true, and the function never even checks
next-error-last-buffer.



In GNU Emacs 27.0.91 (build 1, x86_64-pc-linux-gnu, GTK+ Version
3.24.13, cairo version 1.16.0)
of 2020-04-23 built on desktop
Repository revision: ba6104d1e8db4e8db2f12acaebf092ef579c6632
Repository branch: emacs-27

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Trevor Spiteri]

On 28/04/2020 03:51, Trevor Spiteri wrote:
> 1. Go in a fresh next-error capable buffer (not *grep*).
> 2. Grep for something.
> 3. M-x next-error-select-buffer *grep*
> 4. M-x next-error

And I just realized, this is also a regression from Emacs 26 if step 3
is skipped, as step 2 itself also sets next-error-last-buffer .

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Juri Linkov]

> The next-error-select-buffer documentation states that “the selected
> buffer becomes the source of locations for the subsequent invocation of
> ‘next-error’ or ‘previous-error’.” However, it is not the case for the
> following:
>
> 1. Go in a fresh next-error capable buffer (not *grep*).
> 2. Grep for something.
> 3. M-x next-error-select-buffer *grep*
> 4. M-x next-error
>
> The buffer of 1 (not *grep*) is the source of locations instead of
> the expected *grep*.
>
> This is because although next-error-select-buffer sets the variable
> next-error-last-buffer, it is not used in this case: When next-error
> calls next-error-find-buffer, next-error-buffer has no buffer-local
> value yet, so condition 2. in next-error-find-buffer (that
> next-error-buffer has no buffer-local value and the current buffer is a
> next-error capable buffer) is true, and the function never even checks
> next-error-last-buffer.

Thanks for the report.  Do you think the problem is in implementation,
or only in documentation?  IOW, do you think its behavior is correct,
but the documentation should be fixed to describe more clearly what
next-error was intended to do in this situation?

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Trevor Spiteri]

>> The next-error-select-buffer documentation states that “the selected
>> buffer becomes the source of locations for the subsequent invocation of
>> ‘next-error’ or ‘previous-error’.” However, it is not the case for the
>> following:
>>
>> 1. Go in a fresh next-error capable buffer (not *grep*).
>> 2. Grep for something.
>> 3. M-x next-error-select-buffer *grep*
>> 4. M-x next-error
>>
>> The buffer of 1 (not *grep*) is the source of locations instead of
>> the expected *grep*.
>>
>> This is because although next-error-select-buffer sets the variable
>> next-error-last-buffer, it is not used in this case: When next-error
>> calls next-error-find-buffer, next-error-buffer has no buffer-local
>> value yet, so condition 2. in next-error-find-buffer (that
>> next-error-buffer has no buffer-local value and the current buffer is a
>> next-error capable buffer) is true, and the function never even checks
>> next-error-last-buffer.
> Thanks for the report.  Do you think the problem is in implementation,
> or only in documentation?  IOW, do you think its behavior is correct,
> but the documentation should be fixed to describe more clearly what
> next-error was intended to do in this situation?

I think the error is in the implementation. In fact I added a later
comment to the bug report.


> And I just realized, this is also a regression from Emacs 26 if step 3
> is skipped, as step 2 itself also sets next-error-last-buffer .

As a use case, let's say I'm in a buffer that has next-error
capabilities because of say flycheck, and I grep or compile; I want to
start going through the new errors immediately. That is why
compilation-start finishes  with (setq next-error-last-buffer outbuf)
and that's how Emacs 26 works (without step 3 as
next-error-select-buffer is new). In Emacs 27 not only does that break,
but even using the new function has no effect.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Juri Linkov]

> I think the error is in the implementation.

Then I see no way other than for next-error-select-buffer to say:
"the current buffer was visited from next-error-last-buffer".
Yes, this is a lie, but a white lie with good intentions, so
next-error-find-buffer will trust this misinformation and leave
the buffer alone.  Is this patch morally acceptable?

diff --git a/lisp/simple.el b/lisp/simple.el
index b5ba05426f..b5f148b7d5 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -379,7 +379,8 @@ next-error-select-buffer
    (list (get-buffer
           (read-buffer "Select next-error buffer: " nil nil
                        (lambda (b) (next-error-buffer-p (cdr b)))))))
-  (setq next-error-last-buffer buffer))
+  (setq next-error-last-buffer buffer)
+  (setq next-error-buffer buffer))
 
 (defalias 'goto-next-locus 'next-error)
 (defalias 'next-match 'next-error)

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Trevor Spiteri]

On 29/04/2020 22:38, Juri Linkov wrote:
>> I think the error is in the implementation.
> Then I see no way other than for next-error-select-buffer to say:
> "the current buffer was visited from next-error-last-buffer".
> Yes, this is a lie, but a white lie with good intentions, so
> next-error-find-buffer will trust this misinformation and leave
> the buffer alone.  Is this patch morally acceptable?
>
> diff --git a/lisp/simple.el b/lisp/simple.el
> index b5ba05426f..b5f148b7d5 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -379,7 +379,8 @@ next-error-select-buffer
>     (list (get-buffer
>            (read-buffer "Select next-error buffer: " nil nil
>                         (lambda (b) (next-error-buffer-p (cdr b)))))))
> -  (setq next-error-last-buffer buffer))
> +  (setq next-error-last-buffer buffer)
> +  (setq next-error-buffer buffer))
>  
>  (defalias 'goto-next-locus 'next-error)
>  (defalias 'next-match 'next-error)
>
I think this would work for next-error-select-buffer. Then to fix the
other issue about new compilations, compilation-start can be modified to
use next-error-select-buffer. That way the change in compilation-start
is morally unambiguous :)

diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index 455f181f50..41e77007c6 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -1910,7 +1910,7 @@ compilation-start
     (goto-char (point-max))))
 
     ;; Make it so the next C-x ` will use this buffer.
-    (setq next-error-last-buffer outbuf)))
+    (next-error-select-buffer outbuf)))
 
 (defun compilation-set-window-height (window)
   "Set the height of WINDOW according to `compilation-window-height'."

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Juri Linkov]

>> @@ -379,7 +379,8 @@ next-error-select-buffer
>>     (list (get-buffer
>>            (read-buffer "Select next-error buffer: " nil nil
>>                         (lambda (b) (next-error-buffer-p (cdr b)))))))
>> -  (setq next-error-last-buffer buffer))
>> +  (setq next-error-last-buffer buffer)
>> +  (setq next-error-buffer buffer))
>>  
>>  (defalias 'goto-next-locus 'next-error)
>>  (defalias 'next-match 'next-error)
>>
> I think this would work for next-error-select-buffer. Then to fix the
> other issue about new compilations, compilation-start can be modified to
> use next-error-select-buffer. That way the change in compilation-start
> is morally unambiguous :)

I'm not sure if this is morally right, because users might want to
continue to navigate an old next-error buffer, even after creating
a new next-error buffer.

OTOH, by using next-error-select-buffer they explicitly say that
they want to switch to the new navigation.  Do you think it's right
to implicitly assume the user's intention?

We should base the decision not on precedence set by older versions,
but on expectations of most users.

> diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
> index 455f181f50..41e77007c6 100644
> --- a/lisp/progmodes/compile.el
> +++ b/lisp/progmodes/compile.el
> @@ -1910,7 +1910,7 @@ compilation-start
>      (goto-char (point-max))))
>  
>      ;; Make it so the next C-x ` will use this buffer.
> -    (setq next-error-last-buffer outbuf)))
> +    (next-error-select-buffer outbuf)))
>  
>  (defun compilation-set-window-height (window)
>    "Set the height of WINDOW according to `compilation-window-height'."

There are more places that set next-error-last-buffer: vc-git-grep, occur-1,
xref.el...

But I'm still not sure if this is a preferable behavior for most users.
Maybe this needs a user option?

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Trevor Spiteri]

>> I think this would work for next-error-select-buffer. Then to fix the
>> other issue about new compilations, compilation-start can be modified to
>> use next-error-select-buffer. That way the change in compilation-start
>> is morally unambiguous :)
> I'm not sure if this is morally right, because users might want to
> continue to navigate an old next-error buffer, even after creating
> a new next-error buffer.
>
> OTOH, by using next-error-select-buffer they explicitly say that
> they want to switch to the new navigation.  Do you think it's right
> to implicitly assume the user's intention?
>
> We should base the decision not on precedence set by older versions,
> but on expectations of most users.
>
>> diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
>> index 455f181f50..41e77007c6 100644
>> --- a/lisp/progmodes/compile.el
>> +++ b/lisp/progmodes/compile.el
>> @@ -1910,7 +1910,7 @@ compilation-start
>>      (goto-char (point-max))))
>>  
>>      ;; Make it so the next C-x ` will use this buffer.
>> -    (setq next-error-last-buffer outbuf)))
>> +    (next-error-select-buffer outbuf)))
>>  
>>  (defun compilation-set-window-height (window)
>>    "Set the height of WINDOW according to `compilation-window-height'."
> There are more places that set next-error-last-buffer: vc-git-grep, occur-1,
> xref.el...
>
> But I'm still not sure if this is a preferable behavior for most users.
> Maybe this needs a user option?

I thought that the change was unintentional. If it's intentional that's
another thing. If no one else complains then maybe most users prefer the
new behavior. I've already added advice to compilation-start so it's not
hard for me that I prefer the old behavior.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Juri Linkov]

>> But I'm still not sure if this is a preferable behavior for most users.
>> Maybe this needs a user option?
>
> I thought that the change was unintentional. If it's intentional that's
> another thing. If no one else complains then maybe most users prefer the
> new behavior. I've already added advice to compilation-start so it's not
> hard for me that I prefer the old behavior.

I think the problem occurs only because compilation/grep pop up their
output buffer in another window - this creates ambiguity whether to use
navigation from the current buffer or from another (new) buffer.

It seems there is no right answer suitable for all users.  So I propose
to add more options.

Eli, do you agree with adding more options to `next-error-find-buffer-function'
in emacs-27?

Currently it has one option that defines one of possible behaviors existed
in older versions.  Now we need to add other option for compatibility
with pre-27 versions.

Then users of emacs-27 could decide whether they want to keep
the old behavior or use the new.

I'm not sure about the default value: should it default to pre-27 behavior.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Sun, 03 May 2020 02:38:10 +0300
> Cc: 40919@debbugs.gnu.org
> 
> It seems there is no right answer suitable for all users.  So I propose
> to add more options.
> 
> Eli, do you agree with adding more options to `next-error-find-buffer-function'
> in emacs-27?
> 
> Currently it has one option that defines one of possible behaviors existed
> in older versions.  Now we need to add other option for compatibility
> with pre-27 versions.
> 
> Then users of emacs-27 could decide whether they want to keep
> the old behavior or use the new.

I think the time for adding new options to Emacs 27 has come and gone
long ago.  We should defer that to future releases.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Juri Linkov]

>> It seems there is no right answer suitable for all users.  So I propose
>> to add more options.
>>
>> Eli, do you agree with adding more options to `next-error-find-buffer-function'
>> in emacs-27?
>>
>> Currently it has one option that defines one of possible behaviors existed
>> in older versions.  Now we need to add other option for compatibility
>> with pre-27 versions.
>>
>> Then users of emacs-27 could decide whether they want to keep
>> the old behavior or use the new.
>
> I think the time for adding new options to Emacs 27 has come and gone
> long ago.  We should defer that to future releases.

To do nothing in emacs-27 is one variant, indeed.  Another variant would be
to add an optional choice to the existing option that allows to restore
the old behavior and doesn't affect the current behavior in any way.
A third variant is to explain in the documentation what defadvice
the users could add to their init files to restore the old behavior.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Dmitry Gutov]

On 04.05.2020 01:36, Juri Linkov wrote:
> Another variant would be
> to add an optional choice to the existing option that allows to restore
> the old behavior and doesn't affect the current behavior in any way.

FWIW, I'd consider that more of a documentation change.

More importantly, and certainly only for Emacs 28, Juri could you remind 
me what we'll be losing by removing the case no. 2 from 
next-error-find-buffer?

The ability to switch to an arbitrary Grep buffer and start using it 
with 'M-x next-error'? E.g. if there are several of them. That's more of 
a backward compatibility concern, right? Or do you have scenarios in 
mind where this will really save on keystrokes?

On another note, perhaps we could add a message to 
next-error-select-buffer that would be shown if we suspect this command 
will not have the expected result for the user.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Juri Linkov]

>> Another variant would be
>> to add an optional choice to the existing option that allows to restore
>> the old behavior and doesn't affect the current behavior in any way.
>
> FWIW, I'd consider that more of a documentation change.
>
> More importantly, and certainly only for Emacs 28, Juri could you remind me
> what we'll be losing by removing the case no. 2 from
> next-error-find-buffer?

It is used to handle such cases when navigating one next-error list
visits a buffer that contains another next-error list - visiting
such buffer should not switch the navigation, it should continue
the initial navigation.

But I'm not proud of the case no. 2, so you can drop it :)

Or better move it to a separate function and provide this function
as an option in next-error-find-buffer-function.

> The ability to switch to an arbitrary Grep buffer and start using it with
> 'M-x next-error'? E.g. if there are several of them. That's more of
> a backward compatibility concern, right? Or do you have scenarios in
> mind where this will really save on keystrokes?

The ability to visit an arbitrary Grep buffer and use 'next-error' in it
to switch navigation should always work as the most reliable and implicit
way to switch navigation.  This is the case no. 3 in next-error-find-buffer.

> On another note, perhaps we could add a message to next-error-select-buffer
> that would be shown if we suspect this command will not have the expected
> result for the user.

Or maybe ask the user using the minibuffer to resolve ambiguity.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Dmitry Gutov]

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

On 20.05.2020 01:21, Juri Linkov wrote:
>>> Another variant would be
>>> to add an optional choice to the existing option that allows to restore
>>> the old behavior and doesn't affect the current behavior in any way.
>>
>> FWIW, I'd consider that more of a documentation change.
>>
>> More importantly, and certainly only for Emacs 28, Juri could you remind me
>> what we'll be losing by removing the case no. 2 from
>> next-error-find-buffer?
> 
> It is used to handle such cases when navigating one next-error list
> visits a buffer that contains another next-error list - visiting
> such buffer should not switch the navigation, it should continue
> the initial navigation.

Didn't we fix changelog-mode so this doesn't happen anymore? I think as 
long as file major modes don't set next-error-last-buffer (and minor 
modes either, on initialization), we should be safe.

Can you reproduce a problematic scenario with the attached patch?

> But I'm not proud of the case no. 2, so you can drop it :)
> 
> Or better move it to a separate function and provide this function
> as an option in next-error-find-buffer-function.

Patch attached. Comments welcome.

>> The ability to switch to an arbitrary Grep buffer and start using it with
>> 'M-x next-error'? E.g. if there are several of them. That's more of
>> a backward compatibility concern, right? Or do you have scenarios in
>> mind where this will really save on keystrokes?
> 
> The ability to visit an arbitrary Grep buffer and use 'next-error' in it
> to switch navigation should always work as the most reliable and implicit
> way to switch navigation.  This is the case no. 3 in next-error-find-buffer.

Not if there are, say, two Compilation buffers (or Grep and Occur, etc), 
and you switch to the least recently used one, and press M-g n.

It's probably fine, though.

>> On another note, perhaps we could add a message to next-error-select-buffer
>> that would be shown if we suspect this command will not have the expected
>> result for the user.
> 
> Or maybe ask the user using the minibuffer to resolve ambiguity.

Resolve it how? I think we can safely guess their intention, it's just 
not trivial to execute it.

Anyway, this problem should go away with the attached patch.

[-- Attachment #2: next-error-extract-case-2.diff --]
[-- Type: text/x-patch, Size: 3916 bytes --]

diff --git a/lisp/simple.el b/lisp/simple.el
index 111afa69d1..31d6cbcef2 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -211,6 +211,8 @@ next-error-find-buffer-function
   :type '(choice (const :tag "No default" ignore)
                  (const :tag "Single next-error capable buffer on selected frame"
                         next-error-buffer-on-selected-frame)
+                 (const :tag "Current buffer if next-error capable and outside navigation"
+                        next-error-buffer-try-current-outside-navigation)
                  (function :tag "Other function"))
   :group 'next-error
   :version "27.1")
@@ -240,6 +242,21 @@ next-error-buffer-on-selected-frame
     (if (eq (length window-buffers) 1)
         (car window-buffers))))
 
+(defun next-error-buffer-try-current-outside-navigation (&optional
+                                                         avoid-current
+                                                         extra-test-inclusive
+                                                         extra-test-exclusive)
+  "Maybe Return current buffer if it's next-error capable.
+But return nil if we navigated to the current buffer by the means
+of `next-error' command."
+  ;; Check that next-error-buffer has no buffer-local value
+  ;; (i.e. never navigated to the current buffer from another),
+  ;; and the current buffer is a `next-error' capable buffer.
+  (if (and (not (local-variable-p 'next-error-buffer))
+           (next-error-buffer-p (current-buffer) avoid-current
+                                extra-test-inclusive extra-test-exclusive))
+      (current-buffer)))
+
 (defun next-error-find-buffer (&optional avoid-current
 					 extra-test-inclusive
 					 extra-test-exclusive)
@@ -260,24 +277,16 @@ next-error-find-buffer
    (funcall next-error-find-buffer-function avoid-current
                                             extra-test-inclusive
                                             extra-test-exclusive)
-   ;; 2. If next-error-buffer has no buffer-local value
-   ;; (i.e. never navigated to the current buffer from another),
-   ;; and the current buffer is a `next-error' capable buffer,
-   ;; use it unconditionally, so next-error will always use it.
-   (if (and (not (local-variable-p 'next-error-buffer))
-            (next-error-buffer-p (current-buffer) avoid-current
-			         extra-test-inclusive extra-test-exclusive))
-       (current-buffer))
-   ;; 3. If next-error-last-buffer is an acceptable buffer, use that.
+   ;; 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
                                  extra-test-inclusive extra-test-exclusive))
        next-error-last-buffer)
-   ;; 4. If the current buffer is acceptable, choose it.
+   ;; 3. If the current buffer is acceptable, choose it.
    (if (next-error-buffer-p (current-buffer) avoid-current
 			    extra-test-inclusive extra-test-exclusive)
        (current-buffer))
-   ;; 5. Look for any acceptable buffer.
+   ;; 4. Look for any acceptable buffer.
    (let ((buffers (buffer-list)))
      (while (and buffers
                  (not (next-error-buffer-p
@@ -285,7 +294,7 @@ next-error-find-buffer
 		       extra-test-inclusive extra-test-exclusive)))
        (setq buffers (cdr buffers)))
      (car buffers))
-   ;; 6. Use the current buffer as a last resort if it qualifies,
+   ;; 5. Use the current buffer as a last resort if it qualifies,
    ;; even despite AVOID-CURRENT.
    (and avoid-current
 	(next-error-buffer-p (current-buffer) nil
@@ -293,7 +302,7 @@ next-error-find-buffer
 	(progn
 	  (message "This is the only buffer with error message locations")
 	  (current-buffer)))
-   ;; 7. Give up.
+   ;; 6. Give up.
    (error "No buffers contain error message locations")))
 
 (defun next-error (&optional arg reset)

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Juri Linkov]

>> But I'm not proud of the case no. 2, so you can drop it :)
>> Or better move it to a separate function and provide this function
>> as an option in next-error-find-buffer-function.
>
> Patch attached. Comments welcome.

Is this patch for master?  So there is enough time for thorough testing?

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Dmitry Gutov]

On 24.05.2020 01:24, Juri Linkov wrote:
> Is this patch for master?  So there is enough time for thorough testing?

I think it's for master. Though if we put it into emacs-27 and changed 
the default to the name of the new function, the behavior shouldn't 
change, and yet it would be easier to "fix" this bug through user 
configuration... (ʘ‿ʘ)

But your question seems to confirm that it's for master.

Suggestions for a better (shorter?) name for the new function welcome, 
by the way.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Juri Linkov]

>> Is this patch for master?  So there is enough time for thorough testing?
>
> I think it's for master. Though if we put it into emacs-27 and changed the
> default to the name of the new function, the behavior shouldn't change, and
> yet it would be easier to "fix" this bug through user
> configuration... (ʘ‿ʘ)

Yes, this will fix the reported problem of customizability.
Maybe Eli will agree to fix this in emacs-27.

> Suggestions for a better (shorter?) name for the new function welcome, by
> the way.

Maybe next-error-find-keep-navigation.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Dmitry Gutov]

On 25.05.2020 00:48, Juri Linkov wrote:
>>> Is this patch for master?  So there is enough time for thorough testing?
>> I think it's for master. Though if we put it into emacs-27 and changed the
>> default to the name of the new function, the behavior shouldn't change, and
>> yet it would be easier to "fix" this bug through user
>> configuration... (ʘ‿ʘ)
> Yes, this will fix the reported problem of customizability.
> Maybe Eli will agree to fix this in emacs-27.

I can post the corresponding patch, if it helps.

>> Suggestions for a better (shorter?) name for the new function welcome, by
>> the way.
> Maybe next-error-find-keep-navigation.

Short, but kinda cryptic (why "find"?). Not bad, but some other options:

next-error-maybe-current-keep-navigation
next-error-no-navigation-try-current

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 25 May 2020 04:58:05 +0300
> Cc: 40919@debbugs.gnu.org, tspiteri@ieee.org
> 
> On 25.05.2020 00:48, Juri Linkov wrote:
> >>> Is this patch for master?  So there is enough time for thorough testing?
> >> I think it's for master. Though if we put it into emacs-27 and changed the
> >> default to the name of the new function, the behavior shouldn't change, and
> >> yet it would be easier to "fix" this bug through user
> >> configuration... (ʘ‿ʘ)
> > Yes, this will fix the reported problem of customizability.
> > Maybe Eli will agree to fix this in emacs-27.
> 
> I can post the corresponding patch, if it helps.

It's okay to add a defcustom and a new function, but the other part of
the patch changes the default behavior, and that is less okay.  Can we
do one without the other?

(Btw, the textual descriptions of the options both in the patch and
those already in the code are confusingly obscure, so much so that I
don't think I could understand what each one does.)

All in all, I feel (for quite some time) that this area is
over-engineered and keeps bumping into more and more unintended
consequences.  Maybe it's time to take a step back and rethink the
entire subject?  (But definitely not on the release branch.)

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Dmitry Gutov]

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

On 25.05.2020 18:17, Eli Zaretskii wrote:

>>> Yes, this will fix the reported problem of customizability.
>>> Maybe Eli will agree to fix this in emacs-27.
>>
>> I can post the corresponding patch, if it helps.
> 
> It's okay to add a defcustom and a new function, but the other part of
> the patch changes the default behavior, and that is less okay.  Can we
> do one without the other?

The defcustom already exists. It currently defaults to #'ignore (do 
nothing). It is called in case #1.

The attached patch moves case #2 into a new function and makes it the 
default value of the said defcustom (thus case #2 effectively moves into 
case #1). As a result, the default behavior doesn't change, but the user 
will have a much easier time turning off case #2.

> (Btw, the textual descriptions of the options both in the patch and
> those already in the code are confusingly obscure, so much so that I
> don't think I could understand what each one does.)

Knowing the subject matter somewhat, I think the descriptions are 
meaningful enough, but to make sense of them one has to understand how 
the whole feature comes together. E.g. at what times 
next-error-find-buffer is called.

> All in all, I feel (for quite some time) that this area is
> over-engineered and keeps bumping into more and more unintended
> consequences.  Maybe it's time to take a step back and rethink the
> entire subject?  (But definitely not on the release branch.)

That's what we're doing here.

If the attached patch is accepted for emacs-27, then on master we'll 
change next-error-find-buffer-function's default back to #'ignore (thus 
reverting to the previous path that I sent). And see if we find problems 
with how the result is working.

So in the end, next-error-find-buffer-function will have three possible 
values:

- Pre Emacs 27 behavior.
- Emacs 27 behavior (extracted in the attached patch).
- Simplified behavior with less buffer-local state (Emacs 28, hopefully).

[-- Attachment #2: next-error-extract-case-2-keep-current-behavior.diff --]
[-- Type: text/x-patch, Size: 4333 bytes --]

diff --git a/lisp/simple.el b/lisp/simple.el
index 111afa69d1..a4b81719ce 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -199,7 +199,7 @@ next-error-buffer-p
 	   (and extra-test-inclusive
 		(funcall extra-test-inclusive))))))
 
-(defcustom next-error-find-buffer-function #'ignore
+(defcustom next-error-find-buffer-function #'next-error-no-navigation-try-current
   "Function called to find a `next-error' capable buffer.
 This functions takes the same three arguments as the function
 `next-error-find-buffer', and should return the buffer to be
@@ -211,6 +211,8 @@ next-error-find-buffer-function
   :type '(choice (const :tag "No default" ignore)
                  (const :tag "Single next-error capable buffer on selected frame"
                         next-error-buffer-on-selected-frame)
+                 (const :tag "Current buffer if next-error capable and outside navigation"
+                        next-error-no-navigation-try-current)
                  (function :tag "Other function"))
   :group 'next-error
   :version "27.1")
@@ -240,6 +242,22 @@ next-error-buffer-on-selected-frame
     (if (eq (length window-buffers) 1)
         (car window-buffers))))
 
+(defun next-error-no-navigation-try-current (&optional
+                                             avoid-current
+                                             extra-test-inclusive
+                                             extra-test-exclusive)
+  "Try the current buffer when outside navigation.
+But return nil if we navigated to the current buffer by the means
+of `next-error' command.  Othewise, return it if it's next-error
+capable."
+  ;; Check that next-error-buffer has no buffer-local value
+  ;; (i.e. we never navigated to the current buffer from another),
+  ;; and the current buffer is a `next-error' capable buffer.
+  (if (and (not (local-variable-p 'next-error-buffer))
+           (next-error-buffer-p (current-buffer) avoid-current
+                                extra-test-inclusive extra-test-exclusive))
+      (current-buffer)))
+
 (defun next-error-find-buffer (&optional avoid-current
 					 extra-test-inclusive
 					 extra-test-exclusive)
@@ -260,24 +278,16 @@ next-error-find-buffer
    (funcall next-error-find-buffer-function avoid-current
                                             extra-test-inclusive
                                             extra-test-exclusive)
-   ;; 2. If next-error-buffer has no buffer-local value
-   ;; (i.e. never navigated to the current buffer from another),
-   ;; and the current buffer is a `next-error' capable buffer,
-   ;; use it unconditionally, so next-error will always use it.
-   (if (and (not (local-variable-p 'next-error-buffer))
-            (next-error-buffer-p (current-buffer) avoid-current
-			         extra-test-inclusive extra-test-exclusive))
-       (current-buffer))
-   ;; 3. If next-error-last-buffer is an acceptable buffer, use that.
+   ;; 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
                                  extra-test-inclusive extra-test-exclusive))
        next-error-last-buffer)
-   ;; 4. If the current buffer is acceptable, choose it.
+   ;; 3. If the current buffer is acceptable, choose it.
    (if (next-error-buffer-p (current-buffer) avoid-current
 			    extra-test-inclusive extra-test-exclusive)
        (current-buffer))
-   ;; 5. Look for any acceptable buffer.
+   ;; 4. Look for any acceptable buffer.
    (let ((buffers (buffer-list)))
      (while (and buffers
                  (not (next-error-buffer-p
@@ -285,7 +295,7 @@ next-error-find-buffer
 		       extra-test-inclusive extra-test-exclusive)))
        (setq buffers (cdr buffers)))
      (car buffers))
-   ;; 6. Use the current buffer as a last resort if it qualifies,
+   ;; 5. Use the current buffer as a last resort if it qualifies,
    ;; even despite AVOID-CURRENT.
    (and avoid-current
 	(next-error-buffer-p (current-buffer) nil
@@ -293,7 +303,7 @@ next-error-find-buffer
 	(progn
 	  (message "This is the only buffer with error message locations")
 	  (current-buffer)))
-   ;; 7. Give up.
+   ;; 6. Give up.
    (error "No buffers contain error message locations")))
 
 (defun next-error (&optional arg reset)

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Eli Zaretskii]

> Cc: juri@linkov.net, 40919@debbugs.gnu.org, tspiteri@ieee.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 26 May 2020 02:17:34 +0300
> 
> The attached patch moves case #2 into a new function and makes it the 
> default value of the said defcustom (thus case #2 effectively moves into 
> case #1). As a result, the default behavior doesn't change, but the user 
> will have a much easier time turning off case #2.

Maybe I don't understand something, but it looks to me that this
changes the behavior if next-error-find-buffer-function has a
non-default value: previously what's now
next-error-no-navigation-try-current would be executed after calling
next-error-find-buffer-function, now it isn't.  Is that really
necessary?

> > (Btw, the textual descriptions of the options both in the patch and
> > those already in the code are confusingly obscure, so much so that I
> > don't think I could understand what each one does.)
> 
> Knowing the subject matter somewhat, I think the descriptions are 
> meaningful enough, but to make sense of them one has to understand how 
> the whole feature comes together. E.g. at what times 
> next-error-find-buffer is called.

I think I know something about the subject matter, and still the text
is quite impenetrable for me.

> > All in all, I feel (for quite some time) that this area is
> > over-engineered and keeps bumping into more and more unintended
> > consequences.  Maybe it's time to take a step back and rethink the
> > entire subject?  (But definitely not on the release branch.)
> 
> That's what we're doing here.

Sigh.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Dmitry Gutov]

On 26.05.2020 19:06, Eli Zaretskii wrote:
>> Cc: juri@linkov.net, 40919@debbugs.gnu.org, tspiteri@ieee.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Tue, 26 May 2020 02:17:34 +0300
>>
>> The attached patch moves case #2 into a new function and makes it the
>> default value of the said defcustom (thus case #2 effectively moves into
>> case #1). As a result, the default behavior doesn't change, but the user
>> will have a much easier time turning off case #2.
> 
> Maybe I don't understand something, but it looks to me that this
> changes the behavior if next-error-find-buffer-function has a
> non-default value:

Yes. I only claimed that the _default_ behavior doesn't change. The user 
option was only added in Emacs 27, so this doesn't seem to be a big problem.

> previously what's now
> next-error-no-navigation-try-current would be executed after calling
> next-error-find-buffer-function, now it isn't.  Is that really
> necessary?

It seems to be the best and safest option at the moment. I imagine that 
if someone customized the variable they either used the only available 
non-#'ignore value and thus want the Emacs 26 behavior (so taking out 
case #2 would only make them happier), or wrong their own function, in 
which case they might need to update that function.

>>> (Btw, the textual descriptions of the options both in the patch and
>>> those already in the code are confusingly obscure, so much so that I
>>> don't think I could understand what each one does.)
>>
>> Knowing the subject matter somewhat, I think the descriptions are
>> meaningful enough, but to make sense of them one has to understand how
>> the whole feature comes together. E.g. at what times
>> next-error-find-buffer is called.
> 
> I think I know something about the subject matter, and still the text
> is quite impenetrable for me.

Perhaps they could be improved. Still, the situation is probably better 
than it was before.

Do the 'next-error' entries in NEWS.27 make sense to you, BTW?

>>> All in all, I feel (for quite some time) that this area is
>>> over-engineered and keeps bumping into more and more unintended
>>> consequences.  Maybe it's time to take a step back and rethink the
>>> entire subject?  (But definitely not on the release branch.)
>>
>> That's what we're doing here.
> 
> Sigh.

Also see the related discussion in emacs-devel (one that stems from 2018).

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Eli Zaretskii]

> Cc: 40919@debbugs.gnu.org, tspiteri@ieee.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 26 May 2020 19:20:50 +0300
> 
> > Maybe I don't understand something, but it looks to me that this
> > changes the behavior if next-error-find-buffer-function has a
> > non-default value:
> 
> Yes. I only claimed that the _default_ behavior doesn't change. The user 
> option was only added in Emacs 27, so this doesn't seem to be a big problem.

OK.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Dmitry Gutov]

On 26.05.2020 19:33, Eli Zaretskii wrote:
>> Cc:40919@debbugs.gnu.org,tspiteri@ieee.org,juri@linkov.net
>> From: Dmitry Gutov<dgutov@yandex.ru>
>> Date: Tue, 26 May 2020 19:20:50 +0300
>>
>>> Maybe I don't understand something, but it looks to me that this
>>> changes the behavior if next-error-find-buffer-function has a
>>> non-default value:
>> Yes. I only claimed that the_default_  behavior doesn't change. The user
>> option was only added in Emacs 27, so this doesn't seem to be a big problem.
> OK.

Thank you.

(I'll take this to mean that the patch is OK for emacs-27.)

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Dmitry Gutov]

Version: 27.1

Aand pushed.

Trevor, you can now try different values of 
next-error-find-buffer-function (in particular, #'ignore) and see which 
behaves more to your liking.

Thanks all, closing.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Dmitry Gutov]

On 24.05.2020 01:24, Juri Linkov wrote:
>>> But I'm not proud of the case no. 2, so you can drop it:)
>>> Or better move it to a separate function and provide this function
>>> as an option in next-error-find-buffer-function.
>> Patch attached. Comments welcome.
> Is this patch for master?  So there is enough time for thorough testing?

And it's now in master.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Juri Linkov]

>>> Suggestions for a better (shorter?) name for the new function welcome, by
>>> the way.
>> Maybe next-error-find-keep-navigation.
>
> Short, but kinda cryptic (why "find"?). Not bad, but some other options:

Sorry, a typo, I meant next-error-keep-navigation.

> next-error-maybe-current-keep-navigation
> next-error-no-navigation-try-current

Not bad, I guess it's difficult to find a name that it both short and self-describing.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Juri Linkov]

>>>> But I'm not proud of the case no. 2, so you can drop it:)
>>>> Or better move it to a separate function and provide this function
>>>> as an option in next-error-find-buffer-function.
>>> Patch attached. Comments welcome.
>> Is this patch for master?  So there is enough time for thorough testing?
>
> And it's now in master.

Thanks.  Tho I think in master next-error-find-buffer-function
should support a list of functions, so the user could customize
their order to arrange their priorities.  Then they could be called
with run-hook-with-args-until-success.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Dmitry Gutov]

On 02.06.2020 01:41, Juri Linkov wrote:
> Tho I think in master next-error-find-buffer-function
> should support a list of functions, so the user could customize
> their order to arrange their priorities.  Then they could be called
> with run-hook-with-args-until-success.

I suppose.

We should perhaps wait and see if people do customize that variable at 
all. But the idea is solid.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Juri Linkov]

>> next-error-maybe-current-keep-navigation
>> next-error-no-navigation-try-current
>
> Not bad, I guess it's difficult to find a name that it both short and self-describing.

I found a better name:

next-error-buffer-unnavigated-current

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Juri Linkov]

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

>> Tho I think in master next-error-find-buffer-function
>> should support a list of functions, so the user could customize
>> their order to arrange their priorities.  Then they could be called
>> with run-hook-with-args-until-success.
>
> I suppose.
>
> We should perhaps wait and see if people do customize that variable at
> all. But the idea is solid.

Actually, people already want to customize it to a list (at least, I know one such user :)
So here is a patch:


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

diff --git a/lisp/simple.el b/lisp/simple.el
index 0fe8a1025c..646236879c 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -199,7 +199,7 @@ next-error-buffer-p
 	   (and extra-test-inclusive
 		(funcall extra-test-inclusive))))))
 
-(defcustom next-error-find-buffer-function #'ignore
+(defcustom next-error-find-buffer-function '(ignore)
   "Function called to find a `next-error' capable buffer.
 This functions takes the same three arguments as the function
 `next-error-find-buffer', and should return the buffer to be
@@ -208,12 +208,13 @@ next-error-find-buffer-function
 If the function returns nil, `next-error-find-buffer' will
 try to use the buffer it used previously, and failing that
 all other buffers."
-  :type '(choice (const :tag "No default" ignore)
-                 (const :tag "Single next-error capable buffer on selected frame"
-                        next-error-buffer-on-selected-frame)
-                 (const :tag "Current buffer if next-error capable and outside navigation"
-                        next-error-no-navigation-try-current)
-                 (function :tag "Other function"))
+  :type '(repeat
+          (choice (const :tag "No default" ignore)
+                  (const :tag "Single next-error capable buffer on selected frame"
+                         next-error-buffer-on-selected-frame)
+                  (const :tag "Current buffer if next-error capable and outside navigation"
+                         next-error-no-navigation-try-current)
+                  (function :tag "Other function")))
   :group 'next-error
   :version "28.1")
 
@@ -275,9 +276,13 @@ next-error-find-buffer
 that buffer is rejected."
   (or
    ;; 1. If a customizable function returns a buffer, use it.
-   (funcall next-error-find-buffer-function avoid-current
-                                            extra-test-inclusive
-                                            extra-test-exclusive)
+   (or (and (functionp next-error-find-buffer-function)
+            (funcall next-error-find-buffer-function avoid-current
+                     extra-test-inclusive extra-test-exclusive))
+       (and (listp next-error-find-buffer-function)
+            (run-hook-with-args-until-success
+             '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

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Dmitry Gutov]

On 11.06.2020 02:03, Juri Linkov wrote:
>>> next-error-maybe-current-keep-navigation
>>> next-error-no-navigation-try-current
>> Not bad, I guess it's difficult to find a name that it both short and self-describing.
> I found a better name:
> 
> next-error-buffer-unnavigated-current

Sounds okay to me.

But... do we rename it on the emacs-27 branch? Or do we keep it and 
introduce an obsolete alias in master?

Considering the above complication, I would just keep the current name. 
But it's up to you (and Eli, I guess).

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Dmitry Gutov]

On 11.06.2020 02:05, Juri Linkov wrote:
> Actually, people already want to customize it to a list (at least, I know one such user:)

Say hello to the user to me. ;-)

And maybe ask a question: what kind of functions do they want to put on 
there?

And/or would they be content to advice-add on 
next-error-find-buffer-function instead?

> -(defcustom next-error-find-buffer-function #'ignore
> +(defcustom next-error-find-buffer-function '(ignore)

                                              ^s, maybe?

> +   (or (and (functionp next-error-find-buffer-function)
> +            (funcall next-error-find-buffer-function avoid-current
> +                     extra-test-inclusive extra-test-exclusive))
> +       (and (listp next-error-find-buffer-function)
> +            (run-hook-with-args-until-success
> +             'next-error-find-buffer-function avoid-current
> +             extra-test-inclusive extra-test-exclusive)))

Looks like run_hook_with_args can deal with the case where the value of 
the hook is a single function.

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

Should we take the rest of the cases in next-error-find-buffer and move 
them to the default value of the above hook?

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 11 Jun 2020 02:28:30 +0300
> Cc: 40919@debbugs.gnu.org, tspiteri@ieee.org
> 
> But... do we rename it on the emacs-27 branch? Or do we keep it and 
> introduce an obsolete alias in master?

The latter, I think.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Juri Linkov]

>> I found a better name:
>> next-error-buffer-unnavigated-current
>
> Sounds okay to me.
>
> But... do we rename it on the emacs-27 branch? Or do we keep it and
> introduce an obsolete alias in master?

It makes no sense to introduce an obsolete alias for the function
added two weeks ago.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Juri Linkov]

> what kind of functions do they want to put on there?

Both next-error-buffer-on-selected-frame and next-error-no-navigation-try-current.

> And/or would they be content to advice-add on
> next-error-find-buffer-function instead?

Is it possible to add advice-add by using customization?

>> -(defcustom next-error-find-buffer-function #'ignore
>> +(defcustom next-error-find-buffer-function '(ignore)
>
>                                              ^s, maybe?

Ok, when using as a hook it could be '-functions', but in case
of using advice-add it should be still '-function'.

>> +   (or (and (functionp next-error-find-buffer-function)
>> +            (funcall next-error-find-buffer-function avoid-current
>> +                     extra-test-inclusive extra-test-exclusive))
>> +       (and (listp next-error-find-buffer-function)
>> +            (run-hook-with-args-until-success
>> +             'next-error-find-buffer-function avoid-current
>> +             extra-test-inclusive extra-test-exclusive)))
>
> Looks like run_hook_with_args can deal with the case where the value of the
> hook is a single function.

Ok, if it's impossible to use advice-add then lets simplify the hook case.

>>      ;; 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
>
> Should we take the rest of the cases in next-error-find-buffer and move
> them to the default value of the above hook?

I don't think so, I don't believe someone might want to customize the
rest of the cases.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Fri, 12 Jun 2020 01:39:29 +0300
> Cc: 40919@debbugs.gnu.org, tspiteri@ieee.org
> 
> >> I found a better name:
> >> next-error-buffer-unnavigated-current
> >
> > Sounds okay to me.
> >
> > But... do we rename it on the emacs-27 branch? Or do we keep it and
> > introduce an obsolete alias in master?
> 
> It makes no sense to introduce an obsolete alias for the function
> added two weeks ago.

Then what was the part about renaming it on emacs-27 about?

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Juri Linkov]

>> >> I found a better name:
>> >> next-error-buffer-unnavigated-current
>> >
>> > Sounds okay to me.
>> >
>> > But... do we rename it on the emacs-27 branch? Or do we keep it and
>> > introduce an obsolete alias in master?
>> 
>> It makes no sense to introduce an obsolete alias for the function
>> added two weeks ago.
>
> Then what was the part about renaming it on emacs-27 about?

If you think that the new name is better than the name added 2 weeks ago,
then it's better to rename it on emacs-27, or not to rename it at all.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Dmitry Gutov]

On 12.06.2020 01:43, Juri Linkov wrote:
>> what kind of functions do they want to put on there?
> 
> Both next-error-buffer-on-selected-frame and next-error-no-navigation-try-current.
> 
>> And/or would they be content to advice-add on
>> next-error-find-buffer-function instead?
> 
> Is it possible to add advice-add by using customization?

No, or at least not yet. But if we know of only one user that wants this 
setup, surely that's not a problem?

By the way, you were going to evaluate the new default. Do you now think 
that it's problematic somehow (and, for instance, the previous was a 
better default), or do you want to change it as a purely personal 
preference?

>>> -(defcustom next-error-find-buffer-function #'ignore
>>> +(defcustom next-error-find-buffer-function '(ignore)
>>
>>                                               ^s, maybe?
> 
> Ok, when using as a hook it could be '-functions', but in case
> of using advice-add it should be still '-function'.

Yup. These are the two options.

>>>       ;; 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
>>
>> Should we take the rest of the cases in next-error-find-buffer and move
>> them to the default value of the above hook?
> 
> I don't think so, I don't believe someone might want to customize the
> rest of the cases.

Well, if you're sure about that.

Having them all on the hook seemed logical to me, but indeed I don't 
know how necessary that is.

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Juri Linkov]

>>> what kind of functions do they want to put on there?
>> Both next-error-buffer-on-selected-frame and
>> next-error-no-navigation-try-current.
>>
>>> And/or would they be content to advice-add on
>>> next-error-find-buffer-function instead?
>> Is it possible to add advice-add by using customization?
>
> No, or at least not yet. But if we know of only one user that wants this
> setup, surely that's not a problem?

It's a general problem that hindered the development of other features
that might benefit from customizable advice-add (namely set-multi-message).

> By the way, you were going to evaluate the new default. Do you now think
> that it's problematic somehow (and, for instance, the previous was a 
> better default), or do you want to change it as a purely
> personal preference?

Only personal preference, it seems the default in master is fine
for most users.

>>>>       ;; 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
>>>
>>> Should we take the rest of the cases in next-error-find-buffer and move
>>> them to the default value of the above hook?
>> I don't think so, I don't believe someone might want to customize the
>> rest of the cases.
>
> Well, if you're sure about that.
>
> Having them all on the hook seemed logical to me, but indeed I don't know
> how necessary that is.

The reason why I think no one might want to customize the rest of the cases
is because I believe that next-error-last-buffer is always non-nil, so
all other cases (i.e. 3, 4, 5, 6) are useless and never used.  Isn't it so?

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Juri Linkov]

>>> >> I found a better name:
>>> >> next-error-buffer-unnavigated-current
>>> >
>>> > Sounds okay to me.
>>> >
>>> > But... do we rename it on the emacs-27 branch? Or do we keep it and
>>> > introduce an obsolete alias in master?
>>>
>>> It makes no sense to introduce an obsolete alias for the function
>>> added two weeks ago.
>>
>> Then what was the part about renaming it on emacs-27 about?
>
> If you think that the new name is better than the name added 2 weeks ago,
> then it's better to rename it on emacs-27, or not to rename it at all.

I have to point out that according to the naming convention
the function should have the prefix next-error-buffer-,
so it seems the renaming in emacs-27 is inevitable?

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Dmitry Gutov]

On 15.06.2020 02:15, Juri Linkov wrote:

>>>> And/or would they be content to advice-add on
>>>> next-error-find-buffer-function instead?
>>> Is it possible to add advice-add by using customization?
>>
>> No, or at least not yet. But if we know of only one user that wants this
>> setup, surely that's not a problem?
> 
> It's a general problem that hindered the development of other features
> that might benefit from customizable advice-add (namely set-multi-message).

The Customize UI is definitely not my area, sorry (not as developer, nor 
as a user).

>> By the way, you were going to evaluate the new default. Do you now think
>> that it's problematic somehow (and, for instance, the previous was a
>> better default), or do you want to change it as a purely
>> personal preference?
> 
> Only personal preference, it seems the default in master is fine
> for most users.

Very good.

>> Having them all on the hook seemed logical to me, but indeed I don't know
>> how necessary that is.
> 
> The reason why I think no one might want to customize the rest of the cases
> is because I believe that next-error-last-buffer is always non-nil, so
> all other cases (i.e. 3, 4, 5, 6) are useless and never used.  Isn't it so?

change-log-mode doesn't set it (and it shouldn't). Maybe there will be 
other major modes like that. So #3 should be used sometimes.

#4 doesn't seem very intuitive/useful to me, and I don't understand #5 
(when is AVOID-CURRENT non-nil?)

bug#40919: 27.0.91; next-error-select-buffer does not always behave as documented

[Juri Linkov]

>>>>> And/or would they be content to advice-add on
>>>>> next-error-find-buffer-function instead?
>>>> Is it possible to add advice-add by using customization?
>>>
>>> No, or at least not yet. But if we know of only one user that wants this
>>> setup, surely that's not a problem?
>> It's a general problem that hindered the development of other features
>> that might benefit from customizable advice-add (namely set-multi-message).
>
> The Customize UI is definitely not my area, sorry (not as developer, nor as
> a user).

I retract my patch to use a list of next-error functions,
because it's easy to configure this using add-function:

(add-function :override next-error-find-buffer-function
              #'next-error-buffer-on-selected-frame)

(add-function :after-until next-error-find-buffer-function
              #'next-error-buffer-unnavigated-current)