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

[Dmitry Gutov <dgutov@yandex.ru>]

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

> I'm thinking about adding 3 customizable options for
> next-error-find-buffer-function:
> 
> 1. buffer-local - new default
> 2. window-local - useful for one window per each navigation buffer

I'm not sure either can be congruent to all next-error-function 
applications. Some next-error source buffers contain their own errors 
(so buffer-local is natural), and others point to errors in other 
buffers (supposing they can learn to open those in the same window, 
window-local might be fitting). But both kinds are there, and all users 
deal with both.

> 3. frame-local - old implicit default now separated into its own function

That doesn't sound like a sufficient description: the old default also 
includes visibility-based logic. So it's not just one value per frame.

>> In general, next-error can jump between windows, so window-local is not
>> a good fit for my mental model.
> 
> It's bad when next-error unpredictably jumps between windows.
> I hope we could find a way to fix this erratic behavior.

It basically a rule now that Grep opens errors in a different windows 
(so that the Grep buffer stays visible). So erratic or not, multiple 
windows are used routinely.

>> Do we need the buffer-local-ity changes to next-error-last-buffer for that?
>> Because if we do, that's okay, let's commit it and everything, but
>> otherwise I'd rather wait and look for a more elegant approach (for other
>> issues aside from this one).
> 
> In the last previous patch, next-error visits next-error-find-buffer,
> calls next-error-function that possibly navigates to another buffer,
> then sets both global and buffer-local of next-error-last-buffer.
> This seems quite logical.  And it works in all my tests.

That... doesn't answer my question, I think. Or I didn't understand the 
answer.

So let me try again: is the new next-error-find-buffer stuff necessary 
to fix the current bug? Or is suppressing the change to 
next-error-last-buffer during next-error-function calls enough for that?

> We could use "current" for 0.

Fine by me.

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

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

>>> +(defun next-error-internal ()
>>> +  "Visit the source code corresponding to the `next-error' message at point."
>>> +  (let ((next-error-buffer (current-buffer)))
>>>        ;; we know here that next-error-function is a valid symbol we can funcall
>>> -    (with-current-buffer next-error-last-buffer
>>> -      (funcall next-error-function (prefix-numeric-value arg) reset)
>>> +    (with-current-buffer next-error-buffer
>>> +      (funcall next-error-function 0 nil)
>>> +      ;; next-error-function might change the value of
>>> +      ;; next-error-last-buffer, so set it later
>>> +      (setq next-error-last-buffer next-error-buffer)
>>> +      (setq-default next-error-last-buffer next-error-last-buffer)
>>>          (when next-error-recenter
>>>            (recenter next-error-recenter))
>>> +      (message "Showing another error from %s" next-error-last-buffer)
>>
>> Is it really "another"? Or maybe it's "current", "last" or "closest" error?
> 
> Maybe just "Showing error from %s"?  Or simply "Error from %s".
> Then we could simplify the above messages as well: "%s error from %s"
> for e.g. "Next error from %s", "Previous error from %s", ...

Why not use "current" here as well? After all, we pass 0 to 
next-error-function here.