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.

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

[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

[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

[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

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

[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

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

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

[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

[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

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

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

[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

[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

[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

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

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

Indeed, they do this via display-comint-buffer-action.

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

Do you mean that the users might want to shoot themselves in the foot
and remove '(category . comint)' from the default value of
display-comint-buffer-action?

Then indeed we need to move '(category . comint)' directly
to every display-buffer call that currently uses
display-comint-buffer-action.

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]

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

This is over my head: How can a (category . foo) entry make sense if it
is not initially set up by the 'foo' package?  As a user I might not
have the slightest idea of categories.  I'd proceed as follows:

- Package 'foo' designates a 'display-buffer' call as something
   pertaining to 'foo-some' by adding a (category . foo-some) alist entry
   and facultatively providing one or a couple of suitable action
   functions.

- A knowledgeable user can override the suggestion of 'foo' by putting
   into 'display-buffer-alist' a (category . foo-some) condition (so
   'buffer-match-p' will handle it) with an appropriate ACTION.

- 'display-buffer' has to

   - respect the user customization if there is one (and it's not
     overridden by the caller),

   - tries any action function provided by the caller,

   - automagically tries to do something reasonable by finding a window
     that already has an association with 'foo-some'.

Am I wrong about this concept?

martin

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: Sun, 24 Nov 2024 09:34:51 +0200
> 
> >> 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?
> 
> Indeed, they do this via display-comint-buffer-action.
> 
> > 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?
> 
> Do you mean that the users might want to shoot themselves in the foot
> and remove '(category . comint)' from the default value of
> display-comint-buffer-action?

Yes, that's the scenario that bothers me.  Users could do that without
understanding the subtleties of the issue.

> Then indeed we need to move '(category . comint)' directly
> to every display-buffer call that currently uses
> display-comint-buffer-action.

I think we should do that for Emacs 31, yes.

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

[Juri Linkov]

>> 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.
>
> This is over my head: How can a (category . foo) entry make sense if it
> is not initially set up by the 'foo' package?  As a user I might not
> have the slightest idea of categories.  I'd proceed as follows:
>
> - Package 'foo' designates a 'display-buffer' call as something
>   pertaining to 'foo-some' by adding a (category . foo-some) alist entry
>   and facultatively providing one or a couple of suitable action
>   functions.
>
> - A knowledgeable user can override the suggestion of 'foo' by putting
>   into 'display-buffer-alist' a (category . foo-some) condition (so
>   'buffer-match-p' will handle it) with an appropriate ACTION.
>
> - 'display-buffer' has to
>
>   - respect the user customization if there is one (and it's not
>     overridden by the caller),
>
>   - tries any action function provided by the caller,
>
>   - automagically tries to do something reasonable by finding a window
>     that already has an association with 'foo-some'.
>
> Am I wrong about this concept?

The key word is "automagically".  Only this is a problem.
Many users might not want such magic.

Compare this with 'display-buffer-reuse-mode-window'.
Its alist entry `mode' is not the same as `major-mode' in condition:

  (add-to-list 'display-buffer-alist
               '((major-mode . Info-mode)
                 (display-buffer-reuse-mode-window)
                 (mode . help-mode)))

But the category should be the same to match:

  (add-to-list 'display-buffer-alist
               '((category . Info-mode)
                 (display-buffer-same-window)))

  (display-buffer (get-buffer-create "*info*")
                  '(nil (category . Info-mode)))

So for a similar function like 'display-buffer-reuse-category-window'
we need another alist element, not 'category'.
Maybe 'display-buffer-reuse-group-window' with 'group' list element
for a group of buffers in the same window.

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

[Dmitry Gutov]

On 21/11/2024 09:34, Juri Linkov wrote:
>>> 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'.

No it's okay - I guess if this makes some customizations easier (not 
erroring out), that's also a win.

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

[Juri Linkov]

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

>> Do you mean that the users might want to shoot themselves in the foot
>> and remove '(category . comint)' from the default value of
>> display-comint-buffer-action?
>
> Yes, that's the scenario that bothers me.  Users could do that without
> understanding the subtleties of the issue.
>
>> Then indeed we need to move '(category . comint)' directly
>> to every display-buffer call that currently uses
>> display-comint-buffer-action.
>
> I think we should do that for Emacs 31, yes.

Ok, here is the patch for Emacs 31:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: remove-display-comint-buffer-action.patch --]
[-- Type: text/x-diff, Size: 9082 bytes --]

diff --git a/etc/NEWS b/etc/NEWS
index d7047d0923f..9c5b2db203e 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -154,6 +154,12 @@ that the actual killing or burying of the buffer is done by the caller.
 With this option set, 'quit-restore-window' will delete its window more
 aggressively rather than switching to some other buffer in it.
 
+---
+*** The user option 'display-comint-buffer-action' has been removed.
+It has been obsolete since Emacs 30.1.  Use '(category . comint)' instead.
+Another user option 'display-tex-shell-buffer-action' has been removed too
+for which you can use '(category . tex-shell)'.
+
 ** Frames
 
 +++
diff --git a/lisp/cmuscheme.el b/lisp/cmuscheme.el
index d4316fb1175..b03cf1f1840 100644
--- a/lisp/cmuscheme.el
+++ b/lisp/cmuscheme.el
@@ -238,8 +238,8 @@ run-scheme
 	(inferior-scheme-mode)))
   (setq scheme-program-name cmd)
   (setq scheme-buffer "*scheme*")
-  (with-suppressed-warnings ((obsolete display-comint-buffer-action))
-    (pop-to-buffer "*scheme*" display-comint-buffer-action)))
+  (pop-to-buffer "*scheme*" (append display-buffer--same-window-action
+                                    '((category . comint)))))
 
 (defun scheme-start-file (prog)
   "Return the name of the start file corresponding to PROG.
@@ -359,8 +359,8 @@ switch-to-scheme
   (interactive "P")
   (if (or (and scheme-buffer (get-buffer scheme-buffer))
           (scheme-interactively-start-process))
-      (with-suppressed-warnings ((obsolete display-comint-buffer-action))
-        (pop-to-buffer scheme-buffer display-comint-buffer-action))
+      (pop-to-buffer scheme-buffer (append display-buffer--same-window-action
+                                           '((category . comint))))
     (error "No current process buffer.  See variable `scheme-buffer'"))
   (when eob-p
     (push-mark)
diff --git a/lisp/shell.el b/lisp/shell.el
index 6cfae470cd7..33d80061ada 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -953,8 +953,8 @@ shell
                  (current-buffer)))
   ;; The buffer's window must be correctly set when we call comint
   ;; (so that comint sets the COLUMNS env var properly).
-  (with-suppressed-warnings ((obsolete display-comint-buffer-action))
-    (pop-to-buffer buffer display-comint-buffer-action))
+  (pop-to-buffer buffer (append display-buffer--same-window-action
+                                '((category . comint))))
 
   (with-connection-local-variables
    (when file-name
diff --git a/lisp/window.el b/lisp/window.el
index c790118c5e0..e9d57652ec6 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -9115,35 +9115,6 @@ pop-to-buffer-same-window
 another window."
   (pop-to-buffer buffer display-buffer--same-window-action norecord))
 
-(defcustom display-comint-buffer-action
-  (append display-buffer--same-window-action '((category . comint)))
-  "`display-buffer' action for displaying comint buffers."
-  :type display-buffer--action-custom-type
-  :risky t
-  :version "29.1"
-  :group 'windows
-  :group 'comint)
-
-(make-obsolete-variable
- 'display-comint-buffer-action
- "use a `(category . comint)' condition in `display-buffer-alist'."
- "30.1")
-
-(defcustom display-tex-shell-buffer-action '(display-buffer-in-previous-window
-                                             (inhibit-same-window . t)
-                                             (category . tex-shell))
-  "`display-buffer' action for displaying TeX shell buffers."
-  :type display-buffer--action-custom-type
-  :risky t
-  :version "29.1"
-  :group 'windows
-  :group 'tex-run)
-
-(make-obsolete-variable
- 'display-tex-shell-buffer-action
- "use a `(category . tex-shell)' condition in `display-buffer-alist'."
- "30.1")
-
 (defun read-buffer-to-switch (prompt)
   "Read the name of a buffer to switch to, prompting with PROMPT.
 Return the name of the buffer as a string.
diff --git a/lisp/eshell/eshell.el b/lisp/eshell/eshell.el
index fc08734d5f3..3def918bdd1 100644
--- a/lisp/eshell/eshell.el
+++ b/lisp/eshell/eshell.el
@@ -278,8 +278,8 @@ eshell
 		   (t
 		    (get-buffer-create eshell-buffer-name)))))
     (cl-assert (and buf (buffer-live-p buf)))
-    (with-suppressed-warnings ((obsolete display-comint-buffer-action))
-      (pop-to-buffer buf display-comint-buffer-action))
+    (pop-to-buffer buf (append display-buffer--same-window-action
+                               '((category . comint))))
     (unless (derived-mode-p 'eshell-mode)
       (eshell-mode))
     buf))
diff --git a/lisp/org/ol-eshell.el b/lisp/org/ol-eshell.el
index 595dd0ee0f8..e364a38c17d 100644
--- a/lisp/org/ol-eshell.el
+++ b/lisp/org/ol-eshell.el
@@ -51,9 +51,9 @@ org-eshell-open
     (if (get-buffer eshell-buffer-name)
         (pop-to-buffer
          eshell-buffer-name
-         (if (boundp 'display-comint-buffer-action) ; Emacs >= 29
+         (if (boundp 'display-comint-buffer-action) ; Emacs >= 29, <= 30
              display-comint-buffer-action
-           '(display-buffer-same-window (inhibit-same-window))))
+           '(display-buffer-same-window (inhibit-same-window) (category . comint))))
       (eshell))
     (goto-char (point-max))
     (eshell-kill-input)
diff --git a/lisp/progmodes/inf-lisp.el b/lisp/progmodes/inf-lisp.el
index 85fc6b930f5..b092b3b679c 100644
--- a/lisp/progmodes/inf-lisp.el
+++ b/lisp/progmodes/inf-lisp.el
@@ -308,8 +308,8 @@ inferior-lisp
 			   "inferior-lisp" (car cmdlist) nil (cdr cmdlist)))
 	(inferior-lisp-mode)))
   (setq inferior-lisp-buffer "*inferior-lisp*")
-  (with-suppressed-warnings ((obsolete display-comint-buffer-action))
-    (pop-to-buffer "*inferior-lisp*" display-comint-buffer-action)))
+  (pop-to-buffer "*inferior-lisp*" (append display-buffer--same-window-action
+                                           '((category . comint)))))
 
 ;;;###autoload
 (defalias 'run-lisp 'inferior-lisp)
diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index cf1c94a6d20..94f70c9a854 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -1376,7 +1376,8 @@ project-shell
          (shell-buffer (get-buffer default-project-shell-name)))
     (if (and shell-buffer (not current-prefix-arg))
         (if (comint-check-proc shell-buffer)
-            (pop-to-buffer shell-buffer (bound-and-true-p display-comint-buffer-action))
+            (pop-to-buffer shell-buffer (append display-buffer--same-window-action
+                                                '((category . comint))))
           (shell shell-buffer))
       (shell (generate-new-buffer-name default-project-shell-name)))))
 
@@ -1393,7 +1394,8 @@ project-eshell
          (eshell-buffer-name (project-prefixed-buffer-name "eshell"))
          (eshell-buffer (get-buffer eshell-buffer-name)))
     (if (and eshell-buffer (not current-prefix-arg))
-        (pop-to-buffer eshell-buffer (bound-and-true-p display-comint-buffer-action))
+        (pop-to-buffer eshell-buffer (append display-buffer--same-window-action
+                                             '((category . comint))))
       (eshell t))))
 
 ;;;###autoload
diff --git a/lisp/progmodes/sh-script.el b/lisp/progmodes/sh-script.el
index 8ba64100203..397a66582fa 100644
--- a/lisp/progmodes/sh-script.el
+++ b/lisp/progmodes/sh-script.el
@@ -1435,8 +1435,9 @@ sh-shell-process
 (defun sh-show-shell ()
   "Pop the shell interaction buffer."
   (interactive)
-  (with-suppressed-warnings ((obsolete display-comint-buffer-action))
-    (pop-to-buffer (process-buffer (sh-shell-process t)) display-comint-buffer-action)))
+  (pop-to-buffer (process-buffer (sh-shell-process t))
+                 (append display-buffer--same-window-action
+                         '((category . comint)))))
 
 (defun sh-send-text (text)
   "Send TEXT to `sh-shell-process'."
diff --git a/lisp/textmodes/tex-mode.el b/lisp/textmodes/tex-mode.el
index 9cb95f59da4..06a45112719 100644
--- a/lisp/textmodes/tex-mode.el
+++ b/lisp/textmodes/tex-mode.el
@@ -2092,8 +2092,9 @@ tex-feed-input
 
 (defun tex-display-shell ()
   "Make the TeX shell buffer visible in a window."
-  (with-suppressed-warnings ((obsolete display-tex-shell-buffer-action))
-    (display-buffer (tex-shell-buf) display-tex-shell-buffer-action))
+  (display-buffer (tex-shell-buf) '(display-buffer-in-previous-window
+                                    (inhibit-same-window . t)
+                                    (category . tex-shell)))
   (tex-recenter-output-buffer nil))
 
 (defun tex-shell-sentinel (proc _msg)
@@ -2753,8 +2754,9 @@ tex-recenter-output-buffer
     (if (null tex-shell)
 	(message "No TeX output buffer")
       (when-let* ((window
-                   (with-suppressed-warnings ((obsolete display-tex-shell-buffer-action))
-                     (display-buffer tex-shell display-tex-shell-buffer-action))))
+                   (display-buffer tex-shell '(display-buffer-in-previous-window
+                                               (inhibit-same-window . t)
+                                               (category . tex-shell)))))
         (with-selected-window window
 	  (bury-buffer tex-shell)
 	  (goto-char (point-max))

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]

 > The key word is "automagically".  Only this is a problem.
 > Many users might not want such magic.
[...]
 > But the category should be the same to match:
 >
 >    (add-to-list 'display-buffer-alist
 >                 '((category . Info-mode)
 >                   (display-buffer-same-window)))
 >
 >    (display-buffer (get-buffer-create "*info*")
 >                    '(nil (category . Info-mode)))
 >
 > So for a similar function like 'display-buffer-reuse-category-window'
 > we need another alist element, not 'category'.
 > Maybe 'display-buffer-reuse-group-window' with 'group' list element
 > for a group of buffers in the same window.

We can't reasonably expect packages to set up yet another alist entry
type like 'group'.  package writers just would not understand it.  So it
simply _has_ to be 'category' (or something else but still only one
thing).

Hence any magic users might not want would have to be implemented within
'display-buffer'.  If say 'run-scheme' wants the same window, then we
already have a problem since a *scheme* window might already exist from
a previous run and it should try 'display-buffer-reuse-window' first.
But this is something for people setting up the default.

Now if no 'inhibit-same-window' alist entry is found and the selected
window is not dedicated to another buffer, 'display-buffer' would use
the selected window as requested.  No automagic here.  Otherwise, it
would look for a window with a 'category' window parameter equaling
'comint' which could, let's assume the worst, mean to use an existing
*shell* window with a running process the user might be attentively
following at that moment.  But the same could happen with your proposal
as well.  So any automagic here should not harm either.

The automagic would come into play when neither caller nor user specify
a particular preference for a window or all other preferences have been
exhausted and 'display-buffer' invokes 'display-buffer-use-some-window'
(not 'display-buffer-reuse-window') in which case that function would
look for a window with a matching 'category' parameter.  Which problems
do you see here?

martin

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

[Juri Linkov]

> We can't reasonably expect packages to set up yet another alist entry
> type like 'group'.  package writers just would not understand it.  So it
> simply _has_ to be 'category' (or something else but still only one
> thing).

Better to use something else.  The 'category' alist entry
is intended to be used only in display-buffer calls like this:

  (display-buffer (get-buffer-create "*info*")
                  '(nil (category . Info-mode)))

to allow using 'category' in the condition part of display-buffer-alist
to match the display-buffer call:

  (add-to-list 'display-buffer-alist
               '((category . Info-mode)
                 (display-buffer-same-window)))

What you proposed is a different usage of 'category'
that makes sense in the action part of display-buffer-alist:

  (add-to-list 'display-buffer-alist
               '("\*Help\*"
                 (display-buffer-same-window)
                 (category . Info-mode)))

It would be cleaner to use a different action for this feature, e.g.:

  (add-to-list 'display-buffer-alist
               '("\*Help\*"
                 (display-buffer-reuse-category-window)
                 (category . Info-mode)))

or without an alist entry:

  (add-to-list 'display-buffer-alist
               '("\*Help\*"
                 (display-buffer-reuse-category-window)))

> Hence any magic users might not want would have to be implemented within
> 'display-buffer'.  If say 'run-scheme' wants the same window, then we
> already have a problem since a *scheme* window might already exist from
> a previous run and it should try 'display-buffer-reuse-window' first.
> But this is something for people setting up the default.
>
> Now if no 'inhibit-same-window' alist entry is found and the selected
> window is not dedicated to another buffer, 'display-buffer' would use
> the selected window as requested.  No automagic here.  Otherwise, it
> would look for a window with a 'category' window parameter equaling
> 'comint' which could, let's assume the worst, mean to use an existing
> *shell* window with a running process the user might be attentively
> following at that moment.  But the same could happen with your proposal
> as well.  So any automagic here should not harm either.
>
> The automagic would come into play when neither caller nor user specify
> a particular preference for a window or all other preferences have been
> exhausted and 'display-buffer' invokes 'display-buffer-use-some-window'
> (not 'display-buffer-reuse-window') in which case that function would
> look for a window with a matching 'category' parameter.  Which problems
> do you see here?

The only problem is that it will change the current default behavior,
so instead of using the lru window it will prefer to use the window
with the same category.  I customized 'display-buffer-base-action'
to use 'get-mru-window', so I won't be affected by this change,
but I care about other users.  Are you sure it would be expectable
for users to display buffers with the same category in one window?
How they could revert this behavior back to the previous default?

This is why I suggest to create a separate action.  If you are sure
it should be used by default, then it could be inserted
before display-buffer-use-some-window here:

  (defconst display-buffer-fallback-action
    '((display-buffer--maybe-same-window
       display-buffer-reuse-window
       display-buffer--maybe-pop-up-frame-or-window
       display-buffer-in-previous-window
       display-buffer-reuse-category-window ;; <-- NEW!
       display-buffer-use-some-window
       display-buffer-pop-up-frame))

Otherwise, users will be able to use the new action
display-buffer-reuse-category-window in their customizations
of display-buffer-alist.

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]

 > Better to use something else.  The 'category' alist entry
 > is intended to be used only in display-buffer calls like this:
 >
 >    (display-buffer (get-buffer-create "*info*")
 >                    '(nil (category . Info-mode)))
 >
 > to allow using 'category' in the condition part of display-buffer-alist
 > to match the display-buffer call:
 >
 >    (add-to-list 'display-buffer-alist
 >                 '((category . Info-mode)
 >                   (display-buffer-same-window)))

What I meant was to use precisely that as

     (add-to-list 'display-buffer-alist
                  '((category . foo)
                    (display-buffer-same-window)))

Info-mode OTOH is confusing here: It should IMO be matched by

          (buffer-mode (with-current-buffer buffer major-mode))

in 'display-buffer-reuse-mode-window' since Info-mode is the
buffer-local value of 'major-mode' in Info buffers.  Setting it up as a
category makes hardly sense to me.

'foo' would be much more generic (you don't say 'comint-mode' either in
your proposed change) and completely detached from the specific buffer
you intend to show (in general you can't guess 'comint' from the major
mode of the buffer to show).  Hence a category called 'foo' would in my
concept be matched by 'buffer-match-p' via 'category' passed as car of
CONDITION and a 'category' entry passed via the action alist by the
caller of 'display-buffer'.

 > What you proposed is a different usage of 'category'
 > that makes sense in the action part of display-buffer-alist:
 >
 >    (add-to-list 'display-buffer-alist
 >                 '("\*Help\*"
 >                   (display-buffer-same-window)
 >                   (category . Info-mode)))

No.  A user would have no idea of the name of the buffer.

 > It would be cleaner to use a different action for this feature, e.g.:
 >
 >    (add-to-list 'display-buffer-alist
 >                 '("\*Help\*"
 >                   (display-buffer-reuse-category-window)
 >                   (category . Info-mode)))
 >
 > or without an alist entry:
 >
 >    (add-to-list 'display-buffer-alist
 >                 '("\*Help\*"
 >                   (display-buffer-reuse-category-window)))

 > The only problem is that it will change the current default behavior,
 > so instead of using the lru window it will prefer to use the window
 > with the same category.

Right.  It would (1) address the problem raised in bugs like Bug#74246
and would (2) still allow users to handle 'display-comint-buffer-action'
and 'display-tex-shell-buffer-action' in your sense.  If and when for
the latter 'display-buffer-use-some-window' is called, the original
suggestion of the caller of 'display-buffer' has been levered out
already.

Obviously, the caller has to be aware of the fact that providing
(category . foo) without an action function will eventually give
'display-buffer-use-some-window' the opportunity to override the lru
with using a window that previously displayed a buffer that was supplied
with the same category argument.  Can you think of any harm this could
do?  I think the benefit of getting rid of the lru behavior in these
cases by far outweighs any such harms.

 > I customized 'display-buffer-base-action'
 > to use 'get-mru-window', so I won't be affected by this change,
 > but I care about other users.  Are you sure it would be expectable
 > for users to display buffers with the same category in one window?
 > How they could revert this behavior back to the previous default?

By adding a (some-window . nil) or (some-window . lru) entry which would
override the 'category' entry.

 > This is why I suggest to create a separate action.  If you are sure
 > it should be used by default, then it could be inserted
 > before display-buffer-use-some-window here:
 >
 >    (defconst display-buffer-fallback-action
 >      '((display-buffer--maybe-same-window
 >         display-buffer-reuse-window
 >         display-buffer--maybe-pop-up-frame-or-window
 >         display-buffer-in-previous-window
 >         display-buffer-reuse-category-window ;; <-- NEW!
 >         display-buffer-use-some-window
 >         display-buffer-pop-up-frame))
 >
 > Otherwise, users will be able to use the new action
 > display-buffer-reuse-category-window in their customizations
 > of display-buffer-alist.

What I want is to get rid of the lru behavior in the context of say
Bug#74246 for users who do _not want_ to tinker with
'display-buffer-alist'.  Most Emacs users know how to split windows.
Few of them may want to have ‘display-buffer’ use them up piecemeal by
eventually displaying images in all of them.

martin

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

[Dmitry Gutov]

On 25/11/2024 03:58, Dmitry Gutov wrote:
>> 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'.
> 
> No it's okay - I guess if this makes some customizations easier (not 
> erroring out), that's also a win.

Now pushed to patch to master in commit 0624fe6f849, thanks for the 
comments. I'll wait for the rest of the discussion to settle before 
closing the bug.

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

[Dmitry Gutov]

On 26/11/2024 11:15, martin rudalics wrote:
>  > This is why I suggest to create a separate action.  If you are sure
>  > it should be used by default, then it could be inserted
>  > before display-buffer-use-some-window here:
>  >
>  >    (defconst display-buffer-fallback-action
>  >      '((display-buffer--maybe-same-window
>  >         display-buffer-reuse-window
>  >         display-buffer--maybe-pop-up-frame-or-window
>  >         display-buffer-in-previous-window
>  >         display-buffer-reuse-category-window ;; <-- NEW!
>  >         display-buffer-use-some-window
>  >         display-buffer-pop-up-frame))
>  >
>  > Otherwise, users will be able to use the new action
>  > display-buffer-reuse-category-window in their customizations
>  > of display-buffer-alist.
> 
> What I want is to get rid of the lru behavior in the context of say
> Bug#74246 for users who do _not want_ to tinker with
> 'display-buffer-alist'.  Most Emacs users know how to split windows.
> Few of them may want to have ‘display-buffer’ use them up piecemeal by
> eventually displaying images in all of them.

That's a nice idea.

Just to note - in the patch that I've just pushed for Xref the category 
('xref-jump') doesn't describe the destination buffer but only the 
command that is currently being executed. The destination buffers are 
regular file buffers, usually not distinct from the other file buffers 
belonging to the same project.

So choosing the window by matching the category might not be so natural 
in that case as reusing a window already showing an image. Probably 
won't hurt, though.

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

[Juri Linkov]

> What I want is to get rid of the lru behavior in the context of say
> Bug#74246 for users who do _not want_ to tinker with
> 'display-buffer-alist'.  Most Emacs users know how to split windows.
> Few of them may want to have ‘display-buffer’ use them up piecemeal by
> eventually displaying images in all of them.

I welcome a change to get rid of the lru behavior in favor
of using the category.  What I only suggested is to add a separate
action 'display-buffer-reuse-category-window' with higher precedence
than 'display-buffer-use-some-window'.  But this is just an technical detail.

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]

 > Just to note - in the patch that I've just pushed for Xref the
 > category ('xref-jump') doesn't describe the destination buffer but
 > only the command that is currently being executed. The destination
 > buffers are regular file buffers, usually not distinct from the other
 > file buffers belonging to the same project.

As I understand it, both 'xref--switch-to-buffer' and
'xref--display-buffer-in-window' prefer the selected window.  Only if
the selected window is not suitable, 'display-buffer-use-some-window'
could prefer a window with a 'xref-jump' 'category' parameter instead of
using the 'lru' window.  I don't think anyone would even notice the
difference.

If a user does

+  (setq display-buffer-alist '(((category . xref)
+                                (display-buffer-reuse-window
+                                 display-buffer-use-some-window)
+                                (some-window . mru))))

the 'some-window' entry on the last line would override any automagic in
'display-buffer-use-some-window' as I envision it anyway.

Is there any other 'display-buffer' call in xref.el that would set up a
category and prefer any other but the selected window?  IIUC
'xref--display-buffer-in-other-window' doesn't.  How would a user
customize its behavior?  Note that I'm completely ignorant of the
'xref--with-dedicated-window' trick - is there any reason why
'inhibit-same-window' is not sufficient here?

Personally, I think that a window parameter based choice could be even
better than 'mru' but Juri who uses 'mru' on a regular basis seems to be
fine with it.  For some users 'mru' might not DTRT when they temporarily
pop up a third window they use for showing some other, unrelated buffer
in between jumping to a sequence of references.

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]

 > I welcome a change to get rid of the lru behavior in favor
 > of using the category.  What I only suggested is to add a separate
 > action 'display-buffer-reuse-category-window' with higher precedence
 > than 'display-buffer-use-some-window'.  But this is just an technical detail.

I think so too.  Whatever it is, I think that 'category' should be only
detected by 'buffer-match-p' and both user and 'display-buffer' when
using "some" window should be free to interpret it in their own ways.
Since ‘some-window’ would be always given precedence over 'category' as
far as 'display-buffer' is concerned, the user would be free to override
any automagic the latter does.

martin

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

[Dmitry Gutov]

On 27/11/2024 10:58, martin rudalics wrote:
>  > Just to note - in the patch that I've just pushed for Xref the
>  > category ('xref-jump') doesn't describe the destination buffer but
>  > only the command that is currently being executed. The destination
>  > buffers are regular file buffers, usually not distinct from the other
>  > file buffers belonging to the same project.
> 
> As I understand it, both 'xref--switch-to-buffer' and
> 'xref--display-buffer-in-window' prefer the selected window.  Only if
> the selected window is not suitable, 'display-buffer-use-some-window'
> could prefer a window with a 'xref-jump' 'category' parameter instead of
> using the 'lru' window.  I don't think anyone would even notice the
> difference.

It does seem like it will switch to the "mru" kind of placement though: 
as soon as "some" other window is used for xref-jump once, all the 
following similar calls will follow it.

Might be an improvement, but definitely an incompatible change.

> If a user does
> 
> +  (setq display-buffer-alist '(((category . xref)
> +                                (display-buffer-reuse-window
> +                                 display-buffer-use-some-window)
> +                                (some-window . mru))))
> 
> the 'some-window' entry on the last line would override any automagic in
> 'display-buffer-use-some-window' as I envision it anyway.

Sure.

> Is there any other 'display-buffer' call in xref.el that would set up a
> category and prefer any other but the selected window?

By default, you mean?

> IIUC
> 'xref--display-buffer-in-other-window' doesn't.  How would a user
> customize its behavior?  Note that I'm completely ignorant of the
> 'xref--with-dedicated-window' trick - is there any reason why
> 'inhibit-same-window' is not sufficient here?

We want to select the new window in relation the "original" window (with 
a file-visiting buffer), while avoiding touching the "results list" 
window as well. E.g. when the original command was 
xref-find-definitions-other-window.

It's probably not an ideal solution, but one settled on over several tries.

> Personally, I think that a window parameter based choice could be even
> better than 'mru' but Juri who uses 'mru' on a regular basis seems to be
> fine with it.  For some users 'mru' might not DTRT when they temporarily
> pop up a third window they use for showing some other, unrelated buffer
> in between jumping to a sequence of references.

Either seems reasonable to me, just as long as the choices can be 
anticipated in advance by the user.

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]

 > It does seem like it will switch to the "mru" kind of placement
 > though: as soon as "some" other window is used for xref-jump once, all
 > the following similar calls will follow it.

"mru" is a user option, the default is "lru".  Once "mru" was chosen for
a specific category, it will be chosen again until the user changes it.
But it is true that the default "lru", once overridden by a category,
will be overridden again - for 'xref-jump' iff the same window is not
suitable.  Still ...

 > Might be an improvement, but definitely an incompatible change.

... this is true.

 >> Is there any other 'display-buffer' call in xref.el that would set up a
 >> category and prefer any other but the selected window?
 >
 > By default, you mean?

If default stands for using the ACTION argument of 'display-buffer',
then "yes".

 > We want to select the new window in relation the "original" window
 > (with a file-visiting buffer), while avoiding touching the "results
 > list" window as well. E.g. when the original command was
 > xref-find-definitions-other-window.

Let's see what Juri thinks of an

(unsuitable . window-or-list-of-windows)

alist entry.

 > It's probably not an ideal solution, but one settled on over several tries.

It is a pain for the caller.

 > Either seems reasonable to me, just as long as the choices can be
 > anticipated in advance by the user.

Agreed.

martin

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

[Dmitry Gutov]

On 28/11/2024 11:27, martin rudalics wrote:

>  >> Is there any other 'display-buffer' call in xref.el that would set up a
>  >> category and prefer any other but the selected window?
>  >
>  > By default, you mean?
> 
> If default stands for using the ACTION argument of 'display-buffer',
> then "yes".

There are only two functions in xref.el that specify 'category' for 
'display-buffer', so no.

>  > We want to select the new window in relation the "original" window
>  > (with a file-visiting buffer), while avoiding touching the "results
>  > list" window as well. E.g. when the original command was
>  > xref-find-definitions-other-window.
> 
> Let's see what Juri thinks of an
> 
> (unsuitable . window-or-list-of-windows)
> 
> alist entry.

Could be helpful - keeping in mind having to use a compatibility shim 
for older Emacs.

>  > It's probably not an ideal solution, but one settled on over several 
> tries.
> 
> It is a pain for the caller.

Indeed.

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

[Juri Linkov]

>> We want to select the new window in relation the "original" window
>> (with a file-visiting buffer), while avoiding touching the "results
>> list" window as well. E.g. when the original command was
>> xref-find-definitions-other-window.
>
> Let's see what Juri thinks of an
>
> (unsuitable . window-or-list-of-windows)
>
> alist entry.

This can be achieved by an appropriate function in (some-window . (lambda ...))

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

[Dmitry Gutov]

On 28/11/2024 20:32, Juri Linkov wrote:
>>> We want to select the new window in relation the "original" window
>>> (with a file-visiting buffer), while avoiding touching the "results
>>> list" window as well. E.g. when the original command was
>>> xref-find-definitions-other-window.
>> Let's see what Juri thinks of an
>>
>> (unsuitable . window-or-list-of-windows)
>>
>> alist entry.
> This can be achieved by an appropriate function in (some-window . (lambda ...))

What would than lambda do, though?

Currently xref--display-buffer-in-other-window is user to display the 
buffer in some "other" window compared to the original window, excluding 
the current one.

Would the proposed lambda call window-list, iterate through it, filter 
out the entries (dedicated and such), and check for equality with two 
given windows? The result might be longer than the current implementation.

The replacement for xref--display-buffer-in-window seems easier to do, 
IIUC this keeps all the current behavior (but it's also Emacs 31+ only):

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index e6f029f3fa8..c38b3e9f5f7 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -690,9 +690,9 @@ xref--show-pos-in-buf
                    (window-live-p xref--original-window)
                    (or (not (window-dedicated-p xref--original-window))
                        (eq (window-buffer xref--original-window) buf)))
-                 `((xref--display-buffer-in-window)
+                 `((display-buffer-use-some-window)
                     (category . xref-jump)
-                   (window . ,xref--original-window)))
+                   (some-window . ,(cl-constantly xref--original-window))))
                  (t
                   '(nil (category . xref-jump))))))
      (with-selected-window (display-buffer buf action)
@@ -707,12 +707,6 @@ xref--display-buffer-in-other-window
       (with-selected-window window
         (display-buffer buffer t)))))

-(defun xref--display-buffer-in-window (buffer alist)
-  (let ((window (assoc-default 'window alist)))
-    (cl-assert window)
-    (with-selected-window window
-      (display-buffer buffer '(display-buffer-same-window)))))
-
  (defun xref--show-location (location &optional select)
    "Help `xref-show-xref' and `xref-goto-xref' do their job.
  Go to LOCATION and if SELECT is non-nil select its window.

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

[Juri Linkov]

> The replacement for xref--display-buffer-in-window seems easier to do, 
> IIUC this keeps all the current behavior (but it's also Emacs 31+ only):
>
> @@ -690,9 +690,9 @@ xref--show-pos-in-buf
>                     (window-live-p xref--original-window)
>                     (or (not (window-dedicated-p xref--original-window))
>                         (eq (window-buffer xref--original-window) buf)))
> -                 `((xref--display-buffer-in-window)
> +                 `((display-buffer-use-some-window)
>                      (category . xref-jump)
> -                   (window . ,xref--original-window)))
> +                   (some-window . ,(cl-constantly xref--original-window))))

I see no problems with this patch.  Even better would be if Martin agrees
to modify 'display-buffer-use-some-window' to accept a window in
'(some-window . WINDOW)'.

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

[Juri Linkov]

>>> Do you mean that the users might want to shoot themselves in the foot
>>> and remove '(category . comint)' from the default value of
>>> display-comint-buffer-action?
>>
>> Yes, that's the scenario that bothers me.  Users could do that without
>> understanding the subtleties of the issue.
>>
>>> Then indeed we need to move '(category . comint)' directly
>>> to every display-buffer call that currently uses
>>> display-comint-buffer-action.
>>
>> I think we should do that for Emacs 31, yes.
>
> Ok, here is the patch for Emacs 31:
>
> diff --git a/etc/NEWS b/etc/NEWS
> +*** The user option 'display-comint-buffer-action' has been removed.
> +It has been obsolete since Emacs 30.1.  Use '(category . comint)' instead.
> +Another user option 'display-tex-shell-buffer-action' has been removed too
> +for which you can use '(category . tex-shell)'.

This patch is now pushed to master.

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 see no problems with this patch.  Even better would be if Martin agrees
 > to modify 'display-buffer-use-some-window' to accept a window in
 > '(some-window . WINDOW)'.

He obviously agrees (provided WINDOW is a live window).

martin

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

[Juri Linkov]

>> I see no problems with this patch.  Even better would be if Martin agrees
>> to modify 'display-buffer-use-some-window' to accept a window in
>> '(some-window . WINDOW)'.
>
> He obviously agrees (provided WINDOW is a live window).

Ok.  This reminds me adding '(previous-window . WINDOW)'
to 'display-buffer-in-previous-window' with:

    (when (and previous-window (symbolp previous-window)
               (boundp previous-window))
      (setq previous-window (symbol-value previous-window)))
    (when (and (setq window previous-window)
	       (window-live-p window)

Probably we should support both a symbol and a value for 'some-window'
as well.

BTW, here is an example of using 'previous-window' with an advice on
'window--display-buffer' that could be combined with 'some-window':

  (defvar-local display-buffer-previous-window nil)

  (add-to-list 'display-buffer-alist
               '((category . xref-jump)
                 display-buffer-in-previous-window
                 (previous-window . display-buffer-previous-window)
                 (inhibit-same-window . nil))
               ;; Append to not override display-buffer-same-window
               'append)

  (define-advice xref-goto-xref (:around (ofun &rest args) previous-window)
    (let ((buffer (current-buffer)))
      (apply ofun args)
      (with-current-buffer buffer
        (setq-local display-buffer-previous-window (selected-window)))))

  (define-advice window--display-buffer (:around (ofun &rest args) previous-window)
    (let ((buffer (current-buffer))
          (window (apply ofun args)))
      (with-current-buffer buffer
        (setq-local display-buffer-previous-window window))
      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]

 > Ok.  This reminds me adding '(previous-window . WINDOW)'
 > to 'display-buffer-in-previous-window' with:
 >
 >      (when (and previous-window (symbolp previous-window)
 >                 (boundp previous-window))
 >        (setq previous-window (symbol-value previous-window)))

Where and how is or would this be used?  In the sources all I see is

		          `((previous-window . ,debugger-previous-window))))))

so I suppose the symbol value would be used in customizations only.

 >      (when (and (setq window previous-window)
 > 	       (window-live-p window)
 >
 > Probably we should support both a symbol and a value for 'some-window'
 > as well.

Where and how would this be used?

 > BTW, here is an example of using 'previous-window' with an advice on
 > 'window--display-buffer' that could be combined with 'some-window':
 >
 >    (defvar-local display-buffer-previous-window nil)
 >
 >    (add-to-list 'display-buffer-alist
 >                 '((category . xref-jump)
 >                   display-buffer-in-previous-window
 >                   (previous-window . display-buffer-previous-window)

So here 'display-buffer' would retrieve the symbol-value of whatever is
in the buffer-local value of 'display-buffer-previous-window'.  Right?

 >                   (inhibit-same-window . nil))
 >                 ;; Append to not override display-buffer-same-window
 >                 'append)
 >
 >    (define-advice xref-goto-xref (:around (ofun &rest args) previous-window)
 >      (let ((buffer (current-buffer)))
 >        (apply ofun args)
 >        (with-current-buffer buffer
 >          (setq-local display-buffer-previous-window (selected-window)))))

This relies on 'ofun' selecting a window and would not work for plain
'display-buffer'.  Right?

 >    (define-advice window--display-buffer (:around (ofun &rest args) previous-window)
 >      (let ((buffer (current-buffer))
 >            (window (apply ofun args)))
 >        (with-current-buffer buffer
 >          (setq-local display-buffer-previous-window window))
 >        window))

And this assumes that the current buffer is the one where to set the
variable.  Would this work with a function calling 'xref-goto-xref',
bound to a key and called interactively with an arbitrary window
selected?

martin

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

[Juri Linkov]

>> Ok.  This reminds me adding '(previous-window . WINDOW)'
>> to 'display-buffer-in-previous-window' with:
>>
>>      (when (and previous-window (symbolp previous-window)
>>                 (boundp previous-window))
>>        (setq previous-window (symbol-value previous-window)))
>
> Where and how is or would this be used?  In the sources all I see is
>
> 		          `((previous-window . ,debugger-previous-window))))))
>
> so I suppose the symbol value would be used in customizations only.

I just tried to understand the differences between
display-buffer-in-previous-window and display-buffer-use-some-window.

>>      (when (and (setq window previous-window)
>> 	       (window-live-p window)
>>
>> Probably we should support both a symbol and a value for 'some-window'
>> as well.
>
> Where and how would this be used?

Mostly in customizations to support both ways:

    `((previous-window . display-buffer-previous-window))
    `((previous-window . ,display-buffer-previous-window))

    `((some-window . display-buffer-some-window))
    `((some-window . ,display-buffer-some-window))

>> BTW, here is an example of using 'previous-window' with an advice on
>> 'window--display-buffer' that could be combined with 'some-window':
>>
>>    (defvar-local display-buffer-previous-window nil)
>>
>>    (add-to-list 'display-buffer-alist
>>                 '((category . xref-jump)
>>                   display-buffer-in-previous-window
>>                   (previous-window . display-buffer-previous-window)
>
> So here 'display-buffer' would retrieve the symbol-value of whatever is
> in the buffer-local value of 'display-buffer-previous-window'.  Right?

Right.

>>                   (inhibit-same-window . nil))
>>                 ;; Append to not override display-buffer-same-window
>>                 'append)
>>
>>    (define-advice xref-goto-xref (:around (ofun &rest args) previous-window)
>>      (let ((buffer (current-buffer)))
>>        (apply ofun args)
>>        (with-current-buffer buffer
>>          (setq-local display-buffer-previous-window (selected-window)))))
>
> This relies on 'ofun' selecting a window and would not work for plain
> 'display-buffer'.  Right?

It should work for plain 'display-buffer' because of using 'with-current-buffer'.

>>    (define-advice window--display-buffer (:around (ofun &rest args) previous-window)
>>      (let ((buffer (current-buffer))
>>            (window (apply ofun args)))
>>        (with-current-buffer buffer
>>          (setq-local display-buffer-previous-window window))
>>        window))
>
> And this assumes that the current buffer is the one where to set the
> variable.  Would this work with a function calling 'xref-goto-xref',
> bound to a key and called interactively with an arbitrary window
> selected?

This is how it's designed to work: to associate the displayed window
with the buffer that was current.

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

[Dmitry Gutov]

On 04/12/2024 09:35, Juri Linkov wrote:
>> The replacement for xref--display-buffer-in-window seems easier to do,
>> IIUC this keeps all the current behavior (but it's also Emacs 31+ only):
>>
>> @@ -690,9 +690,9 @@ xref--show-pos-in-buf
>>                      (window-live-p xref--original-window)
>>                      (or (not (window-dedicated-p xref--original-window))
>>                          (eq (window-buffer xref--original-window) buf)))
>> -                 `((xref--display-buffer-in-window)
>> +                 `((display-buffer-use-some-window)
>>                       (category . xref-jump)
>> -                   (window . ,xref--original-window)))
>> +                   (some-window . ,(cl-constantly xref--original-window))))
> I see no problems with this patch.  Even better would be if Martin agrees
> to modify 'display-buffer-use-some-window' to accept a window in
> '(some-window . WINDOW)'.

Thanks.

In any case, like with some other changes, we should probably wait 
one-two Emacs releases before installing. Or organize it as 
backward-compatible dispatch along two paths, which unfortunately 
doesn't make things simpler in the short run.

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]

 >>>         (with-current-buffer buffer
 >>>           (setq-local display-buffer-previous-window (selected-window)))))
 >>
 >> This relies on 'ofun' selecting a window and would not work for plain
 >> 'display-buffer'.  Right?
 >
 > It should work for plain 'display-buffer' because of using 'with-current-buffer'.

'display-buffer' per se doesn't care about the current buffer.  The
assumption that the selected window also holds the current buffer is
nowhere guaranteed.  Just think of 'display-buffer' running from a
timer.

martin

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

[Juri Linkov]

>>>>         (with-current-buffer buffer
>>>>           (setq-local display-buffer-previous-window (selected-window)))))
>>>
>>> This relies on 'ofun' selecting a window and would not work for plain
>>> 'display-buffer'.  Right?
>>
>> It should work for plain 'display-buffer' because of using 'with-current-buffer'.
>
> 'display-buffer' per se doesn't care about the current buffer.  The
> assumption that the selected window also holds the current buffer is
> nowhere guaranteed.  Just think of 'display-buffer' running from a
> timer.

I meant the buffer that was current at the beginning of the command:
(window-buffer (old-selected-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]

 >> 'display-buffer' per se doesn't care about the current buffer.  The
 >> assumption that the selected window also holds the current buffer is
 >> nowhere guaranteed.  Just think of 'display-buffer' running from a
 >> timer.
 >
 > I meant the buffer that was current at the beginning of the command:
 > (window-buffer (old-selected-window)).

The doc-string of 'old-selected-window' says

   The return value is the window selected the last time window change
   functions were run.

One and the same command may encompass an arbitrary number of redisplays
(think of a command reading the minibuffer) which all may affect the
return value of that function.  So there is no guarantee that
'old-selected-window' will get you what you expect.

martin