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

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

[Juri Linkov]

X-Debbugs-CC: Dmitry Gutov <dgutov@yandex.ru>

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?

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 <dgutov@yandex.ru>
> 
> 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.

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

[João Távora]

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

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 <dgutov@yandex.ru wrote:

> Hi Juri,
>
> On 25.12.2018 22:42, Juri Linkov wrote:
> > X-Debbugs-CC: Dmitry Gutov <dgutov@yandex.ru>
> >
> > 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.
>

[-- Attachment #2: Type: text/html, Size: 3147 bytes --]

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

[Juri Linkov]

> 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.

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

[João Távora]

Juri Linkov <juri@linkov.net> 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

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)))

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

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> 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

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

[Juri Linkov]

> 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.

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

[Dmitry Gutov]

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?

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

[Juri Linkov]

>> 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*.

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.

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

[João Távora]

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

On Fri, Dec 28, 2018 at 12:36 AM Dmitry Gutov <dgutov@yandex.ru> 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

[-- Attachment #2: Type: text/html, Size: 1023 bytes --]

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

[Juri Linkov]

>     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)'.

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

[João Távora]

Juri Linkov <juri@linkov.net> 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

making xref.el a core ELPA package

[Dmitry Gutov]

On 26.12.2018 16:48, João Távora wrote:
 > However here's a tangent that might affect the decision.
 > Is there any impediment to making xref.el a core ELPA
 > package?

It *might* be as easy as adding the Version and Package-Requires tags 
and then adding a :core entry to elpa/externals-list.

Especially if you volunteer to test it in the previous release(es). We 
definitely need cl-generic, though. So anything older than 26.1 is out.

 > I can see some advantages...

Do you have anything specific in mind? Aside from simply getting new 
features to the users faster.

Re: making xref.el a core ELPA package

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 26.12.2018 16:48, João Távora wrote:
>> However here's a tangent that might affect the decision.
>> Is there any impediment to making xref.el a core ELPA
>> package?
>
> It *might* be as easy as adding the Version and Package-Requires tags
> and then adding a :core entry to elpa/externals-list.
>
> Especially if you volunteer to test it in the previous release(es). We
> definitely need cl-generic, though. So anything older than 26.1 is
> out.

Unfortunately, I just tested with 26.1 and no go.  It doesn't have
next-error-found :-(  This is the commit:

    commit 0c9e3df3c2088b61feb4b4e00d24419459962273
    Author: Juri Linkov <juri@linkov.net>
    Date:   Tue Apr 17 22:27:48 2018 +0300
     
        Use next-error-found to set next-error-last-buffer.

Either:

* we revert this (I believe we shouldn't)
* we use some indirection that is a no-op in 26.1
* we give up the idea

>> I can see some advantages...
>
> Do you have anything specific in mind? Aside from simply getting new
> features to the users faster.

That would be it mostly :-)

xref is a very important dependency of the Eglot LSP client, which could
make it into core Emacs (and remain in ELPA as a :core package).  There
are some issues in Eglot that suggest new features could be useful:

* https://github.com/joaotavora/eglot/issues/77
* https://github.com/joaotavora/eglot/issues/147
* https://github.com/joaotavora/eglot/issues/76

Also, I would like to see a pluggable API to control the insertion of
xrefs into the *xref* buffer.  Not only for LSP but also for SLY (a fork
of SLIME where xref.el originated).

João

Re: making xref.el a core ELPA package

[Juri Linkov]

> Unfortunately, I just tested with 26.1 and no go.  It doesn't have
> next-error-found :-(  This is the commit:
>
>     commit 0c9e3df3c2088b61feb4b4e00d24419459962273
>     Author: Juri Linkov <juri@linkov.net>
>     Date:   Tue Apr 17 22:27:48 2018 +0300
>
>         Use next-error-found to set next-error-last-buffer.

It's a big surprise to me that xref is a standalone package.
I thought it's simply a better replacement of the old find-tag.

> Either:
>
> * we revert this (I believe we shouldn't)
> * we use some indirection that is a no-op in 26.1

Sure, you could add some conditionals to support older versions.

Re: making xref.el a core ELPA package

[João Távora]

Juri Linkov <juri@linkov.net> writes:

>> Unfortunately, I just tested with 26.1 and no go.  It doesn't have
>> next-error-found :-(  This is the commit:
>>
>>     commit 0c9e3df3c2088b61feb4b4e00d24419459962273
>>     Author: Juri Linkov <juri@linkov.net>
>>     Date:   Tue Apr 17 22:27:48 2018 +0300
>>
>>         Use next-error-found to set next-error-last-buffer.
>
> It's a big surprise to me that xref is a standalone package.

There is a misunderstanding here.  It's not, but I am proposing that it
become a semi-standlone pacakge.

> I thought it's simply a better replacement of the old find-tag.

It is.

>> Either:
>>
>> * we revert this (I believe we shouldn't)
>> * we use some indirection that is a no-op in 26.1
>
> Sure, you could add some conditionals to support older versions.

It's kinda lame to do this in a file that is in Emacs core, but if the
others accept and it's a one-time well-documented thing, fine by be.  A
proper indirection (hook/function variable) would be better, but not
really justified by this one use case.

João

Re: making xref.el a core ELPA package

[Stefan Monnier]

>> Sure, you could add some conditionals to support older versions.

Very much so.

> It's kinda lame to do this in a file that is in Emacs core,

We have lots of that in Emacs's own files, since many of Emacs's
packages are also distributed outside (e.g. cc-mode, Tramp, ...).
It's not lame,


        Stefan

Re: making xref.el a core ELPA package

[João Távora]

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

On Fri, Dec 28, 2018 at 2:35 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> >> Sure, you could add some conditionals to support older versions.
> > It's kinda lame to do this in a file that is in Emacs core,
> It's not lame,


Erm, you realize this is version control with in-code
conditionals... I'm fine with it, really, I don' have any better
alternatives, but let's call the bulls by the names, as they say
back home :-)

João

[-- Attachment #2: Type: text/html, Size: 802 bytes --]

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

[Juri Linkov]

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

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.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: xref.el.patch --]
[-- Type: text/x-diff, Size: 2612 bytes --]

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))

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Thu, 03 Jan 2019 02:18:50 +0200
> Cc: 33870@debbugs.gnu.org, Dmitry Gutov <dgutov@yandex.ru>
> 
> 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.

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

[João Távora]

Hi Juri,

Juri Linkov <juri@linkov.net> writes:

> 1. simplifies ‘xref--show-pos-in-buf’

... and considerably complexifies xref--show-xref-buffer (more on that
later)

> while at the same time preserves the current behavior and respects
> user's customization of display actions;

That's great!

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

After a brief look, I'm not sure I like the UI change.  "not sure" is
not an euphemism for "don't like", I'm ust not sold on the idea yet:

* Certainly you don't mean non-obtrusive, you mean "less obtrusive" and
  really it's "slightly less obtrusive".  It does use potentially less
  space and doesn't temporarily use one of your windows if you happen to
  have several.  I agree this is an good advantage.

* But by using less space it is also less useful.  You don't get to see,
  at a glance, a great deal of xrefs.  And xrefs are different from
  completions, they're closer to grep hits.  You wouldn't put *grep*
  hits in such a potentially tiny window, would you?

Then again, perhaps you would, and the whole point of this patch is to
make the UI configurable.  If so, I'd make the original UI the default,
or at least very very easy to bring back.

> 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.

No problem here I think.

> -          (display-buffer buf action))
> +                 `(,(lambda (buf alist)
> +                      (window--display-buffer buf xref--original-window 'reuse alist)))))))
>

Using internal "--" symbols from window.el is a temporary solution I
hope.

> -        (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)))))


Again, too many --, and seems like a lot of repetition from window.el.
Perhaps you want window.el to export a function that encapsulates
all/some of this cruft to pass as ACTION.  Naming that function would be
the hardest problem (best I could do is
display-buffer-use-completions-like-window).

Or maybe put that function in xref.el.  But as I said above, I think we
also need a function that brings back the current default.

João

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

[Juri Linkov]

>> 1. simplifies ‘xref--show-pos-in-buf’
>
> ... and considerably complexifies xref--show-xref-buffer (more on that
> later)
>
>> while at the same time preserves the current behavior and respects
>> user's customization of display actions;
>
> That's great!

I realized now that it can be simplified more by replacing

(with-selected-window (display-buffer buf action)

with just

(pop-to-buffer buf action)

but I'm not sure about this change because it could change the current behavior.

>> 2. makes the xref buffer non-obtrusive like *Completions*
>>    in xref--show-xref-buffer;
>
> After a brief look, I'm not sure I like the UI change.  "not sure" is
> not an euphemism for "don't like", I'm ust not sold on the idea yet:
>
> * Certainly you don't mean non-obtrusive, you mean "less obtrusive" and
>   really it's "slightly less obtrusive".  It does use potentially less
>   space and doesn't temporarily use one of your windows if you happen to
>   have several.  I agree this is an good advantage.
>
> * But by using less space it is also less useful.  You don't get to see,
>   at a glance, a great deal of xrefs.  And xrefs are different from
>   completions, they're closer to grep hits.  You wouldn't put *grep*
>   hits in such a potentially tiny window, would you?
>
> Then again, perhaps you would, and the whole point of this patch is to
> make the UI configurable.  If so, I'd make the original UI the default,
> or at least very very easy to bring back.

I see what you mean.  For a command like project-find-regexp I'd like
the original UI as well, because most of the time there are many hits
displayed in the xref window.  But when xref-find-definitions pops up
the xref window, usually it contains just 2 lines taking half of the screen
where most space is uselessly empty.

So it seems that project-find-regexp and most other xref-related
commands are more like grep while xref-find-definitions is more like
completions with a small number of lines.

What do you think about allowing only xref-find-definitions
to display a narrow xref window below the original window?

>> -          (display-buffer buf action))
>> +                 `(,(lambda (buf alist)
>> +                      (window--display-buffer buf xref--original-window 'reuse alist)))))))
>>
>
> Using internal "--" symbols from window.el is a temporary solution I
> hope.

Actually this function is not quite internal.  It's intended to be used
in display actions implemented by packages.

>> -        (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)))))
>
>
> Again, too many --, and seems like a lot of repetition from window.el.

The distinction between internal and public window functions is quite fuzzy.

> Perhaps you want window.el to export a function that encapsulates
> all/some of this cruft to pass as ACTION.

Yes, creating a composite display action would be a good thing to do.

> Naming that function would be the hardest problem (best I could do is
> display-buffer-use-completions-like-window).

Or when naming by not its usage but what it does: display-buffer-below-and-resize.

> Or maybe put that function in xref.el.  But as I said above, I think we
> also need a function that brings back the current default.

I propose to use the new function only for xref-find-definitions.

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

[João Távora]

Juri Linkov <juri@linkov.net> writes:

> (with-selected-window (display-buffer buf action)
>   with just
> (pop-to-buffer buf action)
>
> but I'm not sure about this change because it could change the current
> behavior.

Didn't test, but that would be bad.  Is this small reduction worth it?

>>> 2. makes the xref buffer non-obtrusive like *Completions*
>>>    in xref--show-xref-buffer;
> What do you think about allowing only xref-find-definitions
> to display a narrow xref window below the original window?

I don't know.  Can I get back the original behaviour easily?  If so,
how?

I ask because the assumption that xref-find-definitions produces a small
number of lines is really quite brittle.  Generic functions can have
many, many methods.  In Emacs, cl-print-object has 10 definitions lines,
but that could/should easily grow as anyone who devises a new type of
object can write a cl-print-object for it.  In a Common Lisp system
CL:PRINT-OBJECT usually has a ton of methods (and I'm trying to write a
CL IDE that uses xref.el)

>>> -          (display-buffer buf action))
>>> +                 `(,(lambda (buf alist)
>>> +                      (window--display-buffer buf xref--original-window 'reuse alist)))))))
>>>
>>
>> Using internal "--" symbols from window.el is a temporary solution I
>> hope.
>
> Actually this function is not quite internal.  It's intended to be used
> in display actions implemented by packages.

Hmmm, it's used only in lisp/window.el, where it hails from, and in
lisp/windmove.el, where you added it recently.

If it's part of the API, it should really be named
window-display-buffer.  I'm just making sure it isn't an implementation
detail for which Martin reserve the to change at any time.

>> Again, too many --, and seems like a lot of repetition from window.el.
> The distinction between internal and public window functions is quite
> fuzzy.

It shouldn't be.  If a package A uses -- from package B, either A is
going to break soon, or B's API is insufficient.

>> Perhaps you want window.el to export a function that encapsulates
>> all/some of this cruft to pass as ACTION.
> Yes, creating a composite display action would be a good thing to do.

And can you create one such composite display action that brings exactly
the current *xref* behaviour?  Or does one such thing already exist?

>> Naming that function would be the hardest problem (best I could do is
>> display-buffer-use-completions-like-window).
> Or when naming by not its usage but what it does:
> display-buffer-below-and-resize.

OK.  Better, I guess (if that's really what it does).

>> Or maybe put that function in xref.el.  But as I said above, I think we
>> also need a function that brings back the current default.
> I propose to use the new function only for xref-find-definitions.

OK, but I would say this is a separate request:

* This bug is about making the xref.el window-popping behaviour
  configurable using display-buffer-alist&friends while keeping the UI.
  That goal is now apparently within reach;

* The goal of changing the default UI for a certain part of xref-*
  commands is a different one, which I don't necessarily oppose, but it
  should be discussed and implemented separately.

João

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

[Juri Linkov]

>>>> 2. makes the xref buffer non-obtrusive like *Completions*
>>>>    in xref--show-xref-buffer;
>> What do you think about allowing only xref-find-definitions
>> to display a narrow xref window below the original window?
>
> I don't know.  Can I get back the original behaviour easily?  If so,
> how?

This should be easy using a new display action.

> I ask because the assumption that xref-find-definitions produces a small
> number of lines is really quite brittle.  Generic functions can have
> many, many methods.  In Emacs, cl-print-object has 10 definitions lines,
> but that could/should easily grow as anyone who devises a new type of
> object can write a cl-print-object for it.  In a Common Lisp system
> CL:PRINT-OBJECT usually has a ton of methods (and I'm trying to write a
> CL IDE that uses xref.el)

In my experience 10 lines is an exception.  Even with more lines the
completions-like xref window remain pretty usable.

> If it's part of the API, it should really be named
> window-display-buffer.  I'm just making sure it isn't an implementation
> detail for which Martin reserve the to change at any time.

I agree, window--display-buffer is more public towards other packages
and could be renamed.

>>> Perhaps you want window.el to export a function that encapsulates
>>> all/some of this cruft to pass as ACTION.
>> Yes, creating a composite display action would be a good thing to do.
>
> And can you create one such composite display action that brings exactly
> the current *xref* behaviour?  Or does one such thing already exist?

It's as easy as moving components of the particular display action
from the recent patch into a separate function.

>>> Or maybe put that function in xref.el.  But as I said above, I think we
>>> also need a function that brings back the current default.
>> I propose to use the new function only for xref-find-definitions.
>
> OK, but I would say this is a separate request:
>
> * This bug is about making the xref.el window-popping behaviour
>   configurable using display-buffer-alist&friends while keeping the UI.
>   That goal is now apparently within reach;
>
> * The goal of changing the default UI for a certain part of xref-*
>   commands is a different one, which I don't necessarily oppose, but it
>   should be discussed and implemented separately.

Actually the request was about making xref windows more configurable.
I could rename the subject if necessary, or create a separate request
for more discussion.

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

[Dmitry Gutov]

On 04.01.2019 3:07, Juri Linkov wrote:
> It's as easy as moving components of the particular display action
> from the recent patch into a separate function.

Which would make xref.el depend on Emacs 27.1. Just making sure 
everybody remembers that.

> Actually the request was about making xref windows more configurable.

Via display-buffer-alist, right?

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

[João Távora]

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

On Fri, Jan 4, 2019, 00:16 Juri Linkov <juri@linkov.net wrote:

> I don't know.  Can I get back the original behaviour easily?  If so,
> > how?
> This should be easy using a new display action.
>


Ok. Make sure to include that in your next patch, and make it the default.

>
> > I ask because the assumption that xref-find-definitions produces a small
> > number of lines is really quite brittle.  Generic functions can have
> > many, many methods.  In Emacs, cl-print-object has 10 definitions lines,
> > but that could/should easily grow as anyone who devises a new type of
> > object can write a cl-print-object for it.  In a Common Lisp system
> > CL:PRINT-OBJECT usually has a ton of methods (and I'm trying to write a
> > CL IDE that uses xref.el)
>
> In my experience 10 lines is an exception.  Even with more lines the
> completions-like xref window remain pretty usable.


I'm trying to tell your that your experience which seems limited to xref in
emacs lisp, is not a good measure of how xref-find-definitions is used in
the wild. xref-find-definitions came from something called
slime-find-definitions, and has mostly its UI. SLIME is a CL IDE  where
100+ long complicated definitions are the norm, not the exception.  And one
day SLIME could decide to use xref.el.

> If it's part of the API, it should really be named
> > window-display-buffer.  I'm just making sure it isn't an implementation
> > detail for which Martin reserve the to change at any time.
>
> I agree, window--display-buffer is more public towards other packages
> and could be renamed.
>

Good. Include this in your next patch.

>>> Perhaps you want
> > And can you create one such composite display action that brings exactly
> > the current *xref* behaviour?  Or does one such thing already exist?
>
> It's as easy as moving components of the particular display action
> from the recent patch into a separate function.
>

OK.

Actually the request was about making xref windows more configurable.
> I could rename the subject if necessary, or create a separate request
> for more discussion.
>

Don't change subject, create a separate request. Submit a second patch that
makes xref windows configurable and leaves the default UI unchanged. Then
install that patch closing this bug. In the separate discussion we can
continue the discussion of the UI changes you want.

Actually I should have made this separation clearly earlier.

João

>

[-- Attachment #2: Type: text/html, Size: 4150 bytes --]

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Thu, 03 Jan 2019 23:29:18 +0200
> Cc: 33870@debbugs.gnu.org, Dmitry Gutov <dgutov@yandex.ru>
> 
> when xref-find-definitions pops up the xref window, usually it
> contains just 2 lines taking half of the screen where most space is
> uselessly empty.

We have fit-window-to-buffer for these situations.

> The distinction between internal and public window functions is quite fuzzy.

To my mind, internal functions shouldn't be used outside of the file
that defines them.

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

[Juri Linkov]

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

>> when xref-find-definitions pops up the xref window, usually it
>> contains just 2 lines taking half of the screen where most space is
>> uselessly empty.
>
> We have fit-window-to-buffer for these situations.
>
>> The distinction between internal and public window functions is quite fuzzy.
>
> To my mind, internal functions shouldn't be used outside of the file
> that defines them.

This patch addresses all these concerns.

display-buffer--maybe-at-bottom can be renamed to
display-buffer-maybe-at-bottom without a deprecation alias
because it was added in Emacs 27.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: display-bufffer.patch --]
[-- Type: text/x-diff, Size: 4804 bytes --]

diff --git a/lisp/window.el b/lisp/window.el
index 37d82c060c..015933839d 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -7457,6 +7457,21 @@ display-buffer-in-child-frame
       (unless (cdr (assq 'inhibit-switch-frame alist))
 	(window--maybe-raise-frame frame)))))
 
+(defun display-buffer-maybe-below-selected (buffer alist)
+  ;; This is a copy of `display-buffer-fallback-action'
+  ;; where `display-buffer-use-some-window' is replaced
+  ;; with `display-buffer-below-selected'.
+  (let ((alist (append alist `(,(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)))))))
+    (or (display-buffer--maybe-same-window buffer alist)
+        (display-buffer-reuse-window buffer alist)
+        (display-buffer--maybe-pop-up-frame buffer alist)
+        (display-buffer-in-previous-window buffer alist)
+        (display-buffer-below-selected buffer alist))))
+
 (defun display-buffer-below-selected (buffer alist)
   "Try displaying BUFFER in a window below the selected window.
 If there is a window below the selected one and that window
@@ -7503,7 +7518,10 @@ display-buffer-below-selected
 	     (window--display-buffer
               buffer window 'reuse alist display-buffer-mark-dedicated)))))
 
-(defun display-buffer--maybe-at-bottom (buffer alist)
+(defun display-buffer-maybe-at-bottom (buffer alist)
+  ;; This is a copy of `display-buffer-fallback-action'
+  ;; where `display-buffer-use-some-window' is replaced
+  ;; with `display-buffer-at-bottom'.
   (let ((alist (append alist `(,(if temp-buffer-resize-mode
 		                    '(window-height . resize-temp-buffer-window)
 	                          '(window-height . fit-window-to-buffer))
@@ -7512,6 +7530,7 @@ display-buffer--maybe-at-bottom
     (or (display-buffer--maybe-same-window buffer alist)
         (display-buffer-reuse-window buffer alist)
         (display-buffer--maybe-pop-up-frame buffer alist)
+        (display-buffer-in-previous-window buffer alist)
         (display-buffer-at-bottom buffer alist))))
 
 (defun display-buffer-at-bottom (buffer alist)
diff --git a/lisp/files.el b/lisp/files.el
index 6ccb001e35..0741dbc19e 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -3396,7 +3396,7 @@ hack-local-variables-confirm
 
       ;; Display the buffer and read a choice.
       (save-window-excursion
-	(pop-to-buffer buf '(display-buffer--maybe-at-bottom))
+	(pop-to-buffer buf '(display-buffer-maybe-at-bottom))
 	(let* ((exit-chars '(?y ?n ?\s ?\C-g ?\C-v))
 	       (prompt (format "Please type %s%s: "
 			       (if offer-save "y, n, or !" "y or n")
@@ -7053,7 +7053,9 @@ save-buffers-kill-emacs
            (or (not active)
                (with-displayed-buffer-window
                 (get-buffer-create "*Process List*")
-                '(display-buffer--maybe-at-bottom)
+                '(display-buffer-maybe-at-bottom
+                  (window-height . fit-window-to-buffer)
+                  (preserve-size nil . t))
                 #'(lambda (window _value)
                     (with-selected-window window
                       (unwind-protect
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 5760a2e49d..7dede4e616 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1828,18 +1828,12 @@ minibuffer-completion-help
              (display-buffer-mark-dedicated 'soft))
         (with-displayed-buffer-window
           "*Completions*"
-          ;; This is a copy of `display-buffer-fallback-action'
-          ;; where `display-buffer-use-some-window' is replaced
-          ;; with `display-buffer-at-bottom'.
-          `((display-buffer--maybe-same-window
-             display-buffer-reuse-window
-             display-buffer--maybe-pop-up-frame
-             ;; Use `display-buffer-below-selected' for inline completions,
-             ;; but not in the minibuffer (e.g. in `eval-expression')
-             ;; for which `display-buffer-at-bottom' is used.
-             ,(if (eq (selected-window) (minibuffer-window))
-                  'display-buffer-at-bottom
-                'display-buffer-below-selected))
+          ;; Use `display-buffer-maybe-below-selected' for inline completions,
+          ;; but not in the minibuffer (e.g. in `eval-expression')
+          ;; for which `display-buffer-maybe-at-bottom' is used.
+          `((,(if (eq (selected-window) (minibuffer-window))
+                  'display-buffer-maybe-at-bottom
+                'display-buffer-maybe-below-selected))
 	    ,(if temp-buffer-resize-mode
 		 '(window-height . resize-temp-buffer-window)
 	       '(window-height . fit-window-to-buffer))

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

[martin rudalics]

 > display-buffer--maybe-at-bottom can be renamed to
 > display-buffer-maybe-at-bottom without a deprecation alias
 > because it was added in Emacs 27.

The 'display-buffer--maybe-' functions are macros in disguise invented
by Chong to simplify coding the rest.  Unless we can't avoid it, I
would not make them public because then we would have to (1) document
them, (2) explain the semantics of the "maybe" and (3) justify why the
remaining 'display-buffer--maybe-' functions are not public.

Also note that 'display-buffer' resizes a window iff that window is
new or always has shown the buffer to display before.

There's one thing about 'display-buffer-at-bottom' that stupefies me:
Here

	     (let (split-width-threshold)
	       (setq window (window--try-to-split-window bottom-window alist)))

we bind ‘split-width-threshold’ so we can split the bottom window into
two side by side windows.  I recently found a branch of mine where I
bind 'split-height-threshold' to nil instead and now cannot remember
what we really wanted - split that window horizontally or vertically.
Can you?  In either case feel free to change that to what you consider
the more appropriate binding - maybe even binding both.

Thanks, martin

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

[Drew Adams]

> The 'display-buffer--maybe-' functions are macros in disguise invented
> by Chong to simplify coding the rest.  Unless we can't avoid it, I
> would not make them public because then we would have to (1) document
> them, (2) explain the semantics of the "maybe" and (3) justify why the
> remaining 'display-buffer--maybe-' functions are not public.

_Everything_ in Emacs is (and should be) public.

Why would Emacs users not deserve all of #1, #2,
and #3?

Wouldn't you make that info available to Emacs
developers?  How are Emacs users different from
Emacs developers with regard to what info they
deserve to know about, including design and
implementation behavior and their reasons?

Many users might not be interested in digging
into such info, but why hide it?  Please consider
instead making such info clear and explicit for
everyone.

An absolute minimum in this regard is comments
in the code.

But Emacs has doc strings, and beyond design
and implementation information we should
document function behavior in doc strings.
It should make no difference how tentative or
temporary or "internal" we might currently
think some function is - its behavior deserves
to be documented.

"Unless we can't avoid it...".  We should not
avoid it or anything like it.  It should be a
moral imperative, as well as a question of
helpfulness and civility, for Emacs to document
itself to users.

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

[Juri Linkov]

>> The 'display-buffer--maybe-' functions are macros in disguise invented
>> by Chong to simplify coding the rest.  Unless we can't avoid it, I
>> would not make them public because then we would have to (1) document
>> them, (2) explain the semantics of the "maybe" and (3) justify why the
>> remaining 'display-buffer--maybe-' functions are not public.
>
> _Everything_ in Emacs is (and should be) public.

I agree, I don't see how display-buffer--maybe-at-bottom
is more internal and not public than e.g. set-window-buffer
and more low-level functions.

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

[Juri Linkov]

>> display-buffer--maybe-at-bottom can be renamed to
>> display-buffer-maybe-at-bottom without a deprecation alias
>> because it was added in Emacs 27.
>
> The 'display-buffer--maybe-' functions are macros in disguise invented
> by Chong to simplify coding the rest.  Unless we can't avoid it, I
> would not make them public because then we would have to (1) document
> them,

Yes, they are like macros, and this is what makes them useful for public use.

> (2) explain the semantics of the "maybe" and

The semantics is that they do what the default actions do
plus something specific.  Maybe then move the default part
from their body to some other fallback layer?  Then just use e.g.
display-buffer-at-bottom, without the -maybe part.
Or maybe use an alist for that, something like

  ((maybe-try . default-actions))

> (3) justify why the remaining 'display-buffer--maybe-' functions are
> not public.

I don't see any justification.

> Also note that 'display-buffer' resizes a window iff that window is
> new or always has shown the buffer to display before.
>
> There's one thing about 'display-buffer-at-bottom' that stupefies me:
> Here
>
> 	     (let (split-width-threshold)
> 	       (setq window (window--try-to-split-window bottom-window alist)))
>
> we bind ‘split-width-threshold’ so we can split the bottom window into
> two side by side windows.  I recently found a branch of mine where I
> bind 'split-height-threshold' to nil instead and now cannot remember
> what we really wanted - split that window horizontally or vertically.
> Can you?  In either case feel free to change that to what you consider
> the more appropriate binding - maybe even binding both.

It seems this code has no effect, it's never used.  Could you suggest
such window configuration to test that would call it?

There is another problem: in two small vertically split windows
'display-buffer-at-bottom' sometimes displays the buffer in the
upper window.

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

[martin rudalics]

 > The semantics is that they do what the default actions do
 > plus something specific.  Maybe then move the default part
 > from their body to some other fallback layer?  Then just use e.g.
 > display-buffer-at-bottom, without the -maybe part.
 > Or maybe use an alist for that, something like
 >
 >    ((maybe-try . default-actions))

I would never have written these "maybe" function in the first place.
They need more code lines then they spare.  So I have no real opinion.

 >> (3) justify why the remaining 'display-buffer--maybe-' functions are
 >> not public.
 >
 > I don't see any justification.

As we know all buffer display action functions are "maybe" functions
so people might ask why some of them include that word explicitly.

 >> Also note that 'display-buffer' resizes a window iff that window is
 >> new or always has shown the buffer to display before.
 >>
 >> There's one thing about 'display-buffer-at-bottom' that stupefies me:
 >> Here
 >>
 >> 	     (let (split-width-threshold)
 >> 	       (setq window (window--try-to-split-window bottom-window alist)))
 >>
 >> we bind ‘split-width-threshold’ so we can split the bottom window into
 >> two side by side windows.  I recently found a branch of mine where I
 >> bind 'split-height-threshold' to nil instead and now cannot remember
 >> what we really wanted - split that window horizontally or vertically.
 >> Can you?  In either case feel free to change that to what you consider
 >> the more appropriate binding - maybe even binding both.
 >
 > It seems this code has no effect, it's never used.  Could you suggest
 > such window configuration to test that would call it?

A single window frame where the buffer is not displayed runs this
part.

 > There is another problem: in two small vertically split windows
 > 'display-buffer-at-bottom' sometimes displays the buffer in the
 > upper window.

 From judging the code I'd say this is impossible.  But with Emacs
nothing is impossible.

martin

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

[Juri Linkov]

>> The semantics is that they do what the default actions do
>> plus something specific.  Maybe then move the default part
>> from their body to some other fallback layer?  Then just use e.g.
>> display-buffer-at-bottom, without the -maybe part.
>> Or maybe use an alist for that, something like
>>
>>    ((maybe-try . default-actions))
>
> I would never have written these "maybe" function in the first place.
> They need more code lines then they spare.  So I have no real opinion.

I understand that the word “maybe” indicates it's not guaranteed that
the function will do what it's intended to do.  IOW, these functions
are conditional.

Since we can't lightly rename old functions, I have a question only
about functions added in Emacs 27, namely, display-buffer--maybe-at-bottom.
Its current body:

  (let ((alist (append alist `(,(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)))))))
    (or (display-buffer--maybe-same-window buffer alist)
        (display-buffer-reuse-window buffer alist)
        (display-buffer--maybe-pop-up-frame buffer alist)
        (display-buffer-in-previous-window buffer alist)
        (display-buffer-at-bottom buffer alist)))

I propose to remove this function and replace its parts with
more alists, i.e. this blob

                             `(,(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))))

with something shorter like `(fit-to-buffer . t)'

And also to replace a long list of display-buffer-* that is a copy of
`display-buffer-fallback-action' with something shorter like an alist
`(pre-action . display-buffer-fallback-action).

>>> Also note that 'display-buffer' resizes a window iff that window is
>>> new or always has shown the buffer to display before.
>>>
>>> There's one thing about 'display-buffer-at-bottom' that stupefies me:
>>> Here
>>>
>>> 	     (let (split-width-threshold)
>>> 	       (setq window (window--try-to-split-window bottom-window alist)))
>>>
>>> we bind ‘split-width-threshold’ so we can split the bottom window into
>>> two side by side windows.  I recently found a branch of mine where I
>>> bind 'split-height-threshold' to nil instead and now cannot remember
>>> what we really wanted - split that window horizontally or vertically.
>>> Can you?  In either case feel free to change that to what you consider
>>> the more appropriate binding - maybe even binding both.
>>
>> It seems this code has no effect, it's never used.  Could you suggest
>> such window configuration to test that would call it?
>
> A single window frame where the buffer is not displayed runs this
> part.

You are lucky if you can invoke its second branch.  I always get only
its third branch in all tried configurations when testing with
completions of `C-x C-f TAB TAB'.

>> There is another problem: in two small vertically split windows
>> 'display-buffer-at-bottom' sometimes displays the buffer in the
>> upper window.
>
> From judging the code I'd say this is impossible.  But with Emacs
> nothing is impossible.

After resizing an initial frame to 12 lines, so every vertically split
window gets 6 lines, typing `C-x C-f TAB TAB' displays *Completions* in
the upper window, when a previous window where *Completions* was
previously displayed was moved to the upper window, e.g.

0. emacs -Q
1. resize the frame to 12 lines
2. C-x 2
3. C-x C-f TAB TAB C-g   ;; *Completions* were displayed in the bottom window
4. C-x 0
5. C-x 2
6. C-x C-f TAB TAB C-g  ;; *Completions* displayed in the upper window that was previous

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

[martin rudalics]

 > I understand that the word “maybe” indicates it's not guaranteed that
 > the function will do what it's intended to do.  IOW, these functions
 > are conditional.

Like 'display-buffer'.  But we don't call it 'display-buffer-maybe'.

 > Since we can't lightly rename old functions, I have a question only
 > about functions added in Emacs 27, namely, display-buffer--maybe-at-bottom.
 > Its current body:
 >
 >    (let ((alist (append alist `(,(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)))))))
 >      (or (display-buffer--maybe-same-window buffer alist)
 >          (display-buffer-reuse-window buffer alist)
 >          (display-buffer--maybe-pop-up-frame buffer alist)
 >          (display-buffer-in-previous-window buffer alist)
 >          (display-buffer-at-bottom buffer alist)))
 >
 > I propose to remove this function and replace its parts with
 > more alists, i.e. this blob
 >
 >                               `(,(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))))
 >
 > with something shorter like `(fit-to-buffer . t)'

Can't we add this via a special value for the 'window-height' alist
entry?  Where we explicitly state that it obeys
'temp-buffer-resize-mode' if that is active and the buffer qualifies
as temporary and so on ...  Or is that what you mean already?

 > And also to replace a long list of display-buffer-* that is a copy of
 > `display-buffer-fallback-action' with something shorter like an alist
 > `(pre-action . display-buffer-fallback-action).

I'm not sure I understand you.  'display-buffer-fallback-action' is
always tried after everything else failed.  Would you want to run it
_before_ something else?

 >> A single window frame where the buffer is not displayed runs this
 >> part.
 >
 > You are lucky if you can invoke its second branch.  I always get only
 > its third branch in all tried configurations when testing with
 > completions of `C-x C-f TAB TAB'.

I now always display completions in a child frame so I never run into
practical problems with it.

 > After resizing an initial frame to 12 lines, so every vertically split
 > window gets 6 lines, typing `C-x C-f TAB TAB' displays *Completions* in
 > the upper window, when a previous window where *Completions* was
 > previously displayed was moved to the upper window, e.g.
 >
 > 0. emacs -Q
 > 1. resize the frame to 12 lines
 > 2. C-x 2
 > 3. C-x C-f TAB TAB C-g   ;; *Completions* were displayed in the bottom window
 > 4. C-x 0
 > 5. C-x 2
 > 6. C-x C-f TAB TAB C-g  ;; *Completions* displayed in the upper window that was previous

Your bag of tricks is fathomless :-)  Basically, this means that
'display-buffer-in-previous-window' and 'display-buffer-at-bottom' are
inherently irreconcilable when a window was at bottom once and moved
upwards.  We could abuse the existing 'side' action alist entry for
not-atomic, non-side windows in the following sense: If 'side' equals
'bottom', a window is eligible for reuse if and only if it appears on
that side of the frame.  To be obeyed by 'display-buffer-reuse-window'
and 'display-buffer-in-previous-window', I presume.  WDYT?

martin

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

[Juri Linkov]

>> I propose to remove this function and replace its parts with
>> more alists, i.e. this blob
>>
>>                               `(,(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))))
>>
>> with something shorter like `(fit-to-buffer . t)'
>
> Can't we add this via a special value for the 'window-height' alist
> entry?  Where we explicitly state that it obeys
> 'temp-buffer-resize-mode' if that is active and the buffer qualifies
> as temporary and so on ...  Or is that what you mean already?

I meant to make it shorter in any possible way, so using something like
'(window-height . resize)' seems to achieve this goal.

>> And also to replace a long list of display-buffer-* that is a copy of
>> `display-buffer-fallback-action' with something shorter like an alist
>> `(pre-action . display-buffer-fallback-action).
>
> I'm not sure I understand you.  'display-buffer-fallback-action' is
> always tried after everything else failed.  Would you want to run it
> _before_ something else?

Exactly.  There is a long list of actions in display-buffer--maybe-at-bottom
before calling the main action 'display-buffer-at-bottom', so it makes sense
to move them somewhere to a common place.

>>> A single window frame where the buffer is not displayed runs this
>>> part.
>>
>> You are lucky if you can invoke its second branch.  I always get only
>> its third branch in all tried configurations when testing with
>> completions of `C-x C-f TAB TAB'.
>
> I now always display completions in a child frame so I never run into
> practical problems with it.

Then what problems are possible with binding 'split-width-threshold'
or 'split-height-threshold' to nil?

>> After resizing an initial frame to 12 lines, so every vertically split
>> window gets 6 lines, typing `C-x C-f TAB TAB' displays *Completions* in
>> the upper window, when a previous window where *Completions* was
>> previously displayed was moved to the upper window, e.g.
>>
>> 0. emacs -Q
>> 1. resize the frame to 12 lines
>> 2. C-x 2
>> 3. C-x C-f TAB TAB C-g   ;; *Completions* were displayed in the bottom window
>> 4. C-x 0
>> 5. C-x 2
>> 6. C-x C-f TAB TAB C-g  ;; *Completions* displayed in the upper window that was previous
>
> Your bag of tricks is fathomless :-)  Basically, this means that
> 'display-buffer-in-previous-window' and 'display-buffer-at-bottom' are
> inherently irreconcilable when a window was at bottom once and moved
> upwards.  We could abuse the existing 'side' action alist entry for
> not-atomic, non-side windows in the following sense: If 'side' equals
> 'bottom', a window is eligible for reuse if and only if it appears on
> that side of the frame.  To be obeyed by 'display-buffer-reuse-window'
> and 'display-buffer-in-previous-window', I presume.  WDYT?

This makes sense.  Even more, maybe it would be possible to use only
an alist '(side . bottom)' instead of specyfying the action
'display-buffer--maybe-at-bottom'?

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

[martin rudalics]

 >>> I propose to remove this function and replace its parts with
 >>> more alists, i.e. this blob
 >>>
 >>>                                `(,(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))))
 >>>
 >>> with something shorter like `(fit-to-buffer . t)'
 >>
 >> Can't we add this via a special value for the 'window-height' alist
 >> entry?  Where we explicitly state that it obeys
 >> 'temp-buffer-resize-mode' if that is active and the buffer qualifies
 >> as temporary and so on ...  Or is that what you mean already?
 >
 > I meant to make it shorter in any possible way, so using something like
 > '(window-height . resize)' seems to achieve this goal.

'resize' is too short IMHO.  'resize-to-fit' maybe.

 > Exactly.  There is a long list of actions in display-buffer--maybe-at-bottom
 > before calling the main action 'display-buffer-at-bottom', so it makes sense
 > to move them somewhere to a common place.

But running a "fallback" action before the others doesn't sound very
intuitive.

 >> I now always display completions in a child frame so I never run into
 >> practical problems with it.
 >
 > Then what problems are possible with binding 'split-width-threshold'
 > or 'split-height-threshold' to nil?

I can't tell because I'm not sure what we want here.  And if you say
that with your setup this part is never executed, things get even more
obscure.  So let's leave everything as it is until someone files a
"real" complaint.

 >> We could abuse the existing 'side' action alist entry for
 >> not-atomic, non-side windows in the following sense: If 'side' equals
 >> 'bottom', a window is eligible for reuse if and only if it appears on
 >> that side of the frame.  To be obeyed by 'display-buffer-reuse-window'
 >> and 'display-buffer-in-previous-window', I presume.  WDYT?
 >
 > This makes sense.  Even more, maybe it would be possible to use only
 > an alist '(side . bottom)' instead of specyfying the action
 > 'display-buffer--maybe-at-bottom'?

We could use the six abbreviations we have ('left', 'top', 'above',
'right', 'bottom' and 'below') to make a window on the respective side
either of the selected window or the frame.  Then we would need one
action function say 'display-buffer-beside' and yet another action
alist entry say 'beside' with the values 'selected' (on any side of
the selected window), 'main' (on any side of the main window) and a
window (on which side this would have to be created).

martin

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

[Juri Linkov]

>>>> I propose to remove this function and replace its parts with
>>>> more alists, i.e. this blob
>>>>
>>>>                                `(,(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))))
>>>>
>>>> with something shorter like `(fit-to-buffer . t)'
>>>
>>> Can't we add this via a special value for the 'window-height' alist
>>> entry?  Where we explicitly state that it obeys
>>> 'temp-buffer-resize-mode' if that is active and the buffer qualifies
>>> as temporary and so on ...  Or is that what you mean already?
>>
>> I meant to make it shorter in any possible way, so using something like
>> '(window-height . resize)' seems to achieve this goal.
>
> 'resize' is too short IMHO.  'resize-to-fit' maybe.

Good name.

>> Exactly.  There is a long list of actions in display-buffer--maybe-at-bottom
>> before calling the main action 'display-buffer-at-bottom', so it makes sense
>> to move them somewhere to a common place.
>
> But running a "fallback" action before the others doesn't sound very
> intuitive.

Maybe some more suitable name for actions to add between
display-buffer-overriding-action and user-action?

>>> We could abuse the existing 'side' action alist entry for
>>> not-atomic, non-side windows in the following sense: If 'side' equals
>>> 'bottom', a window is eligible for reuse if and only if it appears on
>>> that side of the frame.  To be obeyed by 'display-buffer-reuse-window'
>>> and 'display-buffer-in-previous-window', I presume.  WDYT?
>>
>> This makes sense.  Even more, maybe it would be possible to use only
>> an alist '(side . bottom)' instead of specyfying the action
>> 'display-buffer--maybe-at-bottom'?

Or '(direction . bottom) or shorter '(dir . bottom)
compatible with terminology of window-in-direction
because the word "side" is associated with side windows.

> We could use the six abbreviations we have ('left', 'top', 'above',
> 'right', 'bottom' and 'below') to make a window on the respective side
> either of the selected window or the frame.  Then we would need one
> action function say 'display-buffer-beside' and yet another action
> alist entry say 'beside' with the values 'selected' (on any side of
> the selected window), 'main' (on any side of the main window) and a
> window (on which side this would have to be created).

Maybe better 'frame' instead of 'main'?

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

[martin rudalics]

 > Maybe some more suitable name for actions to add between
 > display-buffer-overriding-action and user-action?

Nothing should ever come between 'display-buffer-overriding-action'
and 'user-action'.

 >>> This makes sense.  Even more, maybe it would be possible to use only
 >>> an alist '(side . bottom)' instead of specyfying the action
 >>> 'display-buffer--maybe-at-bottom'?
 >
 > Or '(direction . bottom) or shorter '(dir . bottom)
 > compatible with terminology of window-in-direction
 > because the word "side" is associated with side windows.

Fine with me.

 > Maybe better 'frame' instead of 'main'?

No.  'frame' includes side windows (the ones you cite above) and the
minibuffer window.  'main' (from 'window-main-window') or 'root' are
better (but still not perfect).

martin

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

[Juri Linkov]

>> Maybe some more suitable name for actions to add between
>> display-buffer-overriding-action and user-action?
>
> Nothing should ever come between 'display-buffer-overriding-action'
> and 'user-action'.

Sorry, rather I wanted to mention 'special-action' because it precedes 'action'
that comes from the display-buffer call (that then could specify just
display-buffer-at-bottom instead of display-buffer--maybe-at-bottom).

>>>> This makes sense.  Even more, maybe it would be possible to use only
>>>> an alist '(side . bottom)' instead of specyfying the action
>>>> 'display-buffer--maybe-at-bottom'?
>>
>> Or '(direction . bottom) or shorter '(dir . bottom)
>> compatible with terminology of window-in-direction
>> because the word "side" is associated with side windows.
>
> Fine with me.
>
>> Maybe better 'frame' instead of 'main'?
>
> No.  'frame' includes side windows (the ones you cite above) and the
> minibuffer window.  'main' (from 'window-main-window') or 'root' are
> better (but still not perfect).

Yes, still not perfect.  Maybe then use just one alist entry with more
possible values whose names clearly indicate where they are applied like

below - bottom
above - top
left - leftmost
right - rightmost

The first column is relative to the selected window,
the second column is relative to the main window.

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

[martin rudalics]

 > Yes, still not perfect.  Maybe then use just one alist entry with more
 > possible values whose names clearly indicate where they are applied like
 >
 > below - bottom
 > above - top
 > left - leftmost
 > right - rightmost
 >
 > The first column is relative to the selected window,
 > the second column is relative to the main window.

This will confuse us and users.  We should try to unify them somehow
in the sense that terms like below/bottom can be used interchangeably
- as you asked for up/above and down/below in 'window-in-direction'.

martin

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

[Juri Linkov]

>> Yes, still not perfect.  Maybe then use just one alist entry with more
>> possible values whose names clearly indicate where they are applied like
>>
>> below - bottom
>> above - top
>> left - leftmost
>> right - rightmost
>>
>> The first column is relative to the selected window,
>> the second column is relative to the main window.
>
> This will confuse us and users.  We should try to unify them somehow
> in the sense that terms like below/bottom can be used interchangeably
> - as you asked for up/above and down/below in 'window-in-direction'.

Actually we already have established naming convention:

display-buffer-below-selected
display-buffer-at-bottom

Removing the common prefix gives us the names of alist entries:

below-selected
at-bottom

Using the same naming convention suggests more names:

above-selected
at-top
...

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

[martin rudalics]

 > Actually we already have established naming convention:
 >
 > display-buffer-below-selected
 > display-buffer-at-bottom
 >
 > Removing the common prefix gives us the names of alist entries:
 >
 > below-selected
 > at-bottom
 >
 > Using the same naming convention suggests more names:
 >
 > above-selected
 > at-top
 > ...

I know.  But I have this idea of providing

- one function that catches all cases

- with one nomenclature for a reference window

- and one nomenclature for the direction wrt the reference window.

So while 'below-selected' fits into this nomenclature, 'at-top'
doesn't.

martin

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

[Juri Linkov]

> I know.  But I have this idea of providing

I agree this is a good idea.

> - one function that catches all cases

Like we already have such functions as window-in-direction and
windmove-display-in-direction, the new function could have a similar name
display-buffer-in-direction.

> - with one nomenclature for a reference window

For clarity the alist entry name could include the word "window":

(window . selected)
(window . main)
(window . <window_object>)

But for disambiguation maybe also add some prefix like

direction-window
from-window
etc.

> - and one nomenclature for the direction wrt the reference window.

Ok, like (direction . up) and all aliases.

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

[martin rudalics]

 > Like we already have such functions as window-in-direction and
 > windmove-display-in-direction, the new function could have a similar name
 > display-buffer-in-direction.

OK (unless we find something better).

 >> - with one nomenclature for a reference window
 >
 > For clarity the alist entry name could include the word "window":
 >
 > (window . selected)
 > (window . main)
 > (window . <window_object>)
 >
 > But for disambiguation maybe also add some prefix like
 >
 > direction-window
 > from-window

'from-window' is not bad.  Maybe also 'reference-window'.  We don't
use such a term in windmove.el.  There we just say that "WINDOW is the
window that movement is relative to".

 > Ok, like (direction . up) and all aliases.

'direction' should be OK then.

martin

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

[Juri Linkov]

>> Like we already have such functions as window-in-direction and
>> windmove-display-in-direction, the new function could have a similar name
>> display-buffer-in-direction.
>
> OK (unless we find something better).

Then two new functions will have consistent names:

display-buffer-in-direction
display-buffer-in-window

>>> - with one nomenclature for a reference window
>>
>> For clarity the alist entry name could include the word "window":
>>
>> (window . selected)
>> (window . main)
>> (window . <window_object>)
>>
>> But for disambiguation maybe also add some prefix like
>>
>> direction-window
>> from-window
>
> 'from-window' is not bad.  Maybe also 'reference-window'.  We don't
> use such a term in windmove.el.  There we just say that "WINDOW is the
> window that movement is relative to".

relative-window?

>> Ok, like (direction . up) and all aliases.
>
> 'direction' should be OK then.

Or 'dir' for short.

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

[martin rudalics]

 > Then two new functions will have consistent names:
 >
 > display-buffer-in-direction
 > display-buffer-in-window

Just that the first is an action function while the second isn't.

 >> 'from-window' is not bad.  Maybe also 'reference-window'.  We don't
 >> use such a term in windmove.el.  There we just say that "WINDOW is the
 >> window that movement is relative to".
 >
 > relative-window?

Not really good IMHO.

 >>> Ok, like (direction . up) and all aliases.
 >>
 >> 'direction' should be OK then.
 >
 > Or 'dir' for short.

dir can be misread as directory.

martin

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

[Juri Linkov]

>> Then two new functions will have consistent names:
>>
>> display-buffer-in-direction
>> display-buffer-in-window
>
> Just that the first is an action function while the second isn't.

Like display-buffer-in-previous-window is an action function
that takes an alist entry `previous-window', couldn't
display-buffer-in-window be an action function that takes
an alist entry with the window where it display the buffer?
But it seems this is not needed.

>>> 'from-window' is not bad.  Maybe also 'reference-window'.  We don't
>>> use such a term in windmove.el.  There we just say that "WINDOW is the
>>> window that movement is relative to".
>>
>> relative-window?
>
> Not really good IMHO.

Agreed.

>>>> Ok, like (direction . up) and all aliases.
>>>
>>> 'direction' should be OK then.
>>
>> Or 'dir' for short.
>
> dir can be misread as directory.

Agreed.

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

[martin rudalics]

 > Like display-buffer-in-previous-window is an action function
 > that takes an alist entry `previous-window', couldn't
 > display-buffer-in-window be an action function that takes
 > an alist entry with the window where it display the buffer?
 > But it seems this is not needed.

It could be confusing.  'window--display-buffer' receives two
distinguished arguments - a live WINDOW that it _has_ to use for
displaying BUFFER in and a TYPE needed for correctly processing
'display-buffer-mark-dedicated' and the 'quite-restore' parameter.  If
callers fail to set these reliably, further processing might be
broken.  And keep in mind that unlike ordinary action functions
'window--display-buffer' never fails.  Any failure would be with a
broken caller.

martin

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

[Juri Linkov]

>> Like we already have such functions as window-in-direction and
>> windmove-display-in-direction, the new function could have a similar name
>> display-buffer-in-direction.
>
> OK (unless we find something better).

We urgently need this.  I discovered another case that will benefit from it.
Currently it can be rewritten as in this patch but I don't like how it requires
a non-trivial alist.  Could these be replaced with something simpler?

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index 52c0b5b74d..e90d70359f 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -252,7 +252,11 @@ widget-choose
 	   (define-key map [?\M--] 'negative-argument)
 	   (save-window-excursion
 	     (let ((buf (get-buffer " widget-choose")))
-	       (fit-window-to-buffer (display-buffer buf))
+	       (display-buffer
+                buf
+                '(display-buffer-maybe-below-selected
+                  (window-height . fit-window-to-buffer)
+                  (preserve-size nil . t)))
 	       (let ((cursor-in-echo-area t)
 		     (arg 1))
                  (while (not value)

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

[martin rudalics]

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

 > We urgently need this.  I discovered another case that will benefit from it.
 > Currently it can be rewritten as in this patch but I don't like how it requires
 > a non-trivial alist.  Could these be replaced with something simpler?

I've already forgotten what we really want.  Find attached a draft of
what I had in mind in the beginning and fill in the details, if
possible.

martin

[-- Attachment #2: display-buffer-in-direction.el --]
[-- Type: application/emacs-lisp, Size: 5272 bytes --]

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

[Juri Linkov]

> I've already forgotten what we really want.  Find attached a draft of
> what I had in mind in the beginning and fill in the details, if
> possible.

Thanks, some suggestions to simplify its usage:

1. avoid using dotted pair notation that often causes problems;

2. instead of asking the user to invent a value to use for the selected window,
   allow omitting it in this case, by reversing WIN and DIR, for example:

   display-buffer-in-direction (direction DIR WIN)
   display-buffer-in-direction (direction up main)
   display-buffer-in-direction (direction up) -- by default means
                                                 from the selected window

PS: what about 'resize-to-fit'?  I guess it's impossible to implement it
as an alist, because currently fit-window-to-buffer/preserve-size usually
are used as an argument of the macro 'with-displayed-buffer-window'.

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

[martin rudalics]

 > Thanks, some suggestions to simplify its usage:
 >
 > 1. avoid using dotted pair notation that often causes problems;

You probably mean separate 'direction' and 'window' entries instead of
the (direction . (WIN . DIR)).  But we didn't find a good term for
denoting the reference window and the two inherently belong together.

 > 2. instead of asking the user to invent a value to use for the selected window,
 >     allow omitting it in this case, by reversing WIN and DIR, for example:
 >
 >     display-buffer-in-direction (direction DIR WIN)
 >     display-buffer-in-direction (direction up main)
 >     display-buffer-in-direction (direction up) -- by default means
 >                                                   from the selected window

We can do that.

 > PS: what about 'resize-to-fit'?  I guess it's impossible to implement it
 > as an alist, because currently fit-window-to-buffer/preserve-size usually
 > are used as an argument of the macro 'with-displayed-buffer-window'.

If we don't implement it already via the window-height/window-width
entries we can add a window-size entry.  But I forgot what you wanted
here.

martin

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

[Juri Linkov]

>> 1. avoid using dotted pair notation that often causes problems;
>
> You probably mean separate 'direction' and 'window' entries instead of
> the (direction . (WIN . DIR)).  But we didn't find a good term for
> denoting the reference window and the two inherently belong together.

I think your idea of combining them is good.

>> 2. instead of asking the user to invent a value to use for the selected window,
>>     allow omitting it in this case, by reversing WIN and DIR, for example:
>>
>>     display-buffer-in-direction (direction DIR WIN)
>>     display-buffer-in-direction (direction up main)
>>     display-buffer-in-direction (direction up) -- by default means
>>                                                   from the selected window
>
> We can do that.
>
>> PS: what about 'resize-to-fit'?  I guess it's impossible to implement it
>> as an alist, because currently fit-window-to-buffer/preserve-size usually
>> are used as an argument of the macro 'with-displayed-buffer-window'.
>
> If we don't implement it already via the window-height/window-width
> entries we can add a window-size entry.  But I forgot what you wanted
> here.

Currently it requires too much boilerplate code to do such simple things
as displaying the buffer below/bottom with resizing to fit its height.
Please grep “-at-bottom” and “-below-selected” for the current cases,
they are all ugly: some of them use ‘with-displayed-buffer-window’ with

  '((window-height . fit-window-to-buffer)
    (preserve-size . (nil . t)))

some are more uglier

  ,(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)))

some use the macro ‘with-current-buffer-window’, some use
‘pop-to-buffer’ with ‘display-buffer-below-selected’ action.

Do you think it's possible to generalize all these cases
to use simpler display actions/alists?

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

[martin rudalics]

 >> You probably mean separate 'direction' and 'window' entries instead of
 >> the (direction . (WIN . DIR)).  But we didn't find a good term for
 >> denoting the reference window and the two inherently belong together.
 >
 > I think your idea of combining them is good.

So using (direction . (DIR . WIN)) would be OK?

 > Currently it requires too much boilerplate code to do such simple things
 > as displaying the buffer below/bottom with resizing to fit its height.
 > Please grep “-at-bottom” and “-below-selected” for the current cases,
 > they are all ugly: some of them use ‘with-displayed-buffer-window’ with
 >
 >    '((window-height . fit-window-to-buffer)
 >      (preserve-size . (nil . t)))
 >
 > some are more uglier
 >
 >    ,(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)))
 >
 > some use the macro ‘with-current-buffer-window’, some use
 > ‘pop-to-buffer’ with ‘display-buffer-below-selected’ action.
 >
 > Do you think it's possible to generalize all these cases
 > to use simpler display actions/alists?

I'm afraid that this one

 >    ,(if temp-buffer-resize-mode
 >         '(window-height . resize-temp-buffer-window)

is not entirely kosher.  'resize-temp-buffer-window' should be called
only from 'temp-buffer-show-hook' or 'temp-buffer-window-show-hook'.
'display-buffer-at-bottom' can't tell whether BUFFER is temporary or
not.  Or am I missing something?

martin

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

[Juri Linkov]

>>> You probably mean separate 'direction' and 'window' entries instead of
>>> the (direction . (WIN . DIR)).  But we didn't find a good term for
>>> denoting the reference window and the two inherently belong together.
>>
>> I think your idea of combining them is good.
>
> So using (direction . (DIR . WIN)) would be OK?

And what to do when the future will require adding a third arg?
This is why better to avoid dotted pairs, and use a list like

  (direction DIR WIN)

>> Currently it requires too much boilerplate code to do such simple things
>> as displaying the buffer below/bottom with resizing to fit its height.
>> Please grep “-at-bottom” and “-below-selected” for the current cases,
>> they are all ugly: some of them use ‘with-displayed-buffer-window’ with
>>
>>    '((window-height . fit-window-to-buffer)
>>      (preserve-size . (nil . t)))
>>
>> some are more uglier
>>
>>    ,(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)))
>>
>> some use the macro ‘with-current-buffer-window’, some use
>> ‘pop-to-buffer’ with ‘display-buffer-below-selected’ action.
>>
>> Do you think it's possible to generalize all these cases
>> to use simpler display actions/alists?
>
> I'm afraid that this one
>
>>    ,(if temp-buffer-resize-mode
>>         '(window-height . resize-temp-buffer-window)
>
> is not entirely kosher.  'resize-temp-buffer-window' should be called
> only from 'temp-buffer-show-hook' or 'temp-buffer-window-show-hook'.
> 'display-buffer-at-bottom' can't tell whether BUFFER is temporary or
> not.  Or am I missing something?

I don't know.  At least, it seems it's doing its job.  What doesn't work
is for example (pop-to-buffer buf '(display-buffer--maybe-at-bottom))
in files.el.  Please try to use a file with Local Variables that ask
for permissions interactively, using a single-window wide frame:
instead of showing the Local Variables buffer at the bottom
it splits windows horizontally and shows the Local Variables at the top
of the right-hand window.  If the windows were already split horizontally,
then it correctly displays the Local Variables at the bottom.

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

[Drew Adams]

> > So using (direction . (DIR . WIN)) would be OK?
> 
> And what to do when the future will require adding a third arg?
> This is why better to avoid dotted pairs, and use a list like
> (direction DIR WIN)

(Caveat: I haven't been following this thread.)

But YES to what Juri wrote there.  This is a
(minor) pet peeve of mine.  I like dotted pairs
for some things, but this is a standard gotcha.

Sometimes doing this might represent premature
optimization.  Sometimes it might come from
focusing too closely on the initial use case
(e.g. the only use case, to start with).  But
it happens - to all of us, no doubt.

(You could later hack the definition to also
allow something else in place of (DIR . WIN),
but that kind of thing becomes ugly, especially
if abused more than once.)

Example:

Whoever designed the Lisp representation of a
noncontiguous region trapped us the same way.
By using a dotted pair of scalar values, that
design pretty much precludes adding other info
besides the start and end limits to a region
segment.

The zones of `zones.el' are similar to the
segments of a noncontiguous region, but
instead of just (BEGIN . END) a zone has the
form (LIMIT1 LIMIT2 . EXTRA).

I provided from the outset for the possibility
of including EXTRA stuff, even though at that
time I had no special use in mind for it.
Later I was very thankful I had included it.

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

[Juri Linkov]

> Example:
>
> Whoever designed the Lisp representation of a
> noncontiguous region trapped us the same way.
> By using a dotted pair of scalar values, that
> design pretty much precludes adding other info
> besides the start and end limits to a region
> segment.

You can override region-extract-function with your own
implementation that can support any shape you want.

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

[Drew Adams]

> > Example:
> >
> > Whoever designed the Lisp representation of a
> > noncontiguous region trapped us the same way.
> > By using a dotted pair of scalar values, that
> > design pretty much precludes adding other info
> > besides the start and end limits to a region
> > segment.
> 
> You can override region-extract-function with your own
> implementation that can support any shape you want.

1. It's not about the region shape - at all.

2. The point is more general.  Code that invokes
the function that is the value of the variable
does not, in general, know what function that is.

It can only expect, based on the default value of
the function, that for input `bounds' it gets a
cons (START . END).

IOW, any function used as the variable value
really needs to return bounds of the same form,
if it expects to be used in more than an odd,
narrow context.  In general, such a function
will not know or care what context it's used in.

As a result, developers will provide functions
that model the args and return values of the
default function.

The default function was designed with a poor
choice for the value returned by input `bounds'.

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

[martin rudalics]

 > And what to do when the future will require adding a third arg?
 > This is why better to avoid dotted pairs, and use a list like
 >
 >    (direction DIR WIN)

OK.  Let's do that.

 > I don't know.  At least, it seems it's doing its job.  What doesn't work
 > is for example (pop-to-buffer buf '(display-buffer--maybe-at-bottom))
 > in files.el.  Please try to use a file with Local Variables that ask
 > for permissions interactively, using a single-window wide frame:
 > instead of showing the Local Variables buffer at the bottom
 > it splits windows horizontally and shows the Local Variables at the top
 > of the right-hand window.  If the windows were already split horizontally,
 > then it correctly displays the Local Variables at the bottom.

Can you give me a simple recipe?  I forgot how to trigger the Local
Variables dialogue.

Thanks, martin

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

[Juri Linkov]

>> What doesn't work is for example (pop-to-buffer buf
>> '(display-buffer--maybe-at-bottom)) in files.el.  Please try to use
>> a file with Local Variables that ask for permissions interactively,
>> using a single-window wide frame: instead of showing the Local
>> Variables buffer at the bottom it splits windows horizontally and
>> shows the Local Variables at the top of the right-hand window.
>> If the windows were already split horizontally, then it correctly
>> displays the Local Variables at the bottom.
>
> Can you give me a simple recipe?  I forgot how to trigger the Local
> Variables dialogue.

Please just add to a new file:

;; Local Variables:
;; foo: bar
;; End:

Visiting it in a single-window wide frame splits it horizontally
instead of displaying Local Variables at the bottom, as it correctly
does when windows are already horizontally split before visiting the file.

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

[martin rudalics]

 > Please just add to a new file:
 >
 > ;; Local Variables:
 > ;; foo: bar
 > ;; End:
 >
 > Visiting it in a single-window wide frame splits it horizontally
 > instead of displaying Local Variables at the bottom, as it correctly
 > does when windows are already horizontally split before visiting the file.

It's a silly bug in 'display-buffer-at-bottom'.  Please try with

  	     (let ((split-height-threshold 0))

instead of

  	     (let (split-height-threshold)

Thanks, martin

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

[Juri Linkov]

>> Visiting it in a single-window wide frame splits it horizontally
>> instead of displaying Local Variables at the bottom, as it correctly
>> does when windows are already horizontally split before visiting the file.
>
> It's a silly bug in 'display-buffer-at-bottom'.  Please try with
>
>  	     (let ((split-height-threshold 0))
>
> instead of
>
>  	     (let (split-height-threshold)

Thanks, this fixed an original problem, but introduced a new problem:
now when windows are already horizontally split, the bottom window
is narrowed to the width of the left window.  Before this change,
the width of Local Variables window was the same as the frame width.

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

[martin rudalics]

 > Thanks, this fixed an original problem, but introduced a new problem:
 > now when windows are already horizontally split, the bottom window
 > is narrowed to the width of the left window.  Before this change,
 > the width of Local Variables window was the same as the frame width.

The entire

	(and (not (frame-parameter nil 'unsplittable))
	     (let ((split-height-threshold 0))
	       (setq window (window--try-to-split-window bottom-window alist)))
	     (window--display-buffer buffer window 'window alist))

is bad design.

Note that I removed this behavior in 'display-buffer-in-direction'.
There I (1) disregard 'split-window-preferred-function' and (2) only
split the argument window which would be the main window when called
from 'display-buffer-at-bottom'.

martin

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

[martin rudalics]

 > The entire
 >
 >      (and (not (frame-parameter nil 'unsplittable))
 >           (let ((split-height-threshold 0))
 >             (setq window (window--try-to-split-window bottom-window alist)))
 >           (window--display-buffer buffer window 'window alist))
 >
 > is bad design.

Removed now from master's 'display-buffer-at-bottom'.

martin

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

[Juri Linkov]

>> The entire
>>
>>      (and (not (frame-parameter nil 'unsplittable))
>>           (let ((split-height-threshold 0))
>>             (setq window (window--try-to-split-window bottom-window alist)))
>>           (window--display-buffer buffer window 'window alist))
>>
>> is bad design.
>
> Removed now from master's 'display-buffer-at-bottom'.

Thanks, glad to see how just removing code is an improvement.
Maybe we could remove more code to fix other problems ;-)

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

[Juri Linkov]

>> And what to do when the future will require adding a third arg?
>> This is why better to avoid dotted pairs, and use a list like
>>
>>    (direction DIR WIN)
>
> OK.  Let's do that.

I'm trying to use your implementation of display-buffer-in-direction,
but with this patch:

diff --git a/lisp/files.el b/lisp/files.el
index 9948bd4a03..dac75fdb78 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -3396,7 +3396,7 @@ hack-local-variables-confirm
 
       ;; Display the buffer and read a choice.
       (save-window-excursion
-	(pop-to-buffer buf '(display-buffer--maybe-at-bottom))
+	(pop-to-buffer buf '(display-buffer-in-direction (direction bottom main)))
 	(let* ((exit-chars '(?y ?n ?\s ?\C-g ?\C-v))
 	       (prompt (format "Please type %s%s: "
 			       (if offer-save "y, n, or !" "y or n")

while visiting a file with Local Variables it fails with:

Debugger entered--Lisp error: (error "Cannot share edge from within live window #<window...")
  signal(error ("Cannot share edge from within live window #<window..."))
  error("Cannot share edge from within live window %s" #<window 6 on etc>)
  windows-sharing-edge(#<window 6 on etc> below t)
  display-buffer-in-direction(#<buffer *Local Variables*> ((direction bottom main)))
  display-buffer(#<buffer *Local Variables*> (display-buffer-in-direction (direction bottom main)))
  pop-to-buffer(#<buffer *Local Variables*> (display-buffer-in-direction (direction bottom main)))

Please help.

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

[martin rudalics]

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

 > I'm trying to use your implementation of display-buffer-in-direction,
 > but with this patch:
 >
 > diff --git a/lisp/files.el b/lisp/files.el
 > index 9948bd4a03..dac75fdb78 100644
 > --- a/lisp/files.el
 > +++ b/lisp/files.el
 > @@ -3396,7 +3396,7 @@ hack-local-variables-confirm
 >
 >         ;; Display the buffer and read a choice.
 >         (save-window-excursion
 > -	(pop-to-buffer buf '(display-buffer--maybe-at-bottom))
 > +	(pop-to-buffer buf '(display-buffer-in-direction (direction bottom main)))
 >   	(let* ((exit-chars '(?y ?n ?\s ?\C-g ?\C-v))
 >   	       (prompt (format "Please type %s%s: "
 >   			       (if offer-save "y, n, or !" "y or n")
 >
 > while visiting a file with Local Variables it fails with:
 >
 > Debugger entered--Lisp error: (error "Cannot share edge from within live window #<window...")
 >    signal(error ("Cannot share edge from within live window #<window..."))
 >    error("Cannot share edge from within live window %s" #<window 6 on etc>)
 >    windows-sharing-edge(#<window 6 on etc> below t)
 >    display-buffer-in-direction(#<buffer *Local Variables*> ((direction bottom main)))
 >    display-buffer(#<buffer *Local Variables*> (display-buffer-in-direction (direction bottom main)))
 >    pop-to-buffer(#<buffer *Local Variables*> (display-buffer-in-direction (direction bottom main)))

I attach a version which should handle this now.  I still can't get
used to a positional specification of direction and reference window
so ALIST now has to contain separate 'direction' and 'window' entries
as in

(pop-to-buffer buf '(display-buffer-in-direction (direction . bottom) (window . main)))

where the 'direction' entry is mandatory.

martin

[-- Attachment #2: display-buffer-in-direction.el --]
[-- Type: application/emacs-lisp, Size: 5200 bytes --]

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

[Juri Linkov]

> I attach a version which should handle this now.  I still can't get
> used to a positional specification of direction and reference window
> so ALIST now has to contain separate 'direction' and 'window' entries
> as in
>
> (pop-to-buffer buf '(display-buffer-in-direction (direction . bottom) (window . main)))

Thanks, it works without errors, but for some reason it doesn't fit
the buffer into the window, i.e. the old version

  (pop-to-buffer buf '(display-buffer--maybe-at-bottom))

correctly resized the window, but the new version above doesn't.

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

[martin rudalics]

 > Thanks, it works without errors, but for some reason it doesn't fit
 > the buffer into the window, i.e. the old version
 >
 >    (pop-to-buffer buf '(display-buffer--maybe-at-bottom))
 >
 > correctly resized the window, but the new version above doesn't.

Sorry.  You will have to step through it with edebug to find out the
cause.  Maybe the new window is not appropriately combined.  IIRC
resizing should by default affect only one neighboring window.

martin

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

[Juri Linkov]

>> Thanks, it works without errors, but for some reason it doesn't fit
>> the buffer into the window, i.e. the old version
>>
>>    (pop-to-buffer buf '(display-buffer--maybe-at-bottom))
>>
>> correctly resized the window, but the new version above doesn't.
>
> Sorry.  You will have to step through it with edebug to find out the
> cause.  Maybe the new window is not appropriately combined.  IIRC
> resizing should by default affect only one neighboring window.

I see the same problem with the *Marked Processes* buffer from proced,
but I'll create a separate request.

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

[Dmitry Gutov]

On 03.01.2019 17:24, João Távora wrote:
> * But by using less space it is also less useful.  You don't get to see,
>    at a glance, a great deal of xrefs.  And xrefs are different from
>    completions, they're closer to grep hits.  You wouldn't put*grep*
>    hits in such a potentially tiny window, would you?

This would be my main objection as well.

> Then again, perhaps you would, and the whole point of this patch is to
> make the UI configurable.  If so, I'd make the original UI the default,
> or at least very very easy to bring back.

Making it configurable on a high level is something I might agree with 
(and indeed, the find-definitions command probably should behave 
differently from the rest), but the way it's done should be transparent 
and not strongly coupled to a particular commands.

Maybe via two different xrefs-show-function values. I'm not sure.

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

[Juri Linkov]

>> * But by using less space it is also less useful.  You don't get to see,
>>    at a glance, a great deal of xrefs.  And xrefs are different from
>>    completions, they're closer to grep hits.  You wouldn't put*grep*
>>    hits in such a potentially tiny window, would you?
>
> This would be my main objection as well.

Actually the window is not tiny.  It gets more than a half window
from the window there is was called.  But now I propose to do this
only for xref-find-definitions.

>> Then again, perhaps you would, and the whole point of this patch is to
>> make the UI configurable.  If so, I'd make the original UI the default,
>> or at least very very easy to bring back.
>
> Making it configurable on a high level is something I might agree with (and
> indeed, the find-definitions command probably should behave differently
> from the rest), but the way it's done should be transparent and not
> strongly coupled to a particular commands.
>
> Maybe via two different xrefs-show-function values. I'm not sure.

Or maybe better xrefs-display-buffer-alist.

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

[Dmitry Gutov]

On 04.01.2019 3:12, Juri Linkov wrote:

>> This would be my main objection as well.
> 
> Actually the window is not tiny.  It gets more than a half window
> from the window there is was called.  But now I propose to do this
> only for xref-find-definitions.

In any case, I'd like to see it in a separate patch (maybe a separate 
bug#/discussion as well). Let's not mix the proposal in question, which 
more or less retains the current behavior, with something where the main 
goal is changing it.

>> Maybe via two different xrefs-show-function values. I'm not sure.
> 
> Or maybe better xrefs-display-buffer-alist.

I'd have to see the patch.

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

[Dmitry Gutov]

And also:

On 03.01.2019 3:18, Juri Linkov wrote:
>           (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)))))

Are we really supposed to use the private functions here, outside of 
window.el?

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

[Dmitry Gutov]

On 03.01.2019 3:18, Juri Linkov wrote:
> 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.

Kind of similar, but it has a different effect, right? So the logic 
doesn't really translate.

Since we already have the xref-quit-and-goto-xref command, I'm not so 
sure this part is particularly useful.

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

[Juri Linkov]

>> 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.
>
> Kind of similar, but it has a different effect, right? So the logic doesn't
> really translate.
>
> Since we already have the xref-quit-and-goto-xref command, I'm not so sure
> this part is particularly useful.

xref-quit-and-goto-xref has no good keybinding.  If you type TAB
in a web browser, do you expect it to close the current window
and open a link in a new window?

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

[Dmitry Gutov]

On 04.01.2019 3:14, Juri Linkov wrote:

> xref-quit-and-goto-xref has no good keybinding.  If you type TAB
> in a web browser, do you expect it to close the current window
> and open a link in a new window?

You yourself likened it to the Completions buffer...

An the binding is customizable anyway. This is just a criticism of the 
default value (which we've agreed upon in a different discussion).

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

[João Távora]

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

On Fri, Jan 4, 2019, 00:17 Juri Linkov <juri@linkov.net wrote:

xref-quit-and-goto-xref has no good keybinding.  If you type TAB
> in a web browser, do you expect it to close the current window
> and open a link in a new window?
>

I suggested TAB because in Emacs, tab completes stuff and closes
*completions*. So it's not consistent with web browsers but is consistent
with emacs, as usual. I agree it's not a brilliant binding, but could we
not focus efforts. changing the UI here and solve the actual bug first?

>

[-- Attachment #2: Type: text/html, Size: 1015 bytes --]

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

[Juri Linkov]

>> xref-quit-and-goto-xref has no good keybinding.  If you type TAB
>> in a web browser, do you expect it to close the current window
>> and open a link in a new window?
>
> I suggested TAB because in Emacs, tab completes stuff and closes
> *completions*. So it's not consistent with web browsers but is consistent
> with emacs, as usual.

Typing TAB in *Completions* moves point to the next completion,
so in xref this corresponds to xref-next-line, and Shift-TAB
moves to the previous completion that corresponds to xref-prev-line.

The closest analogue to "close and do it" in Emacs I see only
dired-find-alternate-file bound to ‘a’:

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 87ce2299c5..169f49a348 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -685,7 +674,9 @@ xref--xref-buffer-mode-map
     (define-key map (kbd "p") #'xref-prev-line)
     (define-key map (kbd "r") #'xref-query-replace-in-results)
     (define-key map (kbd "RET") #'xref-goto-xref)
-    (define-key map (kbd "TAB")  #'xref-quit-and-goto-xref)
+    (define-key map (kbd "TAB") #'xref-next-line)
+    (define-key map [backtab] #'xref-prev-line)
+    (define-key map (kbd "a") #'xref-quit-and-goto-xref)
     (define-key map (kbd "C-o") #'xref-show-location-at-point)
     ;; suggested by Johan Claesson "to further reduce finger movement":
     (define-key map (kbd ".") #'xref-next-line)

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

[Dmitry Gutov]

On 06.01.2019 02:17, Juri Linkov wrote:

>> I suggested TAB because in Emacs, tab completes stuff and closes
>> *completions*. So it's not consistent with web browsers but is consistent
>> with emacs, as usual.

Joao probably meant pressing TAB in the original buffer, which is not 
something you can do with xref.

> Typing TAB in *Completions* moves point to the next completion,
> so in xref this corresponds to xref-next-line, and Shift-TAB
> moves to the previous completion that corresponds to xref-prev-line.

Maybe that's an argument for not bringing xref closed to Completions: I 
would certainly not like if 'n' and 'p' were replaced with 'TAB' and 
'S-TAB'.

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

[Juri Linkov]

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

Here's a better patch that relies on display-buffer-in-previous-window:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: xref-previous-window.patch --]
[-- Type: text/x-diff, Size: 1342 bytes --]

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 87ce2299c5..169f49a348 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))
+                 `((display-buffer-in-previous-window)
+                   (previous-window . ,xref--original-window))))))
+    (with-selected-window (display-buffer buf action)
       (xref--goto-char pos)
       (run-hooks 'xref-after-jump-hook)
       (let ((buf (current-buffer)))

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

[Dmitry Gutov]

On 06.01.2019 02:27, Juri Linkov wrote:
> (with-selected-window (display-buffer buf action)

So using display-buffer is okay here, after all? I thought pop-to-buffer 
is where the magic happens.

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

[João Távora]

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

Juri Linkov <juri@linkov.net> writes:

> 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:

Hi again, Juri

After re-reading your patch more closely and giving it some more
testing, I've discovered it breaks an existing use case:

   Emacs -Q
   C-x 2 ;; split-window-horizontally
   C-x 4 . ;; xref-find-definitions-other-window
   xref-backend-definitions RET
   C-n RET ;; in the resulting *xref* buffer
   
Expected xref.el to appear in the bottom window which was my original
intent when I said "other window".  In the current master this works OK,
in your patch it doesn't.

But don't worry, I've fixed that.  In the patch that I attach to this
message, none of the current UI changes is changed, but the xref window
should now be configurable as is the original request of this bug.

I've also renamed window.el's window--display-buffer to
window-display-buffer throughout Emacs (i.e. made it public).

After we merge this, we can continue the discussion about the changing
the xref UI in the other bug you opened, bug#33992


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-xref-s-choice-of-windows-easier-to-configure.patch --]
[-- Type: text/x-patch, Size: 11884 bytes --]

From 424fe397c86026469a3853e86ca486d549f58100 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Mon, 7 Jan 2019 14:16:35 +0000
Subject: [PATCH] Make xref's choice of windows easier to configure

Fixes: bug#33870

Allow for the usual user configuration strategies involving
display-buffer-alist, etc. while maintaining the "keep original window
intent" of xref-find-definitions, xref-find-references, etc.

* lisp/windmove.el (windmove-display-in-direction): Use
window--display-buffer.

* lisp/window.el (display-buffer-in-atom-window):
(window--make-major-side-window):
(display-buffer-in-side-window):
(display-buffer-in-side-window):
(display-buffer-in-side-window):
(display-buffer-use-some-frame):
(display-buffer-same-window):
(display-buffer-reuse-window):
(display-buffer-reuse-mode-window):
(display-buffer-pop-up-frame):
(display-buffer-pop-up-window):
(display-buffer-in-child-frame):
(display-buffer-below-selected):
(display-buffer-below-selected):
(display-buffer-below-selected):
(display-buffer-below-selected):
(display-buffer-at-bottom):
(display-buffer-in-previous-window):
(display-buffer-use-some-window): Use window-display-buffer.
(window-display-buffer): Rename from window--display-buffer.

* lisp/progmodes/xref.el (xref--show-pos-in-buf): Use
window-display-buffer.
---
 lisp/progmodes/xref.el |  5 ++---
 lisp/windmove.el       |  2 +-
 lisp/window.el         | 46 +++++++++++++++++++++---------------------
 3 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 87ce2299c5..20eaa51bef 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -482,9 +482,8 @@ 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))))))
+                 `(,(lambda (buf alist)
+                      (window-display-buffer buf xref--original-window 'reuse alist)))))))
     (with-selected-window
         (with-selected-window
             ;; Just before `display-buffer', place ourselves in the
diff --git a/lisp/windmove.el b/lisp/windmove.el
index 65270d9bbe..54f0098de7 100644
--- a/lisp/windmove.el
+++ b/lisp/windmove.el
@@ -626,7 +626,7 @@ windmove-display-in-direction
                     (type 'reuse))
                 (unless window
                   (setq window (split-window nil nil dir) type 'window))
-		(setq new-window (window--display-buffer buffer window type alist)))))
+		(setq new-window (window-display-buffer buffer window type alist)))))
           display-buffer-overriding-action)
     (message "[display-%s]" dir)))
 
diff --git a/lisp/window.el b/lisp/window.el
index 37d82c060c..cf923f7dc6 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -700,7 +700,7 @@ display-buffer-in-atom-window
 	(set-window-parameter window 'window-atom 'main))
       (set-window-parameter new 'window-atom side)
       ;; Display BUFFER in NEW and return NEW.
-      (window--display-buffer
+      (window-display-buffer
        buffer new 'window alist display-buffer-mark-dedicated))))
 
 (defun window--atom-check-1 (window)
@@ -985,7 +985,7 @@ window--make-major-side-window
       (with-current-buffer buffer
         (setq window--sides-shown t))
       ;; Install BUFFER in new window and return WINDOW.
-      (window--display-buffer buffer window 'window alist 'side))))
+      (window-display-buffer buffer window 'window alist 'side))))
 
 (defun display-buffer-in-side-window (buffer alist)
   "Display BUFFER in a side window of the selected frame.
@@ -1113,7 +1113,7 @@ display-buffer-in-side-window
 		 ;; Reuse `this-window'.
                  (with-current-buffer buffer
                    (setq window--sides-shown t))
-		 (window--display-buffer
+		 (window-display-buffer
                   buffer this-window 'reuse alist dedicated))
 	    (and (or (not max-slots) (< slots max-slots))
 		 (or (and next-window
@@ -1131,7 +1131,7 @@ display-buffer-in-side-window
 		   (set-window-parameter window 'window-slot slot)
                    (with-current-buffer buffer
                      (setq window--sides-shown t))
-		   (window--display-buffer
+		   (window-display-buffer
                     buffer window 'window alist dedicated))
 	    (and best-window
 		 ;; Reuse `best-window'.
@@ -1140,7 +1140,7 @@ display-buffer-in-side-window
 		   (set-window-parameter best-window 'window-slot slot)
                    (with-current-buffer buffer
                      (setq window--sides-shown t))
-                   (window--display-buffer
+                   (window-display-buffer
 		    buffer best-window 'reuse alist dedicated)))))))))
 
 (defun window-toggle-side-windows (&optional frame)
@@ -6748,7 +6748,7 @@ window--even-window-sizes
            (/ (- (window-total-height window) (window-total-height)) 2))
         (error nil))))))
 
-(defun window--display-buffer (buffer window type &optional alist dedicated)
+(defun window-display-buffer (buffer window type &optional alist dedicated)
   "Display BUFFER in WINDOW.
 TYPE must be one of the symbols `reuse', `window' or `frame' and
 is passed unaltered to `display-buffer-record-window'.  ALIST is
@@ -7190,7 +7190,7 @@ display-buffer-use-some-frame
                 frame nil (cdr (assq 'inhibit-same-window alist))))))
     (when window
       (prog1
-          (window--display-buffer
+          (window-display-buffer
            buffer window 'reuse alist display-buffer-mark-dedicated)
         (unless (cdr (assq 'inhibit-switch-frame alist))
           (window--maybe-raise-frame frame))))))
@@ -7204,7 +7204,7 @@ display-buffer-same-window
   (unless (or (cdr (assq 'inhibit-same-window alist))
 	      (window-minibuffer-p)
 	      (window-dedicated-p))
-    (window--display-buffer buffer (selected-window) 'reuse alist)))
+    (window-display-buffer buffer (selected-window) 'reuse alist)))
 
 (defun display-buffer--maybe-same-window (buffer alist)
   "Conditionally display BUFFER in the selected window.
@@ -7252,7 +7252,7 @@ display-buffer-reuse-window
 			      (get-buffer-window-list buffer 'nomini
 						      frames))))))
     (when (window-live-p window)
-      (prog1 (window--display-buffer buffer window 'reuse alist)
+      (prog1 (window-display-buffer buffer window 'reuse alist)
 	(unless (cdr (assq 'inhibit-switch-frame alist))
 	  (window--maybe-raise-frame (window-frame window)))))))
 
@@ -7316,7 +7316,7 @@ display-buffer-reuse-mode-window
                                 derived-mode-same-frame
                                 derived-mode-other-frame))))
         (when (window-live-p window)
-          (prog1 (window--display-buffer buffer window 'reuse alist)
+          (prog1 (window-display-buffer buffer window 'reuse alist)
             (unless (cdr (assq 'inhibit-switch-frame alist))
               (window--maybe-raise-frame (window-frame window)))))))))
 
@@ -7356,7 +7356,7 @@ display-buffer-pop-up-frame
 	       (with-current-buffer buffer
 		 (setq frame (funcall fun)))
 	       (setq window (frame-selected-window frame)))
-      (prog1 (window--display-buffer
+      (prog1 (window-display-buffer
 	      buffer window 'frame alist display-buffer-mark-dedicated)
 	(unless (cdr (assq 'inhibit-switch-frame alist))
 	  (window--maybe-raise-frame frame))))))
@@ -7386,7 +7386,7 @@ display-buffer-pop-up-window
 				(window--try-to-split-window
 				 (get-lru-window frame t) alist))))
 
-      (prog1 (window--display-buffer
+      (prog1 (window-display-buffer
 	      buffer window 'window alist display-buffer-mark-dedicated)
 	(unless (cdr (assq 'inhibit-switch-frame alist))
 	  (window--maybe-raise-frame (window-frame window)))))))
@@ -7452,7 +7452,7 @@ display-buffer-in-child-frame
         (setq frame (make-frame parameters))
         (setq window (frame-selected-window frame))))
 
-    (prog1 (window--display-buffer
+    (prog1 (window-display-buffer
 	    buffer window 'frame alist display-buffer-mark-dedicated)
       (unless (cdr (assq 'inhibit-switch-frame alist))
 	(window--maybe-raise-frame frame)))))
@@ -7476,7 +7476,7 @@ display-buffer-below-selected
              (eq buffer (window-buffer window))
 	     (or (not (numberp min-height))
 		 (>= (window-height window) min-height)
-		 ;; 'window--display-buffer' can resize this window if
+		 ;; 'window-display-buffer' can resize this window if
 		 ;; and only if it has a 'quit-restore' parameter
 		 ;; certifying that it always showed BUFFER before.
 		 (let ((height (window-height window))
@@ -7484,7 +7484,7 @@ display-buffer-below-selected
 		   (and quit-restore
 			(eq (nth 1 quit-restore) 'window)
 			(window-resizable-p window (- min-height height)))))
-	     (window--display-buffer buffer window 'reuse alist))
+	     (window-display-buffer buffer window 'reuse alist))
 	(and (not (frame-parameter nil 'unsplittable))
              (or (not (numberp min-height))
 		 (window-sizable-p nil (- min-height)))
@@ -7492,7 +7492,7 @@ display-buffer-below-selected
 		   split-width-threshold)
                (setq window (window--try-to-split-window
                              (selected-window) alist)))
-             (window--display-buffer
+             (window-display-buffer
               buffer window 'window alist display-buffer-mark-dedicated))
 	(and (setq window (window-in-direction 'below))
              (not (window-dedicated-p window))
@@ -7500,7 +7500,7 @@ display-buffer-below-selected
 		 ;; A window that showed another buffer before cannot
 		 ;; be resized.
 		 (>= (window-height window) min-height))
-	     (window--display-buffer
+	     (window-display-buffer
               buffer window 'reuse alist display-buffer-mark-dedicated)))))
 
 (defun display-buffer--maybe-at-bottom (buffer alist)
@@ -7533,20 +7533,20 @@ display-buffer-at-bottom
 	 (setq bottom-window window))))
      nil nil 'nomini)
     (or (and bottom-window-shows-buffer
-	     (window--display-buffer
+	     (window-display-buffer
 	      buffer bottom-window 'reuse alist display-buffer-mark-dedicated))
 	(and (not (frame-parameter nil 'unsplittable))
 	     (let (split-width-threshold)
 	       (setq window (window--try-to-split-window bottom-window alist)))
-	     (window--display-buffer
+	     (window-display-buffer
 	      buffer window 'window alist display-buffer-mark-dedicated))
 	(and (not (frame-parameter nil 'unsplittable))
 	     (setq window (split-window-no-error (window-main-window)))
-	     (window--display-buffer
+	     (window-display-buffer
 	      buffer window 'window alist display-buffer-mark-dedicated))
 	(and (setq window bottom-window)
 	     (not (window-dedicated-p window))
-	     (window--display-buffer
+	     (window-display-buffer
 	      buffer window 'reuse alist display-buffer-mark-dedicated)))))
 
 (defun display-buffer-in-previous-window (buffer alist)
@@ -7603,7 +7603,7 @@ display-buffer-in-previous-window
 	(setq best-window window)))
     ;; Return best or second best window found.
     (when (setq window (or best-window second-best-window))
-      (window--display-buffer buffer window 'reuse alist))))
+      (window-display-buffer buffer window 'reuse alist))))
 
 (defun display-buffer-use-some-window (buffer alist)
   "Display BUFFER in an existing window.
@@ -7643,7 +7643,7 @@ display-buffer-use-some-window
 	  (error nil)))
 
       (prog1
-	  (window--display-buffer buffer window 'reuse alist)
+	  (window-display-buffer buffer window 'reuse alist)
 	(window--even-window-sizes window)
 	(unless (cdr (assq 'inhibit-switch-frame alist))
 	  (window--maybe-raise-frame (window-frame window)))))))
-- 
2.19.2

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

[Juri Linkov]

> After re-reading your patch more closely and giving it some more
> testing, I've discovered it breaks an existing use case:
>
>    Emacs -Q
>    C-x 2 ;; split-window-horizontally
>    C-x 4 . ;; xref-find-definitions-other-window
>    xref-backend-definitions RET
>    C-n RET ;; in the resulting *xref* buffer

Of course, it doesn't work if you tried it only with part of my changes.
When I submitted my initial patch, I tested it in all your test cases,
including the above test case that was not broken with my patch.

But you asked to break my patch to several pieces and submit them
separately to different bug reports.  No wonder that each of them
doesn't do what the whole patch did.

> Expected xref.el to appear in the bottom window which was my original
> intent when I said "other window".

Then the xref buffer is obscured by another buffer visited in the same
window, and if the user wants to visit more hits from the xref buffer,
this is not easy to do.

> In the current master this works OK, in your patch it doesn't.

My initial patch solved this problem gracefully by creating a new window
for the xref buffer.

> I've also renamed window.el's window--display-buffer to
> window-display-buffer throughout Emacs (i.e. made it public).

You can't rename old functions lightly.  This will break the existing
code.  This needs many years of deprecation process: in one release
declare the function as obsolete, and in another release delete
old aliases, because there are external packages that rely on this
function name like the `other-frame-window' package from ELPA, etc.

> After we merge this, we can continue the discussion about the changing
> the xref UI in the other bug you opened, bug#33992

Better start with bug#33992 because it supports the above test case,
then we could finish this bug#33870.

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

[Dmitry Gutov]

On 08.01.2019 01:16, Juri Linkov wrote:
>> After re-reading your patch more closely and giving it some more
>> testing, I've discovered it breaks an existing use case:
>>
>>     Emacs -Q
>>     C-x 2 ;; split-window-horizontally
>>     C-x 4 . ;; xref-find-definitions-other-window
>>     xref-backend-definitions RET
>>     C-n RET ;; in the resulting *xref* buffer
> 
> Of course, it doesn't work if you tried it only with part of my changes.
> When I submitted my initial patch, I tested it in all your test cases,
> including the above test case that was not broken with my patch.
> 
> But you asked to break my patch to several pieces and submit them
> separately to different bug reports.  No wonder that each of them
> doesn't do what the whole patch did.

We asked to split the patch into "retains old behavior while allowing 
certain customization" and "changes behavior".

Is that not feasible?

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

[Juri Linkov]

>>>     Emacs -Q
>>>     C-x 2 ;; split-window-horizontally
>>>     C-x 4 . ;; xref-find-definitions-other-window
>>>     xref-backend-definitions RET
>>>     C-n RET ;; in the resulting *xref* buffer
>>
>> Of course, it doesn't work if you tried it only with part of my changes.
>> When I submitted my initial patch, I tested it in all your test cases,
>> including the above test case that was not broken with my patch.
>>
>> But you asked to break my patch to several pieces and submit them
>> separately to different bug reports.  No wonder that each of them
>> doesn't do what the whole patch did.
>
> We asked to split the patch into "retains old behavior while allowing
> certain customization" and "changes behavior".

The former implies the latter, this is why they were in the same patch.
Please look again at the subject - it says nothing about avoiding
the need to improve old behavior, quite contrary.

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

[Dmitry Gutov]

On 08.01.2019 03:23, Juri Linkov wrote:

>> We asked to split the patch into "retains old behavior while allowing
>> certain customization" and "changes behavior".
> 
> The former implies the latter, this is why they were in the same patch.

Nope. The former does not imply a change in default behavior, which you 
are proposing to do.

> Please look again at the subject - it says nothing about avoiding
> the need to improve old behavior, quite contrary.

The subject doesn't mention turning Emacs into a coffee maker software 
(or *not* doing that), and yet, we're still going to avoid it.

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

[João Távora]

Juri Linkov <juri@linkov.net> writes:

> Of course, it doesn't work if you tried it only with part of my changes.
> When I submitted my initial patch, I tested it in all your test cases,
> including the above test case that was not broken with my patch.

You are correct.  I was testing under the assumption that making
xref-goto-xref configurable didn't require the "tiny window" for
xref-find-definitions, which is something you stated that you wanted to
do for other xref.el commands like xref-find-references.

Anyway, can't you add the tiny window xref-find-definitions and the
other UI changes as an addon to *my* patch?

> But you asked to break my patch to several pieces and submit them
> separately to different bug reports.  No wonder that each of them
> doesn't do what the whole patch did.
>> Expected xref.el to appear in the bottom window which was my original
>> intent when I said "other window".
> Then the xref buffer is obscured by another buffer visited in the same
> window, and if the user wants to visit more hits from the xref buffer,
> this is not easy to do.

>> In the current master this works OK, in your patch it doesn't.
>
> My initial patch solved this problem gracefully by creating a new window
> for the xref buffer.

You may well call this a problem, but it's not a bug.  It's the defined
behaviour, it's like this by design.  We are trying to create the
conditions that would enable you, or any other user, to create
alternative ways to present *xref* that have other advantages and
drawbacks.

>> I've also renamed window.el's window--display-buffer to
>> window-display-buffer throughout Emacs (i.e. made it public).
> You can't rename old functions lightly.  This will break the existing
> code.

What other code?  I renamed the uses in Emacs too: do you mean code
outside Emacs?  It shouldn't be using that internal function in the
first place.  But if it is, no reason to punish it: we can just provide
a deprecated function alias, which will at most warn its developers of
the imprudency.

No.  I can because they were internal functions that no-one was supposed
to have been using in the first place.  *That* is precisely why internal
functions are useful, so you can decide when and if to make them public.

You may argue that by making them public *now* we are going to have a
more deprecation problem if we decide to rename them again *in the
future*.  I would agree with you there.

>> After we merge this, we can continue the discussion about the changing
>> the xref UI in the other bug you opened, bug#33992

> Better start with bug#33992 because it supports the above test case,
> then we could finish this bug#33870.

No.  There is consensus on fixing the bug (xref.el doesn't respect
display-buffer-alist et al) without changing the UI.  There is no
consensus on changing the UI.

Also I think some would not really view this is a bug at all.

It would seem Juri that you are trying to shoehorn a behaviour change
into a bugfix.  Both things have less chances to go into Emacs that way.
I'm sure that:

* you can implement the UI changes that you want on top of my patch;

* you can make them optional;

* we can then agree on a good default.

Is that not so?

João

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

[martin rudalics]

 > You may argue that by making them public *now* we are going to have a
 > more deprecation problem if we decide to rename them again *in the
 > future*.  I would agree with you there.

The DEDICATED argument of 'window--display-buffer' is a very gross
hack that nobody among us will understand in all its consequences.
Try to guess its semantics from the fairly underdocumented variable
'display-buffer-mark-dedicated' and how it's set by the various buffer
display actions - most of them copying the call scheme from another.

IIUC the idea is that a "reused" window should not be made dedicated
while a new window could be made dedicated.  So we could guess the
intention from the TYPE argument - unless it's 'reuse', dedicate the
window if asked for.  But we do not implement that consistently.

So before making this function public, we should resolve this calling
convention.  Personally, I'd proceed as follows:

(1) Deprecate the variable 'display-buffer-mark-dedicated'.

(2) Remove the DEDICATED argument from this function.

(3) Add a 'dedicated' action alist entry to implement the
     functionality.

And we should specify once and for all whether a window can remain or
become dedicated when our function displays another buffer in it.

And another thing: The term "reuse" has two meanings in the context of
buffer display: OT1H "reuse" stands for reusing a window showing one
and the same buffer like in 'display-buffer-reuse-window'.  In
'window--display-buffer', if TYPE equals 'reuse' this just means that
an existing window has been reused for showing BUFFER - that window
might have shown another buffer before.  This confusion would have to
be resolved as well before going public with this function.

martin

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

[João Távora]

On Tue, Jan 8, 2019 at 9:25 AM martin rudalics <rudalics@gmx.at> wrote:

> So before making this function public, we should resolve this calling
> convention.

Makes sense.  Then, in my view, the logical sequence to fix this bug is

A. First do these changes to window.el and publish a decent
   window-display-buffer calling convention.

B. Push a xref.el based on the new function that doesn't change
the xref UI.

C. Discuss the xref.el UI in the other bug.

> Personally, I'd proceed as follows:
>
> (1) Deprecate the variable 'display-buffer-mark-dedicated'.
>
> (2) Remove the DEDICATED argument from this function.
>
> (3) Add a 'dedicated' action alist entry to implement the
>      functionality.

When do you think you can do this?  Be advised there is indeed some
third-party code already relying on the internal "--" version of this function.
We might be breaking some of that code (otoh it was "asking for it" for
using such an implementation detail).

> And we should [...] and another thing

Do both of these more ambitious refactorings really need to
make it in before we can do B as outlined above? Or can we
do them later in parallel?

João

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

[martin rudalics]

 >> So before making this function public, we should resolve this calling
 >> convention.
 >
 > Makes sense.  Then, in my view, the logical sequence to fix this bug is
 >
 > A. First do these changes to window.el and publish a decent
 >     window-display-buffer calling convention.
 >
 > B. Push a xref.el based on the new function that doesn't change
 > the xref UI.
 >
 > C. Discuss the xref.el UI in the other bug.

I can only comment on A and even there I have to leave the judgment to
Stefan Monnier as he's our only expert on window dedication and how
'display-buffer' is supposed to handle it.

But I can offer a preambulatory piece of code we could splice into the
function in order to do away with the DEDICATED argument.  Untested!

     ...
     (unless (eq buffer (window-buffer window))
       (set-window-dedicated-p window nil)
       (set-window-buffer window buffer))
     (let ((alist-dedicated (assq 'dedicated alist)))
       (cond
        (alist-dedicated
         (set-window-dedicated-p window (cdr alist-dedicated)))
        ((and (not (eq type 'reuse)) display-buffer-mark-dedicated)
         (set-window-dedicated-p window display-buffer-mark-dedicated))))
     (when (memq type '(window frame))
       (set-window-prev-buffers window nil))
     ...

 > Do both of these more ambitious refactorings really need to
 > make it in before we can do B as outlined above? Or can we
 > do them later in parallel?

I recommend to do these before making that function public.  I don't
understand B and C sufficiently.

martin

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

[João Távora]

On Tue, Jan 8, 2019 at 2:47 PM martin rudalics <rudalics@gmx.at> wrote:

> I recommend to do these before making that function public.  I don't
> understand B and C sufficiently.

Then we can go ahead and do B and C without A, but we would probably
have to tweak B/C later (which is something we have to do already
for many other uses of `window--display-buffer`).

João



-- 
João Távora

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

[Stefan Monnier]

> And we should specify once and for all whether a window can remain or
> become dedicated when our function displays another buffer in it.

The intention behind display-buffer-mark-dedicated is to have it set to
`soft` which in turn means that the window will be undedicated whenever
the user explicitly asks to display another buffer in it (typically via
switch-to-buffer).

IOW the result is windows are dedicated as long as they have only ever
displayed a single buffer.


        Stefan

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

[martin rudalics]

 > The intention behind display-buffer-mark-dedicated is to have it set to
 > `soft` which in turn means that the window will be undedicated whenever
 > the user explicitly asks to display another buffer in it (typically via
 > switch-to-buffer).

Who is supposed to set this and when?  Why don't we provide a normal
action alist entry for this?

 > IOW the result is windows are dedicated as long as they have only ever
 > displayed a single buffer.

You mean one and the same buffer all the time?  Please leave a note
somewhere what the precise intended behavior is.  So far, designers of
buffer display action functions can only guess.

martin

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

[Stefan Monnier]

>> The intention behind display-buffer-mark-dedicated is to have it set to
>> `soft` which in turn means that the window will be undedicated whenever
>> the user explicitly asks to display another buffer in it (typically via
>> switch-to-buffer).
> Who is supposed to set this and when?

The user in ~/.emacs

> Why don't we provide a normal action alist entry for this?

IIRC it was introduced before the new display-buffer-alist and its
action alists.  Also it's a global variable because I needed it to apply
to "normal" buffers (rather than those matched by
special-display-regexps), so it needs to go to
display-buffer-base-action which had no equivalent back then.

>> IOW the result is windows are dedicated as long as they have only ever
>> displayed a single buffer.
> You mean one and the same buffer all the time?

Of course: when set to `soft`, it's marked as `soft-dedicated` when
created, so as soon as some other buffer is displayed, the dedication
is removed.

> Please leave a note somewhere what the precise intended behavior is.
> So far, designers of buffer display action functions can only guess.

I'm not sure what to say there.  The intended behavior is just as it is
described: to mark windows as dedicated when they're created.


        Stefan

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

[martin rudalics]

 > The user in ~/.emacs

Then this should be an option instead of a plain variable.

 >> Why don't we provide a normal action alist entry for this?
 >
 > IIRC it was introduced before the new display-buffer-alist and its
 > action alists.  Also it's a global variable because I needed it to apply
 > to "normal" buffers (rather than those matched by
 > special-display-regexps), so it needs to go to
 > display-buffer-base-action which had no equivalent back then.

We have 'display-buffer-alist' for quite some time now.  So please
consider making this an action alist entry.  That way a user can
decide whether all buffers displayed by 'display-buffer' should be
dedicated or only certain ones and which 'dedicated' value they should
get.

 > I'm not sure what to say there.  The intended behavior is just as it is
 > described: to mark windows as dedicated when they're created.

OK.  I think we can get rid of the DEDICATED argument then.

Thanks, martin

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

[Stefan Monnier]

> We have 'display-buffer-alist' for quite some time now.
> So please consider making this an action alist entry.

Yes, it would be much better, but it never seems to reach the top of my
todo list.

> That way a user can decide whether all buffers displayed by
> 'display-buffer' should be dedicated or only certain ones and which
> 'dedicated' value they should get.

Historically, special-display-buffer-alist always caused
the created frames/windows to be dedicated, so
display-buffer-mark-dedicated extends this to those windows created for
other reasons.

I haven't looked in detail, but this seems to make it less trivial to
just add a new action alist parameter: it should default to `t` if we
matched in display-buffer-alist but to nil if we only rely on
display-buffer-base-action?

Also, some (all?) let-bindings of display-buffer-mark-dedicated should
now be unnecessary (because of the features you added so bury-buffer (or
was it quit-window?) automatically deletes the window).


        Stefan

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

[martin rudalics]

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

 >> We have 'display-buffer-alist' for quite some time now.
 >> So please consider making this an action alist entry.
 >
 > Yes, it would be much better, but it never seems to reach the top of my
 > todo list.
 >
 >> That way a user can decide whether all buffers displayed by
 >> 'display-buffer' should be dedicated or only certain ones and which
 >> 'dedicated' value they should get.
 >
 > Historically, special-display-buffer-alist always caused
 > the created frames/windows to be dedicated, so
 > display-buffer-mark-dedicated extends this to those windows created for
 > other reasons.
 >
 > I haven't looked in detail, but this seems to make it less trivial to
 > just add a new action alist parameter: it should default to `t` if we
 > matched in display-buffer-alist but to nil if we only rely on
 > display-buffer-base-action?

I'm missing you here.  An ALIST argument is equally passed to all
buffer display actions regardless of whether they are specifed by
'display-buffer-base-action' or by someone else.  It's their choice
whether they want to obey or disregard it.  The same currently holds
for 'display-buffer-mark-dedicated'.

 > Also, some (all?) let-bindings of display-buffer-mark-dedicated should

I don't see any such bindings in our current code base.

 > now be unnecessary (because of the features you added so bury-buffer (or
 > was it quit-window?) automatically deletes the window).

This use case of dedicated windows should be no more necessary indeed.

I attach a patch of my proposed changes.  After applying that I have
no more objections against renaming 'window--display-buffer' any way
people want.

martin

[-- Attachment #2: window--display-buffer.diff --]
[-- Type: text/plain, Size: 11957 bytes --]

diff --git a/lisp/window.el b/lisp/window.el
index 37d82c0..e53cc2b 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -700,8 +700,7 @@ display-buffer-in-atom-window
 	(set-window-parameter window 'window-atom 'main))
       (set-window-parameter new 'window-atom side)
       ;; Display BUFFER in NEW and return NEW.
-      (window--display-buffer
-       buffer new 'window alist display-buffer-mark-dedicated))))
+      (window--display-buffer buffer new 'window alist))))
 
 (defun window--atom-check-1 (window)
   "Subroutine of `window--atom-check'."
@@ -958,7 +957,11 @@ window--make-major-side-window
 	 ;; window and not make a new parent window unless needed.
 	 (window-combination-resize 'side)
 	 (window-combination-limit nil)
-	 (window (split-window-no-error next-to nil on-side)))
+	 (window (split-window-no-error next-to nil on-side))
+         (alist (if (or display-buffer-mark-dedicated
+                        (assq 'dedicated alist))
+                    alist
+                  (cons '(dedicated . side) alist))))
     (when window
       ;; Initialize `window-side' parameter of new window to SIDE and
       ;; make that parameter persistent.
@@ -985,7 +988,7 @@ window--make-major-side-window
       (with-current-buffer buffer
         (setq window--sides-shown t))
       ;; Install BUFFER in new window and return WINDOW.
-      (window--display-buffer buffer window 'window alist 'side))))
+      (window--display-buffer buffer window 'window alist))))
 
 (defun display-buffer-in-side-window (buffer alist)
   "Display BUFFER in a side window of the selected frame.
@@ -1019,10 +1022,7 @@ display-buffer-in-side-window
 explicitly provided via a `window-parameters' entry in ALIST."
   (let* ((side (or (cdr (assq 'side alist)) 'bottom))
          (slot (or (cdr (assq 'slot alist)) 0))
-         (left-or-right (memq side '(left right)))
-         ;; Softly dedicate window to BUFFER unless
-         ;; `display-buffer-mark-dedicated' already asks for it.
-         (dedicated (or display-buffer-mark-dedicated 'side)))
+         (left-or-right (memq side '(left right))))
     (cond
      ((not (memq side '(top bottom left right)))
       (error "Invalid side %s specified" side))
@@ -1055,7 +1055,11 @@ display-buffer-in-side-window
 		  ((eq side 'bottom) 3))
 		 window-sides-slots))
            (window--sides-inhibit-check t)
-	   window this-window this-slot prev-window next-window
+           (alist (if (or display-buffer-mark-dedicated
+                          (assq 'dedicated alist))
+                      alist
+                    (cons '(dedicated . side) alist)))
+           window this-window this-slot prev-window next-window
 	   best-window best-slot abs-slot)
 
       (cond
@@ -1113,8 +1117,7 @@ display-buffer-in-side-window
 		 ;; Reuse `this-window'.
                  (with-current-buffer buffer
                    (setq window--sides-shown t))
-		 (window--display-buffer
-                  buffer this-window 'reuse alist dedicated))
+		 (window--display-buffer buffer this-window 'reuse alist))
 	    (and (or (not max-slots) (< slots max-slots))
 		 (or (and next-window
 			  ;; Make new window before `next-window'.
@@ -1131,8 +1134,7 @@ display-buffer-in-side-window
 		   (set-window-parameter window 'window-slot slot)
                    (with-current-buffer buffer
                      (setq window--sides-shown t))
-		   (window--display-buffer
-                    buffer window 'window alist dedicated))
+		   (window--display-buffer buffer window 'window alist))
 	    (and best-window
 		 ;; Reuse `best-window'.
 		 (progn
@@ -1141,7 +1143,7 @@ display-buffer-in-side-window
                    (with-current-buffer buffer
                      (setq window--sides-shown t))
                    (window--display-buffer
-		    buffer best-window 'reuse alist dedicated)))))))))
+                    buffer best-window 'reuse alist)))))))))
 
 (defun window-toggle-side-windows (&optional frame)
   "Toggle display of side windows on specified FRAME.
@@ -6748,20 +6750,47 @@ window--even-window-sizes
            (/ (- (window-total-height window) (window-total-height)) 2))
         (error nil))))))
 
-(defun window--display-buffer (buffer window type &optional alist dedicated)
+(defun window--display-buffer (buffer window type &optional alist)
   "Display BUFFER in WINDOW.
-TYPE must be one of the symbols `reuse', `window' or `frame' and
-is passed unaltered to `display-buffer-record-window'.  ALIST is
-the alist argument of `display-buffer'.  Set `window-dedicated-p'
-to DEDICATED if non-nil.  Return WINDOW if BUFFER and WINDOW are
-live."
+TYPE must be one of the following symbols: 'reuse' (which means
+WINDOW existed before the call of `display-buffer' and may
+already show BUFFER or not), 'window' (WINDOW was created on an
+existing frame) or 'frame' (WINDOW was created on a new frame)
+and is passed unaltered to `display-buffer-record-window'.  ALIST
+is the action alist compiled by `display-buffer'.
+
+Handle WINDOW's dedicated flag as follows: If WINDOW already
+shows BUFFER, leave it alone.  Otherwise, if ALIST contains a
+'dedicated' entry and the window is either new or the cdr of that
+entry equals 'side', set WINDOW's dedicated flag to the cdr of
+that entry.  Otherwise, if 'display-buffer-mark-dedicated' is
+non-nil and TYPE equals 'window' of 'frame', set WINDOW's
+dedicated flag to the value of 'display-buffer-mark-dedicated'.
+In any other case, reset WINDOW's dedicated flag to nil.
+
+Return WINDOW if BUFFER and WINDOW are live."
+  (setq display-buffer--type type)
   (when (and (buffer-live-p buffer) (window-live-p window))
     (display-buffer-record-window type window buffer)
     (unless (eq buffer (window-buffer window))
+      ;; Reset WINDOW's dedicated status unless it already shows
+      ;; BUFFER.
       (set-window-dedicated-p window nil)
       (set-window-buffer window buffer))
-    (when dedicated
-      (set-window-dedicated-p window dedicated))
+    (let ((alist-dedicated (assq 'dedicated alist)))
+      ;; Maybe dedicate WINDOW to BUFFER if asked for.
+      (cond
+       ;; Don't dedicate WINDOW if it is dedicated because it shows
+       ;; BUFFER already or it is reused and is not a side window.
+       ((or (window-dedicated-p window)
+            (and (eq type 'reuse) (not (eq alist-dedicated 'side)))))
+       ;; Otherwise, if ALIST contains a 'dedicated' entry, use that.
+       (alist-dedicated
+        (set-window-dedicated-p window (cdr alist-dedicated)))
+       ;; Otherwise, if 'display-buffer-mark-dedicated' is non-nil,
+       ;; use that.
+       ((and display-buffer-mark-dedicated (memq type '(window frame)))
+        (set-window-dedicated-p window display-buffer-mark-dedicated))))
     (when (memq type '(window frame))
       (set-window-prev-buffers window nil))
     (let ((quit-restore (window-parameter window 'quit-restore))
@@ -7190,8 +7219,7 @@ display-buffer-use-some-frame
                 frame nil (cdr (assq 'inhibit-same-window alist))))))
     (when window
       (prog1
-          (window--display-buffer
-           buffer window 'reuse alist display-buffer-mark-dedicated)
+          (window--display-buffer buffer window 'reuse alist)
         (unless (cdr (assq 'inhibit-switch-frame alist))
           (window--maybe-raise-frame frame))))))
 
@@ -7356,8 +7384,7 @@ display-buffer-pop-up-frame
 	       (with-current-buffer buffer
 		 (setq frame (funcall fun)))
 	       (setq window (frame-selected-window frame)))
-      (prog1 (window--display-buffer
-	      buffer window 'frame alist display-buffer-mark-dedicated)
+      (prog1 (window--display-buffer buffer window 'frame alist)
 	(unless (cdr (assq 'inhibit-switch-frame alist))
 	  (window--maybe-raise-frame frame))))))
 
@@ -7386,8 +7413,7 @@ display-buffer-pop-up-window
 				(window--try-to-split-window
 				 (get-lru-window frame t) alist))))
 
-      (prog1 (window--display-buffer
-	      buffer window 'window alist display-buffer-mark-dedicated)
+      (prog1 (window--display-buffer buffer window 'window alist)
 	(unless (cdr (assq 'inhibit-switch-frame alist))
 	  (window--maybe-raise-frame (window-frame window)))))))
 
@@ -7435,7 +7461,7 @@ display-buffer-in-child-frame
 	 (parent (or (assq 'parent-frame parameters)
                      (selected-frame)))
          (share (assq 'share-child-frame parameters))
-         share1 frame window)
+         share1 frame window type)
     (with-current-buffer buffer
       (when (frame-live-p parent)
         (catch 'frame
@@ -7448,12 +7474,14 @@ display-buffer-in-child-frame
                 (throw 'frame t))))))
 
       (if frame
-          (setq window (frame-selected-window frame))
+          (progn
+            (setq window (frame-selected-window frame))
+            (setq type 'reuse))
         (setq frame (make-frame parameters))
-        (setq window (frame-selected-window frame))))
+        (setq window (frame-selected-window frame))
+        (setq type 'frame)))
 
-    (prog1 (window--display-buffer
-	    buffer window 'frame alist display-buffer-mark-dedicated)
+    (prog1 (window--display-buffer buffer window type alist)
       (unless (cdr (assq 'inhibit-switch-frame alist))
 	(window--maybe-raise-frame frame)))))
 
@@ -7492,16 +7520,14 @@ display-buffer-below-selected
 		   split-width-threshold)
                (setq window (window--try-to-split-window
                              (selected-window) alist)))
-             (window--display-buffer
-              buffer window 'window alist display-buffer-mark-dedicated))
+             (window--display-buffer buffer window 'window alist))
 	(and (setq window (window-in-direction 'below))
              (not (window-dedicated-p window))
 	     (or (not (numberp min-height))
 		 ;; A window that showed another buffer before cannot
 		 ;; be resized.
 		 (>= (window-height window) min-height))
-	     (window--display-buffer
-              buffer window 'reuse alist display-buffer-mark-dedicated)))))
+	     (window--display-buffer buffer window 'reuse alist)))))
 
 (defun display-buffer--maybe-at-bottom (buffer alist)
   (let ((alist (append alist `(,(if temp-buffer-resize-mode
@@ -7533,21 +7559,17 @@ display-buffer-at-bottom
 	 (setq bottom-window window))))
      nil nil 'nomini)
     (or (and bottom-window-shows-buffer
-	     (window--display-buffer
-	      buffer bottom-window 'reuse alist display-buffer-mark-dedicated))
+	     (window--display-buffer buffer bottom-window 'reuse alist))
 	(and (not (frame-parameter nil 'unsplittable))
-	     (let (split-width-threshold)
+	     (let (split-height-threshold)
 	       (setq window (window--try-to-split-window bottom-window alist)))
-	     (window--display-buffer
-	      buffer window 'window alist display-buffer-mark-dedicated))
+	     (window--display-buffer buffer window 'window alist))
 	(and (not (frame-parameter nil 'unsplittable))
 	     (setq window (split-window-no-error (window-main-window)))
-	     (window--display-buffer
-	      buffer window 'window alist display-buffer-mark-dedicated))
+	     (window--display-buffer buffer window 'window alist))
 	(and (setq window bottom-window)
 	     (not (window-dedicated-p window))
-	     (window--display-buffer
-	      buffer window 'reuse alist display-buffer-mark-dedicated)))))
+	     (window--display-buffer buffer window 'reuse alist)))))
 
 (defun display-buffer-in-previous-window (buffer alist)
   "Display BUFFER in a window previously showing it.
@@ -7596,7 +7618,8 @@ display-buffer-in-previous-window
     ;; anything we found so far.
     (when (and (setq window (cdr (assq 'previous-window alist)))
 	       (window-live-p window)
-	       (not (window-dedicated-p window)))
+	       (or (eq buffer (window-buffer window))
+                   (not (window-dedicated-p window))))
       (if (eq window (selected-window))
 	  (unless inhibit-same-window
 	    (setq second-best-window window))

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

[Stefan Monnier]

>> I haven't looked in detail, but this seems to make it less trivial to
>> just add a new action alist parameter: it should default to `t` if we
>> matched in display-buffer-alist but to nil if we only rely on
>> display-buffer-base-action?
> I'm missing you here.  An ALIST argument is equally passed to all
> buffer display actions regardless of whether they are specifed by
> 'display-buffer-base-action' or by someone else.  It's their choice
> whether they want to obey or disregard it.  The same currently holds
> for 'display-buffer-mark-dedicated'.

Never mind, I was confused.

>> Also, some (all?) let-bindings of display-buffer-mark-dedicated should
> I don't see any such bindings in our current code base.

    lisp/dired.el:    (display-buffer-mark-dedicated 'soft))
    lisp/epa.el:      (let ((display-buffer-mark-dedicated 'soft))
    lisp/minibuffer.el:             (display-buffer-mark-dedicated 'soft))

> I attach a patch of my proposed changes.  After applying that I have
> no more objections against renaming 'window--display-buffer' any way
> people want.

LGTM.  See some comment/question below.


        Stefan


> @@ -958,7 +957,11 @@ window--make-major-side-window
>  	 ;; window and not make a new parent window unless needed.
>  	 (window-combination-resize 'side)
>  	 (window-combination-limit nil)
> -	 (window (split-window-no-error next-to nil on-side)))
> +	 (window (split-window-no-error next-to nil on-side))
> +         (alist (if (or display-buffer-mark-dedicated
> +                        (assq 'dedicated alist))
> +                    alist
> +                  (cons '(dedicated . side) alist))))

Hmm... the old code used (or display-buffer-mark-dedicated 'side),
so when display-buffer-mark-dedicated is non-nil but (assq 'dedicated
alist) is nil, I think we need to use (cons `(dedicated
. ,display-buffer-mark-dedicated) alist), no?
Or rather:

         (alist (if (assq 'dedicated alist)
                    alist
                  (cons `(dedicated . ,(or display-buffer-mark-dedicated 'side))
                        alist))))

WDYT?

BTW, this code reappears a second time in your patch, but I haven't
checked if the same reasoning applies there.

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

[martin rudalics]

 > Or rather:
 >
 >           (alist (if (assq 'dedicated alist)
 >                      alist
 >                    (cons `(dedicated . ,(or display-buffer-mark-dedicated 'side))
 >                          alist))))
 >
 > WDYT?

I think you're right.  Unless you see any problems I'll use that.

 > BTW, this code reappears a second time in your patch, but I haven't
 > checked if the same reasoning applies there.

I'll use your code there too.

Thanks, martin

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

[martin rudalics]

 > I think you're right.  Unless you see any problems I'll use that.
 >
 >  > BTW, this code reappears a second time in your patch, but I haven't
 >  > checked if the same reasoning applies there.
 >
 > I'll use your code there too.

Now installed on master, please check.

Anyone interested in renaming 'window--display-buffer' - please go
ahead.  I think a 'display-buffer-' prefix should then be appropriate.

martin

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

[Juri Linkov]

>> Of course, it doesn't work if you tried it only with part of my changes.
>> When I submitted my initial patch, I tested it in all your test cases,
>> including the above test case that was not broken with my patch.
>
> You are correct.  I was testing under the assumption that making
> xref-goto-xref configurable didn't require the "tiny window" for
> xref-find-definitions, which is something you stated that you wanted to
> do for other xref.el commands like xref-find-references.

Actually the xref window is not tiny at all if there are more results.
It doesn't takes more space than needed, therefore there is no wasted space.

>> My initial patch solved this problem gracefully by creating a new window
>> for the xref buffer.
>
> You may well call this a problem, but it's not a bug.  It's the defined
> behaviour, it's like this by design.  We are trying to create the
> conditions that would enable you, or any other user, to create
> alternative ways to present *xref* that have other advantages and
> drawbacks.

Let me reiterate the problem that prompted this report: please imagine
a situation that you have two horizontally split windows with visited files
in each of them, and you happily browse xref definitions in the same window
using M-.

Then suddenly M-. replaces other half of the screen with empty space with
only 2 lines at the top.  This is because there is an ambiguity in finding
definitions, and you need to resolve it.  Then you start trying to reuse some
empty space it creates and trying to split the xref window.  Instead of
this, the split is applied to the original window.  As a result, you have
the original window split, and still half of the screen wasted with empty
space.  And most of all this mess is caused unexpectedly, i.e. you don't
expect the xref window to break your windows when you type M-.

Do you agree that we should respect user configuration, and use
another window only when the user asks for it?  If yes, then a good way
to resolve this problem is to use a part of the original window to display
ambiguous results.  This will keep the original window configuration.

Now the question is what to do when the user asks to display
a definition in another window using ‘C-x 4 .’
(xref-find-definitions-other-window).  The most natural way is to
immediately take the window pointed out by the user configuration
(the user can configure to display it below/above/left/right etc.)
and display the xref window in that window.  Then visiting a definition
still will remain in the same window preferred by the user.

The same logic could also apply to xref-find-definitions-other-frame.

This will allow xref-goto-xref to be configurable.

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

[João Távora]

On Wed, Jan 9, 2019 at 12:29 AM Juri Linkov <juri@linkov.net> wrote:

> This will allow xref-goto-xref to be configurable.

Of course it will.  It will also irrevocably change the current UI,
which might not be what other people want.  So I am proposing another
approach that makes xref-goto-xref configurable too, but *doesn't*
change the default UI, i.e. the UI when the user chooses *not* to
configure anything.

João

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

[Dmitry Gutov]

On 09.01.2019 03:20, Juri Linkov wrote:
> Then suddenly M-. replaces other half of the screen with empty space with
> only 2 lines at the top.  This is because there is an ambiguity in finding
> definitions, and you need to resolve it.  Then you start trying to reuse some
> empty space it creates and trying to split the xref window.  Instead of
> this, the split is applied to the original window.

Could you write down the commands to get there? I failed to reproduce this.

> Now the question is what to do when the user asks to display
> a definition in another window using ‘C-x 4 .’
> (xref-find-definitions-other-window).  The most natural way is to
> immediately take the window pointed out by the user configuration
> (the user can configure to display it below/above/left/right etc.)
> and display the xref window in that window.

I'm not sure if it's the "most natural" way. "A natural" maybe.

> Then visiting a definition
> still will remain in the same window preferred by the user.
> 
> The same logic could also apply to xref-find-definitions-other-frame.
> 
> This will allow xref-goto-xref to be configurable.

The current behavior seems to work okay for me. So the meaning of 
"configurable" I'm expecting here would allow the user to retain the 
current behavior if they want. We can discuss the best default afterwards.

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

[Juri Linkov]

>> Then suddenly M-. replaces other half of the screen with empty space with
>> only 2 lines at the top.  This is because there is an ambiguity in finding
>> definitions, and you need to resolve it.  Then you start trying to reuse some
>> empty space it creates and trying to split the xref window.  Instead of
>> this, the split is applied to the original window.
>
> Could you write down the commands to get there? I failed to reproduce this.

Any command that relies on configuration in display-buffer-alist
or display-buffer-overriding-action such as windmove-display-in-direction.

>> Now the question is what to do when the user asks to display
>> a definition in another window using ‘C-x 4 .’
>> (xref-find-definitions-other-window).  The most natural way is to
>> immediately take the window pointed out by the user configuration
>> (the user can configure to display it below/above/left/right etc.)
>> and display the xref window in that window.
>
> I'm not sure if it's the "most natural" way. "A natural" maybe.

At least, the current behavior can't be described as "natural".
For example, if the user prefers using frames and types `C-x 5 .'
the xref buffer is displayed in another WINDOW, not FRAME.

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

[João Távora]

On Sun, Jan 13, 2019 at 3:04 AM Juri Linkov <juri@linkov.net> wrote:

> > I'm not sure if it's the "most natural" way. "A natural" maybe.
> At least, the current behavior can't be described as "natural".
> For example, if the user prefers using frames and types `C-x 5 .'
> the xref buffer is displayed in another WINDOW, not FRAME.

As you very well know by now, the "other frame" there refers to the
buffer that eventually displays the cross-reference, which
very often doesn't require the *xref* itself, and _not_ *xref* buffer
itself.

Look, I get it that you dislike the current interface very, very
much and would like to change it.  As I have repeatedly asked,
do you  understand that a viable path to do that might be:

1. Make the current interface configurable
2. Present a number of configurations for xref to work
with and how to select them.
3. Choose the "most natural" one to be the default
(this is up for debate, sorry, but other people have opinions,
too)

?

Let's just work on number 1 and 2 here *before* we go
to number 3. If you press on starting with 3, you make
me unhappy, because I don't know how I can get back
the current configuration if I later decide I don't like your
system.  Arguing that it's "natural" won't do it for me, an UI
is too subjective a thing.

-- 
João Távora

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

[Juri Linkov]

>> At least, the current behavior can't be described as "natural".
>> For example, if the user prefers using frames and types `C-x 5 .'
>> the xref buffer is displayed in another WINDOW, not FRAME.
>
> As you very well know by now, the "other frame" there refers to the
> buffer that eventually displays the cross-reference, which
> very often doesn't require the *xref* itself, and _not_ *xref* buffer
> itself.
>
> Look, I get it that you dislike the current interface very, very
> much and would like to change it.  As I have repeatedly asked,
> do you  understand that a viable path to do that might be:

I don't dislike the current interface, thanks for working on what it
does well.  But please don't assume that the current UI is so perfect,
there is no way to make it better.  There are some details that cause
minor annoyances (so minor that you won't get many reports for them,
e.g. when the xref pops up in a wrong window, it's easy to fix manually).

> 1. Make the current interface configurable
> 2. Present a number of configurations for xref to work
> with and how to select them.

Of course, it should be configurable, I completely agree,
this is the whole point of this report.

> 3. Choose the "most natural" one to be the default
> (this is up for debate, sorry, but other people have opinions,
> too)

Or course, this should be discussed, this is what I do all the time:
for example, when recently Dmitry filed a complaint about the next-error
framework, I happily cooperated to resolve all disagreements and other
controversies and implemented fixes for what we have discussed to make
the next-error framework more configurable and change previous defaults
based on reached consensus.

So far we have only 3 opinions in this discussion.  One way to advance is
to ask what others think about this.  Since you have already heard my opinion,
at this moment I have nothing more to say.

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

[João Távora]

Juri Linkov <juri@linkov.net> writes:

> I don't dislike the current interface, thanks for working on what it
> does well.  But please don't assume that the current UI is so perfect,
> there is no way to make it better.

Juri, if I did assume that, as you suggest, why would I be aggreeing to
make it configurable?

>> 1. Make the current interface configurable
>> 2. Present a number of configurations for xref to work
>> with and how to select them.
>
> Of course, it should be configurable, I completely agree,
> this is the whole point of this report.

Then how about reviewing my patch?  It makes this configurable, and
doesn't change the default configuration.

> at this moment I have nothing more to say.

Then if noone objects I'll push the patch I presented earlier in pa few
days.  It makes xref-goto-xref configurable, doesn't change the default
configuration, and closes this bug.

João

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

[Dmitry Gutov]

On 14.01.2019 02:06, João Távora wrote:
> Then if noone objects I'll push the patch I presented earlier in pa few
> days.

Wasn't the part where it renames window--display-buffer still under 
debate? I think using the private (current) version of it would be better.

Or we could wait until the related subthread comes to a conclusion.

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

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 14.01.2019 02:06, João Távora wrote:
>> Then if noone objects I'll push the patch I presented earlier in pa few
>> days.
>
> Wasn't the part where it renames window--display-buffer still under
> debate? I think using the private (current) version of it would be
> better.

I'm OK with that, I'll add a FIXME and then I guess we can change it
when we change the other non-window.el users, like windmove.el.

> Or we could wait until the related subthread comes to a conclusion.

I see the prototype of window--display-buffer has changed recently, and
so has it docstring.  I assumed that was it for the cleanup Martin
requested, but indeed I could be wrong, as I didn't follow that
subthread closely (has it died down or I am just not Cc: anymore?).

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

[martin rudalics]

 > I see the prototype of window--display-buffer has changed recently, and
 > so has it docstring.  I assumed that was it for the cleanup Martin
 > requested, but indeed I could be wrong, as I didn't follow that
 > subthread closely (has it died down or I am just not Cc: anymore?).

The cleanup has been done.  The final message of that subthread says:

   Anyone interested in renaming 'window--display-buffer' - please go
   ahead.  I think a 'display-buffer-' prefix should then be appropriate.

What I meant there is that calling it say 'display-buffer-in-window'
would be appropriate but the final name is up to its clients.

martin

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

[João Távora]

martin rudalics <rudalics@gmx.at> writes:

>> I see the prototype of window--display-buffer has changed recently, and
>> so has it docstring.  I assumed that was it for the cleanup Martin
>> requested, but indeed I could be wrong, as I didn't follow that
>> subthread closely (has it died down or I am just not Cc: anymore?).
>
> The cleanup has been done.  The final message of that subthread says:
>
>   Anyone interested in renaming 'window--display-buffer' - please go
>   ahead.  I think a 'display-buffer-' prefix should then be appropriate.
>
> What I meant there is that calling it say 'display-buffer-in-window'
> would be appropriate but the final name is up to its clients.

Actually seems like a really good name.  If I make a function alias,
code in window.el can still use the old name.  Do you prefer that, or
should I replace all uses that I can find in Emacs?

Actually2, we should keep the old name.  A code search on github.com
reveals that other packages/customizations are using the internal
function, too (which now takes one less arg, but if they break for that,
they were really asking for it).

João

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

[Juri Linkov]

>>> I see the prototype of window--display-buffer has changed recently, and
>>> so has it docstring.  I assumed that was it for the cleanup Martin
>>> requested, but indeed I could be wrong, as I didn't follow that
>>> subthread closely (has it died down or I am just not Cc: anymore?).
>>
>> The cleanup has been done.  The final message of that subthread says:
>>
>>   Anyone interested in renaming 'window--display-buffer' - please go
>>   ahead.  I think a 'display-buffer-' prefix should then be appropriate.
>>
>> What I meant there is that calling it say 'display-buffer-in-window'
>> would be appropriate but the final name is up to its clients.
>
> Actually seems like a really good name.  If I make a function alias,
> code in window.el can still use the old name.  Do you prefer that, or
> should I replace all uses that I can find in Emacs?

I agree that 'display-buffer-in-window' is a good name.

> Actually2, we should keep the old name.  A code search on github.com
> reveals that other packages/customizations are using the internal
> function, too (which now takes one less arg, but if they break for that,
> they were really asking for it).

Then better to keep the old name with its old signature and
declare it obsolete and replace its body with the call to the
function with the new name and one less arg.

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

[martin rudalics]

 > Then better to keep the old name with its old signature and
 > declare it obsolete and replace its body with the call to the
 > function with the new name and one less arg.

Agreed.

martin

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

[Juri Linkov]

>>> 1. Make the current interface configurable
>>> 2. Present a number of configurations for xref to work
>>> with and how to select them.
>>
>> Of course, it should be configurable, I completely agree,
>> this is the whole point of this report.
>
> Then how about reviewing my patch?  It makes this configurable, and
> doesn't change the default configuration.
>
>> at this moment I have nothing more to say.
>
> Then if noone objects I'll push the patch I presented earlier in pa few
> days.  It makes xref-goto-xref configurable, doesn't change the default
> configuration, and closes this bug.

Making xref-goto-xref configurable is really important, this is the
raison d'être of this bug.  I wouldn't complain about configurable things.
For example, I'm not complaining that diff-hl-mode uses non-standard
blue color for changes whereas the standard diff-mode color is yellow.
Also I'm not complaining that flymake steals the fringe indicator from
diff-hl-mode, although there is no conflict because diff-hl-mode
uses the background color whereas flymake uses the foreground color,
so they can peacefully coexist together on the fringe.
Also I'm not complaining that often in ruby-mode C-M-f navigates to
unexpected places, however it's easy to fall back to M-f for more
predictable navigation, and so on.  But in case of xref-goto-xref
nothing helps.  This is why is this bug report.

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

[Dmitry Gutov]

This is offtopic, but:

On 19.01.2019 23:31, Juri Linkov wrote:
> For example, I'm not complaining that diff-hl-mode uses non-standard
> blue color for changes whereas the standard diff-mode color is yellow.

The standard diff-mode color for "change" is "none" (see the 
diff-changed), so I looked at what most other editors use (blue). I've 
only noticed the yellow in diff-refine-changed very recently, and I've 
yet to see it in practice. When is this face used exactly?

> Also I'm not complaining that flymake steals the fringe indicator from
> diff-hl-mode, although there is no conflict because diff-hl-mode
> uses the background color whereas flymake uses the foreground color,
> so they can peacefully coexist together on the fringe.

If only fringes supported that kind of merging. BTW, diff-hl-mode uses 
the foreground color for the border, but any kind of merging would be an 
improvement.

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

[Juri Linkov]

>> For example, I'm not complaining that diff-hl-mode uses non-standard
>> blue color for changes whereas the standard diff-mode color is yellow.
>
> The standard diff-mode color for "change" is "none" (see the diff-changed),
> so I looked at what most other editors use (blue). I've only noticed the
> yellow in diff-refine-changed very recently, and I've yet to see it in
> practice. When is this face used exactly?

It's not used anymore but it was yellow some time ago.  BTW, I still
can't get used to diff-removed yellow and diff-added blue
diff colors on Wikipedia, and they are not configurable.

>> Also I'm not complaining that flymake steals the fringe indicator from
>> diff-hl-mode, although there is no conflict because diff-hl-mode
>> uses the background color whereas flymake uses the foreground color,
>> so they can peacefully coexist together on the fringe.
>
> If only fringes supported that kind of merging. BTW, diff-hl-mode uses the
> foreground color for the border, but any kind of merging would be
> an improvement.

I thought that since add-face-text-property supports face merging
maybe it would be possible to do the same with fringes,
but I have not investigated how feasible it is.

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

[Juri Linkov]

>>> Also I'm not complaining that flymake steals the fringe indicator from
>>> diff-hl-mode, although there is no conflict because diff-hl-mode
>>> uses the background color whereas flymake uses the foreground color,
>>> so they can peacefully coexist together on the fringe.
>>
>> If only fringes supported that kind of merging. BTW, diff-hl-mode uses the
>> foreground color for the border, but any kind of merging would be
>> an improvement.
>
> I thought that since add-face-text-property supports face merging
> maybe it would be possible to do the same with fringes,
> but I have not investigated how feasible it is.

Also the question which one to call on clicking the fringe indicator?
It seems for flymake there is no reasonable action to call on click, so
diff-hl-mode is free to use mouse clicks to show the corresponding diff.

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

[Dmitry Gutov]

On 21.01.2019 23:43, Juri Linkov wrote:

> Also the question which one to call on clicking the fringe indicator?
> It seems for flymake there is no reasonable action to call on click, so
> diff-hl-mode is free to use mouse clicks to show the corresponding diff.

*shrug* Somebody is welcome to implement that and submit a patch. IMO, 
'C-x v =' is easier.

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

[Dmitry Gutov]

On 14.01.2019 00:54, Juri Linkov wrote:
> I don't dislike the current interface, thanks for working on what it
> does well.  But please don't assume that the current UI is so perfect,
> there is no way to make it better.

I wouldn't say it's perfect either, it's still kind of idiosyncratic. 
Not sure your patch will fix that problem, though, instead of just 
swinging it the other way.

We basically have two use cases:

* Jump to this symbol, in this/that window/frame. 
windmove-display-in-direction should probably affect where the target 
buffer ends up, irrespective of whether we have to pop up an *xref* 
buffer to resolve any duplicate matches.

* Show a list of search results. Arguably, in this case 
windmove-display-in-direction should affect where the *xref* buffer is 
displayed.

Neither of y'all's patches solve this, I believe.

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

[João Távora]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 14.01.2019 00:54, Juri Linkov wrote:
>> I don't dislike the current interface, thanks for working on what it
>> does well.  But please don't assume that the current UI is so perfect,
>> there is no way to make it better.
>
> I wouldn't say it's perfect either, it's still kind of
> idiosyncratic. Not sure your patch will fix that problem, though,
> instead of just swinging it the other way.

I think this bug's raison d'être is that everybody gets to swing it they
way they like it swung.

João

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

[Dmitry Gutov]

On 18.01.2019 18:22, João Távora wrote:
> I think this bug's raison d'être is that everybody gets to swing it they
> way they like it swung.

IIUC, supporting buffer display customization via display-buffer-alist 
etc won't be enough.

The question is also when to use it, which buffer to apply it to.

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

[João Távora]

On Fri, Jan 18, 2019 at 3:35 PM Dmitry Gutov <dgutov@yandex.ru> wrote:
>
> On 18.01.2019 18:22, João Távora wrote:
> > I think this bug's raison d'être is that everybody gets to swing it they
> > way they like it swung.
>
> IIUC, supporting buffer display customization via display-buffer-alist
> etc won't be enough.

Maybe, but then we should focus on opening the right doors so that
it is (or at least check if that is very hard to do) instead of automatically
arguing for a permanent UI change.

> The question is also when to use it, which buffer to apply it to.

Doesn't display-buffer-alist have some mechanism for selecting
which buffer the entry applies to?  I'm not an expert in this field.

João

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

[martin rudalics]

 > Doesn't display-buffer-alist have some mechanism for selecting
 > which buffer the entry applies to?

That's indeed the raison d'être of 'display-buffer-alist'.

martin

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

[Dmitry Gutov]

On 18.01.2019 18:40, João Távora wrote:

>> The question is also when to use it, which buffer to apply it to.
> 
> Doesn't display-buffer-alist have some mechanism for selecting
> which buffer the entry applies to?  I'm not an expert in this field.

All right. Maybe just using a function that honors display-buffer-alist 
for displaying both the xref buffer and the target buffers will do the 
trick.

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

[Juri Linkov]

> I wouldn't say it's perfect either, it's still kind of idiosyncratic. Not
> sure your patch will fix that problem, though, instead of just swinging it
> the other way.
>
> We basically have two use cases:
>
> * Jump to this symbol, in this/that
> window/frame. windmove-display-in-direction should probably affect where
> the target buffer ends up, irrespective of whether we have to pop up an
> *xref* buffer to resolve any duplicate matches.
>
> * Show a list of search results. Arguably, in this case
> windmove-display-in-direction should affect where the *xref* buffer
> is displayed.
>
> Neither of y'all's patches solve this, I believe.

Why do you think this display action proposed in my previous patch can't solve this?

  `((display-buffer-in-previous-window)
    (previous-window . ,xref--original-window))

Then there is no need to use a new relative-window alist entry 'from-window'
in display-buffer-in-window because xref--original-window is absolute addressing.

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

[Dmitry Gutov]

On 19.01.2019 23:45, Juri Linkov wrote:
> Why do you think this display action proposed in my previous patch can't solve this?
> 
>    `((display-buffer-in-previous-window)
>      (previous-window . ,xref--original-window))

Sorry. To be honest, I came to this conclusion partly because neither of 
you likes the other's patch. And you two seem to care more for different 
items in that list.

Both patches are a bit too far from my expertise to review just by reading.

> Then there is no need to use a new relative-window alist entry 'from-window'
> in display-buffer-in-window because xref--original-window is absolute addressing.

This makes sense.

If only that patch were able to keep the current behavior by default.

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

[João Távora]

On Sun, Jan 20, 2019 at 12:27 AM Dmitry Gutov <dgutov@yandex.ru> wrote:

> Sorry. To be honest, I came to this conclusion partly because neither of
> you likes the other's patch.

No, no. That's not the case, at least for me. It's not a question of liking.

> If only that patch were able to keep the current behavior by default.

Yep.  If Juri provides a simpler patch that does this I'm all for it.

João

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

[Juri Linkov]

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

>> If only that patch were able to keep the current behavior by default.
>
> Yep.  If Juri provides a simpler patch that does this I'm all for it.

Ok, here's 100% backward-compatible patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: xref.simplify.patch --]
[-- Type: text/x-diff, Size: 1768 bytes --]

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 87ce2299c5..9522d7e475 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -474,27 +474,17 @@ xref--show-pos-in-buf
           (or (eq xref--original-window-intent 'frame)
               pop-up-frames))
          (action
-          (cond ((memq
-                  xref--original-window-intent
-                  '(window frame))
+          (cond ((eq xref--original-window-intent 'frame)
                  t)
+                ((eq xref--original-window-intent 'window)
+                 '(display-buffer-same-window))
                 ((and
                   (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))
+                 `((display-buffer-in-previous-window)
+                   (previous-window . ,xref--original-window))))))
+    (with-selected-window (display-buffer buf action)
       (xref--goto-char pos)
       (run-hooks 'xref-after-jump-hook)
       (let ((buf (current-buffer)))

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

[João Távora]

On Sun, Jan 27, 2019 at 8:42 PM Juri Linkov <juri@linkov.net> wrote:
>
> >> If only that patch were able to keep the current behavior by default.
> >
> > Yep.  If Juri provides a simpler patch that does this I'm all for it.
>
> Ok, here's 100% backward-compatible patch:

Thanks, Juri!

I know I'm late on this, but I've been very busy. Please give me
some more days to try this out.

João Távora

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

[João Távora]

On Thu, Jan 31, 2019 at 10:14 PM João Távora <joaotavora@gmail.com> wrote:
>
> On Sun, Jan 27, 2019 at 8:42 PM Juri Linkov <juri@linkov.net> wrote:
> >
> > >> If only that patch were able to keep the current behavior by default.
> > > Yep.  If Juri provides a simpler patch that does this I'm all for it.
> > Ok, here's 100% backward-compatible patch:
>
> Thanks, Juri!
>
> I know I'm late on this, but I've been very busy. Please give me
> some more days to try this out.

OK, so I did find time to test this briefly and I found some
bugs.  However, they are reasonably hard to reproduce
consistently. Here's the only bug I can reproduce
consistently:

emacs -Q
C-x 2
C-x 4 . xref-backend-definitions RET
C-n
TAB

Expected the definition to appear in the bottom window, but it
goes to the top window instead (the window I used
xref-find-definitions-other-window).  This is wrong and the
current xref.el implementation does not suffer from this bug.

However, in all fairness, the current xref.el implementation
suffers from other bugs that I had never uncovered:

emacs -Q
C-x 2
C-x o
C-x 4 . xref-backend-definitions RET
n

This will open a new frame (!) completely unexpectedly, whereas in
your version, it works quite correctly. It works fine in both versions
if the C-x o is not used.

I did not debug any of the problems.

So which bugs are "worse"? :-) Assuming you can reproduce it
and fix the bug, I would have no more objections, and the patch
does indeed simplify the code.

João

PS: I stress the "assuming you can reproduce it": I could be making
a mistake here: I tested with and without your patch on a recent
Emacs.

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

[Dmitry Gutov]

Joao, thank you for testing.

On 01.02.2019 03:17, João Távora wrote:
> emacs -Q
> C-x 2
> C-x o
> C-x 4 . xref-backend-definitions RET
> n
> 
> <...> in
> your version, it works quite correctly.

When I try this with the new patch, it results in a third window being 
created (the original window is being split, and the definition is shown 
there).

Is this the behavior we want?

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

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 1 Feb 2019 04:39:09 +0300
> Cc: 33870@debbugs.gnu.org
> 
> On 01.02.2019 03:17, João Távora wrote:
> > emacs -Q
> > C-x 2
> > C-x o
> > C-x 4 . xref-backend-definitions RET
> > n
> > 
> > <...> in
> > your version, it works quite correctly.
> 
> When I try this with the new patch, it results in a third window being 
> created (the original window is being split, and the definition is shown 
> there).
> 
> Is this the behavior we want?

No, I don't think so.

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

[João Távora]

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

On Fri, Feb 1, 2019, 07:30 Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Dmitry Gutov <dgutov@yandex.ru>
> > Date: Fri, 1 Feb 2019 04:39:09 +0300
> > Cc: 33870@debbugs.gnu.org
> >
> > On 01.02.2019 03:17, João Távora wrote:
> > > emacs -Q
> > > C-x 2
> > > C-x o
> > > C-x 4 . xref-backend-definitions RET
> > > n
> > >
> > > <...> in
> > > your version, it works quite correctly.
> >
> > When I try this with the new patch, it results in a third window being
> > created (the original window is being split, and the definition is shown
> > there).
> > Is this the behavior we want?
> No, I don't think so.
>

It might not be the behavior you want, but it was the behavior I designed
it to have.

You start with two windows, A and B. You ask to find definitions in another
window from A, because you want to preserve its contents. The symbol you
searched for happened to have multiple definitions so you decide to browse
them from *xref* using bare 'n' and 'p' before settling on the definition
you want. Those "prospects" can't be shown in A because that would break
the original "other-window" contract/intention, and they can't be shown in
B because that's where you're browsing from. They need a new window C which
is not available. When the frame is relatively small (as it is with emacs
-Q), C is created by splitting horizontally, which is kind of akward, but
the decision where to create C changes with larger frames.

For some reason, 26.1 sometimes decides to make another frame for C, but
only if you start from B, i.e you add onde 'C-x o' to the beginning of the
recipe, after splitting. This is a bug in the current xref.el or, more
likely, in window.el's window-splitting heuristics. The bug goes away when
you have larger frames, which explains why I didn't catch it earlier.

Fortunately, the whole point of this bug report opened by Juri is to make
this configurable. Later, we can decide on a better default, something Juri
is also very much in favor of. If you add your voice to his and decide to
change the default, and then give me a way to recover 26.1's behavior
(minus the bug), I won't object (much).

[-- Attachment #2: Type: text/html, Size: 3207 bytes --]

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

[Drew Adams]

> > > When I try this with the new patch, it results in a third window being 
> > > created (the original window is being split, and the definition is shown  there).
> > > Is this the behavior we want?
> > No, I don't think so.
>
> It might not be the behavior you want, but it was the behavior I designed it to have. 
>
> You start with two windows, A and B. You ask to find definitions in another window
> from A, because you want to preserve its contents. The symbol you searched for
> happened to have multiple definitions so you decide to browse them from *xref*
> using bare 'n' and 'p' before settling on the definition you want. Those
> "prospects" can't be shown in A because that would break the original
> "other-window" contract/intention, and they can't be shown in B because that's
> where you're browsing from. They need a new window C which is not available.
> When the frame is relatively small (as it is with emacs -Q), C is created by
> splitting horizontally, which is kind of akward, but the decision where to create
> C changes with larger frames. 

I'm so glad I use separate frames by default.  It's one thing
to explicitly choose to replace the content of a particular
window with other content (another buffer).  It's quite
another thing to have Emacs doing that left and right behind
your back.

It's not Emacs's fault for just trying to DTRT, of course.
The problem is that TRT is hard to specify in advance.  We
can try to require users to specify it in advance, by
configuration, but that runs into the same problem.

This is why `pop-up-windows' and `pop-up-frames' are so
helpful - as a start, at least, to prevent window splitting
and replacing window content left and right.

Of course, if Emacs pops up a new window or frame each time
then you really need a good way to change focus among them.

Yes, I know - mine's a (small) minority opinion.

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

[Dmitry Gutov]

On 01.02.2019 11:19, João Távora wrote:
> You start with two windows, A and B. You ask to find definitions in 
> another window from A, because you want to preserve its contents. The 
> symbol you searched for happened to have multiple definitions so you 
> decide to browse them from *xref* using bare 'n' and 'p' before settling 
> on the definition you want. Those "prospects" can't be shown in A 
> because that would break the original "other-window" contract/intention, 
> and they can't be shown in B because that's where you're browsing from. 
> They need a new window C which is not available. When the frame is 
> relatively small (as it is with emacs -Q), C is created by splitting 
> horizontally, which is kind of akward, but the decision where to create 
> C changes with larger frames.

OK, makes sense to me. Thanks for the reminder.

So, I haven't been able to repro the bugs you mentioned. That's probably 
good.

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

[Dmitry Gutov]

On 02.02.2019 03:00, Dmitry Gutov wrote:

> OK, makes sense to me. Thanks for the reminder.

On the same subject, do you remember why we made RET and TAB behave 
differently?

Maybe that should be in the docstrings. Or at least comments somewhere.

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

[martin rudalics]

 > For some reason, 26.1 sometimes decides to make another frame for C, but
 > only if you start from B, i.e you add onde 'C-x o' to the beginning of the
 > recipe, after splitting. This is a bug in the current xref.el or, more
 > likely, in window.el's window-splitting heuristics. The bug goes away when
 > you have larger frames, which explains why I didn't catch it earlier.

Note that it is never a bug to not split a window in any of these
cases.  Making a new frame is the only way out when (1) existing
windows are too small to get split and (2) windows cannot be reused
because they are dedicated or 'inhibit-same-window' is set.  And the
choice to not split a window when it's too small is usually up to the
user and applications just have to respect it.

martin

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

[Juri Linkov]

> Fortunately, the whole point of this bug report opened by Juri is to make
> this configurable. Later, we can decide on a better default, something Juri
> is also very much in favor of.

With a better default such problems wouldn't happen if definitions were
displayed in a new split window taking space from the original window.

> If you add your voice to his and decide to change the default, and
> then give me a way to recover 26.1's behavior (minus the bug), I won't
> object (much).

How do you prefer to configure old behavior?  Using a customizable option?

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

[João Távora]

On Sat, Feb 2, 2019 at 9:18 PM Juri Linkov <juri@linkov.net> wrote:
>
> > Fortunately, the whole point of this bug report opened by Juri is to make
> > this configurable. Later, we can decide on a better default, something Juri
> > is also very much in favor of.
>
> With a better default such problems wouldn't happen if definitions were
> displayed in a new split window taking space from the original window.

It's only a "problem" in your opinion :-)  I was assuming Eli also thinks it's
a bad default, but he hasn't confirmed this yet (and like Dmitry perhaps
we was misunderstanding the reason for the behaviour). And neither has he
said that your proposal is better.

Again, for the millionth time in this discussion, let's first merge your patch
and clean up any bugs before we decide on defaults (perhaps you have
done that already, I haven't checked).

> > If you add your voice to his and decide to change the default, and
> > then give me a way to recover 26.1's behavior (minus the bug), I won't
> > object (much).
>
> How do you prefer to configure old behavior?  Using a customizable option?

Something that fits in a single line is good IMO. But normally in Emacs we
keep the defaults for a while and give people unhappy with the defaults
a way to choose their preferred configuration.  We change the default when
enough people voice their dissent.   So, at least for some time, the
configuration line should go into your configuration, not mine.

-- 
João Távora

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

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Sat, 2 Feb 2019 22:22:05 +0000
> Cc: Eli Zaretskii <eliz@gnu.org>, Dmitry Gutov <dgutov@yandex.ru>, 33870@debbugs.gnu.org
> 
> On Sat, Feb 2, 2019 at 9:18 PM Juri Linkov <juri@linkov.net> wrote:
> >
> > > Fortunately, the whole point of this bug report opened by Juri is to make
> > > this configurable. Later, we can decide on a better default, something Juri
> > > is also very much in favor of.
> >
> > With a better default such problems wouldn't happen if definitions were
> > displayed in a new split window taking space from the original window.
> 
> It's only a "problem" in your opinion :-)  I was assuming Eli also thinks it's
> a bad default, but he hasn't confirmed this yet (and like Dmitry perhaps
> we was misunderstanding the reason for the behaviour). And neither has he
> said that your proposal is better.

I thought I did express my opinions, but maybe I'm confused wrt the
question(s) you are asking.  Care to repeat them, for my benefit?

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

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Date: Sat, 2 Feb 2019 22:22:05 +0000
>> Cc: Eli Zaretskii <eliz@gnu.org>, Dmitry Gutov <dgutov@yandex.ru>, 33870@debbugs.gnu.org
>> 
>> On Sat, Feb 2, 2019 at 9:18 PM Juri Linkov <juri@linkov.net> wrote:
>> >
>> > > Fortunately, the whole point of this bug report opened by Juri is to make
>> > > this configurable. Later, we can decide on a better default, something Juri
>> > > is also very much in favor of.
>> >
>> > With a better default such problems wouldn't happen if definitions were
>> > displayed in a new split window taking space from the original window.
>> 
>> It's only a "problem" in your opinion :-)  I was assuming Eli also thinks it's
>> a bad default, but he hasn't confirmed this yet (and like Dmitry perhaps
>> we was misunderstanding the reason for the behaviour). And neither has he
>> said that your proposal is better.
>
> I thought I did express my opinions, but maybe I'm confused wrt the
> question(s) you are asking.  Care to repeat them, for my benefit?

Eli, if that helps clear the confusion up front, this it has to do with
this last exchange, not with your email of 2018-12-26, 15:36, where you
said you would _not_ like to change the current default behaviour.  

If that doesn't help, and neither does reading the exchange, I'll try
a summary of the most recent events.

- Juri provided a purportedly 100% backward compatible patch that keeps
  current UI and allows xref.el windows to be configured by users.
  
- I tested the patch with many cases, including a corner use case.

- Dmitry expressed doubts about the behaviour of that case

- You expressed the same doubts as Dmitry's

- I explained that it is the defined behaviour

- Dmitry accepted the explanation

- Drew wrote something that I didn't read/understand fully (sorry Drew!)

- Juri took your doubts as evidence of a problem in the current UI.

- I explained again that it is the defined and default UI, but changing
  is on the table, especially if you, unlike Dmitry, don't accept the
  explanation I gave for the corner case that you said isn't correct.
  
So Eli, maintainer of Emacs, the Editor:

1. Should xref.el be made configurable so that multiple UI's are
   available to users, keeping the current default in in Emacs 26.1?  We
   have at least two candidate patches that do this.
   
2. Should the default UI in Emacs 26.1 be changed?

As has been done at least 10 times in this thread, I propose to do the
former first and then discuss the latter.  I can also say that I am a
bit tired of this: the thread has got so entangled that I'm now spending
time re-explaining these relatively simple premises.

João

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

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: juri@linkov.net,  dgutov@yandex.ru,  33870@debbugs.gnu.org
> Date: Sun, 03 Feb 2019 12:00:42 +0000
> 
> So Eli, maintainer of Emacs, the Editor:
> 
> 1. Should xref.el be made configurable so that multiple UI's are
>    available to users, keeping the current default in in Emacs 26.1?  We
>    have at least two candidate patches that do this.
>    
> 2. Should the default UI in Emacs 26.1 be changed?
> 
> As has been done at least 10 times in this thread, I propose to do the
> former first and then discuss the latter.  I can also say that I am a
> bit tired of this: the thread has got so entangled that I'm now spending
> time re-explaining these relatively simple premises.

I don't think I get this: you have asked for my opinion.  Now you seem
to say you are tired of discussing this and don't want to spend any
more time on it?  Fine with me, then I won't take any more of your
time by trying to chime in.  Do whatever you would have done without
hearing my opinions.  It will certainly be faster, and put less
demands on your time (and on mine).

Thanks.

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

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> As has been done at least 10 times in this thread, I propose to do the
>> former first and then discuss the latter.  I can also say that I am a
>> bit tired of this: the thread has got so entangled that I'm now spending
>> time re-explaining these relatively simple premises.
>
> I don't think I get this: you have asked for my opinion. Now you seem
> to say you are tired of discussing this

Not really.  But it seems in trying to untangle this thread, I've only
tangled it more...

Let me try one last time.  2 days ago, you decided to chime in very
tersely: "No, I don't think so"

  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=33870#380

, presumably saying that the default behaviour in Emacs 26.1 doesn't
make sense for a specific edge case that I had been testing.  Because
you may have been misunderstanding I replied with a detailed explanation
of the reasoning for that particular behaviour in:

  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=33870#383

, which you didn't reply to, but Dmitry accepted as sufficient
validation for the current behaviour.  Meanwhile, Juri used this
opportunity to insist that the current behaviour is sub-optimal.

I didn't state (much) disagreement: IMO changing the default can be on
the table, but I said perhaps we should wait for your confirmation that
the "No, I don't think so" really means that you think the 26.1 default
_should_ be changed.

Somewhere along the line we started miscommunicating that someone was
asking the other for input, but for me this is very simple: let's
install Juri's latest patch, which allows for configuring different
behaviors and _then_ discuss which one, if any, of the set of new
possibilities could become the new default.

Also sorry if I sounded rude: I wasn't attributing the reasons I am
tired of this thread to you.

Thanks,
João

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

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: juri@linkov.net,  dgutov@yandex.ru,  33870@debbugs.gnu.org
> Date: Sun, 03 Feb 2019 20:22:17 +0000
> 
> Let me try one last time.  2 days ago, you decided to chime in very
> tersely: "No, I don't think so"
> 
>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=33870#380
> 
> , presumably saying that the default behaviour in Emacs 26.1 doesn't
> make sense for a specific edge case that I had been testing.  Because
> you may have been misunderstanding I replied with a detailed explanation
> of the reasoning for that particular behaviour in:
> 
>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=33870#383
> 
> , which you didn't reply to, but Dmitry accepted as sufficient
> validation for the current behaviour.  Meanwhile, Juri used this
> opportunity to insist that the current behaviour is sub-optimal.

The current behavior is somewhat sub-optimal, but so is IMO the
alternative that Juri suggests: the "tiny window below the original
one", as I understand it, will make M-. and fiends behave differently
from any other command which needs to use "the other window", like
"M-x compile", "M-x grep", etc.  So I think I like the current
behavior better.  The part of the current behavior where RET after
"C-x 4 ." makes a new window I don't like too much, I think it would
be better to reuse the window where *xref* is shown.  But I wouldn't
insist on such a change, mainly because I think "C-x 4 ." makes little
sense anyway, as we have "M-," to easily return to the buffer we were
in originally.

So on balance I think we don't need to change the current UI.

> Somewhere along the line we started miscommunicating that someone was
> asking the other for input, but for me this is very simple: let's
> install Juri's latest patch, which allows for configuring different
> behaviors and _then_ discuss which one, if any, of the set of new
> possibilities could become the new default.

If that patch doesn't change the default behavior, I have nothing
against installing it on master.

Thanks.

P.S. Sorry for a delay in responding, I had several urgent tasks that
ate up all my free time.

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

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

> behavior better.  The part of the current behavior where RET after
> "C-x 4 ." makes a new window I don't like too much,

I don't know if Juri description says it does, but I agree with you it
shouldn't.

> I think it would
> be better to reuse the window where *xref* is shown.

It does exactly this, in my tests.  Unless, of course, it has another
available window besides the original one and the *xref* one.

> So on balance I think we don't need to change the current UI.

Yes, though I would like to at least try out Juri's proposal in practice
day-to-day programming.

> If that patch doesn't change the default behavior, I have nothing
> against installing it on master.

That's my position, too.  Apparently, Juri's patch makes it easy to
switch between alternatives.

> P.S. Sorry for a delay in responding, I had several urgent tasks that
> ate up all my free time.

No problem.

João

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

[João Távora]

After retesting and not being able to reproduce the problems anymore,
I pushed Juri's patch.

On Tue, Feb 5, 2019 at 6:34 PM João Távora <joaotavora@gmail.com> wrote:
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> > behavior better.  The part of the current behavior where RET after
> > "C-x 4 ." makes a new window I don't like too much,
>
> I don't know if Juri description says it does, but I agree with you it
> shouldn't.
>
> > I think it would
> > be better to reuse the window where *xref* is shown.
>
> It does exactly this, in my tests.  Unless, of course, it has another
> available window besides the original one and the *xref* one.
>
> > So on balance I think we don't need to change the current UI.
>
> Yes, though I would like to at least try out Juri's proposal in practice
> day-to-day programming.
>
> > If that patch doesn't change the default behavior, I have nothing
> > against installing it on master.
>
> That's my position, too.  Apparently, Juri's patch makes it easy to
> switch between alternatives.
>
> > P.S. Sorry for a delay in responding, I had several urgent tasks that
> > ate up all my free time.
>
> No problem.
>
> João



-- 
João Távora

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

[Juri Linkov]

> After retesting and not being able to reproduce the problems anymore,
> I pushed Juri's patch.

Thanks, João.  Before closing this bug, please confirm if you prefer
renaming window--display-buffer to display-buffer-in-window in this bug,
or creating a new request?  I'll also create separate requests for
all other related topics from this bug.

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

[Drew Adams]

> - Drew wrote something that I didn't read/understand
>   fully (sorry Drew!)

I added 4 side points related to points that came
up in the thread.  Possibly you didn't read or
understand one or more of them.  Enjoy.

1. "Everything in Emacs is (and should be) public."
   All `display-buffer*' functions deserve doc
   strings or at least developer-oriented comments.

2. Don't limit future use by having something like
   (direction . (DIR . WIN)).  Instead, as Juri
   suggested, use (direction DIR WIN), so you can
   easily later have (direction DIR WIN FOO), etc.

3. The mistake of #2 was made, e.g., when defining
   "noncontiguous region" segments: (BEG . END).
   A better design is (BEG END) or (BEG END . EXTRA),
   where EXTRA is from the outset undefined (any
   baggage).

4. All-encompassing DWIM for window selection and
   splitting is asking too much of Emacs.  Guessing
   the intention of a user or code in all contexts
   is bound to lose some of the time, frustrating
   users.  The code gets more and more complex,
   which doesn't help users guess what Emacs is
   guessing. ;-)  I'm glad I instead use separate
   frames, by default.

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

[Juri Linkov]

>> It's only a "problem" in your opinion :-)  I was assuming Eli also thinks it's
>> a bad default, but he hasn't confirmed this yet (and like Dmitry perhaps
>> we was misunderstanding the reason for the behaviour). And neither has he
>> said that your proposal is better.
>
> I thought I did express my opinions, but maybe I'm confused wrt the
> question(s) you are asking.  Care to repeat them, for my benefit?

Let me summarize my point of view of the current situation:

* Old behavior:

M-.     pops up the *xref* buffer in an adjacent window
RET     visits references in the original window
TAB     visits references in the original window
n       visits references in the original window

C-x 4 . pops up the *xref* buffer in an adjacent window
RET     visits references in the same window where *xref* buffer was
TAB     depending on window configuration visits references either
        in the same window where *xref* buffer was or in the original window
n       splits the original window and visits references in a tiny window,
        sometimes opens a new frame

C-x 5 . pops up the *xref* buffer in an adjacent window
RET     visits references in a new frame
TAB     visits references in a new frame
n       visits references in a new frame

Problems: the case of 'C-x 4 .' is a mess.  Other cases are consistent,
but take screen space from an adjacent window.  The proposed behavior
solves all these problems:

* Proposed new behavior

M-.     pops up the *xref* buffer in a tiny window below the original window
RET     visits references in the original window
TAB     visits references in the original window
n       visits references in the original window

C-x 4 . pops up the *xref* buffer in a tiny window below the original window
RET     visits references in a new window
TAB     visits references in a new window
n       visits references in a new window

C-x 5 . pops up the *xref* buffer in a tiny window below the original window
RET     visits references in a new frame
TAB     visits references in a new frame
n       visits references in a new frame

However, the problem is that I don't understand the complicated logic
of the 'C-x 4 .' case of the old behavior, so I can't completely
support all its intricacies after code simplification.

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

[João Távora]

Juri Linkov <juri@linkov.net> writes:

>>> It's only a "problem" in your opinion :-)  I was assuming Eli also thinks it's
>>> a bad default, but he hasn't confirmed this yet (and like Dmitry perhaps
>>> we was misunderstanding the reason for the behaviour). And neither has he
>>> said that your proposal is better.
>>
>> I thought I did express my opinions, but maybe I'm confused wrt the
>> question(s) you are asking.  Care to repeat them, for my benefit?
>
> Let me summarize my point of view of the current situation:
>
> * Old behavior:
>
> M-.     pops up the *xref* buffer in an adjacent window
> RET     visits references in the original window
> TAB     visits references in the original window
> n       visits references in the original window
>
> C-x 4 . pops up the *xref* buffer in an adjacent window
> RET     visits references in the same window where *xref* buffer was
> TAB     depending on window configuration visits references either
>         in the same window where *xref* buffer was or in the original window
> n       splits the original window and visits references in a tiny window,
>         sometimes opens a new frame

Unfortunately, you're trying again to kick up a dust cloud around the
matter.  You description is only partially true for the two-window case.
n, for example, doesn't always split the window, only when it needs to
create a new window.  And the "new frame" exception is an _obscure bug_,
and even then it's one that your patch and my patch already solve, so
it's a completely moot point.

Let's use your 100%-backward-compatible patch, (since it is the simpler
of the two). For the millionth time: _after_ we get _some_ patch
installed, I invite you to open a new customization option (or just a
simple variable) that lets me toggle on and off between the current
behaviour and the behaviour that you think is superior.  Then we can all
try it for a while.  _Why_ is this so hard?

João

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

[Juri Linkov]

>> Let me summarize my point of view of the current situation:
>>
>> * Old behavior:
>>
>> M-.     pops up the *xref* buffer in an adjacent window
>> RET     visits references in the original window
>> TAB     visits references in the original window
>> n       visits references in the original window
>>
>> C-x 4 . pops up the *xref* buffer in an adjacent window
>> RET     visits references in the same window where *xref* buffer was
>> TAB     depending on window configuration visits references either
>>         in the same window where *xref* buffer was or in the original window
>> n       splits the original window and visits references in a tiny window,
>>         sometimes opens a new frame
>
> Unfortunately, you're trying again to kick up a dust cloud around the
> matter.  You description is only partially true for the two-window case.

Yes, it's only partially true, I admitted this by saying that
I don't understand its complicated logic.

> Let's use your 100%-backward-compatible patch, (since it is the simpler
> of the two). For the millionth time: _after_ we get _some_ patch
> installed, I invite you to open a new customization option (or just a
> simple variable) that lets me toggle on and off between the current
> behaviour and the behaviour that you think is superior.

I don't think that the behaviour I proposed is superior.
In particular, my previous proposal behaves poorly
in combination with windowmove, e.g. typing
<S-M-up> M-. to display the reference in the upper window,
when there are ambiguities it displays the xref buffer
in the upper window, and then typing RET visits the reference
in a wrong window (in the original window).

I hoped to have more discussion to find the best solution.

> Then we can all try it for a while.  _Why_ is this so hard?

It's hard to support all details of old behavior in all possible interactions.

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

[João Távora]

Juri Linkov <juri@linkov.net> writes:

>>> Let me summarize my point of view of the current situation:
>> Unfortunately, you're trying again to kick up a dust cloud around the
>> matter.  You description is only partially true for the two-window case.
> Yes, it's only partially true, I admitted this by saying that
> I don't understand its complicated logic.

It's complicated in your opinion, and I may agree with that, but it's no
more complicated then, say, a situation where you have two windows, but
one of them is dedicated, and you do, say, describe-symbol, or something
else that pops a window.  Which is just what happens in xref.el: the
starting window where you invoked C-x 4 . behaves like it it is
temporarily dedicated to its buffer.

>> Let's use your 100%-backward-compatible patch, (since it is the simpler
>> of the two). For the millionth time: _after_ we get _some_ patch
>> installed, I invite you to open a new customization option (or just a
>> simple variable) that lets me toggle on and off between the current
>> behaviour and the behaviour that you think is superior.
>
> I don't think that the behaviour I proposed is superior.

"Superior" in UI is always relative.  I think the only way to know if to
test it for a while.  But to do that we have to close this bug first.

> I hoped to have more discussion to find the best solution.

I didn't say we _can't_ have more discussion.  But let's first wrap up
this easy win by installing the patch that makes this configurable.

I'll test your last patch in a fresh Emacs to check if I can still
reproduce the problems I was finding the last time.  If I can't, I'll
just push it , unless someone objects.

João

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

[Dmitry Gutov]

On 03.02.2019 23:33, Juri Linkov wrote:
> Let me summarize my point of view of the current situation:

Could you also describe how M-x project-find-regexp works, with the 
"old" code, and with your proposal?

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

[Juri Linkov]

>> Let me summarize my point of view of the current situation:
>
> Could you also describe how M-x project-find-regexp works, with the "old"
> code, and with your proposal?

Which one?  I have two proposals: one that creates a tiny window,
but has unsolved problems when you ask the window manager to display
a source code buffer in the specific window, it displays the xref buffer
in this window instead.  So a better proposal was to always display
the xref buffer in the same window where the user expected to see
a source code buffer.  But this could be discussed now in bug#33992.

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

[Dmitry Gutov]

On 18.02.2019 00:20, Juri Linkov wrote:

> Which one?  I have two proposals: one that creates a tiny window,
> but has unsolved problems when you ask the window manager to display
> a source code buffer in the specific window, it displays the xref buffer
> in this window instead.  So a better proposal was to always display
> the xref buffer in the same window where the user expected to see
> a source code buffer.

Allow me to rephrase that in the form of a requirement: however the 
behavior of xref-find-definition changes, I don't think the results of 
project-find-regexp (or xref-find-references, for that matter) should be 
displayed in a tiny window.

 > But this could be discussed now in bug#33992.

Sure.

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

[Dmitry Gutov]

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

Hi Juri,

On 27.01.2019 22:29, Juri Linkov wrote:
>>> If only that patch were able to keep the current behavior by default.
>> Yep.  If Juri provides a simpler patch that does this I'm all for it.
> Ok, here's 100% backward-compatible patch:

So apparently this patch was installed, but without a link to the 
relevant bug report, and it was not closed either.

Which is just as well, because my testing shows that it's really not 
100% backward compatible.

1. display-buffer-in-previous-window, as I mentioned in another email, 
does not reliably use the supplied `previous-window' value.

2. 'C-x 4 .' followed by TAB (or RET) is broken: instead of using the 
other window, it uses the original window, just like M-. does. It's a 
misunderstanding inside the code, see below.

> xref.simplify.patch
> 
> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
> index 87ce2299c5..9522d7e475 100644
> --- a/lisp/progmodes/xref.el
> +++ b/lisp/progmodes/xref.el
> @@ -474,27 +474,17 @@ xref--show-pos-in-buf
>             (or (eq xref--original-window-intent 'frame)
>                 pop-up-frames))
>            (action
> -          (cond ((memq
> -                  xref--original-window-intent
> -                  '(window frame))
> +          (cond ((eq xref--original-window-intent 'frame)
>                    t)
> +                ((eq xref--original-window-intent 'window)
> +                 '(display-buffer-same-window))

That's not what the `window' value means. It should mean "other window".

I really don't want to revert this change after all this discussion, but 
implementing it in a different way is not straightforward. My very first 
idea turned out to be to write it more or less like it was before.

But here's an alternative patch. Juri, what do you think? Does it keep 
your customizations working?

[-- Attachment #2: xref-display-buffer-functions.diff --]
[-- Type: text/x-patch, Size: 1868 bytes --]

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index e88f30ca35..8769641b08 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -492,13 +492,14 @@ xref--show-pos-in-buf
           (cond ((eq xref--original-window-intent 'frame)
                  t)
                 ((eq xref--original-window-intent 'window)
-                 '(display-buffer-same-window))
+                 `((xref--display-buffer-in-other-window)
+                   (window . ,xref--original-window)))
                 ((and
                   (window-live-p xref--original-window)
                   (or (not (window-dedicated-p xref--original-window))
                       (eq (window-buffer xref--original-window) buf)))
-                 `((display-buffer-in-previous-window)
-                   (previous-window . ,xref--original-window))))))
+                 `((xref--display-buffer-in-window)
+                   (window . ,xref--original-window))))))
     (with-selected-window (display-buffer buf action)
       (xref--goto-char pos)
       (run-hooks 'xref-after-jump-hook)
@@ -507,6 +508,19 @@ xref--show-pos-in-buf
           (setq-local other-window-scroll-buffer buf)))
       (selected-window))))
 
+(defun xref--display-buffer-in-other-window (buffer alist)
+  (let ((window (assoc-default 'window alist)))
+    (cl-assert window)
+    (xref--with-dedicated-window
+     (with-selected-window window
+       (display-buffer buffer t)))))
+
+(defun xref--display-buffer-in-window (buffer alist)
+  (let ((window (assoc-default 'window alist)))
+    (cl-assert window)
+    (with-selected-window window
+      (display-buffer buffer '(display-buffer-same-window)))))
+
 (defun xref--show-location (location &optional select)
   "Help `xref-show-xref' and `xref-goto-xref' do their job.
 Go to LOCATION and if SELECT is non-nil select its window.  If

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

[Dmitry Gutov]

On 11.06.2019 3:00, Dmitry Gutov wrote:
> But here's an alternative patch. <...>

Installed, and closing.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Tue, 25 Dec 2018 22:42:28 +0200
> Cc: dmitry gutov <dgutov@yandex.ru>
> 
> 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.

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

[Juri Linkov]

>> 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.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 33870@debbugs.gnu.org,  dgutov@yandex.ru
> 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.

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

[Dmitry Gutov]

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.