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

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
Tags: patch
Severity: wishlist
X-Debbugs-CC: João Távora <[hidden email]>, Dmitry Gutov <[hidden email]>

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:


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)
Reply | Threaded
Open this post in threaded view
|

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

João Távora
On Sat, Jan 5, 2019, 23:51 Juri Linkov <[hidden email] wrote:
Tags: patch
Severity: wishlist
X-Debbugs-CC: João Távora <[hidden email]>, Dmitry Gutov <[hidden email]>

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

Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
>> 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.



Reply | Threaded
Open this post in threaded view
|

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

João Távora
On Wed, Mar 20, 2019 at 10:35 PM Juri Linkov <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Dmitry Gutov
In reply to this post by Juri Linkov-2
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?



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
>> 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?



Reply | Threaded
Open this post in threaded view
|

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?



Reply | Threaded
Open this post in threaded view
|

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

Felicián Németh
(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)



Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
In reply to this post by Dmitry Gutov
>>> 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.



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
In reply to this post by Felicián Németh
> 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.



Reply | Threaded
Open this post in threaded view
|

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

Dmitry Gutov
In reply to this post by Juri Linkov-2
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.



Reply | Threaded
Open this post in threaded view
|

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

Juri Linkov-2
>>>>> 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.



Reply | Threaded
Open this post in threaded view
|

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.