bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

I've been trying to understand why when the current file-vising buffer
has next-error-function set localy, sometimes `next-error' uses it, and
sometimes it uses next-error-function from next-error-last-buffer.

Looking at the current `next-error-find-buffer', the logic is this:

- If a buffer visible in the current frame has next-error-function set,
  *and* if there's only one such buffer, use it.

- If next-error-last-buffer has it set, use that.

Othewise, if both of the above fail,

- If the current buffer has next-error-function set, use it.

That's nonsense. Why should the question of whether the current buffer's
next-error-function is used be decided by whether there are any other
visible buffers with that variable set.

Apparently, this peculiarity has been there for 10.5 years now,
introduced in 03e75c7e0 by Juri Linkov. But there's no bug reference, nor
a link to a discussion.

I'm guessing it was an attempt to solve a problem of
next-error-last-buffer never being used if the current buffer has
next-error-function. But I think the solution is worse than the problem.
It least the previous behavior was consistent.

In GNU Emacs 25.0.50.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.12.2)
 of 2015-05-02 on axl
Windowing system distributor `The X.Org Foundation', version 11.0.11601901
System Description:	Ubuntu 14.10

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Stefan Monnier]

> Apparently, this peculiarity has been there for 10.5 years now,
> introduced in 03e75c7e0 by Juri Linkov. But there's no bug reference, nor
> a link to a discussion.

IIRC this had something to do with hitting C-x ` to jump through the
various elements of a *compile* or *grep* buffer, where some of those
C-x ` may end up jumping to a buffer that itself has
next-error-function set.


        Stefan

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 05/03/2015 08:49 AM, Stefan Monnier wrote:

> IIRC this had something to do with hitting C-x ` to jump through the
> various elements of a *compile* or *grep* buffer, where some of those
> C-x ` may end up jumping to a buffer that itself has
> next-error-function set.

That's what I guessed, then. It doesn't solve the problem completely 
anyway. If the *grep* buffer is buried or displayed in a different frame 
(which is arguably is the best case for using C-x `), C-x ` might open a 
buffer with next-error-function set, and it will overtake, if it's the 
only one such in the currently visible frame. There's no easy way to get 
back to Grep's next-error-function either.

Why don't we prioritize, in next-error-find-buffer, 
next-error-last-buffer over everything else (change the order to 2 3 1 4 
5 6)?

And then add some way to change next-error-last-buffer programmatically 
(choosing between the current buffer, if it has next-error-function set, 
and all other buffers with next-error-function set and buffer-file-name 
nil; defaulting to the current one).

M-0 C-x ` (like suggested by Vitaly) might be OK, to change the used 
buffer and move to the next error in one step (and similarly in 
previous-error). But that hijacks the meaning of 0 argument.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Ted Zlatanov]

On Sun, 3 May 2015 15:56:11 +0300 Dmitry Gutov <dgutov@yandex.ru> wrote: 

DG> Why don't we prioritize, in next-error-find-buffer,
DG> next-error-last-buffer over everything else (change the order to 2 3 1
DG> 4 5 6)?

For reference, here are the steps:

   ;; 1. If one window on the selected frame displays such buffer, return it.
   ;; 2. If next-error-last-buffer is an acceptable buffer, use that.
   ;; 3. If the current buffer is acceptable, choose it.
   ;; 4. Look for any acceptable buffer.
   ;; 5. Use the current buffer as a last resort if it qualifies,
   ;; 6. Give up.

How about a `next-error-priority' which can be numerically:

* local (each position refers to the file itself) = 0 (occur)
* finder (each position refers to other files) = 100 (compile, grep)
* or can be set by the mode to something else

Then (1) can become "if one window on the selected frame has the highest
priority, return it."

Ted

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 05/05/2015 01:03 AM, Ted Zlatanov wrote:

> Then (1) can become "if one window on the selected frame has the highest
> priority, return it."

How will this help the user to control which error function to use next?

And why the windows on the selected frame? If *compile* is buried, does 
it become useless? I'd say the opposite.

Or suppose I have 4 windows open in the frame, and each one's buffer has 
a next-error-function that refers only to positions in the current file? 
And there's a *compile* buffer buried somewhere. How do I actually use 
the current buffer's next-error-function, aside from C-x 1?

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Ted Zlatanov]

On Tue, 5 May 2015 01:22:39 +0300 Dmitry Gutov <dgutov@yandex.ru> wrote: 

DG> On 05/05/2015 01:03 AM, Ted Zlatanov wrote:
>> Then (1) can become "if one window on the selected frame has the highest
>> priority, return it."

DG> How will this help the user to control which error function to use next?

It doesn't help the user.  It helps mode writers give their modes the
appropriate priority so the user doesn't have to know.

DG> And why the windows on the selected frame? If *compile* is buried,
DG> does it become useless? I'd say the opposite.

Perhaps?  I am giving a suggestion, not a full implementation.  The
essential thing is that we rank buffers, not just pick "the last one" or
the "one visible with 5 hieroglyphs" or whatever other DWIMmery we
currently use.

DG> Or suppose I have 4 windows open in the frame, and each one's buffer
DG> has a next-error-function that refers only to positions in the current
DG> file? And there's a *compile* buffer buried somewhere. How do I
DG> actually use the current buffer's next-error-function, aside from C-x
DG> 1?

Some history: I actually, long ago when `next-error' turned into a
navigation facility, we[1] had the idea of "meta next-error" which would
navigate one level higher than local. That would have made this whole
discussion, including rankings by priority, moot by simply saying "to
navigate between files (compile, grep) you use `meta-next-error' or
whatever it's called. to navigate inside file locations, use
`next-error'."

I was supposed to write a patch after the release but... the dog ate my
TODO list.

Perhaps you are interested in adapting that code instead of hacking on
the current scheme?  Or should I retry implementing it?  9 years late is
not too bad, right? :)

If there's a time to implement this, I'd say it's now, before the 25.1
release, because it will break how many people expect `next-error' to
work.  But OTOH I think it will improve the UI.

Thanks
Ted

[1] Lars, me, and others: https://lists.gnu.org/archive/html/emacs-devel/2006-04/msg00488.html

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 05/05/2015 01:33 AM, Ted Zlatanov wrote:

> Some history: I actually, long ago when `next-error' turned into a
> navigation facility, we[1] had the idea of "meta next-error" which would
> navigate one level higher than local. That would have made this whole
> discussion, including rankings by priority, moot by simply saying "to
> navigate between files (compile, grep) you use `meta-next-error' or
> whatever it's called. to navigate inside file locations, use
> `next-error'."

Thanks for the link.

That would've been an improvement, but it wouldn't solve a related 
problem: when *grep* buffer was created after a *compile* one, how to 
get back to using *compile*'s list of errors.

> Perhaps you are interested in adapting that code instead of hacking on
> the current scheme?  Or should I retry implementing it?  9 years late is
> not too bad, right? :)

9 years is just right, but I'm not sure how much of that implementation 
we would reuse. It's also not obvious me how to move to the next file, 
if you only have a next-error-function.

M-g M-f/b could switch between next-error-last-buffer values, though. 
Especially if they're organized in a ring, like Helmut suggested. That 
will require an update to any Compilation-like mode.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Ted Zlatanov]

On Tue, 5 May 2015 18:05:29 +0300 Dmitry Gutov <dgutov@yandex.ru> wrote: 

DG> On 05/05/2015 01:33 AM, Ted Zlatanov wrote:
>> Some history: I actually, long ago when `next-error' turned into a
>> navigation facility, we[1] had the idea of "meta next-error" which would
>> navigate one level higher than local. That would have made this whole
>> discussion, including rankings by priority, moot by simply saying "to
>> navigate between files (compile, grep) you use `meta-next-error' or
>> whatever it's called. to navigate inside file locations, use
>> `next-error'."

DG> Thanks for the link.

DG> That would've been an improvement, but it wouldn't solve a related
DG> problem: when *grep* buffer was created after a *compile* one, how to
DG> get back to using *compile*'s list of errors.

I think we have to accept the visible or last used one in that case.

>> Perhaps you are interested in adapting that code instead of hacking on
>> the current scheme?  Or should I retry implementing it?  9 years late is
>> not too bad, right? :)

DG> 9 years is just right, but I'm not sure how much of that
DG> implementation we would reuse. It's also not obvious me how to move to
DG> the next file, if you only have a next-error-function.

I'm happy to make suggestions but please take this in a direction that
makes sense to you.

Ted

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

> - If a buffer visible in the current frame has next-error-function set,
>   *and* if there's only one such buffer, use it.
>
> - If next-error-last-buffer has it set, use that.
>
> Othewise, if both of the above fail,
>
> - If the current buffer has next-error-function set, use it.
>
> Apparently, this peculiarity has been there for 10.5 years now,
> introduced in 03e75c7e0 by Juri Linkov. But there's no bug reference, nor
> a link to a discussion.

Sorry, I missed this bug report and regret that it caused you to revert
xref's next-error-function integration.

The link to the discussion is here:
http://lists.gnu.org/archive/html/emacs-devel/2004-05/msg00614.html
where you can see the case that we need to support.

Now that we have window-local variables, it's possible to support
this case in a proper way.  Instead of checking if a buffer visible
in the current frame, we should check the window-local value of
next-error-last-buffer.  Thus invoking next-error in the window
with the source buffer will continue navigation using the right value
of next-error-last-buffer that navigated to its previous occurrence.

Thus, the steps will be the following:

1. If window-local next-error-last-buffer is an acceptable buffer, use that.
2. If next-error-last-buffer is an acceptable buffer, use that.
3. If the current buffer is acceptable, choose it.
4. Look for any acceptable buffer.
5. Use the current buffer as a last resort if it qualifies, even despite AVOID-CURRENT.
6. Give up.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

Hi Juri,

Thank you for the reply.

But having had time to consider this issue, I think there are benefits 
to how it currently works. For instance, if I want to switch from a 
Compilation's buffer next-error-function to the current file buffer's 
one, I just need to bury Compilation (doing the reverse might be harder, 
however). If we change this logic, we should make sure not to make 
anything worse.

On 01/25/2016 12:10 AM, Juri Linkov wrote:

> The link to the discussion is here:
> http://lists.gnu.org/archive/html/emacs-devel/2004-05/msg00614.html
> where you can see the case that we need to support.

In this scenario, it seems like you could have benefited from 
next-error-last-buffer value being frame-local instead (maybe 
implemented as a frame parameter?).

> Now that we have window-local variables, it's possible to support
> this case in a proper way.  Instead of checking if a buffer visible
> in the current frame, we should check the window-local value of
> next-error-last-buffer.  Thus invoking next-error in the window
> with the source buffer will continue navigation using the right value
> of next-error-last-buffer that navigated to its previous occurrence.

How do linting minor modes (like Flycheck or Flymake) fit into this?

Suppose I called M-x compile (or, better yet, M-x grep), navigated to 
some file buffer from it and then see that it has some linter errors 
highlighed by Flycheck. So I want to use the current buffer's 
next-error-function now, and jump between linter warnings using 
next/previous-error. How do I do that? IIU your plan correctly, the 
current window-local next-error-last-buffer value will continue pointing 
at the Grep buffer, even if I bury it.

Basically, I want to have two at least somewhat guessable sequences of 
actions that would let the user choose which buffer to use for its 
next-error-function.

As discussed in this issue, the best way to do that seems to require:

- Some indicator that a given buffer's next-error-function points to 
other buffer (then, if you're in a different buffer, that other buffer 
is still relevant). Like a buffer-local variable called, for example, 
next-error-function-nonlocal.

- A command (or several) to switch between the plausible candidates for 
next-error-last-buffer. Maybe just have a single command that uses 
read-buffer with a predicate checking the aforementioned variable and an 
extra option that means "just use the current buffer".

- Ignore next-error-last-buffer's visibility. Or make it frame-local, to 
account for your scenario as well (but that would bring extra 
complexity: some people use use frames like almost separate 
applications, and other can use frames instead of windows, and display 
them side-by-side).

WDYT?

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

>> The link to the discussion is here:
>> http://lists.gnu.org/archive/html/emacs-devel/2004-05/msg00614.html
>> where you can see the case that we need to support.
>
> In this scenario, it seems like you could have benefited from
> next-error-last-buffer value being frame-local instead (maybe implemented
> as a frame parameter?).

A frame might contain more pairs of navigational/target windows,
e.g. a window with *grep* output, and a file buffer visited from *grep*;
a window with *Occur*, and another place in the same file buffer
but visited from *Occur*, etc.

So frame-local is not of help here, whereas window-local is,
provided a navigational window (like *grep*/*compilation*)
always displays target buffers in another dedicated window.

As I see this is not what xref currently does: navigating in it
jumps between many different windows that is very inconvenient,
but this can be easily fixed to work the sane way as *grep*
or *compilation* already works.

>> Now that we have window-local variables, it's possible to support
>> this case in a proper way.  Instead of checking if a buffer visible
>> in the current frame, we should check the window-local value of
>> next-error-last-buffer.  Thus invoking next-error in the window
>> with the source buffer will continue navigation using the right value
>> of next-error-last-buffer that navigated to its previous occurrence.
>
> How do linting minor modes (like Flycheck or Flymake) fit into this?

Does Flycheck or Flymake display a navigation window with a list of results?
If not, then this is a new case that we need to support as well.

> Suppose I called M-x compile (or, better yet, M-x grep), navigated to some
> file buffer from it and then see that it has some linter errors highlighed
> by Flycheck. So I want to use the current buffer's next-error-function now,
> and jump between linter warnings using next/previous-error. How do I do
> that? IIU your plan correctly, the current window-local
> next-error-last-buffer value will continue pointing at the Grep buffer,
> even if I bury it.

What if you have two navigations in the same buffer, and both are
without a navigation window that you can't bury?

> Basically, I want to have two at least somewhat guessable sequences of
> actions that would let the user choose which buffer to use for its
> next-error-function.
>
> As discussed in this issue, the best way to do that seems to require:
>
> - Some indicator that a given buffer's next-error-function points to other
> buffer (then, if you're in a different buffer, that other buffer is still
> relevant). Like a buffer-local variable called, for example,
> next-error-function-nonlocal.

Do you mean to bind a navigation buffer with navigated target buffers/windows?

> - A command (or several) to switch between the plausible candidates for
> next-error-last-buffer. Maybe just have a single command that uses
> read-buffer with a predicate checking the aforementioned variable and an
> extra option that means "just use the current buffer".

This would be too complicated to use.

> - Ignore next-error-last-buffer's visibility. Or make it frame-local, to
> account for your scenario as well (but that would bring extra complexity:
> some people use use frames like almost separate applications, and other can
> use frames instead of windows, and display them side-by-side).

Buffers are displayed in windows, so better to bind them to windows.

> WDYT?

I remember the original idea was to always continue the same navigation
that displayed a given target buffer/window, so switching to another
navigation in the same window could be achieved by explicitly navigating
to another result from another navigation, e.g. when current navigation
was from *compilation* then switching to *grep* buffer and typing M-n
for the next grep hit in the same file buffer.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 01/26/2016 12:55 AM, Juri Linkov wrote:

> A frame might contain more pairs of navigational/target windows,
> e.g. a window with *grep* output, and a file buffer visited from *grep*;
> a window with *Occur*, and another place in the same file buffer
> but visited from *Occur*, etc.

Yes, but it wasn't your scenario there. In that scenario, you switched 
to a different frame, which had only a *compilation* buffer visible, and 
were surprised to see a next-error-function used from a non-visible 
buffer you last accessed in a different frame.

> So frame-local is not of help here, whereas window-local is,
> provided a navigational window (like *grep*/*compilation*)
> always displays target buffers in another dedicated window.

How does it really help? You've navigated from *grep*, to that buffer A, 
then did that from *compilation*, and then you can't continue jumping to 
next *grep* occurrences from that buffer.

frame-local could be sufficient if we were sure that different 
navigation windows show error locations in distinctly different sets of 
windows, IMHO.

We can fix xref, but not every next-error-function uses the same window, 
and that's not codified in this variable's docstring. 
change-log-next-error doesn't.

> As I see this is not what xref currently does: navigating in it
> jumps between many different windows that is very inconvenient,
> but this can be easily fixed to work the sane way as *grep*
> or *compilation* already works.

Yes, this can use an improvement.

>> How do linting minor modes (like Flycheck or Flymake) fit into this?
>
> Does Flycheck or Flymake display a navigation window with a list of results?

Flycheck has such a buffer, but it's not displayed by default. And in 
any case, it sets next-error-function locally in file buffers, but not 
in its "list of errors" buffer.

Hmm. Apparently, Flymake doesn't set next-error-function, so we can 
disregard it.

js2-mode is another example: it's a major mode that sets 
next-error-function in its buffers. You can ask it for the "list of 
errors" buffer as well, but it's usually not shown.

> If not, then this is a new case that we need to support as well.

Apparently so.

>> Suppose I called M-x compile (or, better yet, M-x grep), navigated to some
>> file buffer from it and then see that it has some linter errors highlighed
>> by Flycheck. So I want to use the current buffer's next-error-function now,
>> and jump between linter warnings using next/previous-error. How do I do
>> that? IIU your plan correctly, the current window-local
>> next-error-last-buffer value will continue pointing at the Grep buffer,
>> even if I bury it.
>
> What if you have two navigations in the same buffer, and both are
> without a navigation window that you can't bury?

I don't follow. The above was an attempt to point out an hole in your 
plan. I'm not sure you can refute that with a "what if" counter-example.

>> - Some indicator that a given buffer's next-error-function points to other
>> buffer (then, if you're in a different buffer, that other buffer is still
>> relevant). Like a buffer-local variable called, for example,
>> next-error-function-nonlocal.
>
> Do you mean to bind a navigation buffer with navigated target buffers/windows?

I mean to ask compilation-mode (as well as other similar modes) to 
(setq-local next-error-function-nonlocal t), in addition to setting 
next-error-function and next-error-last-buffer.

>> - A command (or several) to switch between the plausible candidates for
>> next-error-last-buffer. Maybe just have a single command that uses
>> read-buffer with a predicate checking the aforementioned variable and an
>> extra option that means "just use the current buffer".
>
> This would be too complicated to use.

Why complicated? It would just be a way to choose the source of errors 
to follow. You'd also be able to do that by clicking on an error, or 
pressing RET, in the "nonlocal" navigation buffers.

The main point, however, which you might not agree with, is to make 
next-error-last-buffer global.

>> - Ignore next-error-last-buffer's visibility. Or make it frame-local, to
>> account for your scenario as well (but that would bring extra complexity:
>> some people use use frames like almost separate applications, and other can
>> use frames instead of windows, and display them side-by-side).
>
> Buffers are displayed in windows, so better to bind them to windows.

Making next-error-last-buffer window-local feels clunkier to me: there 
would be no indication that a given window is following *Compilation*, 
for example. And up until now, next-error worked more or less in a 
global fashion.

> I remember the original idea was to always continue the same navigation
> that displayed a given target buffer/window, so switching to another
> navigation in the same window could be achieved by explicitly navigating
> to another result from another navigation, e.g. when current navigation
> was from *compilation* then switching to *grep* buffer and typing M-n
> for the next grep hit in the same file buffer.

How will you "switch" to the next-error-function set locally by Flycheck 
in the current file-visiting buffer?

How will you switch between Grep and Compilation if they display a 
location in the same buffer (and window)? Won't the desired navigation 
buffer have to be visible? So you'd have to select some window, switch 
to that buffer in it, and then click or press RET on some error?

Using a command to switch between next-error-last-buffer candidates 
seems much quicker.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

> How does it really help? You've navigated from *grep*, to that buffer A,
> then did that from *compilation*, and then you can't continue jumping to
> next *grep* occurrences from that buffer.

I suppose that navigation from every navigational window displays its targets
in own dedicated window that will be associated with its “parent” window.
So e.g. after navigating from *grep* to window A, and from *compilation*
to window B, next-error invoked from windows A or B will continue
the right navigation.

> We can fix xref, but not every next-error-function uses the same window,
> and that's not codified in this variable's docstring. change-log-next-error
> doesn't.

If some next-error-function doesn't use the same window, still there is
no problem because its displayed window will continue the last navigation
visited in that window.

>>> Suppose I called M-x compile (or, better yet, M-x grep), navigated to some
>>> file buffer from it and then see that it has some linter errors highlighed
>>> by Flycheck. So I want to use the current buffer's next-error-function now,
>>> and jump between linter warnings using next/previous-error. How do I do
>>> that? IIU your plan correctly, the current window-local
>>> next-error-last-buffer value will continue pointing at the Grep buffer,
>>> even if I bury it.
>>
>> What if you have two navigations in the same buffer, and both are
>> without a navigation window that you can't bury?
>
> I don't follow. The above was an attempt to point out an hole in your
> plan. I'm not sure you can refute that with a "what if" counter-example.

This adds another problematic case to consider, but we could avoid it
by always requiring creation of a navigation buffer, possibly hidden
when necessary.  (As for your point about a hole, I already addressed it
below - that requires unburing a navigation buffer that you want switch to).

>>> - Some indicator that a given buffer's next-error-function points to other
>>> buffer (then, if you're in a different buffer, that other buffer is still
>>> relevant). Like a buffer-local variable called, for example,
>>> next-error-function-nonlocal.
>>
>> Do you mean to bind a navigation buffer with navigated target buffers/windows?
>
> I mean to ask compilation-mode (as well as other similar modes) to
> (setq-local next-error-function-nonlocal t), in addition to setting
> next-error-function and next-error-last-buffer.

How this could help to point to other buffer?

>>> - A command (or several) to switch between the plausible candidates for
>>> next-error-last-buffer. Maybe just have a single command that uses
>>> read-buffer with a predicate checking the aforementioned variable and an
>>> extra option that means "just use the current buffer".
>>
>> This would be too complicated to use.
>
> Why complicated? It would just be a way to choose the source of errors to
> follow. You'd also be able to do that by clicking on an error, or pressing
> RET, in the "nonlocal" navigation buffers.

I think it would be more WYSIWYG first to switch to the navigation buffer,
and then to click on an error, or press RET.

> The main point, however, which you might not agree with, is to make
> next-error-last-buffer global.

I prefer this precedence:
1. window-local next-error-last-buffer
2. buffer-local next-error-last-buffer
3. global next-error-last-buffer

>>> - Ignore next-error-last-buffer's visibility. Or make it frame-local, to
>>> account for your scenario as well (but that would bring extra complexity:
>>> some people use use frames like almost separate applications, and other can
>>> use frames instead of windows, and display them side-by-side).
>>
>> Buffers are displayed in windows, so better to bind them to windows.
>
> Making next-error-last-buffer window-local feels clunkier to me: there
> would be no indication that a given window is following *Compilation*, for
> example. And up until now, next-error worked more or less in
> a global fashion.

Are you hinting that currently there is such indication in the form of
navigation buffer's window displayed in the same frame (rule#1 in
next-error-find-buffer)?

I proposed window-local next-error-last-buffer only because you had
some problems with this rule using in xref.

>> I remember the original idea was to always continue the same navigation
>> that displayed a given target buffer/window, so switching to another
>> navigation in the same window could be achieved by explicitly navigating
>> to another result from another navigation, e.g. when current navigation
>> was from *compilation* then switching to *grep* buffer and typing M-n
>> for the next grep hit in the same file buffer.
>
> How will you "switch" to the next-error-function set locally by Flycheck in
> the current file-visiting buffer?

By restarting Flycheck?

> How will you switch between Grep and Compilation if they display a location
> in the same buffer (and window)? Won't the desired navigation buffer have
> to be visible? So you'd have to select some window, switch to that buffer
> in it, and then click or press RET on some error?

Yes.

> Using a command to switch between next-error-last-buffer candidates seems
> much quicker.

In case of Flycheck, there will be no next-error-last-buffer, no?

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 01/27/2016 03:57 AM, Juri Linkov wrote:

> I suppose that navigation from every navigational window displays its targets
> in own dedicated window that will be associated with its “parent” window.

That's a considerable change. Associated how? The window itself won't 
have any indication with which navigation buffer it's associated with, 
will it?

And what if you do have the same buffer references in different 
navigation windows? Will they have to show it in both associated 
windows? That seems wasteful.

> So e.g. after navigating from *grep* to window A, and from *compilation*
> to window B, next-error invoked from windows A or B will continue
> the right navigation.

Suppose you've did that, then turned left to look out of the window for 
half a minute, then looked back at Emacs. How are you going to predict 
what M-x next-error will do?

Now, in addition to remembering which navigational command you ran last, 
you have to remember the window associations.

>> We can fix xref, but not every next-error-function uses the same window,
>> and that's not codified in this variable's docstring. change-log-next-error
>> doesn't.
>
> If some next-error-function doesn't use the same window, still there is
> no problem because its displayed window will continue the last navigation
> visited in that window.

And soon, all windows in the frame will refer to the ChangeLog buffer as 
its next-error-last-buffer, right?

`next-error' always continuing the last navigation does not require 
window-local values. We can just have next-error-last-buffer to be global.

And if you're still thinking of the two-frame scenario, here's a little 
modification:

- Run M-x grep in one frame, with several windows, jump to an error 
there, in window A.
- Switch to another frame, run M-x compile there.
- Switch back to the first frame. But select some other window than A. 
Call M-x next-error, and see it use the location from the *compilation* 
buffer, even though *grep* is visible right there.

That scenario looks just as counter-intuitive to me as the original one. 
Your proposal would only fix the original one.

> This adds another problematic case to consider, but we could avoid it
> by always requiring creation of a navigation buffer, possibly hidden
> when necessary.  (As for your point about a hole, I already addressed it
> below - that requires unburing a navigation buffer that you want switch to).

One navigational buffer per file buffer? That's too much.

>> I mean to ask compilation-mode (as well as other similar modes) to
>> (setq-local next-error-function-nonlocal t), in addition to setting
>> next-error-function and next-error-last-buffer.
>
> How this could help to point to other buffer?

It will only tag it to be available for completion in the new 
pick-next-error-last-buffer command.

Or we can put those buffers in a ring, which was also suggested here 
previously. That would also tag them, and allow switching between them 
in a historical fashion.

>> Why complicated? It would just be a way to choose the source of errors to
>> follow. You'd also be able to do that by clicking on an error, or pressing
>> RET, in the "nonlocal" navigation buffers.
>
> I think it would be more WYSIWYG first to switch to the navigation buffer,
> and then to click on an error, or press RET.

That can also be a way to change the global next-error-last-buffer 
value. And whether it's window-local or not, neither scenario solves the 
Flycheck problem.

>> The main point, however, which you might not agree with, is to make
>> next-error-last-buffer global.
>
> I prefer this precedence:
> 1. window-local next-error-last-buffer
> 2. buffer-local next-error-last-buffer
> 3. global next-error-last-buffer

My preference is: 3, 2. Except we never set next-error-last-buffer 
locally, so we should interpret the global nil value to mean "the 
current buffer".

>> Making next-error-last-buffer window-local feels clunkier to me: there
>> would be no indication that a given window is following *Compilation*, for
>> example. And up until now, next-error worked more or less in
>> a global fashion.
>
> Are you hinting that currently there is such indication in the form of
> navigation buffer's window displayed in the same frame (rule#1 in
> next-error-find-buffer)?

That an one option - we can indeed keep some logic reliant on whether 
next-error-last-buffer is visible in the current frame (but drop the 
"not more than one" limitation, because it leads to non-intuitive behavior).

Then the change to the API could be smaller, but the use case of using 
next-error with non-visible navigational buffers will be more broken 
than currently.

But my first choice is to not rely on buffer visibility at all, and 
simply follow the current global next-error-last-buffer value, as well 
as provide an easy way to switch to a different one.

> I proposed window-local next-error-last-buffer only because you had
> some problems with this rule using in xref.

Yes, I did. But IIUC, Eli had more of a problem with the

(if (eq (length window-buffers) 1)

part of that rule. The commit that disabled next-error integration in 
xref has a link to that discussion in its message.

>> How will you "switch" to the next-error-function set locally by Flycheck in
>> the current file-visiting buffer?
>
> By restarting Flycheck?

Restarting how? It's a minor mode that triggers linting checks when the 
buffer is saved, or you've typed something and wait a bit, or the file 
buffer has just been opened. The exact set of conditions is customizable.

And it doesn't set next-error-last-buffer at all.

But suppose it did. Imagine: you M-x compile, jump to an error, that 
opens the file in a new buffer, Flycheck kicks in, runs its linting 
check, and sets next-error-function and next-error-last-buffer in that 
buffer. If you call next-error now, you'll be jumping to the next 
linting error, not *compilation* error.

>> How will you switch between Grep and Compilation if they display a location
>> in the same buffer (and window)? Won't the desired navigation buffer have
>> to be visible? So you'd have to select some window, switch to that buffer
>> in it, and then click or press RET on some error?
>
> Yes.

Lots of clicking.

>> Using a command to switch between next-error-last-buffer candidates seems
>> much quicker.
>
> In case of Flycheck, there will be no next-error-last-buffer, no?

We should interpret nil as "use the current buffer".

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

>> So e.g. after navigating from *grep* to window A, and from *compilation*
>> to window B, next-error invoked from windows A or B will continue
>> the right navigation.
>
> Suppose you've did that, then turned left to look out of the window for
> half a minute, then looked back at Emacs. How are you going to predict what
> M-x next-error will do?

You can see an arrow indication in the left fringe of the navigational window
that points to the location of the current file displayed in an adjacent window.

> But my first choice is to not rely on buffer visibility at all, and simply
> follow the current global next-error-last-buffer value, as well as provide
> an easy way to switch to a different one.

I just posted an IDE-like layout to emacs-devel, and it demonstrates that the
current rule#1 in next-error-find-buffer is the right thing to do in this
scenario: after switching from e.g. *grep* to *xref* in the bottom window,
next-error will continue navigation from the visible navigation buffer.
So no changes are required in this case.

>> I proposed window-local next-error-last-buffer only because you had
>> some problems with this rule using in xref.
>
> Yes, I did. But IIUC, Eli had more of a problem with the
>
> (if (eq (length window-buffers) 1)
>
> part of that rule. The commit that disabled next-error integration in xref
> has a link to that discussion in its message.

The rule (if (eq (length window-buffers) 1) itself is not a problem.

The problem is in the cases that this rule doesn't support, i.e.
(< (length window-buffers) 1) and (> (length window-buffers) 1)
We need to find a way to handle these cases as well.

>>> How will you switch between Grep and Compilation if they display a location
>>> in the same buffer (and window)? Won't the desired navigation buffer have
>>> to be visible? So you'd have to select some window, switch to that buffer
>>> in it, and then click or press RET on some error?
>>
>> Yes.
>
> Lots of clicking.

No clicking at all, I don't use the mouse :)

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 01/28/2016 01:57 AM, Juri Linkov wrote:

> You can see an arrow indication in the left fringe of the navigational window
> that points to the location of the current file displayed in an adjacent window.

That's only true of compilation-mode and its derivatives. It's not a 
part of next-error-function contract (again, change-log-next-error does 
nothing of the sort; diff-next-error doesn't either).

Maybe it's a good idea, but I'm not sure how to enforce something like 
that, to be able to rely on it. And a small arrow in one window is not a 
great indicator anyway.

> I just posted an IDE-like layout to emacs-devel, and it demonstrates that the
> current rule#1 in next-error-find-buffer is the right thing to do in this
> scenario: after switching from e.g. *grep* to *xref* in the bottom window,
> next-error will continue navigation from the visible navigation buffer.
> So no changes are required in this case.

What case? We're not going to introduce IDE-like layout as the 
mandatory, or the default, behavior.

The rule#1, as written, also poorly interacts with Flycheck-like use 
cases. Are you going to comment on that part discussion?

Because if you're going to disregard it, we might as well stop talking 
right now: any acceptable proposal, as far as I'm concerned, handles 
that case.

> The problem is in the cases that this rule doesn't support, i.e.
> (< (length window-buffers) 1) and (> (length window-buffers) 1)
> We need to find a way to handle these cases as well.

Yes: remove that check, for example.

We can also realize that the rule #1 is an attempt to do the following: 
if next-error-last-buffer is no longer visible, try to pick a 
navigational buffer among the currently visible ones.

However, the rule tries to limit the number of visible navigational 
buffer to one, and aborts otherwise. I think that's because it doesn't 
know any better way to distinguish between navigational buffers and 
plain file-visiting buffers that have next-error-function set locally 
(navigational buffers can also be file-visiting, as in the cases of 
change-log-mode and diff-mode). The new variable that I proposed would help.

> No clicking at all, I don't use the mouse :)

Lots of pressing the buttons, then. Which is what I meant.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

> Maybe it's a good idea, but I'm not sure how to enforce something like
> that, to be able to rely on it. And a small arrow in one window is not
> a great indicator anyway.

A good indication could be provided by a new global minor mode
‘global-next-error-minor-mode’ showing in the mode line
the currently active navigation, and allowing switching
to another navigation.

> The rule#1, as written, also poorly interacts with Flycheck-like use
> cases. Are you going to comment on that part discussion?

Flycheck provides its own keybinding ‘C-c ! n’ for ‘flycheck-next-error’,
so really there is no problem.

A real problem is when a navigational buffer does exist, but it's hidden.
IIUC, due to this problem you reverted next-error integration in xref, right?

> We can also realize that the rule #1 is an attempt to do the following: if
> next-error-last-buffer is no longer visible, try to pick a navigational
> buffer among the currently visible ones.

You mean next-error-last-buffer is no longer visible _on the selected frame_?

> However, the rule tries to limit the number of visible navigational buffer
> to one, and aborts otherwise. I think that's because it doesn't know any
> better way to distinguish between navigational buffers and plain
> file-visiting buffers that have next-error-function set locally
> (navigational buffers can also be file-visiting, as in the cases of
> change-log-mode and diff-mode). The new variable that I proposed
> would help.

Yes, this is because it's hard to find a better way, and I'm not sure
how next-error-function-nonlocal could help, because sometimes a navigation
might visit another non-file navigational buffer, e.g. multi-occur
visiting a *compilation* buffer.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 01/29/2016 02:59 AM, Juri Linkov wrote:

> ...and allowing switching
> to another navigation.

Are you coming around to my suggestion now?

>> The rule#1, as written, also poorly interacts with Flycheck-like use
>> cases. Are you going to comment on that part discussion?
>
> Flycheck provides its own keybinding ‘C-c ! n’ for ‘flycheck-next-error’,
> so really there is no problem.

That's basically giving up.

Do you expect me to repeatedly type `C-c ! n' to move across errors in 
the current buffer? It's not like it's inconvenient or anything. 
next-error-function was added exactly so that the user doesn't have to 
learn a bunch of different key bindings for basically the same thing.

There's also e.g. js2-mode, which doesn't have a custom key binding for 
this. And probably other modes that I just don't know about.

> A real problem is when a navigational buffer does exist, but it's hidden.
> IIUC, due to this problem you reverted next-error integration in xref, right?

No: http://lists.gnu.org/archive/html/emacs-devel/2016-01/msg01286.html

See the first sentence there.

>> We can also realize that the rule #1 is an attempt to do the following: if
>> next-error-last-buffer is no longer visible, try to pick a navigational
>> buffer among the currently visible ones.
>
> You mean next-error-last-buffer is no longer visible _on the selected frame_?

I don't really care either way. This question doesn't seem to add any 
big constraints on the final solution.

> Yes, this is because it's hard to find a better way, and I'm not sure
> how next-error-function-nonlocal could help, because sometimes a navigation
> might visit another non-file navigational buffer, e.g. multi-occur
> visiting a *compilation* buffer.

What is the exact problem you have in mind there?

When *multi-occur* jumps to *compilation*, next-error-last-buffer keeps 
referring to *multi-occur*.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

> Do you expect me to repeatedly type `C-c ! n' to move across errors in the
> current buffer? It's not like it's inconvenient or
> anything. next-error-function was added exactly so that the user doesn't
> have to learn a bunch of different key bindings for basically the
> same thing.

Not repeatedly, it's enough to type is only once, and subsequent invocations
of next-error will pick up a new navigation.

>> A real problem is when a navigational buffer does exist, but it's hidden.
>> IIUC, due to this problem you reverted next-error integration in xref, right?
>
> No: http://lists.gnu.org/archive/html/emacs-devel/2016-01/msg01286.html
>
> See the first sentence there.

I reread it every time you reference it, but it adds nothing to the discussion.
Could you provide more details about this problem.  I imagine you meant the case
when *xref* is hidden, but *compilation* is visible.  Is it so?  What are the
preconditions for this situation to occur?

>> Yes, this is because it's hard to find a better way, and I'm not sure
>> how next-error-function-nonlocal could help, because sometimes a navigation
>> might visit another non-file navigational buffer, e.g. multi-occur
>> visiting a *compilation* buffer.
>
> What is the exact problem you have in mind there?
>
> When *multi-occur* jumps to *compilation*, next-error-last-buffer keeps
> referring to *multi-occur*.

But after you hide *compilation*, *multi-occur* will kick in.
This is why I proposed to use window-local values, and your counter-arguments
against it (indication/switching) apply to the already used global value
of next-error-last-buffer as well: its current state is not discoverable
and it's not easy to switch to another navigation.  This issue is real,
but orthogonal to the subject of bug#20489.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 01/30/2016 02:44 AM, Juri Linkov wrote:

> Not repeatedly, it's enough to type is only once, and subsequent invocations
> of next-error will pick up a new navigation.

Fair enough. But the complaint about memorizing different key bindings 
still stands.

>>> A real problem is when a navigational buffer does exist, but it's hidden.
>>> IIUC, due to this problem you reverted next-error integration in xref, right?

Why is that a problem? Depending on the approach, we either keep using 
it, or switch to the visible one.

>> No: http://lists.gnu.org/archive/html/emacs-devel/2016-01/msg01286.html
>>
>> See the first sentence there.
>
> I reread it every time you reference it, but it adds nothing to the discussion.
> Could you provide more details about this problem.  I imagine you meant the case
> when *xref* is hidden, but *compilation* is visible.  Is it so?  What are the
> preconditions for this situation to occur?

You really should ask Eli what exactly he meant there, I'm just 
guessing. I didn't want to keep inquiring at that point. Eli said 
disable, so I disabled.

But IMHO, (eq (length window-buffers) 1) is counter-intuitive: take the 
configuration with three buffers with next-error-function set visible. 
Hide the current last-buffer: nothing changes, `next-error' continues 
working as it did. Hide the next one: and suddenly, `next-error' starts 
behaving differently.

The user is expected to understand too much.

>> When *multi-occur* jumps to *compilation*, next-error-last-buffer keeps
>> referring to *multi-occur*.
>
> But after you hide *compilation*, *multi-occur* will kick in.

So? It's you who's advocating to stop using the non-visible 
last-buffer's. My first choice is to only switch next-error-last-buffer 
when the user requests this explicitly.

On the other hand, if we choose the semantics "not visible => bad 
last-buffer", that would be understandable, too.

I don't see why you consider the case "multi-occur references 
compilation" to be more special than others. It seems no different from 
"both grep and compilation are visible".

> This is why I proposed to use window-local values, and your counter-arguments
> against it (indication/switching) apply to the already used global value
> of next-error-last-buffer as well: its current state is not discoverable
> and it's not easy to switch to another navigation.

Your proposal _complicates_ the current state, making it more of a 
problem. If the global value of next-error-last-buffer is used 
consistently, at least the current state is easier to remember.

I'm also not a big fan of window-local semantics here, personally.

> This issue is real,
> but orthogonal to the subject of bug#20489.

Would you like me to rename the subject to something? The actual problem 
is that `next-error' exhibits surprising behavior, and doesn't properly 
support `next-error-function' being set in file-visiting buffers, which 
is a common situation these days.

Since filing this bug, I've somewhat warmed up to using buffer 
visibility as a condition to choose next-error-last-buffer.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

>>> No: http://lists.gnu.org/archive/html/emacs-devel/2016-01/msg01286.html
>>>
>>> See the first sentence there.
>>
>> I reread it every time you reference it, but it adds nothing to the discussion.
>> Could you provide more details about this problem.  I imagine you meant the case
>> when *xref* is hidden, but *compilation* is visible.  Is it so?  What are the
>> preconditions for this situation to occur?
>
> You really should ask Eli what exactly he meant there, I'm just
> guessing. I didn't want to keep inquiring at that point. Eli said disable,
> so I disabled.

I believe Eli meant this case because it's hard to imagine another one.
So we have to find a solution for this case.

By setting a window-local value of next-error-last-buffer in the
selected window, we can continue the xref-navigation even when
*compilation* is visible in an adjacent window.

> But IMHO, (eq (length window-buffers) 1) is counter-intuitive: take the
> configuration with three buffers with next-error-function set visible. Hide
> the current last-buffer: nothing changes, `next-error' continues working as
> it did. Hide the next one: and suddenly, `next-error' starts
> behaving differently.

When the number of next-error-function windows is more than one, then
there's a dilemma which one to use.

>> This is why I proposed to use window-local values, and your counter-arguments
>> against it (indication/switching) apply to the already used global value
>> of next-error-last-buffer as well: its current state is not discoverable
>> and it's not easy to switch to another navigation.
>
> Your proposal _complicates_ the current state, making it more of
> a problem. If the global value of next-error-last-buffer is used
> consistently, at least the current state is easier to remember.

But it allows the user to continue a paused navigation in another window
in another frame, thus having several simultaneously active navigations
in different windows.

> The actual problem is that `next-error' exhibits surprising behavior,
> and doesn't properly support `next-error-function' being set in
> file-visiting buffers, which is a common situation these days.

What happens when two features set `next-error-function' at the same time?
I guess the latest wins, so there is no problem no matter if using
visibility of next-error-last-buffer or window-local values.

> Since filing this bug, I've somewhat warmed up to using buffer visibility
> as a condition to choose next-error-last-buffer.

Visibility of next-error-last-buffer is not suitable for navigations
without a navigational buffer.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 01/31/2016 02:43 AM, Juri Linkov wrote:

> I believe Eli meant this case because it's hard to imagine another one.
> So we have to find a solution for this case.

Let's not base the rest of the discussion on a guess, shall we?

In the message above, he was replying to my message, where I said: "On 
the other hand, while *xref* is visible, `next-error' will keep working 
for its results".

I can hardly imagine that in his counter-example, *xref* is hidden.

> By setting a window-local value of next-error-last-buffer in the
> selected window, we can continue the xref-navigation even when
> *compilation* is visible in an adjacent window.

Yes. But we _only_ continue it from the same window, which I do not 
believe to be a good goal.

On the other hand, if we just use the global next-error-last-buffer 
value, we'll just as well "continue the xref-navigation even when 
*compilation* is visivle in an adjacent window".

>> But IMHO, (eq (length window-buffers) 1) is counter-intuitive: take the
>> configuration with three buffers with next-error-function set visible. Hide
>> the current last-buffer: nothing changes, `next-error' continues working as
>> it did. Hide the next one: and suddenly, `next-error' starts
>> behaving differently.
>
> When the number of next-error-function windows is more than one, then
> there's a dilemma which one to use.

Let's use the last one. That would definitely simplify things.

On the other hand, if we assign and read next-error-last-buffer value 
via two accessor functions, anyone would be able to change the locality 
of that value. You'd be able to use advice, to store it window-locally.

>> Your proposal _complicates_ the current state, making it more of
>> a problem. If the global value of next-error-last-buffer is used
>> consistently, at least the current state is easier to remember.
>
> But it allows the user to continue a paused navigation in another window
> in another frame, thus having several simultaneously active navigations
> in different windows.

If the "previous" navigation buffer is visible, you can also continue 
navigation by going to it, and using one of the links there.

If it's not visible, it would make remembering which window belongs to 
which navigation, even more difficult.

> What happens when two features set `next-error-function' at the same time?
> I guess the latest wins, so there is no problem no matter if using
> visibility of next-error-last-buffer or window-local values.

Yes, if next-error-function is set locally in a file buffer, we can only 
see the last value.

But rather than "no problem", I'd say that neither approach to 
visibility of next-error-last-buffer solves the Flycheck problem.

>> Since filing this bug, I've somewhat warmed up to using buffer visibility
>> as a condition to choose next-error-last-buffer.
>
> Visibility of next-error-last-buffer is not suitable for navigations
> without a navigational buffer.

Hence my proposal to equate the value nil of next-error-last-buffer with 
"use the current buffer".

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

> In the message above, he was replying to my message, where I said: "On the
> other hand, while *xref* is visible, `next-error' will keep working for its
> results".

Clearly, this describes a successful case as opposed to the problematic one
where *xref* is hidden that evidently needs fixing.

>> By setting a window-local value of next-error-last-buffer in the
>> selected window, we can continue the xref-navigation even when
>> *compilation* is visible in an adjacent window.
>
> Yes. But we _only_ continue it from the same window, which I do not believe
> to be a good goal.
>
> On the other hand, if we just use the global next-error-last-buffer value,
> we'll just as well "continue the xref-navigation even when *compilation* is
> visible in an adjacent window".

And lose the support for the case of simultaneously active navigations
in different windows/frames.

>>> But IMHO, (eq (length window-buffers) 1) is counter-intuitive: take the
>>> configuration with three buffers with next-error-function set visible. Hide
>>> the current last-buffer: nothing changes, `next-error' continues working as
>>> it did. Hide the next one: and suddenly, `next-error' starts
>>> behaving differently.
>>
>> When the number of next-error-function windows is more than one, then
>> there's a dilemma which one to use.
>
> Let's use the last one. That would definitely simplify things.

Indeed, using (get-mru-window 'visible t t) makes sense.

> On the other hand, if we assign and read next-error-last-buffer value via
> two accessor functions, anyone would be able to change the locality of that
> value. You'd be able to use advice, to store it window-locally.

Customizable next-error-find-buffer?  Maybe, if we'll fail to find
a reasonable default.

> If the "previous" navigation buffer is visible, you can also continue
> navigation by going to it, and using one of the links there.
>
> If it's not visible, it would make remembering which window belongs to
> which navigation, even more difficult.

Isn't it so that the user will remember which navigation displayed
a given window?

>> What happens when two features set `next-error-function' at the same time?
>> I guess the latest wins, so there is no problem no matter if using
>> visibility of next-error-last-buffer or window-local values.
>
> Yes, if next-error-function is set locally in a file buffer, we can only
> see the last value.
>
> But rather than "no problem", I'd say that neither approach to visibility
> of next-error-last-buffer solves the Flycheck problem.

At least, with window-local values the Flycheck navigation in the given buffer
will be confined within the selected window and won't affect other navigations
in other windows.  So continuing a navigation in other buffers/windows won't
continue Flychecking of an unrelated buffer.  So Flychecking should not set
the global value of next-error-last-buffer.

>>> Since filing this bug, I've somewhat warmed up to using buffer visibility
>>> as a condition to choose next-error-last-buffer.
>>
>> Visibility of next-error-last-buffer is not suitable for navigations
>> without a navigational buffer.
>
> Hence my proposal to equate the value nil of next-error-last-buffer with
> "use the current buffer".

What/who and how would nullify/reset next-error-last-buffer?

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 02/01/2016 12:57 AM, Juri Linkov wrote:
>> In the message above, he was replying to my message, where I said: "On the
>> other hand, while *xref* is visible, `next-error' will keep working for its
>> results".
>
> Clearly, this describes a successful case as opposed to the problematic one
> where *xref* is hidden that evidently needs fixing.

Yes. This was my appeal to keep the existing integration of xref with 
next-error-function. Eli disagreed.

What can we gather from that?

>> On the other hand, if we just use the global next-error-last-buffer value,
>> we'll just as well "continue the xref-navigation even when *compilation* is
>> visible in an adjacent window".
>
> And lose the support for the case of simultaneously active navigations
> in different windows/frames.

Yup. Did you have many requests, from different users, before 
introducing this support?

>>> When the number of next-error-function windows is more than one, then
>>> there's a dilemma which one to use.
>>
>> Let's use the last one. That would definitely simplify things.
>
> Indeed, using (get-mru-window 'visible t t) makes sense.

I don't follow. The window returned by the above won't necessarily have 
next-error-function set. And, again, this ignores the Flycheck case.

>> If the "previous" navigation buffer is visible, you can also continue
>> navigation by going to it, and using one of the links there.
>>
>> If it's not visible, it would make remembering which window belongs to
>> which navigation, even more difficult.
>
> Isn't it so that the user will remember which navigation displayed
> a given window?

Sorry, _what_ is so?

> At least, with window-local values the Flycheck navigation in the given buffer
> will be confined within the selected window and won't affect other navigations
> in other windows.

With your approach, no window will affect other windows. Even if I ran 
M-x rgrep, and I see its buffer in the current frame, I'll also have to 
remember which window it ended at. And if I never clicked on a link in 
the *grep* buffer, I can't use C-x ` in any window, I'm assuming.

> So continuing a navigation in other buffers/windows won't
> continue Flychecking of an unrelated buffer.  So Flychecking should not set
> the global value of next-error-last-buffer.

Suppose I use flycheck-next-error in foo.el. And I have a *grep* buffer 
visible, and I jumped to bar.el from it. And the next error in *grep* is 
in foo.el. What happens when I, having returned to bar.el's window, call 
next-error again? Does it jump to foo.el's window? Does it display 
foo.el in the window where bar.el previously was?

Does every navigational window get a second, dedicated window for its 
locations? Often, we don't have many windows to spare.

>> Hence my proposal to equate the value nil of next-error-last-buffer with
>> "use the current buffer".
>
> What/who and how would nullify/reset next-error-last-buffer?

A new command. Or maybe a special value of the prefix argument to 
`next-error'? M-0 C-x `, maybe?

But if we have a new command, I could also allow selecting from some of 
the existing buffers which contain "nonlocal" next-error-function's.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

>>> In the message above, he was replying to my message, where I said: "On the
>>> other hand, while *xref* is visible, `next-error' will keep working for its
>>> results".
>>
>> Clearly, this describes a successful case as opposed to the problematic one
>> where *xref* is hidden that evidently needs fixing.
>
> Yes. This was my appeal to keep the existing integration of xref with
> next-error-function. Eli disagreed.
>
> What can we gather from that?

I could gather only that we need to support this case in next-error
before enabling next-error in xref.

>>> On the other hand, if we just use the global next-error-last-buffer value,
>>> we'll just as well "continue the xref-navigation even when *compilation* is
>>> visible in an adjacent window".
>>
>> And lose the support for the case of simultaneously active navigations
>> in different windows/frames.
>
> Yup. Did you have many requests, from different users, before introducing
> this support?

The primary question is not how many users asked for it many years ago,
but how many users are using it now.

>>>> When the number of next-error-function windows is more than one, then
>>>> there's a dilemma which one to use.
>>>
>>> Let's use the last one. That would definitely simplify things.
>>
>> Indeed, using (get-mru-window 'visible t t) makes sense.
>
> I don't follow. The window returned by the above won't necessarily have
> next-error-function set.

Obviously, get-mru-“next-error”-window, i.e. a combination of both.

> And, again, this ignores the Flycheck case.

Alas, this means we have to trade visibility of next-error-last-buffer
for window-local values.

>>> If the "previous" navigation buffer is visible, you can also continue
>>> navigation by going to it, and using one of the links there.
>>>
>>> If it's not visible, it would make remembering which window belongs to
>>> which navigation, even more difficult.
>>
>> Isn't it so that the user will remember which navigation displayed
>> a given window?
>
> Sorry, _what_ is so?

The users hopefully remember which navigation displayed a given window.

>> At least, with window-local values the Flycheck navigation in the given buffer
>> will be confined within the selected window and won't affect other navigations
>> in other windows.
>
> With your approach, no window will affect other windows. Even if I ran M-x
> rgrep, and I see its buffer in the current frame, I'll also have to
> remember which window it ended at. And if I never clicked on a link in the
> *grep* buffer, I can't use C-x ` in any window, I'm assuming.

C-x ` in any window will use the global value, i.e. the last navigation.

>> So continuing a navigation in other buffers/windows won't
>> continue Flychecking of an unrelated buffer.  So Flychecking should not set
>> the global value of next-error-last-buffer.
>
> Suppose I use flycheck-next-error in foo.el. And I have a *grep* buffer
> visible, and I jumped to bar.el from it. And the next error in *grep* is in
> foo.el. What happens when I, having returned to bar.el's window, call
> next-error again? Does it jump to foo.el's window? Does it display foo.el
> in the window where bar.el previously was?

It will display foo.el in the window where bar.el previously was.
AFAIS, this is the point of the current discussion on emacs-devel -
to fix xref's window management to work like *compilation* and visit
navigated windows predictably.

> Does every navigational window get a second, dedicated window for its
> locations? Often, we don't have many windows to spare.
>
>>> Hence my proposal to equate the value nil of next-error-last-buffer with
>>> "use the current buffer".
>>
>> What/who and how would nullify/reset next-error-last-buffer?
>
> A new command. Or maybe a special value of the prefix argument to
> `next-error'? M-0 C-x `, maybe?
>
> But if we have a new command, I could also allow selecting from some of the
> existing buffers which contain "nonlocal" next-error-function's.

Why only “nonlocal”?

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 02/02/2016 03:44 AM, Juri Linkov wrote:

>> Yes. This was my appeal to keep the existing integration of xref with
>> next-error-function. Eli disagreed.
>>
>> What can we gather from that?
>
> I could gather only that we need to support this case in next-error
> before enabling next-error in xref.

So I stated the same as you, Eli disagreed, and we're going to conclude 
that you are right? Good job.

> The primary question is not how many users asked for it many years ago,
> but how many users are using it now.

You're welcome to poll, but so far we only have the data about a single 
user.

> Obviously, get-mru-“next-error”-window, i.e. a combination of both.

I have no idea what that means, really. Are we going to store a 
reference to a window instead of a reference to a buffer? And then take 
the reference to the buffer from that window?

Seem like lots of complexity for nothing.

>> And, again, this ignores the Flycheck case.
>
> Alas, this means we have to trade visibility of next-error-last-buffer
> for window-local values.

What means? And why?

>>> Isn't it so that the user will remember which navigation displayed
>>> a given window?
>>
>> Sorry, _what_ is so?
>
> The users hopefully remember which navigation displayed a given window.

Did your previous sentence make sense? Did you actually mean "Won't 
users remember..."?

And to answer the question, I don't think so. At least, not every time.

It seems kind of pointless to answer an observation about added 
complexity with "won't users remember everything?", don't you think?

>> With your approach, no window will affect other windows. Even if I ran M-x
>> rgrep, and I see its buffer in the current frame, I'll also have to
>> remember which window it ended at. And if I never clicked on a link in the
>> *grep* buffer, I can't use C-x ` in any window, I'm assuming.
>
> C-x ` in any window will use the global value, i.e. the last navigation.

So, not only we'll have local values, we'll also have a global one?

Upon invoking that command, will focus immediately jump to the one 
window associated with the navigation buffer?

>>> So continuing a navigation in other buffers/windows won't
>>> continue Flychecking of an unrelated buffer.  So Flychecking should not set
>>> the global value of next-error-last-buffer.
>>
>> Suppose I use flycheck-next-error in foo.el. And I have a *grep* buffer
>> visible, and I jumped to bar.el from it. And the next error in *grep* is in
>> foo.el. What happens when I, having returned to bar.el's window, call
>> next-error again? Does it jump to foo.el's window? Does it display foo.el
>> in the window where bar.el previously was?
>
> It will display foo.el in the window where bar.el previously was.

Aren't you at all worried about running out of windows (and not being 
allowed to split them)? In our last discussion with Martin, he expressed 
concern that some users use one window per frame. In this scenario, 
you'll need at least 3. And with two navigational buffers visible, the 
number of necessary windows will grow to 5.

> AFAIS, this is the point of the current discussion on emacs-devel -
> to fix xref's window management to work like *compilation* and visit
> navigated windows predictably.

*compilation* isn't afraid to use different windows when it's 
convenient. It definitely reuses another window if the buffer it's going 
to jump to is already displayed.

>> But if we have a new command, I could also allow selecting from some of the
>> existing buffers which contain "nonlocal" next-error-function's.
>
> Why only “nonlocal”?

Because otherwise there'll be too many options too choose from. To use a 
different buffer, the user can switch to it first anyway.

Nonlocal next-error-functions represent groups of windows that are 
somehow related (parts of the same problem, matches for the same input, 
or so on). And with them, we can at least expect that the next error 
still might be in the current buffer.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

>> The primary question is not how many users asked for it many years ago,
>> but how many users are using it now.
>
> You're welcome to poll, but so far we only have the data about
> a single user.

Yes, a single user who wants to remove it ;)

>> Obviously, get-mru-“next-error”-window, i.e. a combination of both.
>
> I have no idea what that means, really. Are we going to store a reference
> to a window instead of a reference to a buffer? And then take the reference
> to the buffer from that window?

Just traverse all windows in the order of recency until finding a window
with next-error-function.

>>> And, again, this ignores the Flycheck case.
>>
>> Alas, this means we have to trade visibility of next-error-last-buffer
>> for window-local values.
>
> What means? And why?

In case of Flycheck it's next-error-last-buffer is the current buffer
and is always visible.

>>> With your approach, no window will affect other windows. Even if I ran M-x
>>> rgrep, and I see its buffer in the current frame, I'll also have to
>>> remember which window it ended at. And if I never clicked on a link in the
>>> *grep* buffer, I can't use C-x ` in any window, I'm assuming.
>>
>> C-x ` in any window will use the global value, i.e. the last navigation.
>
> So, not only we'll have local values, we'll also have a global one?

When there is no local values, then indeed use a global one.

> Upon invoking that command, will focus immediately jump to the one window
> associated with the navigation buffer?

Naturally.

>>> Suppose I use flycheck-next-error in foo.el. And I have a *grep* buffer
>>> visible, and I jumped to bar.el from it. And the next error in *grep* is in
>>> foo.el. What happens when I, having returned to bar.el's window, call
>>> next-error again? Does it jump to foo.el's window? Does it display foo.el
>>> in the window where bar.el previously was?
>>
>> It will display foo.el in the window where bar.el previously was.
>
> Aren't you at all worried about running out of windows (and not being
> allowed to split them)? In our last discussion with Martin, he expressed
> concern that some users use one window per frame. In this scenario, you'll
> need at least 3. And with two navigational buffers visible, the number of
> necessary windows will grow to 5.

I meant reusing an existing window with a file buffer, not creating a new
window.  I see no reason not to use this behavior by default.

>> AFAIS, this is the point of the current discussion on emacs-devel -
>> to fix xref's window management to work like *compilation* and visit
>> navigated windows predictably.
>
> *compilation* isn't afraid to use different windows when it's
> convenient. It definitely reuses another window if the buffer it's going to
> jump to is already displayed.

What I see is that *compilation* always reuses another window to not replace
the navigational *compilation* buffer with a file buffer.  Do you know
under what circumstances this behavior might fail.

>>> But if we have a new command, I could also allow selecting from some of the
>>> existing buffers which contain "nonlocal" next-error-function's.
>>
>> Why only “nonlocal”?
>
> Because otherwise there'll be too many options too choose from. To use
> a different buffer, the user can switch to it first anyway.
>
> Nonlocal next-error-functions represent groups of windows that are somehow
> related (parts of the same problem, matches for the same input, or so
> on). And with them, we can at least expect that the next error still might
> be in the current buffer.

This is why I proposed window-local variables to bind a group of windows
to their parent navigational window.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 02/03/2016 03:35 AM, Juri Linkov wrote:

>> You're welcome to poll, but so far we only have the data about
>> a single user.
>
> Yes, a single user who wants to remove it ;)

Apparently, you have no data at all, then. As a user, I don't care about 
it either way.

As a _developer_, I might remove it. Which is what should be done if a 
feature can't be implemented in a sane way without trampling on user 
experience in other aspects.

> Just traverse all windows in the order of recency until finding a window
> with next-error-function.

This will almost always default to the current window if you write 
Elisp, and have global-flycheck-mode enabled.

Can't you give a full, constructive proposal, instead of giving pieces 
that are easy to poke holes in?

>>>> And, again, this ignores the Flycheck case.
>>>
>>> Alas, this means we have to trade visibility of next-error-last-buffer
>>> for window-local values.
>>
>> What means? And why?
>
> In case of Flycheck it's next-error-last-buffer is the current buffer
> and is always visible.

You haven't answered the "what" question. But assuming I managed to 
guess your meaning...

How come window-local values are any better? If "next-error-last-buffer 
is the current buffer" is the current buffer when the variable is stored 
globally, it will be the window-local value when the variable is stored 
window-locally. Unless the buffer is not visible, in which case the 
question is moot.

>>>> Suppose I use flycheck-next-error in foo.el. And I have a *grep* buffer
>>>> visible, and I jumped to bar.el from it. And the next error in *grep* is in
>>>> foo.el. What happens when I, having returned to bar.el's window, call
>>>> next-error again? Does it jump to foo.el's window? Does it display foo.el
>>>> in the window where bar.el previously was?
>>>
>>> It will display foo.el in the window where bar.el previously was.
>>
>> Aren't you at all worried about running out of windows (and not being
>> allowed to split them)? In our last discussion with Martin, he expressed
>> concern that some users use one window per frame. In this scenario, you'll
>> need at least 3. And with two navigational buffers visible, the number of
>> necessary windows will grow to 5.
>
> I meant reusing an existing window with a file buffer, not creating a new
> window.

Then you are contradicting yourself.

> I see no reason not to use this behavior by default.

It undermines your proposal: either you have a separate dedicated window 
for each navigational buffer to show locations in, or the navigational 
buffers trample on each other's windows when they visit the same files.

Which creates the same problem you had, except in certain specific 
circumstances. But those circumstances are not *that* rare. This will 
make the behavior less consistent and increase user confusion.

> What I see is that *compilation* always reuses another window to not replace
> the navigational *compilation* buffer with a file buffer.  Do you know
> under what circumstances this behavior might fail.

*compilation* jumps to foo.el, then *grep* jumps to foo.el. Both use the 
same window (the default behavior), and both have an arrow in their 
buffers indicating that foo.el's window is "theirs". However, the last 
navigational buffer wins.

>> Nonlocal next-error-functions represent groups of windows that are somehow
>> related (parts of the same problem, matches for the same input, or so
>> on). And with them, we can at least expect that the next error still might
>> be in the current buffer.
>
> This is why I proposed window-local variables to bind a group of windows
> to their parent navigational window.

I don't see how that solves anything.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

Hi Eli,

Now that the xref window is more "persistent", could you try 
uncommenting the relevant lines in xref--xref-buffer-mode, and see if 
the situation has improved here?

Or if it hasn't, please describe your main complaint from
http://lists.gnu.org/archive/html/emacs-devel/2016-01/msg01286.html in 
more detail: what happens "when you have both of them"?

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Eli Zaretskii]

> Cc: Eli Zaretskii <eliz@gnu.org>
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 22 Feb 2016 02:01:21 +0200
> 
> Now that the xref window is more "persistent", could you try 
> uncommenting the relevant lines in xref--xref-buffer-mode, and see if 
> the situation has improved here?

Will do.  To make this more efficient, can you suggest what situations
to try?

Thanks.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 02/22/2016 07:22 PM, Eli Zaretskii wrote:

> Will do.  To make this more efficient, can you suggest what situations
> to try?

That's what I'm really asking: which scenario did you find most broken 
before, and does it work better now?

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Eli Zaretskii]

> Cc: 20489@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 22 Feb 2016 19:30:21 +0200
> 
> On 02/22/2016 07:22 PM, Eli Zaretskii wrote:
> 
> > Will do.  To make this more efficient, can you suggest what situations
> > to try?
> 
> That's what I'm really asking: which scenario did you find most broken 
> before, and does it work better now?

OK, I will dig into my failing memory and try to remember that.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 02/22/2016 07:39 PM, Eli Zaretskii wrote:

> OK, I will dig into my failing memory and try to remember that.

(Or you could just try using it and maybe encountering the same problem 
again.)

Thanks. It's not urgent, but it seems to be the best way to process in 
this issue.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 02/22/2016 08:09 PM, Dmitry Gutov wrote:

Sorry,

> Thanks. It's not urgent, but it seems to be the best way to process in
                                                               ^
                                                               proceed
> this issue.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Eli Zaretskii]

> Cc: 20489@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 22 Feb 2016 20:09:16 +0200
> 
> On 02/22/2016 07:39 PM, Eli Zaretskii wrote:
> 
> > OK, I will dig into my failing memory and try to remember that.
> 
> (Or you could just try using it and maybe encountering the same problem 
> again.)

Yes.  But I wanted to recollect what made me nervous.

Anyway, I found it in the discussion we had back then, so I know what
to try.

> Thanks. It's not urgent, but it seems to be the best way to process in 
> this issue.

Right.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Eli Zaretskii]

> Cc: Eli Zaretskii <eliz@gnu.org>
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 22 Feb 2016 02:01:21 +0200
> 
> Now that the xref window is more "persistent", could you try 
> uncommenting the relevant lines in xref--xref-buffer-mode, and see if 
> the situation has improved here?

It looks much better now, thanks.  I think we can uncomment those
lines now.

A couple of minor nits:

 . When one uses next-error to step through hits found by Dired's 'A'
   command, point in the *xref* buffer doesn't move to the hit that is
   visited in the window displayed above *xref*.  Given how next-error
   works in other cases, I think users will expect point to move
   accordingly; at least I did.

 . I see the places I visited marked by a special face in *xref*
   (good!), but I don't quite understand when they get marked.  They
   certainly don't get marked as I move through hits with next-error
   or with an explicit RET on a hit in the *xref* buffer.  Perhaps we
   should mark them in real time?  That would also help in
   understanding what that face means, I think.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 02/27/2016 12:14 PM, Eli Zaretskii wrote:

>  . When one uses next-error to step through hits found by Dired's 'A'
>    command, point in the *xref* buffer doesn't move to the hit that is
>    visited in the window displayed above *xref*.  Given how next-error
>    works in other cases, I think users will expect point to move
>    accordingly; at least I did.

It would be helpful, but it doesn't seem like `next-error' was designed 
with this in mind. After all, xref--next-error-function does move point, 
and that doesn't help (probably because it's called inside 
with-current-buffer, see next-error-internal).

Wrapping xref--next-error-function's definition in

   (with-selected-window (get-buffer-window xref-buffer-name)
    ...)

does help, but that seems silly.

>  . I see the places I visited marked by a special face in *xref*
>    (good!), but I don't quite understand when they get marked.

No special face, these are just unadorned buffer-substring values: the 
ones that have faces applied, are from buffer areas that had been 
touched by font-lock, the others hadn't. We could remove faces from all 
lines for consistency, but seeing them on at least some results is nice.

> They
>    certainly don't get marked as I move through hits with next-error
>    or with an explicit RET on a hit in the *xref* buffer.  Perhaps we
>    should mark them in real time?

Not sure how to introduce that feature into the API in a generic 
fashion. Perhaps if we decided that the "summary" of each xref-match 
instance is always defined by the buffer contents?

Having match-xrefs use a distinct structure from "normal" xrefs seems 
appropriate, but to fully go this way, I think we'll need the 
find-buffer-delayed feature first 
(http://lists.gnu.org/archive/html/emacs-devel/2015-08/msg00060.html). 
Because then we couldn't afford to have the location buffers closed (or 
reopen them all) when the xref buffer is rendered.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Eli Zaretskii]

> Cc: 20489@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Mon, 29 Feb 2016 05:15:10 +0200
> 
> On 02/27/2016 12:14 PM, Eli Zaretskii wrote:
> 
> >  . When one uses next-error to step through hits found by Dired's 'A'
> >    command, point in the *xref* buffer doesn't move to the hit that is
> >    visited in the window displayed above *xref*.  Given how next-error
> >    works in other cases, I think users will expect point to move
> >    accordingly; at least I did.
> 
> It would be helpful, but it doesn't seem like `next-error' was designed 
> with this in mind.

Really?  Isn't that strange?  Doesn't every user of next-error want
that?

> Wrapping xref--next-error-function's definition in
> 
>    (with-selected-window (get-buffer-window xref-buffer-name)
>     ...)
> 
> does help, but that seems silly.

Right.  Too bad if this doesn't have an easy solution, but if so, I
guess we will have to live with that.

> Not sure how to introduce that feature into the API in a generic 
> fashion. Perhaps if we decided that the "summary" of each xref-match 
> instance is always defined by the buffer contents?

OK, then let's forget about this until the necessary infrastructure is
in place.

Please uncomment those lines, if you didn't already.

Thanks.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 02/29/2016 06:23 PM, Eli Zaretskii wrote:

> Really?  Isn't that strange?  Doesn't every user of next-error want
> that?

Just yet another way next-error-function is broken, it seems.

> Please uncomment those lines, if you didn't already.

Already did, in aae436e2d898a8c8cc243c73d6cec5a8c566a061.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

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

It seems this discussion that came to a standstill here
hopefully is revived and reached a preliminary consensus in bug#28864.
Here's the current patch:


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

diff --git a/lisp/simple.el b/lisp/simple.el
index 375a79e..ceefbc1 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -143,6 +143,7 @@ next-error-last-buffer
 A buffer becomes most recent when its compilation, grep, or
 similar mode is started, or when it is used with \\[next-error]
 or \\[compile-goto-error].")
+(make-variable-buffer-local 'next-error-last-buffer)
 
 (defvar next-error-function nil
   "Function to use to find the next error in the current buffer.
@@ -191,6 +192,31 @@ next-error-buffer-p
 	   (and extra-test-inclusive
 		(funcall extra-test-inclusive))))))
 
+(defcustom next-error-find-buffer-function nil
+  "Function called to find a `next-error' capable buffer."
+  :type '(choice (const :tag "Single next-error capable buffer on selected frame"
+                        next-error-buffer-on-selected-frame)
+                 (const :tag "No default" nil)
+                 (function :tag "Other function"))
+  :group 'next-error
+  :version "27.1")
+
+(defun next-error-buffer-on-selected-frame (&optional avoid-current
+                                                      extra-test-inclusive
+                                                      extra-test-exclusive)
+  "Return a single visible next-error buffer on the selected frame."
+  (let ((window-buffers
+         (delete-dups
+          (delq nil (mapcar (lambda (w)
+                              (if (next-error-buffer-p
+        			   (window-buffer w)
+                                   avoid-current
+                                   extra-test-inclusive extra-test-exclusive)
+                                  (window-buffer w)))
+                            (window-list))))))
+    (if (eq (length window-buffers) 1)
+        (car window-buffers))))
+
 (defun next-error-find-buffer (&optional avoid-current
 					 extra-test-inclusive
 					 extra-test-exclusive)
@@ -207,18 +233,11 @@ next-error-find-buffer
 that would normally be considered usable.  If it returns nil,
 that buffer is rejected."
   (or
-   ;; 1. If one window on the selected frame displays such buffer, return it.
-   (let ((window-buffers
-          (delete-dups
-           (delq nil (mapcar (lambda (w)
-                               (if (next-error-buffer-p
-				    (window-buffer w)
-                                    avoid-current
-                                    extra-test-inclusive extra-test-exclusive)
-                                   (window-buffer w)))
-                             (window-list))))))
-     (if (eq (length window-buffers) 1)
-         (car window-buffers)))
+   ;; 1. If a customizable function returns a buffer, use it.
+   (when next-error-find-buffer-function
+     (funcall next-error-find-buffer-function avoid-current
+                                              extra-test-inclusive
+                                              extra-test-exclusive))
    ;; 2. If next-error-last-buffer is an acceptable buffer, use that.
    (if (and next-error-last-buffer
             (next-error-buffer-p next-error-last-buffer avoid-current
@@ -283,11 +302,20 @@ next-error
     (when buffer
       ;; We know here that next-error-function is a valid symbol we can funcall
       (with-current-buffer buffer
+        ;; Allow next-error to be used from the next-error capable buffer.
+        (setq next-error-last-buffer buffer)
         (funcall next-error-function (prefix-numeric-value arg) reset)
         ;; Override possible change of next-error-last-buffer in next-error-function
         (setq next-error-last-buffer buffer)
+        (setq-default next-error-last-buffer buffer)
         (when next-error-recenter
           (recenter next-error-recenter))
+        (message "%s error from %s"
+                 (cond (reset                             "First")
+                       ((eq (prefix-numeric-value arg) 0) "Current")
+                       ((< (prefix-numeric-value arg) 0)  "Previous")
+                       (t                                 "Next"))
+                 next-error-last-buffer)
         (run-hooks 'next-error-hook)))))
 
 (defun next-error-internal ()
@@ -295,13 +323,26 @@ next-error-internal
   (let ((buffer (current-buffer)))
     ;; We know here that next-error-function is a valid symbol we can funcall
     (with-current-buffer buffer
+      ;; Allow next-error to be used from the next-error capable buffer.
+      (setq next-error-last-buffer buffer)
       (funcall next-error-function 0 nil)
       ;; Override possible change of next-error-last-buffer in next-error-function
       (setq next-error-last-buffer buffer)
+      (setq-default next-error-last-buffer buffer)
       (when next-error-recenter
         (recenter next-error-recenter))
+      (message "Current error from %s" next-error-last-buffer)
       (run-hooks 'next-error-hook))))
 
+(defun next-error-select-buffer (buffer)
+  "Select a `next-error' capable buffer and set it as the last used."
+  (interactive
+   (list (get-buffer
+          (read-buffer "Select next-error buffer: " nil nil
+                       (lambda (b) (next-error-buffer-p (cdr b)))))))
+  (setq next-error-last-buffer buffer)
+  (setq-default next-error-last-buffer buffer))
+
 (defalias 'goto-next-locus 'next-error)
 (defalias 'next-match 'next-error)
 

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

> It seems this discussion that came to a standstill here
> hopefully is revived and reached a preliminary consensus in bug#28864.
> Here's the current patch:

Dmitry, could you please confirm that this patch works with the current code
in xref--next-error-function?  If yes, I propose to push it to master.
Then we will get more feedback if something is broken :)

PS: When I typed the closing paren ‘)’ in the last sentence in ‘message-mode’,
it blinked on the letter ‘b’.  Do you know the reason of this weird behavior?

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

Hi Juri,

I'm sorry for the late reply.

On 2/16/18 12:16 AM, Juri Linkov wrote:

> Dmitry, could you please confirm that this patch works with the current code
> in xref--next-error-function?  If yes, I propose to push it to master.

It doesn't seem to help, unfortunately, in the case of xref buffers. 
Take this example:

- emacs -Q
- Navigate to the Emacs source directory, if not already there
- M-x project-find-regexp RET Bazaar RET
- See the buffer that popped up, and the value of next-error-last-buffer 
is *xref*. But it's only set locally (the global value is nil).
- Type 'M-x next-error'. That pops up the ChangeLog.1 buffer.
- next-error-last-buffer is *xref* in the *xref* buffer, and ChangeLog.1 
in the ChangeLog.1 buffer.
- Type 'M-x next-error' again, and see the error:

change-log-goto-source: Cannot find tag or file near ‘point’

Whereas we should still have been using the navigation from *xreF*.

Further, I'm not sure I like bug#20489 being closed with this patch 
only. Even if it worked 100%, it doesn't address everything that came up 
in that discussion. Bug#28864 being closed is fine, and indeed that 
scenario has been addressed.

> PS: When I typed the closing paren ‘)’ in the last sentence in ‘message-mode’,
> it blinked on the letter ‘b’.  Do you know the reason of this weird behavior?

Sorry, never seen this, and can't reproduce. I haven't been using Emacs 
for email lately, though.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 2/27/18 3:21 AM, Dmitry Gutov wrote:

> Further, I'm not sure I like bug#20489 being closed with this patch 
> only. Even if it worked 100%, it doesn't address everything that came up 
> in that discussion. Bug#28864 being closed is fine, and indeed that 
> scenario has been addressed.

Actually, I retract this. next-error-select-buffer should address the 
other major pain point, as soon as the basics work fine.

On that subject: why did we make next-error-last-buffer always buffer-local?

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

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

On 2/27/18 3:54 AM, Dmitry Gutov wrote:

> On that subject: why did we make next-error-last-buffer always 
> buffer-local?

So... Any objections to this patch? It does fix the problem I described.


[-- Attachment #2: next-error-last-buffer-global-again.diff --]
[-- Type: text/x-patch, Size: 2522 bytes --]

diff --git a/lisp/simple.el b/lisp/simple.el
index 2101cfe833..b3ec30edba 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -121,7 +121,6 @@ next-error-last-buffer
 A buffer becomes most recent when its compilation, grep, or
 similar mode is started, or when it is used with \\[next-error]
 or \\[compile-goto-error].")
-(make-variable-buffer-local 'next-error-last-buffer)
 
 (defvar next-error-function nil
   "Function to use to find the next error in the current buffer.
@@ -280,12 +279,10 @@ next-error
     (when buffer
       ;; We know here that next-error-function is a valid symbol we can funcall
       (with-current-buffer buffer
-        ;; Allow next-error to be used from the next-error capable buffer.
-        (setq next-error-last-buffer buffer)
         (funcall next-error-function (prefix-numeric-value arg) reset)
-        ;; Override possible change of next-error-last-buffer in next-error-function
+        ;; Allow next-error to be used from the next-error capable buffer.
+        ;; (If next-error-function changed this var, it will have no effect).
         (setq next-error-last-buffer buffer)
-        (setq-default next-error-last-buffer buffer)
         (when next-error-recenter
           (recenter next-error-recenter))
         (message "%s error from %s"
@@ -301,12 +298,10 @@ next-error-internal
   (let ((buffer (current-buffer)))
     ;; We know here that next-error-function is a valid symbol we can funcall
     (with-current-buffer buffer
-      ;; Allow next-error to be used from the next-error capable buffer.
-      (setq next-error-last-buffer buffer)
       (funcall next-error-function 0 nil)
-      ;; Override possible change of next-error-last-buffer in next-error-function
+      ;; Allow next-error to be used from the next-error capable buffer.
+      ;; (If next-error-function changed this var, it will have no effect).
       (setq next-error-last-buffer buffer)
-      (setq-default next-error-last-buffer buffer)
       (when next-error-recenter
         (recenter next-error-recenter))
       (message "Current error from %s" next-error-last-buffer)
@@ -318,8 +313,7 @@ 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-default next-error-last-buffer buffer))
+  (setq next-error-last-buffer buffer))
 
 (defalias 'goto-next-locus 'next-error)
 (defalias 'next-match 'next-error)

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

> On that subject: why did we make next-error-last-buffer always buffer-local?

next-error-last-buffer is buffer-local to keep the reference to the
parent buffer used to navigate to the current buffer, so the next call of
next-buffer will use the same parent buffer to continue navigation from it.

This works well in most cases except the case of xref buffers.
Below is the explanation for code from next-error with added remarks:

(defun next-error (&optional arg reset)
  (let ((buffer (next-error-find-buffer)))
    (when buffer
      (with-current-buffer buffer

Here the current buffer is *xref*

        (funcall next-error-function (prefix-numeric-value arg) reset)

next-error-function should navigate from *xref* to another buffer
and change the current-buffer to the navigated buffer, e.g. ChangeLog.1.
This works fine in most cases, for example when next-error-function is
compilation-next-error-function, but fails when next-error-function is
xref--next-error-function that switches to ChangeLog.1, but doesn't set
the value current-buffer to ChangeLog.1.

        (setq next-error-last-buffer buffer)

In normal cases this sets buffer-local next-error-last-buffer in the
navigated buffer, e.g. ChangeLog.1 that should be the current buffer.
But since xref--next-error-function doesn't set the right current buffer
to ChangeLog.1, this sets buffer-local next-error-last-buffer in the wrong
buffer, i.e. in *xref*.  IOW, the value returned from (current-buffer)
is wrong here after xref--next-error-function call.

So the question is: in xref--next-error-function can we use code similar
to what is used in compilation-next-error-function that works without problems?

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 2/27/18 11:16 PM, Juri Linkov wrote:
>> On that subject: why did we make next-error-last-buffer always buffer-local?
> 
> next-error-last-buffer is buffer-local to keep the reference to the
> parent buffer used to navigate to the current buffer, so the next call of
> next-buffer will use the same parent buffer to continue navigation from it.

Thank you for the reminder. Looking at the previous discussion, I wasn't 
fond of this design back then, and still think it's largely unnecessary 
complexity (it introduces implicit state, and in the situations that it 
can be useful in, the user will have to remember which windows came from 
which navigation).

Anyway, I've fixed the current problem (see below), so this is a matter 
of opinion. If you still consider this feature to be important, I think 
ideally we'd abstract it away behind a new -function variable as well. 
This way, someone would also be able to implement window-local 
navigation relationship instead of buffer-local (you've mentioned this 
option before).

> This works well in most cases except the case of xref buffers.
> Below is the explanation for code from next-error with added remarks:
> 
> (defun next-error (&optional arg reset)
>    (let ((buffer (next-error-find-buffer)))
>      (when buffer
>        (with-current-buffer buffer
> 
> Here the current buffer is *xref*
> 
>          (funcall next-error-function (prefix-numeric-value arg) reset)
> 
> next-error-function should navigate from *xref* to another buffer
> and change the current-buffer to the navigated buffer, e.g. ChangeLog.1.

If that's something next-error-function must do, let's document it 
better. Right now it only says "Function to use to find the next error 
in the current buffer" (how does one "find" an error?) and describes 
input arguments, but not the return value (which is luckily unused) or 
which buffer must be current, or which window selected at the end.

> This works fine in most cases, for example when next-error-function is
> compilation-next-error-function, but fails when next-error-function is
> xref--next-error-function that switches to ChangeLog.1, but doesn't set
> the value current-buffer to ChangeLog.1.
> 
>          (setq next-error-last-buffer buffer)
> 
> In normal cases this sets buffer-local next-error-last-buffer in the
> navigated buffer, e.g. ChangeLog.1 that should be the current buffer.
> But since xref--next-error-function doesn't set the right current buffer
> to ChangeLog.1, this sets buffer-local next-error-last-buffer in the wrong
> buffer, i.e. in *xref*.  IOW, the value returned from (current-buffer)
> is wrong here after xref--next-error-function call.

I see.

> So the question is: in xref--next-error-function can we use code similar
> to what is used in compilation-next-error-function that works without problems?

You probably mean compilation-goto-locus (it contains the navigation 
part), but even so, it's not easy to tell which part you mean (the 
function does more than one would expect).

In short, adapting that code is kind of difficult, but hopefully I found 
and fixed the problem in xref directly in 
11c58c4fc495ea4f7bff52ca077fd3e4382aa900.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

> Anyway, I've fixed the current problem (see below), so this is a matter of
> opinion. If you still consider this feature to be important, I think
> ideally we'd abstract it away behind a new -function variable as well.

Please clarify what do you have in mind.  In what place in code such
function could be called?

> This way, someone would also be able to implement window-local navigation
> relationship instead of buffer-local (you've mentioned this option before).

Window-local navigation could be implemented by something like below.
But maybe window-local specific code in next-error and next-error-internal
could be abstracted away in a function that you proposed above?

diff --git a/lisp/simple.el b/lisp/simple.el
index edcb73c..c0da57d 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -174,6 +174,8 @@ next-error-find-buffer-function
   "Function called to find a `next-error' capable buffer."
   :type '(choice (const :tag "Single next-error capable buffer on selected frame"
                         next-error-buffer-on-selected-frame)
+                 (const :tag "Previous next-error capable buffer on selected window"
+                        next-error-buffer-on-selected-window)
                  (const :tag "No default" ignore)
                  (function :tag "Other function"))
   :group 'next-error
@@ -195,6 +197,12 @@ next-error-buffer-on-selected-frame
     (if (eq (length window-buffers) 1)
         (car window-buffers))))
 
+(defun next-error-buffer-on-selected-window (&optional _avoid-current
+                                                       _extra-test-inclusive
+                                                       _extra-test-exclusive)
+  "Return the previous next-error buffer used in the selected window."
+  (window-parameter nil 'next-error-buffer))
+
 (defun next-error-find-buffer (&optional avoid-current
 					 extra-test-inclusive
 					 extra-test-exclusive)
@@ -285,6 +293,9 @@ next-error
         ;; Override possible change of next-error-last-buffer in next-error-function
         (setq next-error-last-buffer buffer)
         (setq-default next-error-last-buffer buffer)
+        (when (eq next-error-find-buffer-function
+                  'next-error-buffer-on-selected-window)
+          (set-window-parameter nil 'next-error-buffer buffer))
         (when next-error-recenter
           (recenter next-error-recenter))
         (message "%s error from %s"
@@ -306,6 +317,9 @@ next-error-internal
       ;; Override possible change of next-error-last-buffer in next-error-function
       (setq next-error-last-buffer buffer)
       (setq-default next-error-last-buffer buffer)
+      (when (eq next-error-find-buffer-function
+                'next-error-buffer-on-selected-window)
+        (set-window-parameter nil 'next-error-buffer buffer))
       (when next-error-recenter
         (recenter next-error-recenter))
       (message "Current error from %s" next-error-last-buffer)

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 2/28/18 11:17 PM, Juri Linkov wrote:
>> Anyway, I've fixed the current problem (see below), so this is a matter of
>> opinion. If you still consider this feature to be important, I think
>> ideally we'd abstract it away behind a new -function variable as well.
> 
> Please clarify what do you have in mind.  In what place in code such
> function could be called?

Any place that contains

   (setq next-error-last-buffer buffer)
   (setq-default next-error-last-buffer buffer)

now, would instead call

   (funcall next-error-save-last-buffer-function target-buf target-win)

>> This way, someone would also be able to implement window-local navigation
>> relationship instead of buffer-local (you've mentioned this option before).
> 
> Window-local navigation could be implemented by something like below.
> But maybe window-local specific code in next-error and next-error-internal
> could be abstracted away in a function that you proposed above?

Yes.

next-error-save-last-buffer-function and next-error-find-buffer-function 
would have to be in sync (we'd have to somehow make sure the user will 
have to try hard to set them to incompatible values, at least if they do 
that via the Customize interface).

Further, I think next-error-find-buffer-function will also have to 
encompass the item 2 in next-error-find-buffer, so that the alternative 
could supply the window-local value instead.

Anyway, I'd really like to put a pin in both buffer-local and 
window-local values for now, because they both complicate the picture.

Instead, could we address bug#30674 first? Personally, moving between 
errors and warnings in the current file using next/previous-error feels 
a lot more useful.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

>>> Anyway, I've fixed the current problem (see below), so this is a matter of
>>> opinion. If you still consider this feature to be important, I think
>>> ideally we'd abstract it away behind a new -function variable as well.
>>
>> Please clarify what do you have in mind.  In what place in code such
>> function could be called?
>
> Any place that contains
>
>   (setq next-error-last-buffer buffer)
>   (setq-default next-error-last-buffer buffer)
>
> now, would instead call
>
>   (funcall next-error-save-last-buffer-function target-buf target-win)

But isn't it possible to do this in next-error-hook?
It's called by run-hooks in the same place in next-error.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 3/7/18 12:17 AM, Juri Linkov wrote:

>>> Please clarify what do you have in mind.  In what place in code such
>>> function could be called?
>>
>> Any place that contains
>>
>>    (setq next-error-last-buffer buffer)
>>    (setq-default next-error-last-buffer buffer)
>>
>> now, would instead call
>>
>>    (funcall next-error-save-last-buffer-function target-buf target-win)
> 
> But isn't it possible to do this in next-error-hook?
> It's called by run-hooks in the same place in next-error.

For one thing, next-error-select-buffer doesn't call next-error-hook.

And there are other such places (e.g. like you were saying 
xref--xref-buffer-mode should set next-error-last-buffer similarly).

Aside from that, why would we want to obscure this piece of logic behind 
a hook?

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

>>>> Please clarify what do you have in mind.  In what place in code such
>>>> function could be called?
>>>
>>> Any place that contains
>>>
>>>    (setq next-error-last-buffer buffer)
>>>    (setq-default next-error-last-buffer buffer)
>>>
>>> now, would instead call
>>>
>>>    (funcall next-error-save-last-buffer-function target-buf target-win)
>>
>> But isn't it possible to do this in next-error-hook?
>> It's called by run-hooks in the same place in next-error.
>
> For one thing, next-error-select-buffer doesn't call next-error-hook.
>
> And there are other such places (e.g. like you were saying
> xref--xref-buffer-mode should set next-error-last-buffer similarly).

3. And hooks are intended only for user customization.

> Aside from that, why would we want to obscure this piece of logic behind
> a hook?

Instead of a hook I think it would be fine to define an “advisable” function
like Stefan asked to do for next-error-find-buffer-function to be able
to put advices on it.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 3/7/18 11:11 PM, Juri Linkov wrote:

>> For one thing, next-error-select-buffer doesn't call next-error-hook.
>>
>> And there are other such places (e.g. like you were saying
>> xref--xref-buffer-mode should set next-error-last-buffer similarly).
> 
> 3. And hooks are intended only for user customization.

Well, we do use them in the core as well. E.g. in 
define-globalized-minor-mode.

>> Aside from that, why would we want to obscure this piece of logic behind
>> a hook?
> 
> Instead of a hook I think it would be fine to define an “advisable” function
> like Stefan asked to do for next-error-find-buffer-function to be able
> to put advices on it.

next-error-find-buffer-function would advise it? How? I don't quite see 
the design.

Also, next-error-save-last-buffer-function might be invoked before 
next-error-find-buffer-function is ever called. E.g. by compilation-start.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

> This way, someone would also be able to implement window-local navigation
> relationship instead of buffer-local (you've mentioned this option before).

In addition to code sent in the previous message that should select the right
next-error capable buffer from the navigated buffer (e.g. on typing ‘next-error’),
for window-local navigation we also need to select the last used window (e.g.
typing RET on the list of matches), so all navigation will be in the same window.

(defun display-buffer-in-next-error-last-window (buffer alist)
  "Return a window used to display the last next-error buffer."
  (let* ((window (car (delq nil
          (mapcar (lambda (w) (when (and (local-variable-p 'next-error-last-buffer
                                                           (window-buffer w))
                                         (eq (current-buffer)
                                             (buffer-local-value
                                              'next-error-last-buffer
                                              (window-buffer w))))
                                w))
                  (window-list-1 nil 'nomini))))))
    (when (window-live-p window)
      window
      (prog1 (window--display-buffer buffer window 'reuse alist)
        (unless (cdr (assq 'inhibit-switch-frame alist))
          (window--maybe-raise-frame (window-frame window)))))))

(push '("\\`\\*\\(compilation\\|grep\\|Occur\\)\\*\\'"
	display-buffer-in-next-error-last-window
	(inhibit-same-window . t))
      display-buffer-from-alist)

But this requires a feature that is not yet implemented, namely
a customization like ‘display-buffer-from-alist’ to match the source
buffer name (that was the current buffer before navigation) instead
of the target buffer (the buffer to display) like ‘display-buffer-alist’
currently specifies.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

> But this requires a feature that is not yet implemented, namely
> a customization like ‘display-buffer-from-alist’ to match the source
> buffer name (that was the current buffer before navigation) instead
> of the target buffer (the buffer to display) like ‘display-buffer-alist’
> currently specifies.

Actually no new feature is needed, ‘display-buffer-alist’ is powerful enough,
and this can be easily implemented with a few lines of code:

1. next-error for *xref* to reuse the same window:

(push '(display-buffer-condition-next-error
        display-buffer-same-window)
      display-buffer-alist)

(defun display-buffer-condition-next-error (_buffer-name _action)
  (memq this-command '(next-error previous-error)))

2. with e.g. two *grep* windows, typing RET to select the right locus window:

(push '(display-buffer-condition-from-next-error-buffer
        display-buffer-in-next-error-last-window
        (inhibit-same-window . t))
      display-buffer-alist)

(defun display-buffer-condition-from-next-error-buffer (_buffer-name _action)
  (string-match-p "\\`\\*\\(compilation\\|grep\\|Occur\\)\\*\\(\\|<[0-9]+>\\)\\'"
		  (buffer-name (current-buffer))))

(defun display-buffer-in-next-error-last-window (buffer alist)
  "Return a window used to display the last next-error buffer."
  (let* ((window (car (delq nil
          (mapcar (lambda (w) (when (and (local-variable-p 'next-error-last-buffer
                                                           (window-buffer w))
                                         (eq (current-buffer)
                                             (buffer-local-value
                                              'next-error-last-buffer
                                              (window-buffer w))))
                                w))
                  (window-list-1 nil 'nomini))))))
    (when (window-live-p window)
      window
      (prog1 (window--display-buffer buffer window 'reuse alist)
        (unless (cdr (assq 'inhibit-switch-frame alist))
          (window--maybe-raise-frame (window-frame window)))))))

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 3/2/18 12:58 AM, Juri Linkov wrote:

> Actually no new feature is needed, ‘display-buffer-alist’ is powerful enough,
> and this can be easily implemented with a few lines of code:
> 
> 1. next-error for *xref* to reuse the same window:
> 
> ...
> 
> 2. with e.g. two *grep* windows, typing RET to select the right locus window:
> 
> ...
Both of these sound pretty cool (and a fair distance over my head). I'm 
not sure they'd make a radical change to any of my workflows. In many 
ways, I feel window management is a bit of a lost cause in Emacs, and if 
anyone's going to fix that, next-error is probably not the place to start.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

>> Actually no new feature is needed, ‘display-buffer-alist’ is powerful enough,
>> and this can be easily implemented with a few lines of code:
>>
>> 1. next-error for *xref* to reuse the same window:
>>
>> ...
>>
>> 2. with e.g. two *grep* windows, typing RET to select the right locus window:
>>
>> ...
> Both of these sound pretty cool (and a fair distance over my head). I'm not
> sure they'd make a radical change to any of my workflows.

I created such complex solution to work around the peculiarities of xref:
navigating with next-error displays every new opened buffer in another window,
thus eventually obscuring the *xref* buffer itself.  Could you change xref
to use the same logic as is used in compilation/grep that shows all next-error
places in the same window adjacent to the initial window with the list of
compilation errors or grep hits that are always visible on the screen.

> In many ways, I feel window management is a bit of a lost cause in
> Emacs, and if anyone's going to fix that, next-error is probably not
> the place to start.

For better window management we need a higher declarative layer of
configuration based on display-buffer-alist.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 3/7/18 12:25 AM, Juri Linkov wrote:

> I created such complex solution to work around the peculiarities of xref:
> navigating with next-error displays every new opened buffer in another window,
> thus eventually obscuring the *xref* buffer itself.

That's odd. I remember having such problems before, but not since Joao's 
efforts landed in 2a973edeacefcabb9fd8024188b7e167f0f9a9b6.

> Could you change xref
> to use the same logic as is used in compilation/grep that shows all next-error
> places in the same window adjacent to the initial window with the list of
> compilation errors or grep hits that are always visible on the screen.

More generally, if such behavior is desirable by all next-error-function 
setups, shouldn't next-error somehow enforce it? Or provide helpers, at 
least.

Anyway, it works fine for me in xref already. Do you have a repro 
scenario in Emacs master?

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

>> I created such complex solution to work around the peculiarities of xref:
>> navigating with next-error displays every new opened buffer in another window,
>> thus eventually obscuring the *xref* buffer itself.
>
> That's odd. I remember having such problems before, but not since Joao's
> efforts landed in 2a973edeacefcabb9fd8024188b7e167f0f9a9b6.
> [...]
> Anyway, it works fine for me in xref already. Do you have a repro scenario
> in Emacs master?

Sorry, can't reproduce anymore, looks like already fixed by João.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

>> This works well in most cases except the case of xref buffers.
>> Below is the explanation for code from next-error with added remarks:
>>
>> (defun next-error (&optional arg reset)
>>    (let ((buffer (next-error-find-buffer)))
>>      (when buffer
>>        (with-current-buffer buffer
>>
>> Here the current buffer is *xref*
>>
>>          (funcall next-error-function (prefix-numeric-value arg) reset)
>>
>> next-error-function should navigate from *xref* to another buffer
>> and change the current-buffer to the navigated buffer, e.g. ChangeLog.1.
>
> If that's something next-error-function must do, let's document it
> better. Right now it only says "Function to use to find the next error in
> the current buffer" (how does one "find" an error?) and describes input
> arguments, but not the return value (which is luckily unused) or which
> buffer must be current, or which window selected at the end.

It's not yet clear which window should be selected at the end,
please see the recent bug#30646.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 2/28/18 11:32 PM, Juri Linkov wrote:

>> If that's something next-error-function must do, let's document it
>> better. Right now it only says "Function to use to find the next error in
>> the current buffer" (how does one "find" an error?) and describes input
>> arguments, but not the return value (which is luckily unused) or which
>> buffer must be current, or which window selected at the end.
> 
> It's not yet clear which window should be selected at the end,
> please see the recent bug#30646.

I'm not seeing that problem with xref, or Grep. Guessing it's a bug in 
occur-next-error, which as you noted in a more recent message, doesn't 
set the current buffer to the right value at the end.

No need to change next-error for this.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

>> So the question is: in xref--next-error-function can we use code similar
>> to what is used in compilation-next-error-function that works without problems?
>
> You probably mean compilation-goto-locus (it contains the navigation part),
> but even so, it's not easy to tell which part you mean (the function does
> more than one would expect).
>
> In short, adapting that code is kind of difficult, but hopefully I found
> and fixed the problem in xref directly
> in 11c58c4fc495ea4f7bff52ca077fd3e4382aa900.

While looking at this, I discovered more problems:

1. the same problem that you fixed for xref, also exists in occur:
   it doesn't set the right current-buffer for next-error.

2. also typing RET in *Occur* doesn't set next-error-last-buffer.
   Maybe we need to set it in occur-mode-goto-occurrence (but not in occur-next-error)

3. typing RET in *xref* should set next-error-last-buffer as well

4. Maybe these 2 lines needed to be added also to xref--xref-buffer-mode:

        (setq next-error-last-buffer buffer)
        (setq-default next-error-last-buffer buffer)

like in next-error to set both buffer-local and global values.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Dmitry Gutov]

On 3/2/18 1:04 AM, Juri Linkov wrote:

>> In short, adapting that code is kind of difficult, but hopefully I found
>> and fixed the problem in xref directly
>> in 11c58c4fc495ea4f7bff52ca077fd3e4382aa900.
> 
> While looking at this, I discovered more problems:
> 
> 1. the same problem that you fixed for xref, also exists in occur:
>     it doesn't set the right current-buffer for next-error.

Sounds indeed like a bug.

> 2. also typing RET in *Occur* doesn't set next-error-last-buffer.
>     Maybe we need to set it in occur-mode-goto-occurrence (but not in occur-next-error)

Should it? Why? Hopefully, not just because Compilation does it (if we 
just want consistency, we can fix it in the other way, too).

> 3. typing RET in *xref* should set next-error-last-buffer as well

Same question.

> 4. Maybe these 2 lines needed to be added also to xref--xref-buffer-mode:
> 
>          (setq next-error-last-buffer buffer)
>          (setq-default next-error-last-buffer buffer)
> 
> like in next-error to set both buffer-local and global values.

I guess so (but one of them is already there). TBH, seeing these two 
lines together still makes me cringe a little.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > On that subject: why did we make next-error-last-buffer always buffer-local?

  > next-error-last-buffer is buffer-local to keep the reference to the
  > parent buffer used to navigate to the current buffer, so the next call of
  > next-buffer will use the same parent buffer to continue navigation from it.

How about adding a comment to this effect?

-- 
Dr Richard Stallman
President, Free Software Foundation (https://gnu.org, https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)
Skype: No way! See https://stallman.org/skype.html.

bug#20489: 25.0.50; next-error-find-buffer chooses non-current buffer without good reason

[Juri Linkov]

> It seems this discussion that came to a standstill here
> hopefully is revived and reached a preliminary consensus in bug#28864.
> Here's the current patch:

Finally pushed to master and closed.