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

[Trevor Spiteri]

Post by Trevor Spiteri
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 .

[Juri Linkov]

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

[Trevor Spiteri]

Post by Juri Linkov

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

Post by Juri Linkov
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.

[Juri Linkov]

Post by Trevor Spiteri
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)

[Trevor Spiteri]

Post by Juri Linkov

Post by Trevor Spiteri
I think the error is in the implementation.

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

[Juri Linkov]

Post by Trevor Spiteri

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

Post by Trevor Spiteri
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?

[Trevor Spiteri]

Post by Juri Linkov

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

Post by Trevor Spiteri
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.

[Juri Linkov]

Post by Trevor Spiteri

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

[Eli Zaretskii]

Date: Sun, 03 May 2020 02:38:10 +0300
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.

[Juri Linkov]

Post by Eli Zaretskii

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

[Dmitry Gutov]

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

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.

[Juri Linkov]

Post by Dmitry Gutov

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

Post by Dmitry Gutov
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.

Post by Dmitry Gutov
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.

[Dmitry Gutov]

Post by Juri Linkov

Post by Dmitry Gutov

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

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?

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

Post by Juri Linkov

Post by Dmitry Gutov
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.

Post by Juri Linkov

Post by Dmitry Gutov
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.

[Juri Linkov]

Post by Dmitry Gutov

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

[Dmitry Gutov]

Post by 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... (ʘ‿ʘ)

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.

[Juri Linkov]

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

[Dmitry Gutov]

Post by Juri Linkov

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

I can post the corresponding patch, if it helps.

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

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

[Eli Zaretskii]

Date: Mon, 25 May 2020 04:58:05 +0300

Post by Juri Linkov

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

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

[Dmitry Gutov]

Post by Eli Zaretskii

Post by Dmitry Gutov

Post by Juri Linkov
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.

Post by Eli Zaretskii
(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.

Post by Eli Zaretskii
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).

[Eli Zaretskii]

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?

Post by Eli Zaretskii
(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.

Post by Eli Zaretskii
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.

[Dmitry Gutov]

Post by Eli Zaretskii

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

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.

Post by Eli Zaretskii
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.

Post by Eli Zaretskii

Post by Eli Zaretskii
(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?

Post by Eli Zaretskii

Post by Eli Zaretskii
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).

[Eli Zaretskii]

Date: Tue, 26 May 2020 19:20:50 +0300

Post by Eli Zaretskii
Maybe I don't understand something, but it looks to me that this
changes the behavior if next-error-find-buffer-function has a

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.

[Dmitry Gutov]

Date: Tue, 26 May 2020 19:20:50 +0300

Post by Eli Zaretskii
Maybe I don't understand something, but it looks to me that this
changes the behavior if next-error-find-buffer-function has a

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

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

[Dmitry Gutov]

Post by Juri Linkov

Post by Dmitry Gutov

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.

[Juri Linkov]

Post by Dmitry Gutov

Post by Juri Linkov

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

Maybe next-error-find-keep-navigation.

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

Post by Dmitry Gutov
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.

[Juri Linkov]

Post by Dmitry Gutov

Post by Juri Linkov

Post by Dmitry Gutov

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.

[Dmitry Gutov]

Post by Juri Linkov
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.

[Juri Linkov]

Post by Juri Linkov

Post by Dmitry Gutov
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

[Juri Linkov]

Post by Dmitry Gutov

Post by Juri Linkov
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:

[Dmitry Gutov]

Post by Juri Linkov

Post by Juri Linkov

Post by Dmitry Gutov
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.

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

[Dmitry Gutov]

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?

[Eli Zaretskii]

Date: Thu, 11 Jun 2020 02:28:30 +0300
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.

[Juri Linkov]

Post by Dmitry Gutov

Post by Juri Linkov
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.

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

[Eli Zaretskii]

Date: Fri, 12 Jun 2020 01:39:29 +0300

Post by Dmitry Gutov

Post by Juri Linkov
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?

[Juri Linkov]

Post by Eli Zaretskii

Post by Juri Linkov

Post by Dmitry Gutov

Post by Juri Linkov
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.

[Dmitry Gutov]

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

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?

Post by Juri Linkov

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

Post by Juri Linkov

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

[Juri Linkov]

Post by Dmitry Gutov

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

Post by Dmitry Gutov
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.

Post by Dmitry Gutov

Post by Juri Linkov

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

[Juri Linkov]

Post by Juri Linkov

Post by Eli Zaretskii

Post by Juri Linkov

Post by Dmitry Gutov

Post by Juri Linkov
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?

[Dmitry Gutov]

Post by Juri Linkov

Post by Dmitry Gutov

Post by Juri Linkov

Post by Dmitry Gutov
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).

Post by Juri Linkov

Post by Dmitry Gutov
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.

Post by Juri Linkov

Post by Dmitry Gutov
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?)

[Juri Linkov]

Post by Juri Linkov

Post by Dmitry Gutov

Post by Juri Linkov

Post by Dmitry Gutov
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)