bug#33992: 27.0.50; xref-find-definitions wastes too much space

bug#33992: 27.0.50; xref-find-definitions wastes too much space

[Juri Linkov]

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

Tags: patch
Severity: wishlist
X-Debbugs-CC: João Távora <joaotavora@gmail.com>, Dmitry Gutov <dgutov@yandex.ru>

As noted in bug#33870 the buffer produced by xref-find-definitions
(bound to ‘M-.’) has a transient nature like the *Completions* buffer.
Therefore it makes sense not to waste its space needlessly:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: display-action-alist.patch --]
[-- Type: text/x-diff, Size: 1778 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
@@ -782,7 +773,7 @@ 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) (assoc-default 'display-action-alist alist))
         (goto-char (point-min))
         (setq xref--original-window (assoc-default 'window alist)
               xref--original-window-intent (assoc-default 'display-action alist))
@@ -808,12 +799,15 @@ xref--show-xrefs
   (cond
    ((and (not (cdr xrefs)) (not always-show-list))
     (xref-push-marker-stack)
-    (xref--pop-to-location (car xrefs) display-action))
+    (xref--pop-to-location (car xrefs) (unless (listp display-action) display-action)))
    (t
     (xref-push-marker-stack)
     (funcall xref-show-xrefs-function xrefs
              `((window . ,(selected-window))
-               (display-action . ,display-action))))))
+               (display-action
+                . ,(unless (listp display-action) display-action))
+               (display-action-alist
+                . ,(when (listp display-action) display-action)))))))
 
 (defun xref--prompt-p (command)
   (or (eq xref-prompt-for-identifier t)
@@ -864,7 +858,7 @@ xref-find-definitions
 Otherwise, display the list of the possible definitions in a
 buffer where the user can select from the list."
   (interactive (list (xref--read-identifier "Find definitions of: ")))
-  (xref--find-definitions identifier nil))
+  (xref--find-definitions identifier '((display-buffer-maybe-below-selected))))
 
 ;;;###autoload
 (defun xref-find-definitions-other-window (identifier)

bug#33992: 27.0.50; xref-find-definitions wastes too much space

[João Távora]

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

On Sat, Jan 5, 2019, 23:51 Juri Linkov <juri@linkov.net wrote:

> Tags: patch
> Severity: wishlist
> X-Debbugs-CC: João Távora <joaotavora@gmail.com>, Dmitry Gutov <
> dgutov@yandex.ru>
>
> As noted in bug#33870 the buffer produced by xref-find-definitions
> (bound to ‘M-.’) has a transient nature like the *Completions* buffer.
> Therefore it makes sense not to waste its space needlessly:
>

It doesn't make sense to review this patch until the patch that fixes 33870
has been produced.

João

>
>

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

bug#33992: 27.0.50; xref-find-definitions wastes too much space

[Juri Linkov]

>> As noted in bug#33870 the buffer produced by xref-find-definitions
>> (bound to ‘M-.’) has a transient nature like the *Completions* buffer.
>> Therefore it makes sense not to waste its space needlessly:
>
> It doesn't make sense to review this patch until the patch that fixes 33870
> has been produced.

Since the patch from bug#33870 has been already committed,
this bug#33992 is undeadlocked now.

The problem with xref-find-definitions is its unexpected outcome:
sometimes it pops up a window, sometimes doesn't.

It's like Russian roulette: pull the trigger, bang and there is
a hole in the screen that takes the form of glaringly empty space.

The proposed change is not to break the window configuration:
either split the selected window and show *xref* below,
or do inline placement, i.e. to replace the content of the
selected window with the *xref* buffer.

bug#33992: 27.0.50; xref-find-definitions wastes too much space

[João Távora]

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

On Wed, Mar 20, 2019 at 10:35 PM Juri Linkov <juri@linkov.net> wrote:

> >> As noted in bug#33870 the buffer produced by xref-find-definitions
> >> (bound to ‘M-.’) has a transient nature like the *Completions* buffer.
> >> Therefore it makes sense not to waste its space needlessly:
> >
> > It doesn't make sense to review this patch until the patch that fixes
> 33870
> > has been produced.
>
> Since the patch from bug#33870 has been already committed,
> this bug#33992 is undeadlocked now.
>
> The problem with xref-find-definitions is its unexpected outcome:
> sometimes it pops up a window, sometimes doesn't.
>
> It's like Russian roulette: pull the trigger, bang and there is
> a hole in the screen that takes the form of glaringly empty space.
>

I understand, but somehow I'm not very offended by that.  It's just
something I've grown so used to, even before xref.  Just C-h f to
a function description of which you know nothing of the length
and boom, a lot of empty space too.

While I do vaguely remember being annoyed by it many years ago,
I don't now. And for xref I like to see it well highlighted away from any
other text, so I can easily count the references.  Displaying next to the
minibuffer are would surround it with more noise.

It's just the way I work.  Fortunately now it is customizable, right? How
about
adding an entry to the manual, in the xref section, mentioning a couple of
commands or customization variables that can be used to get the interface
that you prefer, Juri?

João

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

bug#33992: 27.0.50; xref-find-definitions wastes too much space

[Dmitry Gutov]

On 20.03.2019 23:37, Juri Linkov wrote:
> The problem with xref-find-definitions is its unexpected outcome:
> sometimes it pops up a window, sometimes doesn't.

Imagine the process of code completion.

Sometimes you press TAB (or C-M-i) and it completes to the symbol you 
wanted. Sometimes it pops up a window with all matching completions instead.

Does it feel the same way to you?

bug#33992: 27.0.50; xref-find-definitions wastes too much space

[Juri Linkov]

>> The problem with xref-find-definitions is its unexpected outcome:
>> sometimes it pops up a window, sometimes doesn't.
>
> Imagine the process of code completion.
>
> Sometimes you press TAB (or C-M-i) and it completes to the symbol you
> wanted. Sometimes it pops up a window with all matching
> completions instead.
>
> Does it feel the same way to you?

The difference is that completions pop up in a small unobtrusive window.
But this should be easy to do in xref now too.

Thanks to João, we now have configurable window management in xref,
so I tried different customizations, and one of the most appealing
is this:

  (defun display-buffer-condition-xref (buffer-name _action)
    (and (string-match-p "\\`\\*\\(xref\\)\\*\\(\\|<[0-9]+>\\)\\'"
                         buffer-name)
         (memq this-command '(xref-find-definitions))))

  (defun display-buffer-condition-from-xref (_buffer-name _action)
    (string-match-p "\\`\\*\\(xref\\)\\*\\(\\|<[0-9]+>\\)\\'"
                    (buffer-name (current-buffer))))

  (setq display-buffer-alist
     '((display-buffer-condition-xref
        display-buffer-in-direction
       (direction . below) (window-height . fit-window-to-buffer))))

  (with-eval-after-load 'xref
    (define-key xref--button-map [(control ?m)] #'xref-quit-and-goto-xref))

How do you like that?

bug#33992: 27.0.50; xref-find-definitions wastes too much space

[Dmitry Gutov]

On 04.04.2019 23:49, Juri Linkov wrote:

>> Does it feel the same way to you?
> 
> The difference is that completions pop up in a small unobtrusive window.

Small window? I usually have a side-by-side fullscreen split, and if I 
initiate completion in one of the windows, *Completion* takes up the 
whole other window. Temporarily, of course.

> But this should be easy to do in xref now too.
> 
> Thanks to João, we now have configurable window management in xref,
> so I tried different customizations, and one of the most appealing
> is this:
> 
>    (defun display-buffer-condition-xref (buffer-name _action)
>      (and (string-match-p "\\`\\*\\(xref\\)\\*\\(\\|<[0-9]+>\\)\\'"
>                           buffer-name)
>           (memq this-command '(xref-find-definitions))))
> 
>    (defun display-buffer-condition-from-xref (_buffer-name _action)
>      (string-match-p "\\`\\*\\(xref\\)\\*\\(\\|<[0-9]+>\\)\\'"
>                      (buffer-name (current-buffer))))

This function seems unused.

>    (setq display-buffer-alist
>       '((display-buffer-condition-xref
>          display-buffer-in-direction

And this function is undefined in my Emacs.

>         (direction . below) (window-height . fit-window-to-buffer))))
> 
>    (with-eval-after-load 'xref
>      (define-key xref--button-map [(control ?m)] #'xref-quit-and-goto-xref))
> 
> How do you like that?

I might, but since I can't really try your customization myself yet, 
I'll repeat a question you might be familiar with already:

   Will this also affect xref-find-references and project-find-regexp?

bug#33992: 27.0.50; xref-find-definitions wastes too much space

[Felician Nemeth]

(Sorry for replying late, I've just read this bug report.)

I thought that I didn't need to see the list of the xref results and the
xrefs' window shrank the view of the code I wanted to study.  So, I came
up with the defun below.  It presents the xref results without showing
the xref window.  I think this idea can be further developed.
xref-show--xrefs-buffer could have an 'm' key binding that "minimizes"
its window by switching to xref-show-xrefs-without-buffer (below) and
that function can "maximize" back with the same 'm' key.  A customizable
variable could define the initial behavior.

Also, I think we can enhance xref-pulse-momentarily to use a different
face if there's only one xref to present.

(defun xref-show-xrefs-without-buffer (xrefs alist)
  "Present the results of an xref query in a simple manner.
To activate this feature, customize `xref-show-xrefs-function'."
  (xref--show-xref-buffer xrefs alist)
  (quit-window)
  (next-error)
  (message "%s (%s xrefs in total)"
           "\",\": previous xref \".\":next xref \"m\":show xref buffer"
           (length xrefs))
  (set-transient-map
   (let ((map (make-sparse-keymap)))
     (define-key map (kbd ",") 'previous-error)
     (define-key map (kbd ".") 'next-error)
     (define-key map (kbd "m")
       (lambda () (interactive) (pop-to-buffer xref-buffer-name)))
     map)
   t))
(setq xref-show-xrefs-function 'xref-show-xrefs-without-buffer)

bug#33992: 27.0.50; xref-find-definitions wastes too much space

[Dmitry Gutov]

On 05.04.2019 12:44, Felician Nemeth wrote:
> (Sorry for replying late, I've just read this bug report.)
> 
> I thought that I didn't need to see the list of the xref results and the
> xrefs' window shrank the view of the code I wanted to study.  So, I came
> up with the defun below.  It presents the xref results without showing
> the xref window.  I think this idea can be further developed.

For instance, by not calling xref--show-xref-buffer right away, and 
instead storing the arguments. And then maybe calling it later if the 
user types 'm'.

You won't be able to use previous/next-error for such an implementation, 
though.

> xref-show--xrefs-buffer could have an 'm' key binding that "minimizes"
> its window by switching to xref-show-xrefs-without-buffer (below) and
> that function can "maximize" back with the same 'm' key.  A customizable
> variable could define the initial behavior.

I'm not a fan of this interface, personally. It's good that it's 
available, though.

We will make xref-show-xrefs-function a defcustom sooner or later, when 
we're sure that it doesn't need to be changed much.

> Also, I think we can enhance xref-pulse-momentarily to use a different
> face if there's only one xref to present.

Yes, but I'm not sure the user will quickly understand the meaning of 
the different face.

Anyway, we can make the exact face name a defcustom (it's currently 
'next-error'), and you'd be able to change it via 'let'.

bug#33992: 27.0.50; xref-find-definitions wastes too much space

[Juri Linkov]

> I thought that I didn't need to see the list of the xref results and the
> xrefs' window shrank the view of the code I wanted to study.  So, I came
> up with the defun below.  It presents the xref results without showing
> the xref window.  I think this idea can be further developed.
> xref-show--xrefs-buffer could have an 'm' key binding that "minimizes"
> its window by switching to xref-show-xrefs-without-buffer (below) and
> that function can "maximize" back with the same 'm' key.  A customizable
> variable could define the initial behavior.

This looks like good ol' find-tag and tags-loop-continue bound to M-,
with an optional pop-up to the xref buffer by a dedicated key.

bug#33992: 27.0.50; xref-find-definitions wastes too much space

[Juri Linkov]

>>> Does it feel the same way to you?
>>
>> The difference is that completions pop up in a small unobtrusive window.
>
> Small window? I usually have a side-by-side fullscreen split, and if
> I initiate completion in one of the windows, *Completion* takes up the
> whole other window. Temporarily, of course.

The key word here is 'Temporarily'.  Unlike *Completions*,
the *xref* buffer doesn't go out easily.

>>    (defun display-buffer-condition-xref (buffer-name _action)
>>      (and (string-match-p "\\`\\*\\(xref\\)\\*\\(\\|<[0-9]+>\\)\\'"
>>                           buffer-name)
>>           (memq this-command '(xref-find-definitions))))
>>
>>    (defun display-buffer-condition-from-xref (_buffer-name _action)
>>      (string-match-p "\\`\\*\\(xref\\)\\*\\(\\|<[0-9]+>\\)\\'"
>>                      (buffer-name (current-buffer))))
>
> This function seems unused.

It's unused because it would be useful only in the *xref* buffer
created by the xref-find-definitions command, so xref needs to
provide a way to distinguish such case.

>>    (setq display-buffer-alist
>>       '((display-buffer-condition-xref
>>          display-buffer-in-direction
>
> And this function is undefined in my Emacs.

This function is implemented by Martin in bug#33870.

>>    (with-eval-after-load 'xref
>>      (define-key xref--button-map [(control ?m)] #'xref-quit-and-goto-xref))
>>
>> How do you like that?
>
> I might, but since I can't really try your customization myself yet, I'll
> repeat a question you might be familiar with already:
>
>   Will this also affect xref-find-references and project-find-regexp?

It should not affect them due to (memq this-command '(xref-find-definitions))
above.  But also to not affect commands active in the *xref* buffer,
xref should provide a way to check if the *xref* buffer was created
by xref-find-definitions.

bug#33992: 27.0.50; xref-find-definitions wastes too much space

[Dmitry Gutov]

On 07.04.2019 0:03, Juri Linkov wrote:
>>>> Does it feel the same way to you?
>>>
>>> The difference is that completions pop up in a small unobtrusive window.
>>
>> Small window? I usually have a side-by-side fullscreen split, and if
>> I initiate completion in one of the windows, *Completion* takes up the
>> whole other window. Temporarily, of course.
> 
> The key word here is 'Temporarily'.  Unlike *Completions*,
> the *xref* buffer doesn't go out easily.

I can understand that. So yes, I can see myself preferring some 
different behavior for a particular command.

>>>     (defun display-buffer-condition-from-xref (_buffer-name _action)
>>>       (string-match-p "\\`\\*\\(xref\\)\\*\\(\\|<[0-9]+>\\)\\'"
>>>                       (buffer-name (current-buffer))))
>>
>> This function seems unused.
> 
> It's unused because it would be useful only in the *xref* buffer
> created by the xref-find-definitions command, so xref needs to
> provide a way to distinguish such case.

Shouldn't it be referenced somewhere else in your patch as well?

>>>     (setq display-buffer-alist
>>>        '((display-buffer-condition-xref
>>>           display-buffer-in-direction
>>
>> And this function is undefined in my Emacs.
> 
> This function is implemented by Martin in bug#33870.

OK, found it, tried it. Seems to work okay-ish for 
xref-find-definitions, except xref-quit-and-goto-xref doesn't seem to be 
functioning too well together with your customization (every other time 
it seemed to use a different window to display the location, not the one 
I called xref-find-definitions from).

>>>     (with-eval-after-load 'xref
>>>       (define-key xref--button-map [(control ?m)] #'xref-quit-and-goto-xref))
>>>
>>> How do you like that?
>>
>> I might, but since I can't really try your customization myself yet, I'll
>> repeat a question you might be familiar with already:
>>
>>    Will this also affect xref-find-references and project-find-regexp?
> 
> It should not affect them due to (memq this-command '(xref-find-definitions))
> above.

It would affect them due to the modification of xref--button-map above, 
though. This part I don't like.

> But also to not affect commands active in the *xref* buffer,
> xref should provide a way to check if the *xref* buffer was created
> by xref-find-definitions.

Yes, we should retain some extra information, e.g. to support revert-buffer.

bug#33992: 27.0.50; xref-find-definitions wastes too much space

[Juri Linkov]

>>>>> Does it feel the same way to you?
>>>>
>>>> The difference is that completions pop up in a small unobtrusive window.
>>>
>>> Small window? I usually have a side-by-side fullscreen split, and if
>>> I initiate completion in one of the windows, *Completion* takes up the
>>> whole other window. Temporarily, of course.
>>
>> The key word here is 'Temporarily'.  Unlike *Completions*,
>> the *xref* buffer doesn't go out easily.
>
> I can understand that. So yes, I can see myself preferring some different
> behavior for a particular command.
>
>>>>     (defun display-buffer-condition-from-xref (_buffer-name _action)
>>>>       (string-match-p "\\`\\*\\(xref\\)\\*\\(\\|<[0-9]+>\\)\\'"
>>>>                       (buffer-name (current-buffer))))
>>>
>>> This function seems unused.
>>
>> It's unused because it would be useful only in the *xref* buffer
>> created by the xref-find-definitions command, so xref needs to
>> provide a way to distinguish such case.
>
> Shouldn't it be referenced somewhere else in your patch as well?

A patch is proposed in a separate bug#35737.

>>>>     (setq display-buffer-alist
>>>>        '((display-buffer-condition-xref
>>>>           display-buffer-in-direction
>>>
>>> And this function is undefined in my Emacs.
>>
>> This function is implemented by Martin in bug#33870.
>
> OK, found it, tried it. Seems to work okay-ish for xref-find-definitions,

Created a separate bug#35592.

> except xref-quit-and-goto-xref doesn't seem to be functioning too well
> together with your customization (every other time it seemed to use
> a different window to display the location, not the one I called
> xref-find-definitions from).

Yes, it should change its behavior depending on xref--original-command.

>>>>     (with-eval-after-load 'xref
>>>>       (define-key xref--button-map [(control ?m)] #'xref-quit-and-goto-xref))
>>>>
>>>> How do you like that?
>>>
>>> I might, but since I can't really try your customization myself yet, I'll
>>> repeat a question you might be familiar with already:
>>>
>>>    Will this also affect xref-find-references and project-find-regexp?
>>
>> It should not affect them due to (memq this-command '(xref-find-definitions))
>> above.
>
> It would affect them due to the modification of xref--button-map above,
> though. This part I don't like.
>
>> But also to not affect commands active in the *xref* buffer,
>> xref should provide a way to check if the *xref* buffer was created
>> by xref-find-definitions.
>
> Yes, we should retain some extra information, e.g. to support revert-buffer.

Created a separate bug#35702.

bug#33992: 27.0.50; xref-find-definitions wastes too much space

[Dmitry Gutov]

On 15.05.2019 23:57, Juri Linkov wrote:

>>>>>      (defun display-buffer-condition-from-xref (_buffer-name _action)
>>>>>        (string-match-p "\\`\\*\\(xref\\)\\*\\(\\|<[0-9]+>\\)\\'"
>>>>>                        (buffer-name (current-buffer))))
>>>>
>>>> This function seems unused.
>>>
>>> It's unused because it would be useful only in the *xref* buffer
>>> created by the xref-find-definitions command, so xref needs to
>>> provide a way to distinguish such case.
>>
>> Shouldn't it be referenced somewhere else in your patch as well?
> 
> A patch is proposed in a separate bug#35737.

I don't see display-buffer-condition-from-xref used in that patch 
either, but it's not hugely important, really.

> Created a separate bug#35592.

Thanks.

> Created a separate bug#35702.

Thanks.

bug#33992: 27.0.50; xref-find-definitions wastes too much space

[Juri Linkov]

>> A patch is proposed in a separate bug#35737.

I'm closing this request because now xref is customizable enough.
And we have no more opinions for changing the default behavior.

FWIW, this is what I currently use to customize xref-find-definitions
to act more like Completions:

(custom-set-variables
 '(display-buffer-alist
   '((display-buffer-to-xref-p display-buffer-in-direction
                               (direction . below)
                               (window-height . fit-window-to-buffer)))))

(defun display-buffer-to-xref-p (buffer-name _action)
  (and (string-match-p "\\`\\*\\(xref\\)\\*\\(\\|<[0-9]+>\\)\\'"
                       buffer-name)
       (memq this-command '(xref-find-definitions))))

(with-eval-after-load 'xref
  (defvar xref--original-command nil)
  (advice-add 'xref-find-definitions :after
	      (lambda (&rest _args)
                (with-current-buffer (window-buffer)
                  (setq-local xref--original-command 'xref-find-definitions))))
  (define-key xref--button-map [(control ?m)]
    (lambda ()
      (interactive)
      (if (memq xref--original-command '(xref-find-definitions))
          (call-interactively 'xref-quit-and-goto-xref)
        (setq xref--original-window nil)
        (call-interactively 'xref-goto-xref)))))

bug#33992: 27.0.50; xref-find-definitions wastes too much space

[Dmitry Gutov]

Hi Juri,

On 28.05.2019 23:35, Juri Linkov wrote:

> FWIW, this is what I currently use to customize xref-find-definitions
> to act more like Completions:
> 
> (custom-set-variables
>   '(display-buffer-alist
>     '((display-buffer-to-xref-p display-buffer-in-direction
>                                 (direction . below)
>                                 (window-height . fit-window-to-buffer)))))
> 
> (defun display-buffer-to-xref-p (buffer-name _action)
>    (and (string-match-p "\\`\\*\\(xref\\)\\*\\(\\|<[0-9]+>\\)\\'"
>                         buffer-name)
>         (memq this-command '(xref-find-definitions))))
> 
> (with-eval-after-load 'xref
>    (defvar xref--original-command nil)
>    (advice-add 'xref-find-definitions :after
> 	      (lambda (&rest _args)
>                  (with-current-buffer (window-buffer)
>                    (setq-local xref--original-command 'xref-find-definitions))))
>    (define-key xref--button-map [(control ?m)]
>      (lambda ()
>        (interactive)
>        (if (memq xref--original-command '(xref-find-definitions))
>            (call-interactively 'xref-quit-and-goto-xref)
>          (setq xref--original-window nil)
>          (call-interactively 'xref-goto-xref)))))

JFYI, one of the changes I've pushed yesterday was the patch I've shown 
before, and it should let you have the same behavior with one line:

(setq xref-show-definitions-function 'xref--show-defs-buffer-at-bottom)

It doesn't seem like xref-quit-and-goto-xref works well, though.

It doesn't always honor the intention to open the location in the window 
the command was called from. It can show the location in a different 
window, and then if I press M-, from there, Emacs does not return to the 
previous window configuration.

I've tracked this down to xref--show-pos-in-buf. Apparently, calling 
display-buffer with

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

as its second argument doesn't do the trick. Looking at it, it contains 
logic where when (eq window (selected-window)), it sets best-window to 
something else (in particular, it can favor a window where this buffer 
had been displayed previously). So, should some other action function be 
used?

I can trace this choice back to your commit 94b320849e9 where this bug 
was apparently introduced.