xref window switching problems

xref window switching problems

[Ingo Lohmar]

It seems to me that xref has quite.. unusual behavior when it comes to
popping to a location; it surely broke my expectations.  I could not
find any bug report for this; so before "patching" sth which isn't
broken, it cannot hurt to ask...

xref--pop-to-location *first* uses xref--goto-location to select the
target buffer and move point to the proper position.  As long as the
target buffer is different from the source buffer, that's fine.

However, when I call xref-find-definitions-other-window (typically "C-x
4 ."), and the definition is in the same buffer I am in now, strange
things happen:

- the point of the *current* window is moved to the target location
  (just what the user wanted to avoid, I suppose)

- when the current window is the only one showing the buffer in
  question, then the same buffer is newly displayed in another window we
  pop to (hence at the same position)

- when another window already shows the same buffer, we pop to this
  buffer --- less annoying, but still unexpected and not helpful

The behavior is perfectly understandable looking at the code of
xref--pop-to-location and xref--goto-location, but is this intended?

The old pre-xref elisp implementations and the tags functions got the
"...-other-window" behavior right IMO: Leave my current window's buffer
and point alone in any case, and pop to the target shown in another
window.

The change would be easy (see xref--pop-to-location), first get the
buffer, then do the window business, and only then xref--goto-location.

Any objections/opinions?  It always puzzles me how hard it can be to get
the window-changing stuff to do just what I want, so any feedback is
helpful.  Maybe xref-pop-marker-stack could benefit from a discussion as
well.

Ingo

Re: xref window switching problems

[Stephen Leake]

Ingo Lohmar <i.lohmar@gmail.com> writes:

> The behavior is perfectly understandable looking at the code of
> xref--pop-to-location and xref--goto-location, but is this intended?

I can't speak to whether it's intended, but I agree it's not good.

> The old pre-xref elisp implementations and the tags functions got the
> "...-other-window" behavior right IMO: Leave my current window's buffer
> and point alone in any case, and pop to the target shown in another
> window.

+1

> The change would be easy (see xref--pop-to-location), first get the
> buffer, then do the window business, and only then xref--goto-location.
>
> Any objections/opinions?  

Sounds good to me

> Maybe xref-pop-marker-stack could benefit from a discussion as
> well.

I haven't used that much, but so far it does what I want; change the
current window to display the previous location.

If I want to preserve the current location in a window, I would first
move to a different window; I haven't tested to see if that works in the
one buffer/two locations scenario.

-- 
-- Stephe

Re: xref window switching problems

[Dmitry Gutov]

On 08/05/2015 09:56 AM, Stephen Leake wrote:

>> Maybe xref-pop-marker-stack could benefit from a discussion as
>> well.
>
> I haven't used that much, but so far it does what I want; change the
> current window to display the previous location.
>
> If I want to preserve the current location in a window, I would first
> move to a different window; I haven't tested to see if that works in the
> one buffer/two locations scenario.

I think the behavior of xref-find-definitions-other-window followed by 
xref-pop-marker-stack leaves a lot to be desired.

However, I don't see an obvious way of fixing it. Apparently, 
xref--marker-ring will have to store more information.

Would it keep track of the buffer opened in the new window as well? So 
that xref-pop-marker-stack would quit that window?

Re: xref window switching problems

[Stephen Leake]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 08/05/2015 09:56 AM, Stephen Leake wrote:
>
>>> Maybe xref-pop-marker-stack could benefit from a discussion as
>>> well.
>>
>> I haven't used that much, but so far it does what I want; change the
>> current window to display the previous location.
>>
>> If I want to preserve the current location in a window, I would first
>> move to a different window; I haven't tested to see if that works in the
>> one buffer/two locations scenario.
>
> I think the behavior of xref-find-definitions-other-window followed by
> xref-pop-marker-stack leaves a lot to be desired.

What, exactly, do you not like?

Suppose I'm in ede/emacs.el, with one window in one frame, point is on
'file-name-extension',  and I invoke C-x 4 .

The current window is split, and point is now in files.el at "(defun
file-name-extension".

Now I invoke M-,. The window displaying files.el now has emacs.el; I now
have two windows displaying the same thing.

That's exactly what I asked for, so I don't have a problem with it.

I agree it's not particularly useful, but the solution is simple; don't
do that.

Often I will invoke M-, multiple times, to go back to where I started; I
don't want Emacs messing with windows on the way. I'll manage my own
windows, thank you.

There might be a use case for xref-pop-marker-stack-other-window. I'm
working on a more general solution, that uses a prefix key to specify
'other window' or 'other frame' to _any_ command that uses
display-buffer or switch-to-buffer. The code is almost ready; stay tuned
:).

> However, I don't see an obvious way of fixing it. Apparently,
> xref--marker-ring will have to store more information.
>
> Would it keep track of the buffer opened in the new window as well? So
> that xref-pop-marker-stack would quit that window?

No, the user probably wants to display something else in it. Using a
subsequent M-,, for example.

The user specifically requested that a second window be created. Don't
delete it unless the user specifically requests that.

A window created by an -other-window command is not at all like a
temporary prompt window. For xref-find-definitions, it could be
considered to be like a *Help* window; those hang around until the user
explicitly deletes them.

-- 
-- Stephe

Re: xref window switching problems

[Dmitry Gutov]

On 08/04/2015 10:42 PM, Ingo Lohmar wrote:

> - the point of the *current* window is moved to the target location
>    (just what the user wanted to avoid, I suppose)

That's clearly a bug. Should be fixed now, thanks.

> - when the current window is the only one showing the buffer in
>    question, then the same buffer is newly displayed in another window we
>    pop to (hence at the same position)

Aside from the bug above, this seems to be the intended behavior.

> - when another window already shows the same buffer, we pop to this
>    buffer --- less annoying, but still unexpected and not helpful

I'm not sure if it's a problem, actually. That's how pop-to-buffer 
works. In general, it would even use the current window if it already 
displays the buffer in question, but for 
xref-find-definitions-other-window, we explicitly ask it to use another 
window. So it uses the other window displaying the same buffer.

> The old pre-xref elisp implementations and the tags functions got the
> "...-other-window" behavior right IMO: Leave my current window's buffer
> and point alone in any case, and pop to the target shown in another
> window.

Are you sure it doesn't reuse the second window that displays the same 
buffer?

> The change would be easy (see xref--pop-to-location), first get the
> buffer, then do the window business, and only then xref--goto-location.

The choice of how to implement it was not so easy. Pretty much because

(with-current-buffer (current-buffer) (goto-char (point-min)))

moves point in the current window.

> Maybe xref-pop-marker-stack could benefit from a discussion as well.

Maybe it could benefit from a patch proposal.

Re: xref window switching problems

[Ingo Lohmar]

On Wed, Aug 05 2015 15:33 (+0300), Dmitry Gutov wrote:

> On 08/04/2015 10:42 PM, Ingo Lohmar wrote:
>
>> - the point of the *current* window is moved to the target location
>>    (just what the user wanted to avoid, I suppose)
>
> That's clearly a bug. Should be fixed now, thanks.

Awesome, thanks.

>
>> - when the current window is the only one showing the buffer in
>>    question, then the same buffer is newly displayed in another window we
>>    pop to (hence at the same position)
>
> Aside from the bug above, this seems to be the intended behavior.
>

This was to say that at that point, *neither* of the two windows showed
the original position.  In any case, the behavior is definitely fixed
now.

>> - when another window already shows the same buffer, we pop to this
>>    buffer --- less annoying, but still unexpected and not helpful
>
> I'm not sure if it's a problem, actually. That's how pop-to-buffer
> works. In general, it would even use the current window if it already
> displays the buffer in question, but for
> xref-find-definitions-other-window, we explicitly ask it to use another
> window. So it uses the other window displaying the same buffer.
>
>> The old pre-xref elisp implementations and the tags functions got the
>> "...-other-window" behavior right IMO: Leave my current window's buffer
>> and point alone in any case, and pop to the target shown in another
>> window.
>
> Are you sure it doesn't reuse the second window that displays the same
> buffer?
>

Maybe I was not expressing my thoughts clearly.  Reusing the second
window is what the functions did, and that's just fine --- only the
current window when calling the command should be left alone (and now,
it is).

>> The change would be easy (see xref--pop-to-location), first get the
>> buffer, then do the window business, and only then xref--goto-location.
>
> The choice of how to implement it was not so easy. Pretty much because
>
> (with-current-buffer (current-buffer) (goto-char (point-min)))
>
> moves point in the current window.
>

Except for factoring out xref--goto-char, your patch looks just like my
private testing.

>> Maybe xref-pop-marker-stack could benefit from a discussion as well.
>
> Maybe it could benefit from a patch proposal.

That sounds a bit snarky to me... I am sorry if the last sentence came
across wrong.  Given the preceding sentences (which you did not quote),
I thought it would have been clear this is merely a related
afterthought, and not meant to disparage your work on xref in any way.
In fact, I am happy about that effort to unify and standardize commands.

You're obviously right that a concrete suggestion is more helpful.

Disclaimer: Recent discussion seems to have centered around the question
if the newly opened file buffers should be cleaned up somehow.  I am not
worried nor bothered by having all these buffers opened.  It's always
been that way in Emacs, and it's never been a problem for me.

I care more (but it's not all that important to me) about the buffers
and points *shown* in windows.  In that respect, what if
xref--marker-ring stored, together with the location where
xref-push-marker-stack is called, the window where it is called.  Then
xref-pop-marker-stack could look at the top of the stack and:
- if window is same as current: same as now, ie, goto buffer and point
  in same window
- if window is different, still there and showing the buffer of the
  stack location, switch to that window and go to location's point
- if the window no longer exists, or shows a different buffer now, use
  the current window to switch to the buffer and goto location's point

That would, I think, be quite close to going back "in time" to the
window/buffer setup that the user had before, whenever that is possible.
If that is deemed a reasonable goal, the last clause seems the only one
where the behavior could differ.  Maybe one could reuse another window
showing the target buffer, or even choose one of two or three actions,
customizable by an option.

Re: xref window switching problems

[Dmitry Gutov]

On 08/05/2015 11:30 PM, Ingo Lohmar wrote:

> Maybe I was not expressing my thoughts clearly.  Reusing the second
> window is what the functions did, and that's just fine --- only the
> current window when calling the command should be left alone (and now,
> it is).

Ah, ok.

> Except for factoring out xref--goto-char, your patch looks just like my
> private testing.

Good.

>>> Maybe xref-pop-marker-stack could benefit from a discussion as well.
>>
>> Maybe it could benefit from a patch proposal.
>
> That sounds a bit snarky to me...

Sorry if it came across wrong. I only meant that a technical proposal 
would be the best way to start that discussion.

> across wrong.  Given the preceding sentences (which you did not quote),
> I thought it would have been clear this is merely a related
> afterthought, and not meant to disparage your work on xref in any way.
> In fact, I am happy about that effort to unify and standardize commands.

Thank you.

> Disclaimer: Recent discussion seems to have centered around the question
> if the newly opened file buffers should be cleaned up somehow.  I am not
> worried nor bothered by having all these buffers opened.  It's always
> been that way in Emacs, and it's never been a problem for me.

It's nice, but there will be scenarios when it's convenient for an xref 
implementation to open lots of buffers. Depending on you input to 
xref-find-regexp, likely too many.

> I care more (but it's not all that important to me) about the buffers
> and points *shown* in windows.  In that respect, what if
> xref--marker-ring stored, together with the location where
> xref-push-marker-stack is called, the window where it is called.

That's possible. Although then the callers will have to be more careful 
and not call it in some intermediate state.

> Then
> xref-pop-marker-stack could look at the top of the stack and:
> - if window is same as current: same as now, ie, goto buffer and point
>    in same window
> - if window is different, still there and showing the buffer of the
>    stack location, switch to that window and go to location's point

Then you'll still have the newly opened window (or this window might've 
been showing a different buffer before). Still, this could be the best 
we can reasonably do.

> - if the window no longer exists, or shows a different buffer now, use
>    the current window to switch to the buffer and goto location's point

I guess so.

Re: xref window switching problems

[Stephen Leake]

Ingo Lohmar <i.lohmar@gmail.com> writes:

> Disclaimer: Recent discussion seems to have centered around the question
> if the newly opened file buffers should be cleaned up somehow.  I am not
> worried nor bothered by having all these buffers opened.  It's always
> been that way in Emacs, and it's never been a problem for me.
>
> I care more (but it's not all that important to me) about the buffers
> and points *shown* in windows.  

+1

> In that respect, what if
> xref--marker-ring stored, together with the location where
> xref-push-marker-stack is called, the window where it is called.  Then
> xref-pop-marker-stack could look at the top of the stack and:
> - if window is same as current: same as now, ie, goto buffer and point
>   in same window
> - if window is different, still there and showing the buffer of the
>   stack location, switch to that window and go to location's point
> - if the window no longer exists, or shows a different buffer now, use
>   the current window to switch to the buffer and goto location's point

That's nice; I would like that.

> That would, I think, be quite close to going back "in time" to the
> window/buffer setup that the user had before, whenever that is possible.
> If that is deemed a reasonable goal, 

I don't think it is.

xref-pop-marker-stack should cause the previous marker location to be
displayed; that's _not_ the same as "also cause everything else that was
displayed at that point in time to be displayed".

Currently, it always does so in the current window; your proposal
extends that to consider the other currently visible windows.

> the last clause seems the only one where the behavior could differ.
> Maybe one could reuse another window showing the target buffer, 

Yes, I think that is best. No need to store the window; just look in the
other currently visible windows; if the location is visible (using
pos-visible-in-window-p ?), go there. It doesn't matter if it's the same
window that location was in at some earlier time; it's visible now.

You could almost implement this with `display-buffer-use-some-frame',
(that I just added), with a frame-predicate that checks
pos-visible-in-window-p, but that's not precise enough. We'd have to
extend that with a window-predicate. Which I need to do for another use
case. Hmm, it might be better to add window-predicate to
display-buffer-use-some-window.


> or even choose one of two or three actions, customizable by an option.

Ah, the Emacs Prime Directive again :).

-- 
-- Stephe

Re: xref window switching problems

[Ingo Lohmar]

Stephen Leake <stephen_leake <at> stephe-leake.org> writes:

> > That would, I think, be quite close to going back "in time" to the
> > window/buffer setup that the user had before, whenever that is possible.
> > If that is deemed a reasonable goal, 
> 
> I don't think it is.
> 
> xref-pop-marker-stack should cause the previous marker location to be
> displayed; that's _not_ the same as "also cause everything else that was
> displayed at that point in time to be displayed".
> 
> Currently, it always does so in the current window; your proposal
> extends that to consider the other currently visible windows.

You're right, I overstretched the comparison.  But I also think the proposal
is a reasonable and not-too-complex behavior.

> 
> > the last clause seems the only one where the behavior could differ.
> > Maybe one could reuse another window showing the target buffer, 
> 
> Yes, I think that is best. No need to store the window; just look in the
> other currently visible windows; if the location is visible (using
> pos-visible-in-window-p ?), go there. It doesn't matter if it's the same
> window that location was in at some earlier time; it's visible now.

Thinking a bit more about this, I agree: if the user has dumped/changed the
original window, another window showing the same buffer should be reused

> 
> You could almost implement this with `display-buffer-use-some-frame',
> (that I just added), with a frame-predicate that checks
> pos-visible-in-window-p, but that's not precise enough. We'd have to
> extend that with a window-predicate. Which I need to do for another use
> case. Hmm, it might be better to add window-predicate to
> display-buffer-use-some-window.
> 
> > or even choose one of two or three actions, customizable by an option.
> 
> Ah, the Emacs Prime Directive again :).
> 

Yeah, given some time, I think that is not really useful unless somebody
asks for it and presents a compelling case.  Anyway, it would be just a
varibale holding a function, defaulting to #'pop-to-buffer.