* bug#74361: [PATCH] New option xref-navigation-display-window-action
@ 2024-11-14 22:29 Dmitry Gutov
2024-11-15 0:50 ` Dmitry Gutov
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Dmitry Gutov @ 2024-11-14 22:29 UTC (permalink / raw)
To: 74361; +Cc: juri linkov
[-- Attachment #1: Type: text/plain, Size: 868 bytes --]
X-Debbugs-Cc: Juri Linkov <juri@linkov.net>
This adds a capability to customize the destination window selection
logic for navigation (xref-find-definitions, xref-go-back,
xref-go-forward) by allowing a user-supplied display window function.
Inspired by the Merlin package and its user option
merlin-locate-in-new-window
(https://github.com/ocaml/merlin/blob/a36f42a5b181d0c9cc84174e8eb241b11eeabc0f/emacs/merlin.el#L177C12-L177C39)
- where the value 'diff' uses a different window if the destination is
in an file different from the current one.
With the attached patch the customization looks a bit noisier though:
(setq xref-navigation-display-window-action
'(display-buffer-reuse-window))
^ This makes it try to reuse an existing window and fall back to
pop-to-window, but the effect is similar to what's described above.
Comments welcome.
[-- Attachment #2: xref-navigation-display-window-action.diff --]
[-- Type: text/x-patch, Size: 3980 bytes --]
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index cc06e06ef78..670e80ea40b 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -431,6 +431,21 @@ xref-auto-jump-to-first-xref
:version "28.1"
:package-version '(xref . "1.2.0"))
+(defcustom xref-navigation-display-window-action nil
+ "When non-nil, the display action to use for navigation commands.
+
+The value should be nil or a buffer display action like described in
+docstring for `display-buffer'.
+
+This does not affect commands that specify the action explicitly,
+such as `xref-find-definitions-other-window'."
+ :type '(choice (const :tag "Use selected window" nil)
+ (const :tag "Reuse window showing destination or use another"
+ (display-buffer-reuse-window))
+ display-buffer--action-custom-type)
+ :version "31.1"
+ :package-version '(xref . "1.8.0"))
+
(defcustom xref-history-storage #'xref-global-history
"Function that returns xref history.
@@ -513,6 +528,11 @@ xref-push-marker-stack
;;;###autoload
(define-obsolete-function-alias 'xref-pop-marker-stack #'xref-go-back "29.1")
+(defun xref--switch-to-buffer (buf)
+ (if xref-navigation-display-window-action
+ (pop-to-buffer buf xref-navigation-display-window-action)
+ (switch-to-buffer buf)))
+
;;;###autoload
(defun xref-go-back ()
"Go back to the previous position in xref history.
@@ -523,8 +543,8 @@ xref-go-back
(user-error "At start of xref history")
(let ((marker (pop (car history))))
(xref--push-forward (point-marker))
- (switch-to-buffer (or (marker-buffer marker)
- (user-error "The marked buffer has been deleted")))
+ (xref--switch-to-buffer (or (marker-buffer marker)
+ (user-error "The marked buffer has been deleted")))
(goto-char (marker-position marker))
(set-marker marker nil nil)
(run-hooks 'xref-after-return-hook)))))
@@ -538,8 +558,8 @@ xref-go-forward
(user-error "At end of xref history")
(let ((marker (pop (cdr history))))
(xref--push-backward (point-marker))
- (switch-to-buffer (or (marker-buffer marker)
- (user-error "The marked buffer has been deleted")))
+ (xref--switch-to-buffer (or (marker-buffer marker)
+ (user-error "The marked buffer has been deleted")))
(goto-char (marker-position marker))
(set-marker marker nil nil)
(run-hooks 'xref-after-return-hook)))))
@@ -612,7 +632,7 @@ xref-pop-to-location
(xref-location-marker (xref-item-location item))))
(buf (marker-buffer marker)))
(cl-ecase action
- ((nil) (switch-to-buffer buf))
+ ((nil) (xref--switch-to-buffer buf))
(window (pop-to-buffer buf t))
(frame (let ((pop-up-frames t)) (pop-to-buffer buf t))))
(xref--goto-char marker))
@@ -683,6 +703,9 @@ xref--show-pos-in-buf
((eq xref--original-window-intent 'window)
`((xref--display-buffer-in-other-window)
(window . ,xref--original-window)))
+ (xref-navigation-display-window-action
+ `(,xref-navigation-display-window-action
+ (window . ,xref--original-window)))
((and
(window-live-p xref--original-window)
(or (not (window-dedicated-p xref--original-window))
@@ -1628,6 +1651,9 @@ xref-find-definitions
Otherwise, display the list of the possible definitions in a
buffer where the user can select from the list.
+See also `xref-navigation-display-window-action' which can change
+the destination window.
+
Use \\[xref-go-back] to return back to where you invoked this command."
(interactive (list (xref--read-identifier "Find definitions of: ")))
(xref--find-definitions identifier nil))
^ permalink raw reply related [flat|nested] 20+ messages in thread* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-14 22:29 bug#74361: [PATCH] New option xref-navigation-display-window-action Dmitry Gutov
@ 2024-11-15 0:50 ` Dmitry Gutov
2024-11-15 7:49 ` Juri Linkov
2024-11-15 12:13 ` Eli Zaretskii
2 siblings, 0 replies; 20+ messages in thread
From: Dmitry Gutov @ 2024-11-15 0:50 UTC (permalink / raw)
To: 74361; +Cc: juri linkov
[-- Attachment #1: Type: text/plain, Size: 232 bytes --]
On 15/11/2024 00:29, Dmitry Gutov wrote:
> Comments welcome.
Here's a small revision in xref--show-pos-in-buf that allows including
ALIST elements as well - just because those are allowed in the structure
of ACTION as documented.
[-- Attachment #2: xref-navigation-display-window-action-v2.diff --]
[-- Type: text/x-patch, Size: 4007 bytes --]
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index cc06e06ef78..7d296191da8 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -431,6 +431,21 @@ xref-auto-jump-to-first-xref
:version "28.1"
:package-version '(xref . "1.2.0"))
+(defcustom xref-navigation-display-window-action nil
+ "When non-nil, the display action to use for navigation commands.
+
+The value should be nil or a buffer display action like described in
+docstring for `display-buffer'.
+
+This does not affect commands that specify the action explicitly,
+such as `xref-find-definitions-other-window'."
+ :type '(choice (const :tag "Use selected window" nil)
+ (const :tag "Reuse window showing destination or use another"
+ (display-buffer-reuse-window))
+ display-buffer--action-custom-type)
+ :version "31.1"
+ :package-version '(xref . "1.8.0"))
+
(defcustom xref-history-storage #'xref-global-history
"Function that returns xref history.
@@ -513,6 +528,11 @@ xref-push-marker-stack
;;;###autoload
(define-obsolete-function-alias 'xref-pop-marker-stack #'xref-go-back "29.1")
+(defun xref--switch-to-buffer (buf)
+ (if xref-navigation-display-window-action
+ (pop-to-buffer buf xref-navigation-display-window-action)
+ (switch-to-buffer buf)))
+
;;;###autoload
(defun xref-go-back ()
"Go back to the previous position in xref history.
@@ -523,8 +543,8 @@ xref-go-back
(user-error "At start of xref history")
(let ((marker (pop (car history))))
(xref--push-forward (point-marker))
- (switch-to-buffer (or (marker-buffer marker)
- (user-error "The marked buffer has been deleted")))
+ (xref--switch-to-buffer (or (marker-buffer marker)
+ (user-error "The marked buffer has been deleted")))
(goto-char (marker-position marker))
(set-marker marker nil nil)
(run-hooks 'xref-after-return-hook)))))
@@ -538,8 +558,8 @@ xref-go-forward
(user-error "At end of xref history")
(let ((marker (pop (cdr history))))
(xref--push-backward (point-marker))
- (switch-to-buffer (or (marker-buffer marker)
- (user-error "The marked buffer has been deleted")))
+ (xref--switch-to-buffer (or (marker-buffer marker)
+ (user-error "The marked buffer has been deleted")))
(goto-char (marker-position marker))
(set-marker marker nil nil)
(run-hooks 'xref-after-return-hook)))))
@@ -612,7 +632,7 @@ xref-pop-to-location
(xref-location-marker (xref-item-location item))))
(buf (marker-buffer marker)))
(cl-ecase action
- ((nil) (switch-to-buffer buf))
+ ((nil) (xref--switch-to-buffer buf))
(window (pop-to-buffer buf t))
(frame (let ((pop-up-frames t)) (pop-to-buffer buf t))))
(xref--goto-char marker))
@@ -683,6 +703,10 @@ xref--show-pos-in-buf
((eq xref--original-window-intent 'window)
`((xref--display-buffer-in-other-window)
(window . ,xref--original-window)))
+ (xref-navigation-display-window-action
+ (append
+ xref-navigation-display-window-action
+ `((window . ,xref--original-window))))
((and
(window-live-p xref--original-window)
(or (not (window-dedicated-p xref--original-window))
@@ -1628,6 +1652,9 @@ xref-find-definitions
Otherwise, display the list of the possible definitions in a
buffer where the user can select from the list.
+See also `xref-navigation-display-window-action' which can change
+the destination window.
+
Use \\[xref-go-back] to return back to where you invoked this command."
(interactive (list (xref--read-identifier "Find definitions of: ")))
(xref--find-definitions identifier nil))
^ permalink raw reply related [flat|nested] 20+ messages in thread* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-14 22:29 bug#74361: [PATCH] New option xref-navigation-display-window-action Dmitry Gutov
2024-11-15 0:50 ` Dmitry Gutov
@ 2024-11-15 7:49 ` Juri Linkov
2024-11-15 19:05 ` Dmitry Gutov
2024-11-15 12:13 ` Eli Zaretskii
2 siblings, 1 reply; 20+ messages in thread
From: Juri Linkov @ 2024-11-15 7:49 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 74361
> This adds a capability to customize the destination window selection logic
> for navigation (xref-find-definitions, xref-go-back, xref-go-forward) by
> allowing a user-supplied display window function.
>
> Inspired by the Merlin package and its user option
> merlin-locate-in-new-window
> (https://github.com/ocaml/merlin/blob/a36f42a5b181d0c9cc84174e8eb241b11eeabc0f/emacs/merlin.el#L177C12-L177C39)
> - where the value 'diff' uses a different window if the destination is in
> an file different from the current one.
>
> With the attached patch the customization looks a bit noisier though:
>
> (setq xref-navigation-display-window-action
> '(display-buffer-reuse-window))
>
> ^ This makes it try to reuse an existing window and fall back to
> pop-to-window, but the effect is similar to what's described above.
>
> Comments welcome.
This option looks similar to 'display-comint-buffer-action'
whose addition was deemed to be a mistake, so we needed to declare it
obsolete and replace it with '(category . comint)'.
So instead of adding 'xref-navigation-display-window-action',
could you just add a category 'xref' to xref display function calls?
Then users will be able to customize it with e.g.:
(setq display-buffer-alist '(((category . xref)
(display-buffer-reuse-window))))
Or maybe I misunderstand other requirements for this feature.
Ok, meanwhile I'll play more with your patch to see if 'category'
could really help here.
^ permalink raw reply [flat|nested] 20+ messages in thread* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-15 7:49 ` Juri Linkov
@ 2024-11-15 19:05 ` Dmitry Gutov
2024-11-16 19:12 ` Juri Linkov
0 siblings, 1 reply; 20+ messages in thread
From: Dmitry Gutov @ 2024-11-15 19:05 UTC (permalink / raw)
To: Juri Linkov; +Cc: 74361
Hi Juri,
On 15/11/2024 09:49, Juri Linkov wrote:
> This option looks similar to 'display-comint-buffer-action'
> whose addition was deemed to be a mistake, so we needed to declare it
> obsolete and replace it with '(category . comint)'.
>
> So instead of adding 'xref-navigation-display-window-action',
> could you just add a category 'xref' to xref display function calls?
> Then users will be able to customize it with e.g.:
>
> (setq display-buffer-alist '(((category . xref)
> (display-buffer-reuse-window))))
>
> Or maybe I misunderstand other requirements for this feature.
Thanks for the comments. The other priority here (I think) seems to
retain the current behavior by default:
You can see this decision being made in
(defun xref--switch-to-buffer (buf)
(if xref-navigation-display-window-action
(pop-to-buffer buf xref-navigation-display-window-action)
(switch-to-buffer buf)))
So... I suppose one way to do that would be to add an option in Xref
which make it use 'pop-to-buffer' instead of 'switch-to-buffer' (similar
to 'switch-to-buffer-obey-display-actions'). And then the user would
customize 'display-buffer-alist' like in your example. Something like:
(setq xref-navigation-obey-display-actions t
display-buffer-alist '(((category . xref)
(display-buffer-reuse-window))))
Is that not too complex, what do we think?
> Ok, meanwhile I'll play more with your patch to see if 'category'
> could really help here.
Thank you.
^ permalink raw reply [flat|nested] 20+ messages in thread* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-15 19:05 ` Dmitry Gutov
@ 2024-11-16 19:12 ` Juri Linkov
2024-11-18 1:28 ` Dmitry Gutov
0 siblings, 1 reply; 20+ messages in thread
From: Juri Linkov @ 2024-11-16 19:12 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 74361
>> This option looks similar to 'display-comint-buffer-action'
>> whose addition was deemed to be a mistake, so we needed to declare it
>> obsolete and replace it with '(category . comint)'.
>> So instead of adding 'xref-navigation-display-window-action',
>> could you just add a category 'xref' to xref display function calls?
>> Then users will be able to customize it with e.g.:
>> (setq display-buffer-alist '(((category . xref)
>> (display-buffer-reuse-window))))
>> Or maybe I misunderstand other requirements for this feature.
>
> Thanks for the comments. The other priority here (I think) seems to retain
> the current behavior by default:
>
> You can see this decision being made in
>
> (defun xref--switch-to-buffer (buf)
> (if xref-navigation-display-window-action
> (pop-to-buffer buf xref-navigation-display-window-action)
> (switch-to-buffer buf)))
>
> So... I suppose one way to do that would be to add an option in Xref which
> make it use 'pop-to-buffer' instead of 'switch-to-buffer' (similar to
> 'switch-to-buffer-obey-display-actions'). And then the user would customize
> 'display-buffer-alist' like in your example. Something like:
>
> (setq xref-navigation-obey-display-actions t
> display-buffer-alist '(((category . xref)
> (display-buffer-reuse-window))))
>
> Is that not too complex, what do we think?
Instead of a new option and 'switch-to-buffer',
a simpler way would be allow the users just to
customize the category with 'display-buffer-same-window':
(setq display-buffer-alist '(((category . xref)
(display-buffer-same-window))))
It works nicely when (category . xref) is added to the same place
like in your patch in 'xref--show-pos-in-buf':
```
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index cf061a18ee0..0193c2e35e0 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -689,7 +689,7 @@ xref--show-pos-in-buf
(eq (window-buffer xref--original-window) buf)))
`((xref--display-buffer-in-window)
(window . ,xref--original-window))))))
- (with-selected-window (display-buffer buf action)
+ (with-selected-window (display-buffer buf (append action '((category . xref))))
(xref--goto-char pos)
(run-hooks 'xref-after-jump-hook)
(selected-window))))
```
^ permalink raw reply related [flat|nested] 20+ messages in thread* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-16 19:12 ` Juri Linkov
@ 2024-11-18 1:28 ` Dmitry Gutov
2024-11-19 18:33 ` Juri Linkov
0 siblings, 1 reply; 20+ messages in thread
From: Dmitry Gutov @ 2024-11-18 1:28 UTC (permalink / raw)
To: Juri Linkov; +Cc: 74361
[-- Attachment #1: Type: text/plain, Size: 2112 bytes --]
On 16/11/2024 21:12, Juri Linkov wrote:
> Instead of a new option and 'switch-to-buffer',
> a simpler way would be allow the users just to
> customize the category with 'display-buffer-same-window':
>
> (setq display-buffer-alist '(((category . xref)
> (display-buffer-same-window))))
>
> It works nicely when (category . xref) is added to the same place
> like in your patch in 'xref--show-pos-in-buf':
>
> ```
> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
> index cf061a18ee0..0193c2e35e0 100644
> --- a/lisp/progmodes/xref.el
> +++ b/lisp/progmodes/xref.el
> @@ -689,7 +689,7 @@ xref--show-pos-in-buf
> (eq (window-buffer xref--original-window) buf)))
> `((xref--display-buffer-in-window)
> (window . ,xref--original-window))))))
> - (with-selected-window (display-buffer buf action)
> + (with-selected-window (display-buffer buf (append action '((category . xref))))
> (xref--goto-char pos)
> (run-hooks 'xref-after-jump-hook)
> (selected-window))))
> ```
Thanks! That covers the code path which uses 'display-buffer', and we
also need to handle xref-pop-to-location, which calls switch-to-buffer
now. Does
(pop-to-buffer buf '((display-buffer-same-window) (category . xref)))
look compatible enough?
Also, when using the suggested change in xref--show-pos-in-buf, if we
simply annotate it with 'category' in the display-buffer call, this also
extends to the scenarios where xref--original-window-intent is non-nil.
See the attached patch where we make sure to exclude those cases. I
suppose that loses the semantic consistency, though:
xref-find-definitions-other-window and xref-find-definitions-other-frame
are "xref" commands but they will call 'display-buffer' without 'category'.
The corresponding customization (described in the first message) can be:
(setq display-buffer-alist '(((category . xref)
(display-buffer-reuse-window
display-buffer-use-some-window))))
[-- Attachment #2: xref-display-buffer-with-category.diff --]
[-- Type: text/x-patch, Size: 2655 bytes --]
diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index cc06e06ef78..56e4b0c7518 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -513,6 +513,9 @@ xref-push-marker-stack
;;;###autoload
(define-obsolete-function-alias 'xref-pop-marker-stack #'xref-go-back "29.1")
+(defun xref--switch-to-buffer (buf)
+ (pop-to-buffer buf '((display-buffer-same-window) (category . xref))))
+
;;;###autoload
(defun xref-go-back ()
"Go back to the previous position in xref history.
@@ -523,8 +526,8 @@ xref-go-back
(user-error "At start of xref history")
(let ((marker (pop (car history))))
(xref--push-forward (point-marker))
- (switch-to-buffer (or (marker-buffer marker)
- (user-error "The marked buffer has been deleted")))
+ (xref--switch-to-buffer (or (marker-buffer marker)
+ (user-error "The marked buffer has been deleted")))
(goto-char (marker-position marker))
(set-marker marker nil nil)
(run-hooks 'xref-after-return-hook)))))
@@ -538,8 +541,8 @@ xref-go-forward
(user-error "At end of xref history")
(let ((marker (pop (cdr history))))
(xref--push-backward (point-marker))
- (switch-to-buffer (or (marker-buffer marker)
- (user-error "The marked buffer has been deleted")))
+ (xref--switch-to-buffer (or (marker-buffer marker)
+ (user-error "The marked buffer has been deleted")))
(goto-char (marker-position marker))
(set-marker marker nil nil)
(run-hooks 'xref-after-return-hook)))))
@@ -612,7 +615,7 @@ xref-pop-to-location
(xref-location-marker (xref-item-location item))))
(buf (marker-buffer marker)))
(cl-ecase action
- ((nil) (switch-to-buffer buf))
+ ((nil) (xref--switch-to-buffer buf))
(window (pop-to-buffer buf t))
(frame (let ((pop-up-frames t)) (pop-to-buffer buf t))))
(xref--goto-char marker))
@@ -688,7 +691,10 @@ xref--show-pos-in-buf
(or (not (window-dedicated-p xref--original-window))
(eq (window-buffer xref--original-window) buf)))
`((xref--display-buffer-in-window)
- (window . ,xref--original-window))))))
+ (category . xref)
+ (window . ,xref--original-window)))
+ (t
+ '((category . xref))))))
(with-selected-window (display-buffer buf action)
(xref--goto-char pos)
(run-hooks 'xref-after-jump-hook)
^ permalink raw reply related [flat|nested] 20+ messages in thread* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-18 1:28 ` Dmitry Gutov
@ 2024-11-19 18:33 ` Juri Linkov
2024-11-19 19:43 ` Dmitry Gutov
0 siblings, 1 reply; 20+ messages in thread
From: Juri Linkov @ 2024-11-19 18:33 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: martin rudalics, 74361
> Thanks! That covers the code path which uses 'display-buffer', and we also
> need to handle xref-pop-to-location, which calls switch-to-buffer now. Does
>
> (pop-to-buffer buf '((display-buffer-same-window) (category . xref)))
>
> look compatible enough?
Maybe Martin could confirm whether this is equivalent to switch-to-buffer.
> Also, when using the suggested change in xref--show-pos-in-buf, if we
> simply annotate it with 'category' in the display-buffer call, this also
> extends to the scenarios where xref--original-window-intent is non-nil.
>
> See the attached patch where we make sure to exclude those cases. I suppose
> that loses the semantic consistency, though:
> xref-find-definitions-other-window and xref-find-definitions-other-frame
> are "xref" commands but they will call 'display-buffer' without 'category'.
I agree the category should not override the window when used
from a command that specifies a window/frame.
> The corresponding customization (described in the first message) can be:
>
> (setq display-buffer-alist '(((category . xref)
> (display-buffer-reuse-window
> display-buffer-use-some-window))))
I tried your patch, and it works even with 'mru':
(setq display-buffer-alist '(((category . xref)
(display-buffer-reuse-window
display-buffer-use-some-window)
(some-window . mru))))
^ permalink raw reply [flat|nested] 20+ messages in thread* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-19 18:33 ` Juri Linkov
@ 2024-11-19 19:43 ` Dmitry Gutov
0 siblings, 0 replies; 20+ messages in thread
From: Dmitry Gutov @ 2024-11-19 19:43 UTC (permalink / raw)
To: Juri Linkov; +Cc: martin rudalics, 74361
On 19/11/2024 20:33, Juri Linkov wrote:
>> Thanks! That covers the code path which uses 'display-buffer', and we also
>> need to handle xref-pop-to-location, which calls switch-to-buffer now. Does
>>
>> (pop-to-buffer buf '((display-buffer-same-window) (category . xref)))
>>
>> look compatible enough?
>
> Maybe Martin could confirm whether this is equivalent to switch-to-buffer.
display-buffer-same-window *is* the function that's ultimately used by
switch-to-buffer. This does bring some incompatibilities, though, such
as forcung switch-to-buffer-in-dedicated-window to nil, or the use of
display-buffer-alist in the first place (normally configured by
switch-to-buffer-obey-display-actions).
>> Also, when using the suggested change in xref--show-pos-in-buf, if we
>> simply annotate it with 'category' in the display-buffer call, this also
>> extends to the scenarios where xref--original-window-intent is non-nil.
>>
>> See the attached patch where we make sure to exclude those cases. I suppose
>> that loses the semantic consistency, though:
>> xref-find-definitions-other-window and xref-find-definitions-other-frame
>> are "xref" commands but they will call 'display-buffer' without 'category'.
>
> I agree the category should not override the window when used
> from a command that specifies a window/frame.
Okay then.
>> The corresponding customization (described in the first message) can be:
>>
>> (setq display-buffer-alist '(((category . xref)
>> (display-buffer-reuse-window
>> display-buffer-use-some-window))))
>
> I tried your patch, and it works even with 'mru':
>
> (setq display-buffer-alist '(((category . xref)
> (display-buffer-reuse-window
> display-buffer-use-some-window)
> (some-window . mru))))
Yep, even the 'mru' strategy doesn't choose the currently selected
window, so this should also work well, selecting "some window" in a
stable fashion (whereas 'lru' - the default - uses a different window
each time).
^ permalink raw reply [flat|nested] 20+ messages in thread
* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-14 22:29 bug#74361: [PATCH] New option xref-navigation-display-window-action Dmitry Gutov
2024-11-15 0:50 ` Dmitry Gutov
2024-11-15 7:49 ` Juri Linkov
@ 2024-11-15 12:13 ` Eli Zaretskii
2024-11-15 17:20 ` martin rudalics via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-11-15 19:10 ` Dmitry Gutov
2 siblings, 2 replies; 20+ messages in thread
From: Eli Zaretskii @ 2024-11-15 12:13 UTC (permalink / raw)
To: Dmitry Gutov, martin rudalics; +Cc: 74361, juri
> Cc: juri linkov <juri@linkov.net>
> Date: Fri, 15 Nov 2024 00:29:14 +0200
> From: Dmitry Gutov <dmitry@gutov.dev>
>
> This adds a capability to customize the destination window selection
> logic for navigation (xref-find-definitions, xref-go-back,
> xref-go-forward) by allowing a user-supplied display window function.
>
> Inspired by the Merlin package and its user option
> merlin-locate-in-new-window
> (https://github.com/ocaml/merlin/blob/a36f42a5b181d0c9cc84174e8eb241b11eeabc0f/emacs/merlin.el#L177C12-L177C39)
> - where the value 'diff' uses a different window if the destination is
> in an file different from the current one.
>
> With the attached patch the customization looks a bit noisier though:
>
> (setq xref-navigation-display-window-action
> '(display-buffer-reuse-window))
>
> ^ This makes it try to reuse an existing window and fall back to
> pop-to-window, but the effect is similar to what's described above.
>
> Comments welcome.
I added Martin to the discussion, and have a few minor comments below.
> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
> index cc06e06ef78..670e80ea40b 100644
> --- a/lisp/progmodes/xref.el
> +++ b/lisp/progmodes/xref.el
> @@ -431,6 +431,21 @@ xref-auto-jump-to-first-xref
> :version "28.1"
> :package-version '(xref . "1.2.0"))
>
> +(defcustom xref-navigation-display-window-action nil
> + "When non-nil, the display action to use for navigation commands.
This is too general, when taken alone (as in the apropos commands). I
suggest something like
If non-nil, the `display-buffer' action for showing results of Xref commands.
(The "navigation" part seems misleading, since xref-find-definitions
is not a navigation command.)
> +This does not affect commands that specify the action explicitly,
I guess "...that specify the window to use explicitly" is more
accurate?
> +such as `xref-find-definitions-other-window'."
> + :type '(choice (const :tag "Use selected window" nil)
> + (const :tag "Reuse window showing destination or use another"
I think "If possible, reuse window already showing destination" is
better?
> +(defun xref--switch-to-buffer (buf)
> + (if xref-navigation-display-window-action
> + (pop-to-buffer buf xref-navigation-display-window-action)
Should we have some sanity checks for the value of
xref-navigation-display-window-action? It's a user option, so
theoretically the user could use setq to set it to any value.
^ permalink raw reply [flat|nested] 20+ messages in thread
* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-15 12:13 ` Eli Zaretskii
@ 2024-11-15 17:20 ` martin rudalics via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-11-15 19:10 ` Dmitry Gutov
1 sibling, 0 replies; 20+ messages in thread
From: martin rudalics via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-11-15 17:20 UTC (permalink / raw)
To: Eli Zaretskii, Dmitry Gutov; +Cc: 74361, juri
>> Comments welcome.
>
> I added Martin to the discussion, and have a few minor comments below.
Let's see whether Juri has any success with adding an 'xref' category
first.
martin
^ permalink raw reply [flat|nested] 20+ messages in thread
* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-15 12:13 ` Eli Zaretskii
2024-11-15 17:20 ` martin rudalics via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-11-15 19:10 ` Dmitry Gutov
2024-11-16 8:43 ` Eli Zaretskii
1 sibling, 1 reply; 20+ messages in thread
From: Dmitry Gutov @ 2024-11-15 19:10 UTC (permalink / raw)
To: Eli Zaretskii, martin rudalics; +Cc: 74361, juri
Hi Eli,
On 15/11/2024 14:13, Eli Zaretskii wrote:
> I added Martin to the discussion, and have a few minor comments below.
Thanks.
>> +(defcustom xref-navigation-display-window-action nil
>> + "When non-nil, the display action to use for navigation commands.
>
> This is too general, when taken alone (as in the apropos commands). I
> suggest something like
>
> If non-nil, the `display-buffer' action for showing results of Xref commands.
>
> (The "navigation" part seems misleading, since xref-find-definitions
> is not a navigation command.)
That's the term I would use both both, but maybe there could be better
wording. If xref-find-definition is not a navigation command, is it a
"search command"?
>> +This does not affect commands that specify the action explicitly,
>
> I guess "...that specify the window to use explicitly" is more
> accurate?
Or frame. We basically have two built-in commands like that: one
specifies the window (kind of -- only that the "other" window should be
used, but not a specific window), and another for "other frame".
>> +such as `xref-find-definitions-other-window'."
>> + :type '(choice (const :tag "Use selected window" nil)
>> + (const :tag "Reuse window showing destination or use another"
>
> I think "If possible, reuse window already showing destination" is
> better?
This is better, thanks.
>> +(defun xref--switch-to-buffer (buf)
>> + (if xref-navigation-display-window-action
>> + (pop-to-buffer buf xref-navigation-display-window-action)
>
> Should we have some sanity checks for the value of
> xref-navigation-display-window-action? It's a user option, so
> theoretically the user could use setq to set it to any value.
I think whatever sanity checks 'display-buffer' has for the ACTION
argument would be used anyway. Also this might be moot given the
"category" suggestion.
^ permalink raw reply [flat|nested] 20+ messages in thread
* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-15 19:10 ` Dmitry Gutov
@ 2024-11-16 8:43 ` Eli Zaretskii
2024-11-18 1:42 ` Dmitry Gutov
0 siblings, 1 reply; 20+ messages in thread
From: Eli Zaretskii @ 2024-11-16 8:43 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: rudalics, 74361, juri
> Date: Fri, 15 Nov 2024 21:10:25 +0200
> Cc: 74361@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dmitry@gutov.dev>
>
> >> +(defcustom xref-navigation-display-window-action nil
> >> + "When non-nil, the display action to use for navigation commands.
> >
> > This is too general, when taken alone (as in the apropos commands). I
> > suggest something like
> >
> > If non-nil, the `display-buffer' action for showing results of Xref commands.
> >
> > (The "navigation" part seems misleading, since xref-find-definitions
> > is not a navigation command.)
>
> That's the term I would use both both, but maybe there could be better
> wording. If xref-find-definition is not a navigation command, is it a
> "search command"?
Let me turn the table and ask: which Xref commands will NOT use this
action, if we exclude commands like
xref-find-definitions-other-window, which specify the window/frame to
use?
> >> +This does not affect commands that specify the action explicitly,
> >
> > I guess "...that specify the window to use explicitly" is more
> > accurate?
>
> Or frame.
Yes, so "...specify the window/frame to use...".
^ permalink raw reply [flat|nested] 20+ messages in thread
* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-16 8:43 ` Eli Zaretskii
@ 2024-11-18 1:42 ` Dmitry Gutov
2024-11-18 12:25 ` Eli Zaretskii
2024-11-19 18:36 ` Juri Linkov
0 siblings, 2 replies; 20+ messages in thread
From: Dmitry Gutov @ 2024-11-18 1:42 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: rudalics, 74361, juri
On 16/11/2024 10:43, Eli Zaretskii wrote:
>> That's the term I would use both both, but maybe there could be better
>> wording. If xref-find-definition is not a navigation command, is it a
>> "search command"?
> Let me turn the table and ask: which Xref commands will NOT use this
> action, if we exclude commands like
> xref-find-definitions-other-window, which specify the window/frame to
> use?
xref-query-replace-in-results will not (or its twin
xref-find-references-and-replace). I suppose not many people would
expect them to.
Also, there is a nuance: when the Xref buffer itself is shown (i.e. when
there are multiple locations matching a xref-find-definitions search),
we're not going to pass (category . xref) to display-buffer either - it
is reserved for displaying the buffers of destination locations.
^ permalink raw reply [flat|nested] 20+ messages in thread
* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-18 1:42 ` Dmitry Gutov
@ 2024-11-18 12:25 ` Eli Zaretskii
2024-11-18 16:10 ` Dmitry Gutov
2024-11-19 18:36 ` Juri Linkov
1 sibling, 1 reply; 20+ messages in thread
From: Eli Zaretskii @ 2024-11-18 12:25 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: rudalics, 74361, juri
> Date: Mon, 18 Nov 2024 03:42:34 +0200
> Cc: rudalics@gmx.at, 74361@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dmitry@gutov.dev>
>
> On 16/11/2024 10:43, Eli Zaretskii wrote:
> >> That's the term I would use both both, but maybe there could be better
> >> wording. If xref-find-definition is not a navigation command, is it a
> >> "search command"?
> > Let me turn the table and ask: which Xref commands will NOT use this
> > action, if we exclude commands like
> > xref-find-definitions-other-window, which specify the window/frame to
> > use?
>
> xref-query-replace-in-results will not (or its twin
> xref-find-references-and-replace). I suppose not many people would
> expect them to.
This seems to indicate that my proposal is actually okay, since the
above two commands do not "show results of Xref commands"?
> Also, there is a nuance: when the Xref buffer itself is shown (i.e. when
> there are multiple locations matching a xref-find-definitions search),
> we're not going to pass (category . xref) to display-buffer either - it
> is reserved for displaying the buffers of destination locations.
And this is a separate issue, not related to the doc string?
^ permalink raw reply [flat|nested] 20+ messages in thread
* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-18 12:25 ` Eli Zaretskii
@ 2024-11-18 16:10 ` Dmitry Gutov
2024-11-18 17:03 ` Eli Zaretskii
0 siblings, 1 reply; 20+ messages in thread
From: Dmitry Gutov @ 2024-11-18 16:10 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: rudalics, 74361, juri
On 18/11/2024 14:25, Eli Zaretskii wrote:
>> Date: Mon, 18 Nov 2024 03:42:34 +0200
>> Cc: rudalics@gmx.at, 74361@debbugs.gnu.org, juri@linkov.net
>> From: Dmitry Gutov <dmitry@gutov.dev>
>>
>> On 16/11/2024 10:43, Eli Zaretskii wrote:
>>>> That's the term I would use both both, but maybe there could be better
>>>> wording. If xref-find-definition is not a navigation command, is it a
>>>> "search command"?
>>> Let me turn the table and ask: which Xref commands will NOT use this
>>> action, if we exclude commands like
>>> xref-find-definitions-other-window, which specify the window/frame to
>>> use?
>>
>> xref-query-replace-in-results will not (or its twin
>> xref-find-references-and-replace). I suppose not many people would
>> expect them to.
>
> This seems to indicate that my proposal is actually okay, since the
> above two commands do not "show results of Xref commands"?
IIUC your suggestion was:
If non-nil, the `display-buffer' action for showing results of Xref
commands.
And those are commands belonging to the package (for the moment), and as
such fitting the description.
>> Also, there is a nuance: when the Xref buffer itself is shown (i.e. when
>> there are multiple locations matching a xref-find-definitions search),
>> we're not going to pass (category . xref) to display-buffer either - it
>> is reserved for displaying the buffers of destination locations.
>
> And this is a separate issue, not related to the doc string?
Still about the doc string. If we say "showing results of Xref
commands", then the Xref buffer with the list of locations also matches
that description, doesn't it? Or rather it might be the first thing a
user would think of - but our customization wouldn't apply to it
immediately.
Note that this subthread might seem moot if we don't introduce a new
option anyway, but somewhere we'd probably want to enumerate the cases
which the 'xref' category applies to, and those would be the same.
^ permalink raw reply [flat|nested] 20+ messages in thread
* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-18 16:10 ` Dmitry Gutov
@ 2024-11-18 17:03 ` Eli Zaretskii
2024-11-19 1:21 ` Dmitry Gutov
0 siblings, 1 reply; 20+ messages in thread
From: Eli Zaretskii @ 2024-11-18 17:03 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: rudalics, 74361, juri
> Date: Mon, 18 Nov 2024 18:10:02 +0200
> Cc: rudalics@gmx.at, 74361@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dmitry@gutov.dev>
>
> >> xref-query-replace-in-results will not (or its twin
> >> xref-find-references-and-replace). I suppose not many people would
> >> expect them to.
> >
> > This seems to indicate that my proposal is actually okay, since the
> > above two commands do not "show results of Xref commands"?
>
> IIUC your suggestion was:
>
> If non-nil, the `display-buffer' action for showing results of Xref
> commands.
>
> And those are commands belonging to the package (for the moment), and as
> such fitting the description.
Not in my book (they are replacement commands), but feel free to find
better text.
> >> Also, there is a nuance: when the Xref buffer itself is shown (i.e. when
> >> there are multiple locations matching a xref-find-definitions search),
> >> we're not going to pass (category . xref) to display-buffer either - it
> >> is reserved for displaying the buffers of destination locations.
> >
> > And this is a separate issue, not related to the doc string?
>
> Still about the doc string.
But not important enough to affect the first line of it.
> If we say "showing results of Xref
> commands", then the Xref buffer with the list of locations also matches
> that description, doesn't it? Or rather it might be the first thing a
> user would think of - but our customization wouldn't apply to it
> immediately.
The rest of the doc string could explain this subtlety.
^ permalink raw reply [flat|nested] 20+ messages in thread
* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-18 17:03 ` Eli Zaretskii
@ 2024-11-19 1:21 ` Dmitry Gutov
2024-11-19 15:33 ` Eli Zaretskii
0 siblings, 1 reply; 20+ messages in thread
From: Dmitry Gutov @ 2024-11-19 1:21 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: rudalics, 74361, juri
On 18/11/2024 19:03, Eli Zaretskii wrote:
>> IIUC your suggestion was:
>>
>> If non-nil, the `display-buffer' action for showing results of Xref
>> commands.
>>
>> And those are commands belonging to the package (for the moment), and as
>> such fitting the description.
>
> Not in my book (they are replacement commands), but feel free to find
> better text.
Okay, if the phrasing still makes sense to you with the above details,
let's use this version.
>>>> Also, there is a nuance: when the Xref buffer itself is shown (i.e. when
>>>> there are multiple locations matching a xref-find-definitions search),
>>>> we're not going to pass (category . xref) to display-buffer either - it
>>>> is reserved for displaying the buffers of destination locations.
>>>
>>> And this is a separate issue, not related to the doc string?
>>
>> Still about the doc string.
>
> But not important enough to affect the first line of it.
>
>> If we say "showing results of Xref
>> commands", then the Xref buffer with the list of locations also matches
>> that description, doesn't it? Or rather it might be the first thing a
>> user would think of - but our customization wouldn't apply to it
>> immediately.
>
> The rest of the doc string could explain this subtlety.
Fair point, thanks.
Next step is to find the appropriate place for it. Maybe not a docstring
if the user option to customize will be the global one
(display-buffer-alist), but either in Commentary or somewhere in the manual.
Speaking of the reference that we used for this solution, it seems that
(category . comint)
is only mentioned in NEWS and in the obsoletion message for
'display-comint-buffer-action'. There is also a hit for it in the
manual, but only in the form "If the caller display-buffer passes a
category as a symbol ...", not saying exactly that comint modes do or
should do that. Maybe that's something to be fixed as well.
^ permalink raw reply [flat|nested] 20+ messages in thread
* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-19 1:21 ` Dmitry Gutov
@ 2024-11-19 15:33 ` Eli Zaretskii
2024-11-19 19:51 ` Dmitry Gutov
0 siblings, 1 reply; 20+ messages in thread
From: Eli Zaretskii @ 2024-11-19 15:33 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: rudalics, 74361, juri
> Date: Tue, 19 Nov 2024 03:21:26 +0200
> Cc: rudalics@gmx.at, 74361@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dmitry@gutov.dev>
>
> Next step is to find the appropriate place for it. Maybe not a docstring
> if the user option to customize will be the global one
> (display-buffer-alist), but either in Commentary or somewhere in the manual.
Maybe just mention in the doc string that the commands use
display-buffer to select the window?
> Speaking of the reference that we used for this solution, it seems that
>
> (category . comint)
>
> is only mentioned in NEWS and in the obsoletion message for
> 'display-comint-buffer-action'. There is also a hit for it in the
> manual, but only in the form "If the caller display-buffer passes a
> category as a symbol ...", not saying exactly that comint modes do or
> should do that. Maybe that's something to be fixed as well.
I will see about documenting it properly, if no one beats me to it.
^ permalink raw reply [flat|nested] 20+ messages in thread
* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-19 15:33 ` Eli Zaretskii
@ 2024-11-19 19:51 ` Dmitry Gutov
0 siblings, 0 replies; 20+ messages in thread
From: Dmitry Gutov @ 2024-11-19 19:51 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: rudalics, 74361, juri
On 19/11/2024 17:33, Eli Zaretskii wrote:
>> Date: Tue, 19 Nov 2024 03:21:26 +0200
>> Cc: rudalics@gmx.at, 74361@debbugs.gnu.org, juri@linkov.net
>> From: Dmitry Gutov <dmitry@gutov.dev>
>>
>> Next step is to find the appropriate place for it. Maybe not a docstring
>> if the user option to customize will be the global one
>> (display-buffer-alist), but either in Commentary or somewhere in the manual.
>
> Maybe just mention in the doc string that the commands use
> display-buffer to select the window?
The important part is using a specific category, not just display-buffer.
And there doesn't seem to be a single docstring, or even a main
docstring to mention this in. There are at least 3 of them:
xref-find-definitions, xref-go-back and xref-go-forward, and these are
not the only ones affected.
>> Speaking of the reference that we used for this solution, it seems that
>>
>> (category . comint)
>>
>> is only mentioned in NEWS and in the obsoletion message for
>> 'display-comint-buffer-action'. There is also a hit for it in the
>> manual, but only in the form "If the caller display-buffer passes a
>> category as a symbol ...", not saying exactly that comint modes do or
>> should do that. Maybe that's something to be fixed as well.
>
> I will see about documenting it properly, if no one beats me to it.
Thanks.
^ permalink raw reply [flat|nested] 20+ messages in thread
* bug#74361: [PATCH] New option xref-navigation-display-window-action
2024-11-18 1:42 ` Dmitry Gutov
2024-11-18 12:25 ` Eli Zaretskii
@ 2024-11-19 18:36 ` Juri Linkov
1 sibling, 0 replies; 20+ messages in thread
From: Juri Linkov @ 2024-11-19 18:36 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: rudalics, Eli Zaretskii, 74361
> Also, there is a nuance: when the Xref buffer itself is shown (i.e. when
> there are multiple locations matching a xref-find-definitions search),
> we're not going to pass (category . xref) to display-buffer either - it is
> reserved for displaying the buffers of destination locations.
For displaying the buffers of destination locations
we could add another category like e.g. 'xref-goto' or 'xref-file'.
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2024-11-19 19:51 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-14 22:29 bug#74361: [PATCH] New option xref-navigation-display-window-action Dmitry Gutov
2024-11-15 0:50 ` Dmitry Gutov
2024-11-15 7:49 ` Juri Linkov
2024-11-15 19:05 ` Dmitry Gutov
2024-11-16 19:12 ` Juri Linkov
2024-11-18 1:28 ` Dmitry Gutov
2024-11-19 18:33 ` Juri Linkov
2024-11-19 19:43 ` Dmitry Gutov
2024-11-15 12:13 ` Eli Zaretskii
2024-11-15 17:20 ` martin rudalics via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-11-15 19:10 ` Dmitry Gutov
2024-11-16 8:43 ` Eli Zaretskii
2024-11-18 1:42 ` Dmitry Gutov
2024-11-18 12:25 ` Eli Zaretskii
2024-11-18 16:10 ` Dmitry Gutov
2024-11-18 17:03 ` Eli Zaretskii
2024-11-19 1:21 ` Dmitry Gutov
2024-11-19 15:33 ` Eli Zaretskii
2024-11-19 19:51 ` Dmitry Gutov
2024-11-19 18:36 ` Juri Linkov
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).