bug#74361: [PATCH] New option xref-navigation-display-window-action

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Dmitry Gutov]

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

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Dmitry Gutov]

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

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Juri Linkov]

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

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Eli Zaretskii]

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

bug#74361: [PATCH] New option xref-navigation-display-window-action

[martin rudalics via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Dmitry Gutov]

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.

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Dmitry Gutov]

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.

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Eli Zaretskii]

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

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Juri Linkov]

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

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Dmitry Gutov]

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

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Dmitry Gutov]

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.

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Eli Zaretskii]

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

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Dmitry Gutov]

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.

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Eli Zaretskii]

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

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Dmitry Gutov]

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.

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Eli Zaretskii]

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

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Juri Linkov]

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

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Juri Linkov]

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

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Dmitry Gutov]

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

bug#74361: [PATCH] New option xref-navigation-display-window-action

[Dmitry Gutov]

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.