Re: [Emacs-diffs] master 108ce84 2/2: xref--next-error-function: Move xref's window point

Re: [Emacs-diffs] master 108ce84 2/2: xref--next-error-function: Move xref's window point

[Dmitry Gutov]

Eli, what would you say about backporting this to emacs-26?

This change is limited in scope, it fixes annoying "looping" behavior 
I've seen in some cases, and it adds window point movement you've asked 
for before in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=20489#110

> branch: master
> commit 108ce84432d597f92637ea74bd0a094224d157de
> Author: Dmitry Gutov <dgutov@yandex.ru>
> Commit: Dmitry Gutov <dgutov@yandex.ru>
> 
>      xref--next-error-function: Move xref's window point
>      
>      * lisp/progmodes/xref.el (xref--next-error-function): Move
>      xref's window point if it's visible.  When we don't do that,
>      navigation can start looping after a while.
> ---
>   lisp/progmodes/xref.el | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
> index 1c1fc59..5a9a7a9 100644
> --- a/lisp/progmodes/xref.el
> +++ b/lisp/progmodes/xref.el
> @@ -693,6 +693,10 @@ references displayed in the current *xref* buffer."
>       (dotimes (_ n)
>         (setq xref (xref--search-property 'xref-item backward)))
>       (cond (xref
> +           ;; Save the current position (when the buffer is visible,
> +           ;; it gets reset to that window's point from time to time).
> +           (let ((win (get-buffer-window (current-buffer))))
> +             (and win (set-window-point win (point))))
>              (xref--show-location (xref-item-location xref) t))
>             (t
>              (error "No %s xref" (if backward "previous" "next"))))))
> 
> _______________________________________________
> Emacs-diffs mailing list
> Emacs-diffs@gnu.org
> https://lists.gnu.org/mailman/listinfo/emacs-diffs
> 

Re: [Emacs-diffs] master 108ce84 2/2: xref--next-error-function: Move xref's window point

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 28 Feb 2018 04:13:22 +0200
> 
> Eli, what would you say about backporting this to emacs-26?
> 
> This change is limited in scope, it fixes annoying "looping" behavior 
> I've seen in some cases, and it adds window point movement you've asked 
> for before in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=20489#110
> 
> > branch: master
> > commit 108ce84432d597f92637ea74bd0a094224d157de
> > Author: Dmitry Gutov <dgutov@yandex.ru>
> > Commit: Dmitry Gutov <dgutov@yandex.ru>
> > 
> >      xref--next-error-function: Move xref's window point
> >      
> >      * lisp/progmodes/xref.el (xref--next-error-function): Move
> >      xref's window point if it's visible.  When we don't do that,
> >      navigation can start looping after a while.

Sorry, this doesn't tell which problem(s) it attempts to fix, and
there are no references to discussions or bug reports that could help
me figure that out.  Without that, I cannot make up my mind about the
importance of this fix.

Re: [Emacs-diffs] master 108ce84 2/2: xref--next-error-function: Move xref's window point

[Dmitry Gutov]

On 2/28/18 5:28 PM, Eli Zaretskii wrote:

>>> branch: master
>>> commit 108ce84432d597f92637ea74bd0a094224d157de
>>> Author: Dmitry Gutov <dgutov@yandex.ru>
>>> Commit: Dmitry Gutov <dgutov@yandex.ru>
>>>
>>>       xref--next-error-function: Move xref's window point
>>>       
>>>       * lisp/progmodes/xref.el (xref--next-error-function): Move
>>>       xref's window point if it's visible.  When we don't do that,
>>>       navigation can start looping after a while.
> 
> Sorry, this doesn't tell which problem(s) it attempts to fix, and
> there are no references to discussions or bug reports that could help
> me figure that out.  Without that, I cannot make up my mind about the
> importance of this fix.

next-error can misbehave (resume from an earlier position) after several 
invocations when next-error-last-buffer is an xref buffer, and that 
buffer is visible.

Try:

1. cd .../emacs/source/dir
2. emacs -Q
3. M-x project-find-regexp RET It is not necessary to RET
(The searches for "It is not necessary to", which only occurs in .el files).
4. M-x next-error, do that a few times. Make sure *xref* is still 
visible (otherwise, you need to tweak the split-threshold variables).
5. Select the window showing the xref buffer, then go back to where you 
were (by typing 'C-x o' a couple of times).
6. M-x next-error will return to the beginning. Or, if the window point 
of the xref window is not at the first line, start over from there.

The reason I figured it's urgent, is step 5 is not necessary with my 
work configuration (not 'emacs -Q'). It's hard for me to tell now which 
package/setting/timer has this effect, but it seems reasonable to 
believe that some other users will see this too. Essentially, the code 
in emacs-26 currently relies on "undefined behavior", in C terms.

Re: [Emacs-diffs] master 108ce84 2/2: xref--next-error-function: Move xref's window point

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 28 Feb 2018 17:58:59 +0200
> 
> > Sorry, this doesn't tell which problem(s) it attempts to fix, and
> > there are no references to discussions or bug reports that could help
> > me figure that out.  Without that, I cannot make up my mind about the
> > importance of this fix.
> 
> next-error can misbehave (resume from an earlier position) after several 
> invocations when next-error-last-buffer is an xref buffer, and that 
> buffer is visible.
> 
> Try:
> 
> 1. cd .../emacs/source/dir
> 2. emacs -Q
> 3. M-x project-find-regexp RET It is not necessary to RET
> (The searches for "It is not necessary to", which only occurs in .el files).
> 4. M-x next-error, do that a few times. Make sure *xref* is still 
> visible (otherwise, you need to tweak the split-threshold variables).
> 5. Select the window showing the xref buffer, then go back to where you 
> were (by typing 'C-x o' a couple of times).
> 6. M-x next-error will return to the beginning. Or, if the window point 
> of the xref window is not at the first line, start over from there.

Is this due to switch-to-buffer-preserve-window-point being t by
default now?

> The reason I figured it's urgent, is step 5 is not necessary with my 
> work configuration (not 'emacs -Q'). It's hard for me to tell now which 
> package/setting/timer has this effect, but it seems reasonable to 
> believe that some other users will see this too. Essentially, the code 
> in emacs-26 currently relies on "undefined behavior", in C terms.

I'm not sure I see where does "undefined behavior" come from.

Re: [Emacs-diffs] master 108ce84 2/2: xref--next-error-function: Move xref's window point

[Dmitry Gutov]

On 3/2/18 3:39 PM, Eli Zaretskii wrote:

>> Try:
>>
>> 1. cd .../emacs/source/dir
>> 2. emacs -Q
>> 3. M-x project-find-regexp RET It is not necessary to RET
>> (The searches for "It is not necessary to", which only occurs in .el files).
>> 4. M-x next-error, do that a few times. Make sure *xref* is still
>> visible (otherwise, you need to tweak the split-threshold variables).
>> 5. Select the window showing the xref buffer, then go back to where you
>> were (by typing 'C-x o' a couple of times).
>> 6. M-x next-error will return to the beginning. Or, if the window point
>> of the xref window is not at the first line, start over from there.
> 
> Is this due to switch-to-buffer-preserve-window-point being t by
> default now?

I want to say yes because that fits my understanding of the problem. But 
(setq switch-to-buffer-preserve-window-point nil) doesn't change neither 
the scenario above, nor its behavior in my work configuration.
>> The reason I figured it's urgent, is step 5 is not necessary with my
>> work configuration (not 'emacs -Q'). It's hard for me to tell now which
>> package/setting/timer has this effect, but it seems reasonable to
>> believe that some other users will see this too. Essentially, the code
>> in emacs-26 currently relies on "undefined behavior", in C terms.
> 
> I'm not sure I see where does "undefined behavior" come from.

"Behavior I don't understand" might be closer to the truth. :-)

Re: [Emacs-diffs] master 108ce84 2/2: xref--next-error-function: Move xref's window point

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 2 Mar 2018 16:10:23 +0200
> 
> >> The reason I figured it's urgent, is step 5 is not necessary with my
> >> work configuration (not 'emacs -Q'). It's hard for me to tell now which
> >> package/setting/timer has this effect, but it seems reasonable to
> >> believe that some other users will see this too. Essentially, the code
> >> in emacs-26 currently relies on "undefined behavior", in C terms.
> > 
> > I'm not sure I see where does "undefined behavior" come from.
> 
> "Behavior I don't understand" might be closer to the truth. :-)

That's not very inspiring for a release branch...

But okay, please push.

Re: [Emacs-diffs] master 108ce84 2/2: xref--next-error-function: Move xref's window point

[Dmitry Gutov]

On 3/2/18 4:50 PM, Eli Zaretskii wrote:
>> Cc: emacs-devel@gnu.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Fri, 2 Mar 2018 16:10:23 +0200
>>
>>>> The reason I figured it's urgent, is step 5 is not necessary with my
>>>> work configuration (not 'emacs -Q'). It's hard for me to tell now which
>>>> package/setting/timer has this effect, but it seems reasonable to
>>>> believe that some other users will see this too. Essentially, the code
>>>> in emacs-26 currently relies on "undefined behavior", in C terms.
>>>
>>> I'm not sure I see where does "undefined behavior" come from.
>>
>> "Behavior I don't understand" might be closer to the truth. :-)
> 
> That's not very inspiring for a release branch...

Well, at least the scope is very limited.

> But okay, please push.

Done, thanks.