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

[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

[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

[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

[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

[Juri Linkov]

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

Unless the user customizes it to call with NOT-SELECTED=nil explicitly:

  (setq display-buffer-alist
        '(((category . xref)
           (display-buffer-use-some-window)
           (some-window . (lambda (_buffer alist)
                            (get-mru-window nil nil nil))))))

PS: after testing I noticed that in your patch
nil needs to be added as a placeholder for empty action:

                  `((xref--display-buffer-in-window)
                    (category . xref)
                    (window . ,xref--original-window)))
                 (t
-                 '((category . xref))))))
+                 '(nil (category . xref))))))

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

[Dmitry Gutov]

On 20/11/2024 09:11, Juri Linkov wrote:
>> 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).
> Unless the user customizes it to call with NOT-SELECTED=nil explicitly:
> 
>    (setq display-buffer-alist
>          '(((category . xref)
>             (display-buffer-use-some-window)
>             (some-window . (lambda (_buffer alist)
>                              (get-mru-window nil nil nil))))))

Yeah, being able to compute the specific window is nice too.

> PS: after testing I noticed that in your patch
> nil needs to be added as a placeholder for empty action:
> 
>                    `((xref--display-buffer-in-window)
>                      (category . xref)
>                      (window . ,xref--original-window)))
>                   (t
> -                 '((category . xref))))))
> +                 '(nil (category . xref))))))

Did you trigger some error with the original patch? LGTM, but I'm not 
seeing a difference in behavior.

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

[Juri Linkov]

>> PS: after testing I noticed that in your patch
>> nil needs to be added as a placeholder for empty action:
>>                    `((xref--display-buffer-in-window)
>>                      (category . xref)
>>                      (window . ,xref--original-window)))
>>                   (t
>> -                 '((category . xref))))))
>> +                 '(nil (category . xref))))))
>
> Did you trigger some error with the original patch? LGTM, but I'm not
> seeing a difference in behavior.

While using your patch I got this error caused by missing nil:

Debugger entered--Lisp error: (wrong-type-argument listp xref)
  append(nil nil nil (category . xref) nil nil ...)
  display-buffer(#<buffer xref.el> ((category . xref)))
  xref--show-pos-in-buf(...)
  xref--show-location(...)
  command-execute(xref-goto-xref)

This is not reproducible in `emacs -Q`.  If you want,
I could try to find minimal customization.
But the main thing is that with my customization
(window-live-p xref--original-window) is nil,
so 'xref--show-pos-in-buf' uses the last branch of 'cond'.

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]

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

The function ultimately used by both is 'set-window-buffer'.  One basic
feature of 'switch-to-buffer' not shared by 'display-buffer-same-window'
is 'switch-to-buffer-preserve-window-point'.  And obviously
'switch-to-buffer-obey-display-actions' gives the user the opportunity
to override the behavior of 'switch-to-buffer' in an arbitrary way while
'display-buffer-reuse-window' per se cannot be overridden.  OTOH the
behavior of the latter can be controlled via alist entries.

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

I think that the most important improvement of "category" should be to
override "lru".  It might also help with 'image-dired' for Bug#74246.

And there should be a convention for setting it up maybe with the help
of some sort of sub-categories.  IIUC 'comint', 'xref' and 'image' might
not be distinctive enough for all purposes.  I cannot suggest much here
because I do not use either of these.

martin

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

[Juri Linkov]

>> 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).
>
> I think that the most important improvement of "category" should be to
> override "lru".  It might also help with 'image-dired' for Bug#74246.

Agreed.

> And there should be a convention for setting it up maybe with the help
> of some sort of sub-categories.  IIUC 'comint', 'xref' and 'image' might
> not be distinctive enough for all purposes.  I cannot suggest much here
> because I do not use either of these.

A sub-category could be represented by the same prefix, e.g.
for different calls of display-buffer in xref.el:
'xref-results' for displaying the result buffer (or simply
'xref' since it's the main buffer of the xref package),
'xref-other-window', 'xref-other-frame' for display-buffer
from the respective commands, etc.

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

[Dmitry Gutov]

On 20/11/2024 19:31, Juri Linkov wrote:
>> And there should be a convention for setting it up maybe with the help
>> of some sort of sub-categories.  IIUC 'comint', 'xref' and 'image' might
>> not be distinctive enough for all purposes.  I cannot suggest much here
>> because I do not use either of these.
> A sub-category could be represented by the same prefix, e.g.
> for different calls of display-buffer in xref.el:
> 'xref-results' for displaying the result buffer (or simply
> 'xref' since it's the main buffer of the xref package),
> 'xref-other-window', 'xref-other-frame' for display-buffer
> from the respective commands, etc.

Maybe we'll call the current addition's category 'xref-jump', for extra 
clarity. Later, 'xref-results' could be added as well.

'xref-other-*' are possible too, but seem less likely to actually get 
used in customizations. Why not, though.

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

[Juri Linkov]

>> A sub-category could be represented by the same prefix, e.g.
>> for different calls of display-buffer in xref.el:
>> 'xref-results' for displaying the result buffer (or simply
>> 'xref' since it's the main buffer of the xref package),
>> 'xref-other-window', 'xref-other-frame' for display-buffer
>> from the respective commands, etc.
>
> Maybe we'll call the current addition's category 'xref-jump', for extra
> clarity.

'xref-jump' is a nice descriptive name.

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

[Dmitry Gutov]

On 20/11/2024 10:37, martin rudalics wrote:
> 
> The function ultimately used by both is 'set-window-buffer'.  One basic
> feature of 'switch-to-buffer' not shared by 'display-buffer-same-window'
> is 'switch-to-buffer-preserve-window-point'.  And obviously
> 'switch-to-buffer-obey-display-actions' gives the user the opportunity
> to override the behavior of 'switch-to-buffer' in an arbitrary way while
> 'display-buffer-reuse-window' per se cannot be overridden.  OTOH the
> behavior of the latter can be controlled via alist entries.

I guess the main possible concern is somebody's display-buffer-alist 
customizations that don't use a category, but are just non-specific 
enough to trigger by the new code.

There's not much that could be done, though.

>  > 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).
> 
> I think that the most important improvement of "category" should be to
> override "lru".

The customization capability, you mean? Nice to have indeed.

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]

 >> I think that the most important improvement of "category" should be to
 >> override "lru".
 >
 > The customization capability, you mean? Nice to have indeed.

What I meant was the following: Calling 'get-lru-window' in
'display-buffer-use-some-window' is a heritage from the times when
'display-buffer' was implemented in C.  It was hard-coded there and
could not be customized.  And it conceptually made sense with multiple
windows because the lru window should be the one whose contents a user
could most likely dispense with.  And it can even work when multiple
'display-buffer' calls are invoked in one and the same command.

The idea misfires in one case: When a user wants one and the same window
display several buffers in sequence.  These buffers could represent
images resulting from browsing an image directory or files containing
grep or xref hits or sources of compile errors.  Anything a user might
want to browse sequentially or, with other words, things a user might
not want to look at at the same time.

In these cases, the lru window will change continuously when multiple
windows are present.  Now if these related buffers were made subject of
a common category and that category were passed as argument to
'display-buffer-use-some-window', the latter could decide - if a window
showing a buffer belonging to the same category existed already - to use
that window instead of the lru one.

Obviously, someone has to decide on setting up the name of the category.
This is the task of the caller of 'display-buffer' - 'image', 'xref',
'grep', 'compile', 'comint' or 'tex' - and would have to be done in a
coordinated fashion so the same category is used twice iff that's really
intended.

In either case, 'display-buffer' would look whether an appropriate
window exists and use that window, maybe also ignoring certain aspects
(dedicatedness, minimum size) that would otherwise prevent its use.

An orthogonal issue is whether an initial command expresses the desire
to show the buffer in "another" window or on "another" frame.  My
suggestion would be to have these suppress any 'category' argument and
have 'display-buffer' proceed as usual, that is use the lru window on
the specified frame , pop up a new frame ...

martin

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]

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

 > In either case, 'display-buffer' would look whether an appropriate
 > window exists and use that window, maybe also ignoring certain aspects
 > (dedicatedness, minimum size) that would otherwise prevent its use.

Attached find how a 'category' list entry could be handled by
'display-buffer-use-some-window' where a 'some-window' entry would be
given precedence.  Tested with

(display-buffer
  (get-buffer-create "*foo*")
  '((display-buffer-use-some-window) (inhibit-same-window . t) (category . foo)))

(display-buffer
  (get-buffer-create "*bar*")
  '((display-buffer-use-some-window) (inhibit-same-window . t) (category . foo)))

martin

[-- Attachment #2: category.diff --]
[-- Type: text/x-patch, Size: 3246 bytes --]

diff --git a/lisp/window.el b/lisp/window.el
index b50770cbd7e..ba79c6e64af 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -2652,6 +2652,24 @@ get-buffer-window-list
 	(setq windows (cons window windows))))
     (nreverse windows)))
 
+(defun get-window-with-category (category &optional frames dedicated not-selected no-other)
+  "Return window matching CATEGORY on specified FRAMES.
+Return first window whose `category' parameter contains CATEGORY.
+The argument FRAMES has the same meaning as for `window-list'.
+
+A minibuffer window is never a candidate.  A dedicated window is never a
+candidate unless DEDICATED is non-nil, so if all windows are dedicated,
+the value is nil.  Optional argument NOT-SELECTED non-nil means never
+return the selected window.  Optional argument NO-OTHER non-nil means to
+never return a window for which `window-no-other-p' returns non-nil."
+  (catch 'found
+    (dolist (window (window-list-1 nil 'nomini frames))
+      (when (and (memq category (window-parameter window 'category))
+		 (or dedicated (not (window-dedicated-p window)))
+		 (or (not not-selected) (not (eq window (selected-window))))
+		 (or (not no-other) (window-no-other-p window)))
+	(throw 'found window)))))
+
 (defun minibuffer-window-active-p (window)
   "Return t if WINDOW is the currently active minibuffer window."
   (and (window-live-p window) (eq window (active-minibuffer-window))))
@@ -7665,6 +7725,13 @@ window--display-buffer
 	  (window-preserve-size window t (car preserve-size))
 	  (window-preserve-size window nil (cdr preserve-size)))))
 
+      (let ((category (cdr (assq 'category alist))))
+	(when category
+	  (let ((parameter (window-parameter window 'category)))
+	    (unless (memq category parameter)
+	      (set-window-parameter
+	       window 'category (cons category parameter))))))
+
       ;; Assign any window parameters specified.
       (let ((parameters (cdr (assq 'window-parameters alist))))
         (dolist (parameter parameters)
@@ -8921,12 +8988,17 @@ display-buffer-use-some-window
 called only by `display-buffer' or a function directly or
 indirectly called by the latter."
   (let* ((not-this-window (cdr (assq 'inhibit-same-window alist)))
-	 (some-window-method (cdr (assq 'some-window alist)))
+	 (category (cdr (assq 'category alist)))
+	 (some-window (assq 'some-window alist))
+	 (some-window-method (cdr some-window))
 	 (frame (or (window--frame-usable-p (selected-frame))
 		    (window--frame-usable-p (last-nonminibuffer-frame))))
 	 (window
 	  ;; Reuse an existing window.
-	  (or (cond
+	  (or (and category (not some-window)
+		   (get-window-with-category
+		    category 'visible  nil not-this-window))
+	      (cond
 	       ((memq some-window-method '(nil lru))
 		(display-buffer--lru-window
 		 ;; If ALIST specifies 'lru-frames' or 'window-min-width'
@@ -8937,6 +9009,9 @@ display-buffer-use-some-window
 		(get-mru-window nil nil t))
 	       ((functionp some-window-method)
 		(funcall some-window-method buffer alist)))
+	      (and category
+		   (get-window-with-category
+		    category 'visible  nil not-this-window))
 	      (let ((window (get-buffer-window buffer 'visible)))
 		(unless (and not-this-window
 			     (eq window (selected-window)))

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

[Juri Linkov]

>> In either case, 'display-buffer' would look whether an appropriate
>> window exists and use that window, maybe also ignoring certain aspects
>> (dedicatedness, minimum size) that would otherwise prevent its use.
>
> Attached find how a 'category' list entry could be handled by
> 'display-buffer-use-some-window' where a 'some-window' entry would be
> given precedence.  Tested with
>
> (display-buffer
>  (get-buffer-create "*foo*")
>  '((display-buffer-use-some-window) (inhibit-same-window . t) (category . foo)))
>
> (display-buffer
>  (get-buffer-create "*bar*")
>  '((display-buffer-use-some-window) (inhibit-same-window . t) (category . foo)))

This is a nice feature.  But please use a different name
since the 'category' alist entry is reserved exclusively
to match display-buffer calls in user's configuration
in the user option 'display-buffer-alist'.
When using the same name for different purposes
then the users won't be able to match by category
and not to reuse the same window.

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

[Juri Linkov]

>>> In either case, 'display-buffer' would look whether an appropriate
>>> window exists and use that window, maybe also ignoring certain aspects
>>> (dedicatedness, minimum size) that would otherwise prevent its use.
>>
>> Attached find how a 'category' list entry could be handled by
>> 'display-buffer-use-some-window' where a 'some-window' entry would be
>> given precedence.  Tested with
>>
>> (display-buffer
>>  (get-buffer-create "*foo*")
>>  '((display-buffer-use-some-window) (inhibit-same-window . t) (category . foo)))
>>
>> (display-buffer
>>  (get-buffer-create "*bar*")
>>  '((display-buffer-use-some-window) (inhibit-same-window . t) (category . foo)))
>
> This is a nice feature.  But please use a different name
> since the 'category' alist entry is reserved exclusively
> to match display-buffer calls in user's configuration
> in the user option 'display-buffer-alist'.
> When using the same name for different purposes
> then the users won't be able to match by category
> and not to reuse the same window.

And maybe it makes more sense to implement this feature in
'display-buffer-reuse-window' instead of
'display-buffer-use-some-window'?

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]

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

I doubt anyone can confirm that even for an uncustomized session.

The more important question for me is rather whether we really want to
"handle xref-pop-to-location".  It's currently used in
'xref-show-definitions-buffer', 'xref-show-definitions-buffer-at-bottom'
and 'xref-show-definitions-completing-read'.  Why should "show" select
the target window rather than just display it?  And the first line of
the doc-string says "Go to the location of ITEM and display the buffer"
which doesn't say anything about selecting the buffer's window either.
BTW what is SELECT in that function's doc-string?

So I think the first thing to do is to agree on what
'xref-pop-to-location' should conceptually do, then decide on who should
call it and finally decide on how to implement it possibly with the help
of a category.

martin

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

[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

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

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

[Eli Zaretskii]

> Date: Tue, 19 Nov 2024 21:51:32 +0200
> Cc: rudalics@gmx.at, 74361@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> 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.

The doc string could mention the category as well.

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

We have this situation elsewhere, and I personally simply copy/paste
the same text into the doc strings.  If the text is short enough, it
works well IME.

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

[Eli Zaretskii]

> Cc: rudalics@gmx.at, 74361@debbugs.gnu.org, juri@linkov.net
> Date: Tue, 19 Nov 2024 17:33:18 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > 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.

I've added some minor improvements to the documentation of 'category',
but I don't really see what, if anything, needs to be said about
'comint' in particular.  AFAIU, there's no significance to the
'comint' symbol itself, it's just a convenience feature to allow users
of comint buffers to customize how those buffers are displayed.  (And
I'm not sure I understand how that works, since comint.el doesn't use
that category, AFAICT.)  Which reminds me that I was quite unhappy
about the way this feature was documented, and its current state is
not much better, sadly.

So I don't see what I could add more to the documentation at this
time.

If someone explains to me in small words how this works in comint (and
other modes which provide a similar category symbol), I will see about
improving and clarifying the docs.

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

[Juri Linkov]

> If someone explains to me in small words how this works in comint (and
> other modes which provide a similar category symbol), I will see about
> improving and clarifying the docs.

'(category . comint)' is used in 'display-comint-buffer-action'
that is marked obsolete.  So for Emacs 31 its defcustom could be
downgraded to a simple defvar, and moved to comint.el, and maybe
also autoloaded when it's used by modes independent from comint.el.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: dmitry@gutov.dev,  rudalics@gmx.at,  74361@debbugs.gnu.org
> Date: Thu, 21 Nov 2024 20:01:48 +0200
> 
> > If someone explains to me in small words how this works in comint (and
> > other modes which provide a similar category symbol), I will see about
> > improving and clarifying the docs.
> 
> '(category . comint)' is used in 'display-comint-buffer-action'
> that is marked obsolete.  So for Emacs 31 its defcustom could be
> downgraded to a simple defvar, and moved to comint.el, and maybe
> also autoloaded when it's used by modes independent from comint.el.

But nothing in comint.el uses this category when calling
display-buffer, AFAICS.  So how can users use this category to
customize the way comint buffers are displayed?

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

[Juri Linkov]

>> > If someone explains to me in small words how this works in comint (and
>> > other modes which provide a similar category symbol), I will see about
>> > improving and clarifying the docs.
>> 
>> '(category . comint)' is used in 'display-comint-buffer-action'
>> that is marked obsolete.  So for Emacs 31 its defcustom could be
>> downgraded to a simple defvar, and moved to comint.el, and maybe
>> also autoloaded when it's used by modes independent from comint.el.
>
> But nothing in comint.el uses this category when calling
> display-buffer, AFAICS.  So how can users use this category to
> customize the way comint buffers are displayed?

This category is used by modes that inherit from comint-mode, e.g.:

cmuscheme.el:
  (define-derived-mode inferior-scheme-mode comint-mode "Inferior Scheme"
   ...
    (pop-to-buffer "*scheme*" display-comint-buffer-action)

inf-lisp.el:
  (define-derived-mode inferior-lisp-mode comint-mode "Inferior Lisp"
   ...
    (pop-to-buffer "*inferior-lisp*" display-comint-buffer-action)

shell.el:
  (define-derived-mode shell-mode comint-mode "Shell"
   ...
    (pop-to-buffer buffer display-comint-buffer-action)

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: dmitry@gutov.dev,  rudalics@gmx.at,  74361@debbugs.gnu.org
> Date: Thu, 21 Nov 2024 21:39:39 +0200
> 
> >> > If someone explains to me in small words how this works in comint (and
> >> > other modes which provide a similar category symbol), I will see about
> >> > improving and clarifying the docs.
> >> 
> >> '(category . comint)' is used in 'display-comint-buffer-action'
> >> that is marked obsolete.  So for Emacs 31 its defcustom could be
> >> downgraded to a simple defvar, and moved to comint.el, and maybe
> >> also autoloaded when it's used by modes independent from comint.el.
> >
> > But nothing in comint.el uses this category when calling
> > display-buffer, AFAICS.  So how can users use this category to
> > customize the way comint buffers are displayed?
> 
> This category is used by modes that inherit from comint-mode, e.g.:
> 
> cmuscheme.el:
>   (define-derived-mode inferior-scheme-mode comint-mode "Inferior Scheme"
>    ...
>     (pop-to-buffer "*scheme*" display-comint-buffer-action)
> 
> inf-lisp.el:
>   (define-derived-mode inferior-lisp-mode comint-mode "Inferior Lisp"
>    ...
>     (pop-to-buffer "*inferior-lisp*" display-comint-buffer-action)
> 
> shell.el:
>   (define-derived-mode shell-mode comint-mode "Shell"
>    ...
>     (pop-to-buffer buffer display-comint-buffer-action)

Those are not the categories, those are the deprecated user options
for which the categories are supposed to be a replacement we
recommend:

  (make-obsolete-variable
   'display-comint-buffer-action
   "use a `(category . comint)' condition in `display-buffer-alist'."
   "30.1")

So if some user wants to take our advice and modernize his/her
customizations to use '(category . comint)' instead of
display-comint-buffer-action, the customization will stop working for
him/her when we remove those obsolete options in some future Emacs
version.  Right?  Or what am I missing?

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

[Juri Linkov]

>> cmuscheme.el:
>>   (define-derived-mode inferior-scheme-mode comint-mode "Inferior Scheme"
>>    ...
>>     (pop-to-buffer "*scheme*" display-comint-buffer-action)
>> 
>> inf-lisp.el:
>>   (define-derived-mode inferior-lisp-mode comint-mode "Inferior Lisp"
>>    ...
>>     (pop-to-buffer "*inferior-lisp*" display-comint-buffer-action)
>> 
>> shell.el:
>>   (define-derived-mode shell-mode comint-mode "Shell"
>>    ...
>>     (pop-to-buffer buffer display-comint-buffer-action)
>
> Those are not the categories, those are the deprecated user options
> for which the categories are supposed to be a replacement we
> recommend:
>
>   (make-obsolete-variable
>    'display-comint-buffer-action
>    "use a `(category . comint)' condition in `display-buffer-alist'."
>    "30.1")

The categories are a replacement only for customization.
But all display-buffer calls should still contain
`(category . comint)' that currently presented
in these variables.  IOW, the variables currently
are used in two ways:
1. as the default values for display-buffer calls
2. as user options

But now customization is supposed to be only with
the help of `(category . comint)' in `display-buffer-alist'.
So these variable will be used only in display-buffer calls.

> So if some user wants to take our advice and modernize his/her
> customizations to use '(category . comint)' instead of
> display-comint-buffer-action, the customization will stop working for
> him/her when we remove those obsolete options in some future Emacs
> version.  Right?  Or what am I missing?

These are two possible solutions:
1. Demote these options to variables not intended for customization.
2. Move their current default values to display-buffer calls.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: dmitry@gutov.dev,  rudalics@gmx.at,  74361@debbugs.gnu.org
> Date: Fri, 22 Nov 2024 09:29:00 +0200
> 
> >> cmuscheme.el:
> >>   (define-derived-mode inferior-scheme-mode comint-mode "Inferior Scheme"
> >>    ...
> >>     (pop-to-buffer "*scheme*" display-comint-buffer-action)
> >> 
> >> inf-lisp.el:
> >>   (define-derived-mode inferior-lisp-mode comint-mode "Inferior Lisp"
> >>    ...
> >>     (pop-to-buffer "*inferior-lisp*" display-comint-buffer-action)
> >> 
> >> shell.el:
> >>   (define-derived-mode shell-mode comint-mode "Shell"
> >>    ...
> >>     (pop-to-buffer buffer display-comint-buffer-action)
> >
> > Those are not the categories, those are the deprecated user options
> > for which the categories are supposed to be a replacement we
> > recommend:
> >
> >   (make-obsolete-variable
> >    'display-comint-buffer-action
> >    "use a `(category . comint)' condition in `display-buffer-alist'."
> >    "30.1")
> 
> The categories are a replacement only for customization.
> But all display-buffer calls should still contain
> `(category . comint)' that currently presented
> in these variables.  IOW, the variables currently
> are used in two ways:
> 1. as the default values for display-buffer calls
> 2. as user options
> 
> But now customization is supposed to be only with
> the help of `(category . comint)' in `display-buffer-alist'.
> So these variable will be used only in display-buffer calls.
> 
> > So if some user wants to take our advice and modernize his/her
> > customizations to use '(category . comint)' instead of
> > display-comint-buffer-action, the customization will stop working for
> > him/her when we remove those obsolete options in some future Emacs
> > version.  Right?  Or what am I missing?
> 
> These are two possible solutions:
> 1. Demote these options to variables not intended for customization.
> 2. Move their current default values to display-buffer calls.

I expected to see us do #2 at the same time we deprecated the user
options.  I don't understand why we didn't do that.  The deprecation
message clearly tells users not to use these variables, so it's
reasonable to expect them to be deleted.  Moreover, their presence in
our sources is a potential cause for byte-compilation warnings.

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

[Juri Linkov]

>> >   (make-obsolete-variable
>> >    'display-comint-buffer-action
>> >    "use a `(category . comint)' condition in `display-buffer-alist'."
>> >    "30.1")
>> 
>> The categories are a replacement only for customization.
>> But all display-buffer calls should still contain
>> `(category . comint)' that currently presented
>> in these variables.  IOW, the variables currently
>> are used in two ways:
>> 1. as the default values for display-buffer calls
>> 2. as user options
>> 
>> But now customization is supposed to be only with
>> the help of `(category . comint)' in `display-buffer-alist'.
>> So these variable will be used only in display-buffer calls.
>> 
>> > So if some user wants to take our advice and modernize his/her
>> > customizations to use '(category . comint)' instead of
>> > display-comint-buffer-action, the customization will stop working for
>> > him/her when we remove those obsolete options in some future Emacs
>> > version.  Right?  Or what am I missing?
>> 
>> These are two possible solutions:
>> 1. Demote these options to variables not intended for customization.
>> 2. Move their current default values to display-buffer calls.
>
> I expected to see us do #2 at the same time we deprecated the user
> options.  I don't understand why we didn't do that.  The deprecation
> message clearly tells users not to use these variables, so it's
> reasonable to expect them to be deleted.  Moreover, their presence in
> our sources is a potential cause for byte-compilation warnings.

Immediate #2 will break customization for many users.
We have to give enough time between two releases
to allow the users to see a warning and adapt their
config files to upcoming deletion of these options.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: dmitry@gutov.dev,  rudalics@gmx.at,  74361@debbugs.gnu.org
> Date: Sat, 23 Nov 2024 20:25:30 +0200
> 
> >> 1. Demote these options to variables not intended for customization.
> >> 2. Move their current default values to display-buffer calls.
> >
> > I expected to see us do #2 at the same time we deprecated the user
> > options.  I don't understand why we didn't do that.  The deprecation
> > message clearly tells users not to use these variables, so it's
> > reasonable to expect them to be deleted.  Moreover, their presence in
> > our sources is a potential cause for byte-compilation warnings.
> 
> Immediate #2 will break customization for many users.
> We have to give enough time between two releases
> to allow the users to see a warning and adapt their
> config files to upcoming deletion of these options.

But isn't it true that if users adapt their config files, the
customization will stop working for them because category is not used
by comint?

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

[Juri Linkov]

>> >> 1. Demote these options to variables not intended for customization.
>> >> 2. Move their current default values to display-buffer calls.
>> >
>> > I expected to see us do #2 at the same time we deprecated the user
>> > options.  I don't understand why we didn't do that.  The deprecation
>> > message clearly tells users not to use these variables, so it's
>> > reasonable to expect them to be deleted.  Moreover, their presence in
>> > our sources is a potential cause for byte-compilation warnings.
>> 
>> Immediate #2 will break customization for many users.
>> We have to give enough time between two releases
>> to allow the users to see a warning and adapt their
>> config files to upcoming deletion of these options.
>
> But isn't it true that if users adapt their config files, the
> customization will stop working for them because category is not used
> by comint?

Adapting config files means replacing such settings

  (setopt display-comint-buffer-action
          '((display-buffer-same-window)
            (inhibit-same-window . nil)))

with

  (add-to-list 'display-buffer-alist
               '((category . comint)
                 (display-buffer-same-window)
                 (inhibit-same-window . nil)))

This already works since all corresponding display-buffer calls
already provide the 'comint' category.

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

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: dmitry@gutov.dev,  rudalics@gmx.at,  74361@debbugs.gnu.org
> Date: Sat, 23 Nov 2024 21:14:13 +0200
> 
> >> >> 1. Demote these options to variables not intended for customization.
> >> >> 2. Move their current default values to display-buffer calls.
> >> >
> >> > I expected to see us do #2 at the same time we deprecated the user
> >> > options.  I don't understand why we didn't do that.  The deprecation
> >> > message clearly tells users not to use these variables, so it's
> >> > reasonable to expect them to be deleted.  Moreover, their presence in
> >> > our sources is a potential cause for byte-compilation warnings.
> >> 
> >> Immediate #2 will break customization for many users.
> >> We have to give enough time between two releases
> >> to allow the users to see a warning and adapt their
> >> config files to upcoming deletion of these options.
> >
> > But isn't it true that if users adapt their config files, the
> > customization will stop working for them because category is not used
> > by comint?
> 
> Adapting config files means replacing such settings
> 
>   (setopt display-comint-buffer-action
>           '((display-buffer-same-window)
>             (inhibit-same-window . nil)))
> 
> with
> 
>   (add-to-list 'display-buffer-alist
>                '((category . comint)
>                  (display-buffer-same-window)
>                  (inhibit-same-window . nil)))
> 
> This already works since all corresponding display-buffer calls
> already provide the 'comint' category.

They do?  I thought they use display-comint-buffer-action instead?

I just searched the entire Lisp tree, and didn't find even a single
match for "(category . comint)" except in the default value of
display-comint-buffer-action.  So if that user option's value is
changed, the replacement above will stop working, no?

But I already said that, and it doesn't seem to worry you.  So what am
I missing here?

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