bug#33870: 27.0.50; xref-goto-xref not configurable

classic Classic list List threaded Threaded
157 messages Options
1234 ... 8
Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

Juri Linkov-2
X-Debbugs-CC: Dmitry Gutov <[hidden email]>

There is no more need to replace switch-to-buffer with
pop-to-buffer-same-window in xref--pop-to-location
like was asked in https://debbugs.gnu.org/32790#206
because now a new option switch-to-buffer-obey-display-actions
can be customized to t.

But still there is one xref command, namely `xref-goto-xref' bound to
RET in the *xref* buffer that always displays the buffer in the
predefined window, and there is no way to change this behavior.

Is it possible to change it to use either pop-to-buffer-same-window
or at least switch-to-buffer?



Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

Dmitry Gutov
Hi Juri,

On 25.12.2018 22:42, Juri Linkov wrote:
> X-Debbugs-CC: Dmitry Gutov <[hidden email]>
>
> There is no more need to replace switch-to-buffer with
> pop-to-buffer-same-window in xref--pop-to-location
> like was asked in https://debbugs.gnu.org/32790#206
> because now a new option switch-to-buffer-obey-display-actions
> can be customized to t.

Sorry I never responded, see the message in that other thread.

> But still there is one xref command, namely `xref-goto-xref' bound to
> RET in the *xref* buffer that always displays the buffer in the
> predefined window, and there is no way to change this behavior.
>
> Is it possible to change it to use either pop-to-buffer-same-window
> or at least switch-to-buffer?

IIUC you want to change xref--show-pos-in-buf to use
pop-to-buffer-same-window or switch-to-buffer instead of display-buffer.

Would you like to propose a patch that would still honor the 'action'
argument (or the values of xref--original-window* directly)?

Also pinging João who wrote that code.



Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

João Távora
Hi Juri and Dmitry

Any simplification to the implementation that keeps the
"keep original window intent" behavior across xref
intermediate buffers is very welcome.

Juri, do you understand what that particular sub-feature
is about? I wish I had written some tests to go with it
but testing window code is tricky. I will still try, that way
you needn't worry about understanding it and can program
"against the rails".

However here's a tangent that might affect the decision.
Is there any impediment to making xref.el a core ELPA
package? I can see some advantages... The reason I bring
this up here is that using very new elisp in a file can reduce
the usefulness of that goal, which in this case would be
to bring new xref features to users of Emacs 26.1/26.2. 
Perhaps it is already using post-26 features in which case
the ship has sailed.  In that case, disregard this tangent.

João



On Wed, Dec 26, 2018, 02:10 Dmitry Gutov <[hidden email] wrote:
Hi Juri,

On 25.12.2018 22:42, Juri Linkov wrote:
> X-Debbugs-CC: Dmitry Gutov <[hidden email]>
>
> There is no more need to replace switch-to-buffer with
> pop-to-buffer-same-window in xref--pop-to-location
> like was asked in https://debbugs.gnu.org/32790#206
> because now a new option switch-to-buffer-obey-display-actions
> can be customized to t.

Sorry I never responded, see the message in that other thread.

> But still there is one xref command, namely `xref-goto-xref' bound to
> RET in the *xref* buffer that always displays the buffer in the
> predefined window, and there is no way to change this behavior.
>
> Is it possible to change it to use either pop-to-buffer-same-window
> or at least switch-to-buffer?

IIUC you want to change xref--show-pos-in-buf to use
pop-to-buffer-same-window or switch-to-buffer instead of display-buffer.

Would you like to propose a patch that would still honor the 'action'
argument (or the values of xref--original-window* directly)?

Also pinging João who wrote that code.
Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

Eli Zaretskii
In reply to this post by Juri Linkov-2
> From: Juri Linkov <[hidden email]>
> Date: Tue, 25 Dec 2018 22:42:28 +0200
> Cc: dmitry gutov <[hidden email]>
>
> But still there is one xref command, namely `xref-goto-xref' bound to
> RET in the *xref* buffer that always displays the buffer in the
> predefined window, and there is no way to change this behavior.

How would you like to change this behavior, and why?  There are quite
a few tricky use cases, which took us some of time to get right, and I
wouldn't like to break them.



Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

Juri Linkov-2
>> But still there is one xref command, namely `xref-goto-xref' bound to
>> RET in the *xref* buffer that always displays the buffer in the
>> predefined window, and there is no way to change this behavior.
>
> How would you like to change this behavior, and why?  There are quite
> a few tricky use cases, which took us some of time to get right, and I
> wouldn't like to break them.

Since we have a convention that code displaying a buffer in the
window have to obey display actions that are customizable by
`display-buffer-alist', `display-buffer-overriding-action',
and other display related variables (e.g. like implemented
recently with switch-to-buffer-obey-display-actions),
I see no reason why *xref* should differ from *grep* or *Occur*.
It should not involve more complexity than necessary.



Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

Juri Linkov-2
In reply to this post by João Távora
> Any simplification to the implementation that keeps the
> "keep original window intent" behavior across xref
> intermediate buffers is very welcome.
>
> Juri, do you understand what that particular sub-feature
> is about?

I don't understand what does this "keep original window intent" mean.
Please explain.  I want to understand how it is different from other
modes that display a buffer in another window, such as e.g.
after `C-x C-b' (list-buffers) typing `C-o' displays the buffer
in another window, or e.g. typing `C-o' in the Dired buffer
opens it in another window, and many other cases.

> However here's a tangent that might affect the decision.
> Is there any impediment to making xref.el a core ELPA
> package? I can see some advantages... The reason I bring
> this up here is that using very new elisp in a file can reduce
> the usefulness of that goal, which in this case would be
> to bring new xref features to users of Emacs 26.1/26.2.
> Perhaps it is already using post-26 features in which case
> the ship has sailed.  In that case, disregard this tangent.

Display actions have been in Emacs for a long time customizable
by `display-buffer-alist', so if you need some specific logic,
it's easy to implement the corresponding display action
that can be overridden by the users.



Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

João Távora
Juri Linkov <[hidden email]> writes:

> I don't understand what does this "keep original window intent" mean.
> Please explain.

Hi Juri.

You can read up the whole bug here:

    https://debbugs.gnu.org/cgi/bugreport.cgi?bug=28814

I quote from that thread:

    Here are two very simple Emacs -Q recipes that demonstrate [the bug]
     
       emacs -Q
       C-x 3 [split-window-right]
       C-x 2 [split-window-below]
       M-. xref-backend-definitions RET [xref-find-definitions]
       C-n [next-line]
       RET [xref-goto-xref]
     
    Expected the definition to be found in the original window where I
    pressed M-. but instead it was found in another. Another case:
     
       emacs -Q
       C-x 4 . xref-backend-definitions RET [xref-find-definitions-other-window]
       C-n
       RET
     
    Expected the definition to be found in some other window, different
    from the one I pressed M-. on. Instead went to the same one. Also,
    in both situations, expected the window configuration to be the same
    as if I had searched for, say, xref-backend-functions [which only
    has a single definition].

The bugfix makes these two recipes work like expected (that is the
"Expected the definition" sentence is now fulfilled).  We want to make
sure this is not broken.

> I want to understand how it is different from other modes that display
> a buffer in another window, such as e.g.  after `C-x C-b'
> (list-buffers) typing `C-o' displays the buffer in another window, or
> e.g. typing `C-o' in the Dired buffer opens it in another window, and
> many other cases.

*Before* pressing C-x C-b, you probably had no expectations as to what
window should be used to display your decision.  But M-. you have:
M-. means "use the same window", C-x 4 . means use "other window" and
C-x 5 . means "other frame".  Crucially, the existance or not of
multiple loci of the (something that generally cannot be known in
advange) shouldn't influence the results of "other window" or "other
frame" intention.

So, by default, if display-* variables not are set specially by the
user, then final product of those two recipes should stay the same and,
more importantly, the principle that I described in the previous
paragraph should hold.

>> However here's a tangent that might affect the decision.
>> Is there any impediment to making xref.el a core ELPA
>> package? I can see some advantages... The reason I bring
>> this up here is that using very new elisp in a file can reduce
>> the usefulness of that goal, which in this case would be
>> to bring new xref features to users of Emacs 26.1/26.2.
>> Perhaps it is already using post-26 features in which case
>> the ship has sailed.  In that case, disregard this tangent.
>
> Display actions have been in Emacs for a long time customizable
> by `display-buffer-alist', so if you need some specific logic,
> it's easy to implement the corresponding display action
> that can be overridden by the users.

I know that.  I think either I or you missed something in this
paragraph.  Let me put it like this: are you planning to use, in
master's xref.el a new Elisp feature (a new function, argument to a
function, or a new semantics of a specific argument) that would make
loading that file in Emacs 26.1 fail in some way or other?

If master's xref.el works in 26.1 before your changes, and not anymore
after your changes, then the possibility of putting xref.el in ELPA as a
"core" package is lost.  (But perhaps that possibility is not desirable
to begin with, so this is why I said this was a tangent).

João



Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

Dmitry Gutov
On 27.12.2018 2:05, João Távora wrote:
> You can read up the whole bug here:
>
>      https://debbugs.gnu.org/cgi/bugreport.cgi?bug=28814
>
> I quote from that thread:
>
>      Here are two very simple Emacs -Q recipes that demonstrate [the bug]

Does this work well for everybody?

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index c71802c918..85d4325d9e 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -494,7 +494,8 @@ xref--show-pos-in-buf
              (or (and (window-live-p xref--original-window)
                       xref--original-window)
                  (selected-window))
-          (display-buffer buf action))
+          (pop-to-buffer buf action)
+          (selected-window))
        (xref--goto-char pos)
        (run-hooks 'xref-after-jump-hook)
        (let ((buf (current-buffer)))



Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

Eli Zaretskii
In reply to this post by Juri Linkov-2
> From: Juri Linkov <[hidden email]>
> Cc: [hidden email],  [hidden email]
> Date: Thu, 27 Dec 2018 01:17:00 +0200
>
> Since we have a convention that code displaying a buffer in the
> window have to obey display actions that are customizable by
> `display-buffer-alist', `display-buffer-overriding-action',
> and other display related variables (e.g. like implemented
> recently with switch-to-buffer-obey-display-actions),
> I see no reason why *xref* should differ from *grep* or *Occur*.
> It should not involve more complexity than necessary.

We are under no obligation to maintain 100% consistency between
different commands, just because some of their aspects are similar.
I'd like to hear of specific real-life situations where the current
code doesn't DTRT, before I'd agree to a change in this area.



Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

João Távora
In reply to this post by Dmitry Gutov
Dmitry Gutov <[hidden email]> writes:

> On 27.12.2018 2:05, João Távora wrote:
>> You can read up the whole bug here:
>>
>>      https://debbugs.gnu.org/cgi/bugreport.cgi?bug=28814
>>
>> I quote from that thread:
>>
>>      Here are two very simple Emacs -Q recipes that demonstrate [the bug]
>
> Does this work well for everybody?
>
> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
> index c71802c918..85d4325d9e 100644
> --- a/lisp/progmodes/xref.el
> +++ b/lisp/progmodes/xref.el
> @@ -494,7 +494,8 @@ xref--show-pos-in-buf
>              (or (and (window-live-p xref--original-window)
>                       xref--original-window)
>                  (selected-window))
> -          (display-buffer buf action))
> +          (pop-to-buffer buf action)
> +          (selected-window))
>        (xref--goto-char pos)
>        (run-hooks 'xref-after-jump-hook)
>        (let ((buf (current-buffer)))

It works for those two recipes, because `action' is passed directly to
display-buffer.  Don't know:

* if it might be introducing a new untested corner case because of the
  "recording" of select-window inside.  Perhaps this corner case is
  unwanted, but it might also be wanted (and we didn't know).

* if it will bring Yuri what he wants.  Doesn't seem like it will, since
  display buffer is given an explicit ACTION.

João



Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

Dmitry Gutov
In reply to this post by Eli Zaretskii
On 27.12.2018 17:27, Eli Zaretskii wrote:

> We are under no obligation to maintain 100% consistency between
> different commands, just because some of their aspects are similar.
> I'd like to hear of specific real-life situations where the current
> code doesn't DTRT, before I'd agree to a change in this area.

I think it *would* be handy if in the future we could make 'C-x 4' and
similar prefixes work via some common code, instead of requiring
separate key bindings and handling inside each command.

So if we manage support Juri's request without complicating the code too
much, we really should.



Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

Juri Linkov-2
In reply to this post by João Távora
>     Here are two very simple Emacs -Q recipes that demonstrate [the bug]

Thanks for the recipes.

>        emacs -Q
>        C-x 3 [split-window-right]
>        C-x 2 [split-window-below]
>        M-. xref-backend-definitions RET [xref-find-definitions]
>        C-n [next-line]
>        RET [xref-goto-xref]
>      
>     Expected the definition to be found in the original window where I
>     pressed M-. but instead it was found in another. Another case:

It could help to try using 'get-mru-window'.  Please ask Martin
if there is a display action that uses 'get-mru-window', or how
to temporarily change the default behavior from 'get-lru-window'
to 'get-mru-window'.

>        emacs -Q
>        C-x 4 . xref-backend-definitions RET [xref-find-definitions-other-window]
>        C-n
>        RET
>      
>     Expected the definition to be found in some other window, different
>     from the one I pressed M-. on. Instead went to the same one. Also,
>     in both situations, expected the window configuration to be the same
>     as if I had searched for, say, xref-backend-functions [which only
>     has a single definition].

This can be configured with the display buffer alist
`(inhibit-same-window . t)'.



Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

Juri Linkov-2
In reply to this post by Dmitry Gutov
> Does this work well for everybody?
>
> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
> index c71802c918..85d4325d9e 100644
> --- a/lisp/progmodes/xref.el
> +++ b/lisp/progmodes/xref.el
> @@ -494,7 +494,8 @@ xref--show-pos-in-buf
>              (or (and (window-live-p xref--original-window)
>                       xref--original-window)
>                  (selected-window))
> -          (display-buffer buf action))
> +          (pop-to-buffer buf action)
> +          (selected-window))
>        (xref--goto-char pos)
>        (run-hooks 'xref-after-jump-hook)
>        (let ((buf (current-buffer)))

Unfortunately, I see no difference.  It still selects the original window
before calling pop-to-buffer, so the display action is relative
to the original window, not to the currently selected window
as it would be natural to expect.



Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

João Távora
In reply to this post by Juri Linkov-2
Juri Linkov <[hidden email]> writes:

>>     Here are two very simple Emacs -Q recipes that demonstrate [the bug]
>
> Thanks for the recipes.
>
>>        emacs -Q
>>        C-x 3 [split-window-right]
>>        C-x 2 [split-window-below]
>>        M-. xref-backend-definitions RET [xref-find-definitions]
>>        C-n [next-line]
>>        RET [xref-goto-xref]
>>      
>>     Expected the definition to be found in the original window where I
>>     pressed M-. but instead it was found in another. Another case:
>
> It could help to try using 'get-mru-window'.  Please ask Martin
> if there is a display action that uses 'get-mru-window', or how
> to temporarily change the default behavior from 'get-lru-window'
> to 'get-mru-window'.

There may be a misunderstanding here.  Those recipes are for a bug that
has already been fixed: this code is now working like it should.

Are you saying that you could make the code use other functions to
produce the same behaviour, i.e. refactor it?  That's fine by me: feel
free to try, but I don't see a lot of motivation for it.

>
>>        emacs -Q
>>        C-x 4 . xref-backend-definitions RET [xref-find-definitions-other-window]
>>        C-n
>>        RET
>>      
>>     Expected the definition to be found in some other window, different
>>     from the one I pressed M-. on. Instead went to the same one. Also,
>>     in both situations, expected the window configuration to be the same
>>     as if I had searched for, say, xref-backend-functions [which only
>>     has a single definition].
>
> This can be configured with the display buffer alist
> `(inhibit-same-window . t)'.

Same here.  I'm not an expert in the `display-buffer-alist' DSL, but I
think you are again papering over the fact that between
xref-find-definitions-other-window and the final destination buffer
there is sometimes an *xref* buffer in its own window.  So I don't think
'inhibit-same-window' wouldn't help here.  But again, feel free to
rework the code to your standards and if it passes these two tests, it's
a good start.

João



Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

Dmitry Gutov
In reply to this post by Juri Linkov-2
On 27.12.2018 23:21, Juri Linkov wrote:
> Unfortunately, I see no difference.  It still selects the original window
> before calling pop-to-buffer, so the display action is relative
> to the original window, not to the currently selected window
> as it would be natural to expect.

That was the intention, making the xref window transient, in a way.

Is it really the thing you want to fix?



Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

Juri Linkov-2
>> Unfortunately, I see no difference.  It still selects the original window
>> before calling pop-to-buffer, so the display action is relative
>> to the original window, not to the currently selected window
>> as it would be natural to expect.
>
> That was the intention, making the xref window transient, in a way.
>
> Is it really the thing you want to fix?

Transient is a window like e.g. *Completions*.  I don't see any
indication that *xref* is transient in the same sense.  It looks
more like multi-occur *Occur*.



Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

Dmitry Gutov
On 28.12.2018 1:47, Juri Linkov wrote:

> Transient is a window like e.g. *Completions*.  I don't see any
> indication that *xref* is transient in the same sense.  It looks
> more like multi-occur *Occur*.
Well, it's the idea behind the feature which we merged last year, see
2a973edeacefcabb9fd8024188b7e167f0f9a9b6.

I think it makes a certain amount of sense, behavior-wise. Should we add
some visual cues?

I'm not a huge fan of how *Completions* behaves, though, so we're not
going to make it look exactly alike.



Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

João Távora
On Fri, Dec 28, 2018 at 12:36 AM Dmitry Gutov <[hidden email]> wrote:
On 28.12.2018 1:47, Juri Linkov wrote:

> Transient is a window like e.g. *Completions*.  I don't see any
> indication that *xref* is transient in the same sense.  It looks
> more like multi-occur *Occur*.
Well, it's the idea behind the feature which we merged last year, see
2a973edeacefcabb9fd8024188b7e167f0f9a9b6.

What's more, in a sense *xref* is closer to *Completions* than
to *Occur*, though it's not exactly like any of the two, because
*xref*, like *Completions*, only appears in certain situations.

João
Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

Juri Linkov-2
In reply to this post by João Távora
Hi João

> Any simplification to the implementation that keeps the
> "keep original window intent" behavior across xref
> intermediate buffers is very welcome.

Thanks for the explanation.  Now I understand better the intent in
xref--show-pos-in-buf.  Generally, I'd like to see the “keep original
window intent” behavior in more places, e.g. in *Occur*, *grep*, etc.
Based on your explanation, I've been able to write the patch that does
the following:

1. simplifies ‘xref--show-pos-in-buf’ while at the same time
   preserves the current behavior and respects user's customization
   of display actions;

2. makes the xref buffer non-obtrusive like *Completions*
   in xref--show-xref-buffer;

3. turns the existing arg QUIT of xref-goto-xref into a prefix arg,
   so a natural key sequence ‘C-u RET’ will quit the window.
   This is similar to the prefix arg of quit-window.


diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 87ce2299c5..a166f8299c 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -482,19 +482,9 @@ xref--show-pos-in-buf
                   (window-live-p xref--original-window)
                   (or (not (window-dedicated-p xref--original-window))
                       (eq (window-buffer xref--original-window) buf)))
-                 `(,(lambda (buf _alist)
-                      (set-window-buffer xref--original-window buf)
-                      xref--original-window))))))
-    (with-selected-window
-        (with-selected-window
-            ;; Just before `display-buffer', place ourselves in the
-            ;; original window to suggest preserving it. Of course, if
-            ;; user has deleted the original window, all bets are off,
-            ;; just use the selected one.
-            (or (and (window-live-p xref--original-window)
-                     xref--original-window)
-                (selected-window))
-          (display-buffer buf action))
+                 `(,(lambda (buf alist)
+                      (window--display-buffer buf xref--original-window 'reuse alist)))))))
+    (with-selected-window (display-buffer buf action)
       (xref--goto-char pos)
       (run-hooks 'xref-after-jump-hook)
       (let ((buf (current-buffer)))
@@ -548,9 +538,8 @@ xref--item-at-point
 
 (defun xref-goto-xref (&optional quit)
   "Jump to the xref on the current line and select its window.
-Non-interactively, non-nil QUIT means to first quit the *xref*
-buffer."
-  (interactive)
+A prefix arg QUIT means to first quit the *xref* buffer."
+  (interactive "P")
   (let* ((buffer (current-buffer))
          (xref (or (xref--item-at-point)
                    (user-error "No reference at point")))
@@ -782,7 +771,17 @@ xref--show-xref-buffer
         (erase-buffer)
         (xref--insert-xrefs xref-alist)
         (xref--xref-buffer-mode)
-        (pop-to-buffer (current-buffer))
+        (pop-to-buffer
+         (current-buffer)
+         `((display-buffer--maybe-same-window
+            display-buffer-reuse-window
+            display-buffer--maybe-pop-up-frame
+            display-buffer-below-selected)
+   ,(if temp-buffer-resize-mode
+ '(window-height . resize-temp-buffer-window)
+      '(window-height . fit-window-to-buffer))
+   ,(when temp-buffer-resize-mode
+      '(preserve-size . (nil . t)))))
         (goto-char (point-min))
         (setq xref--original-window (assoc-default 'window alist)
               xref--original-window-intent (assoc-default 'display-action alist))
Reply | Threaded
Open this post in threaded view
|

bug#33870: 27.0.50; xref-goto-xref not configurable

Eli Zaretskii
> From: Juri Linkov <[hidden email]>
> Date: Thu, 03 Jan 2019 02:18:50 +0200
> Cc: [hidden email], Dmitry Gutov <[hidden email]>
>
> 1. simplifies ‘xref--show-pos-in-buf’ while at the same time
>    preserves the current behavior and respects user's customization
>    of display actions;
>
> 2. makes the xref buffer non-obtrusive like *Completions*
>    in xref--show-xref-buffer;
>
> 3. turns the existing arg QUIT of xref-goto-xref into a prefix arg,
>    so a natural key sequence ‘C-u RET’ will quit the window.
>    This is similar to the prefix arg of quit-window.

Please be sure to document any user-visible behavior changes in NEWS
and in the manual.

Thanks.



1234 ... 8