bug#74246: [PATCH] Reuse display windows in image-dired

bug#74246: [PATCH] Reuse display windows in image-dired

[Morgan Smith]

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

Tags: patch

Hello,

When using image-dired, one can use SPC (image-dired-display-next)
repeatedly to display successive images in a dedicated buffer.  However,
it seems to choose a different window each time I press SPC.

I can reproduce this in "emacs -Q" when I have 4 windows.

The following patch works on my system.

Thanks,

Morgan

In GNU Emacs 30.0.91 (build 1, x86_64-pc-linux-gnu, GTK+ Version
3.24.41, cairo version 1.18.0)
System Description: Guix System

Configured using:
 'configure
CONFIG_SHELL=/gnu/store/3jhfhxdf6v5ms10x5zmnl166dh3yhbr1-bash-minimal-5.1.16/bin/bash
SHELL=/gnu/store/3jhfhxdf6v5ms10x5zmnl166dh3yhbr1-bash-minimal-5.1.16/bin/bash
--prefix=/gnu/store/9x5277gcjzwlis5yd0iq7ypq8f78jhpa-emacs-next-pgtk-30.0.91-1.9a1c76b
--enable-fast-install --with-pgtk --with-cairo --with-modules
--with-native-compilation=aot --disable-build-details'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Reuse-display-windows-in-image-dired.patch --]
[-- Type: text/patch, Size: 1347 bytes --]

From a02b37cbcdbee4c773336a82f2109b1c03467107 Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Thu, 7 Nov 2024 14:24:01 -0500
Subject: [PATCH] Reuse display windows in image-dired

* lisp/image/image-dired.el(image-dired-display-image): Reuse
the windows used by the previous buffer.
---
 lisp/image/image-dired.el | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lisp/image/image-dired.el b/lisp/image/image-dired.el
index 1928b0a2955..37f23665755 100644
--- a/lisp/image/image-dired.el
+++ b/lisp/image/image-dired.el
@@ -1268,11 +1268,15 @@ image-dired-display-image
   (setq file (expand-file-name file))
   (when (not (file-exists-p file))
     (error "No such file: %s" file))
-  (let ((buf (get-buffer image-dired-display-image-buffer))
-        (cur-win (selected-window)))
+  (let* ((buf (get-buffer image-dired-display-image-buffer))
+         (windows (and buf
+                       (get-buffer-window-list buf t t)))
+         (cur-win (selected-window)))
     (when buf
       (kill-buffer buf))
     (when-let ((buf (find-file-noselect file nil t)))
+      (dolist (window windows)
+        (set-window-buffer window buf))
       (pop-to-buffer buf)
       (rename-buffer image-dired-display-image-buffer)
       (if (string-match (image-file-name-regexp) file)
-- 
2.46.0

bug#74246: [PATCH] Reuse display windows in image-dired

[Eli Zaretskii]

> From: Morgan Smith <Morgan.J.Smith@outlook.com>
> Date: Thu, 07 Nov 2024 15:19:15 -0500
> 
> When using image-dired, one can use SPC (image-dired-display-next)
> repeatedly to display successive images in a dedicated buffer.  However,
> it seems to choose a different window each time I press SPC.
> 
> I can reproduce this in "emacs -Q" when I have 4 windows.
> 
> The following patch works on my system.

Thanks.

Martin and Stefan, any comments or suggestions?

bug#74246: [PATCH] Reuse display windows in image-dired

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

 >> When using image-dired, one can use SPC (image-dired-display-next)
 >> repeatedly to display successive images in a dedicated buffer.  However,
 >> it seems to choose a different window each time I press SPC.
 >>
 >> I can reproduce this in "emacs -Q" when I have 4 windows.
 >>
 >> The following patch works on my system.
 >
 > Thanks.
 >
 > Martin and Stefan, any comments or suggestions?

It's a problem we've seen in other contexts before: For historical
reasons 'display-buffer' by default tries to use the least recently used
window which changes whenever we display an image in it.  Juri invented
the 'some-window' buffer display option and I quote the corresponding
entry from the Elisp manual here:

      The above describes the behavior when the ‘some-window’ ALIST entry
      is ‘lru’ or ‘nil’ which is the default.  Another possible value is
      ‘mru’.  If, for example, ‘display-buffer-base-action’ is customized
      to ‘(nil . ((some-window . mru)))’, then this function will prefer
      the most recently used window.  This will try to display several
      buffers from consecutive calls of ‘display-buffer’ in the same
      window.  Consider a configuration of three or more windows where a
      user wants to consult, in a non-selected window, one after the
      other, the results of a query spread among several buffers.  With
      the ‘lru’ strategy, Emacs may continuously choose another window
      because the least recently used window changes with every call of
      ‘display-buffer-use-some-window’.  With the ‘mru’ strategy, the
      window chosen would always remain the same, resulting in a
      predictable user experience.

So I think that Morgan should try to use this approach first.  But if
the image is always displayed in 'image-dired-image-display' mode then
the appropriate action function 'image-dired' should use is
'display-buffer-reuse-mode-window'.

+  (let* ((buf (get-buffer image-dired-display-image-buffer))
+         (windows (and buf
+                       (get-buffer-window-list buf t t)))
+         (cur-win (selected-window)))
      (when buf
        (kill-buffer buf))
      (when-let ((buf (find-file-noselect file nil t)))
+      (dolist (window windows)
+        (set-window-buffer window buf))

Is this supposed to show the same image in all windows that previously
displayed 'image-dired-display-image-buffer'?

FWIW, I would rewrite 'image-dired-display-image' as the (largely
untested)

(defun image-dired-display-image (file &optional _ignored)
   "Display image FILE in the image buffer window.
If FILE is an image, the window will use `image-dired-image-mode'
which is based on `image-mode'."
   (declare (advertised-calling-convention (file) "29.1"))
   (setq file (expand-file-name file))
   (when (not (file-exists-p file))
     (error "No such file: %s" file))
   (let* ((buffer (get-buffer-create image-dired-display-image-buffer))
	 (window (get-buffer-window buffer)))
     (find-file-noselect-1 buffer file nil t nil nil)
     (with-current-buffer buffer
       (if (string-match (image-file-name-regexp) file)
           (image-dired-image-mode)
         ;; Support visiting PDF files.
         (normal-mode)))

     (unless window
	(display-buffer buffer))))

instead of doing all that 'kill-buffer' (which could delete the selected
window as a side-effect so the final 'select-window' will throw an
error) 'rename-buffer' rigmarole.

martin

bug#74246: [PATCH] Reuse display windows in image-dired

[Eli Zaretskii]

Ping! Morgan, could you please try Martin's suggestions and report
back?

> Date: Sat, 9 Nov 2024 18:36:12 +0100
> Cc: 74246@debbugs.gnu.org
> From: martin rudalics <rudalics@gmx.at>
> 
>  >> When using image-dired, one can use SPC (image-dired-display-next)
>  >> repeatedly to display successive images in a dedicated buffer.  However,
>  >> it seems to choose a different window each time I press SPC.
>  >>
>  >> I can reproduce this in "emacs -Q" when I have 4 windows.
>  >>
>  >> The following patch works on my system.
>  >
>  > Thanks.
>  >
>  > Martin and Stefan, any comments or suggestions?
> 
> It's a problem we've seen in other contexts before: For historical
> reasons 'display-buffer' by default tries to use the least recently used
> window which changes whenever we display an image in it.  Juri invented
> the 'some-window' buffer display option and I quote the corresponding
> entry from the Elisp manual here:
> 
>       The above describes the behavior when the ‘some-window’ ALIST entry
>       is ‘lru’ or ‘nil’ which is the default.  Another possible value is
>       ‘mru’.  If, for example, ‘display-buffer-base-action’ is customized
>       to ‘(nil . ((some-window . mru)))’, then this function will prefer
>       the most recently used window.  This will try to display several
>       buffers from consecutive calls of ‘display-buffer’ in the same
>       window.  Consider a configuration of three or more windows where a
>       user wants to consult, in a non-selected window, one after the
>       other, the results of a query spread among several buffers.  With
>       the ‘lru’ strategy, Emacs may continuously choose another window
>       because the least recently used window changes with every call of
>       ‘display-buffer-use-some-window’.  With the ‘mru’ strategy, the
>       window chosen would always remain the same, resulting in a
>       predictable user experience.
> 
> So I think that Morgan should try to use this approach first.  But if
> the image is always displayed in 'image-dired-image-display' mode then
> the appropriate action function 'image-dired' should use is
> 'display-buffer-reuse-mode-window'.
> 
> +  (let* ((buf (get-buffer image-dired-display-image-buffer))
> +         (windows (and buf
> +                       (get-buffer-window-list buf t t)))
> +         (cur-win (selected-window)))
>       (when buf
>         (kill-buffer buf))
>       (when-let ((buf (find-file-noselect file nil t)))
> +      (dolist (window windows)
> +        (set-window-buffer window buf))
> 
> Is this supposed to show the same image in all windows that previously
> displayed 'image-dired-display-image-buffer'?
> 
> FWIW, I would rewrite 'image-dired-display-image' as the (largely
> untested)
> 
> (defun image-dired-display-image (file &optional _ignored)
>    "Display image FILE in the image buffer window.
> If FILE is an image, the window will use `image-dired-image-mode'
> which is based on `image-mode'."
>    (declare (advertised-calling-convention (file) "29.1"))
>    (setq file (expand-file-name file))
>    (when (not (file-exists-p file))
>      (error "No such file: %s" file))
>    (let* ((buffer (get-buffer-create image-dired-display-image-buffer))
> 	 (window (get-buffer-window buffer)))
>      (find-file-noselect-1 buffer file nil t nil nil)
>      (with-current-buffer buffer
>        (if (string-match (image-file-name-regexp) file)
>            (image-dired-image-mode)
>          ;; Support visiting PDF files.
>          (normal-mode)))
> 
>      (unless window
> 	(display-buffer buffer))))
> 
> instead of doing all that 'kill-buffer' (which could delete the selected
> window as a side-effect so the final 'select-window' will throw an
> error) 'rename-buffer' rigmarole.
> 
> martin

bug#74246: [PATCH] Reuse display windows in image-dired

[Morgan Smith]

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

Eli Zaretskii <eliz@gnu.org> writes:

> Ping! Morgan, could you please try Martin's suggestions and report
> back?
>
>>
>> FWIW, I would rewrite 'image-dired-display-image' as the (largely
>> untested)
>>
>> (defun image-dired-display-image (file &optional _ignored)
>>    "Display image FILE in the image buffer window.
>> If FILE is an image, the window will use `image-dired-image-mode'
>> which is based on `image-mode'."
>>    (declare (advertised-calling-convention (file) "29.1"))
>>    (setq file (expand-file-name file))
>>    (when (not (file-exists-p file))
>>      (error "No such file: %s" file))
>>    (let* ((buffer (get-buffer-create image-dired-display-image-buffer))
>> 	 (window (get-buffer-window buffer)))
>>      (find-file-noselect-1 buffer file nil t nil nil)
>>      (with-current-buffer buffer
>>        (if (string-match (image-file-name-regexp) file)
>>            (image-dired-image-mode)
>>          ;; Support visiting PDF files.
>>          (normal-mode)))
>>
>>      (unless window
>> 	(display-buffer buffer))))
>>
>> instead of doing all that 'kill-buffer' (which could delete the selected
>> window as a side-effect so the final 'select-window' will throw an
>> error) 'rename-buffer' rigmarole.
>>
>> martin

I initially explored this option but was weary to use
'find-file-noselect-1` as it looks like it should be an internal
function and is used only in the file it's defined in.

However, I do like this solution the most.  Attached is a patch almost
identical to the proposed function.  The proposed function would be
perfect if it contained the line '(display-buffer buffer t)'.

Morgan


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Reuse-display-windows-in-image-dired-bug-74246.patch --]
[-- Type: text/x-patch, Size: 1569 bytes --]

From 7d0d9133b5276565589aadea9c0804522ed3cc64 Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Wed, 27 Nov 2024 19:22:27 -0500
Subject: [PATCH] Reuse display windows in image-dired (bug#74246)

* lisp/image/image-dired.el(image-dired-display-image): Reuse
the buffer if it exists.
---
 lisp/image/image-dired.el | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/lisp/image/image-dired.el b/lisp/image/image-dired.el
index 83745e88f09..29863acea8e 100644
--- a/lisp/image/image-dired.el
+++ b/lisp/image/image-dired.el
@@ -1268,18 +1268,15 @@ image-dired-display-image
   (setq file (expand-file-name file))
   (when (not (file-exists-p file))
     (error "No such file: %s" file))
-  (let ((buf (get-buffer image-dired-display-image-buffer))
-        (cur-win (selected-window)))
-    (when buf
-      (kill-buffer buf))
-    (when-let* ((buf (find-file-noselect file nil t)))
-      (pop-to-buffer buf)
-      (rename-buffer image-dired-display-image-buffer)
+  (let ((buf (find-file-noselect-1
+              (get-buffer-create image-dired-display-image-buffer)
+              file nil t nil nil)))
+    (display-buffer buf t)
+    (with-current-buffer buf
       (if (string-match (image-file-name-regexp) file)
           (image-dired-image-mode)
         ;; Support visiting PDF files.
-        (normal-mode))
-      (select-window cur-win))))
+        (normal-mode)))))
 
 (defun image-dired-display-this (&optional arg)
   "Display current thumbnail's original image in display buffer.
-- 
2.46.0

bug#74246: [PATCH] Reuse display windows in image-dired

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

 > I initially explored this option but was weary to use
 > 'find-file-noselect-1` as it looks like it should be an internal
 > function and is used only in the file it's defined in.

You're right.  Which means that we should try to "promote" and document
that function.  Let's see whether people see any problems with it.

 > However, I do like this solution the most.  Attached is a patch almost
 > identical to the proposed function.  The proposed function would be
 > perfect if it contained the line '(display-buffer buffer t)'.

I'm currently investigating (in the context of Bug#74361) the
possibility to provide an option for reusing a window that already has
shown a related buffer.  'image-dired-display-next' would indirectly
pass that "option" to 'display-buffer' and the latter would try to find
an appropriate window with the help of a window parameter reserved for
that purpose.

Below are ways to implement that "option".  Common to all of them is
that 'display-buffer' sets up a window parameter for the first window
used to display such a buffer and 'display-buffer' would search for a
window with the corresponding value ('image-dired' in this case) for
that parameter.  And the parameter would stay with the window until it
is explicitly reset or the window gets collected.

- Use the existing 'category' alist entry like

   (pop-to-buffer buf '(nil (category . image-dired)))

   This was my original idea.  The downside of this approach is that if
   "category" were used for its already existing purpose (whatever that
   is), the new purpose we'd gave it here would side-effect the behavior
   for the existing purpose.

- Allow for the value of the existing 'some-window' alist entry to be
   specified as a string like

   (pop-to-buffer buf '(nil (some-window . "image-dired")))

   Slightly unnatural, but I see no harm with it.

- Use a new alist entry, say "parameter" like

   (pop-to-buffer buf '(nil (parameter . image-dired)))

   More intuitive maybe.  People would have to learn about it.

- Write a new action function 'display-buffer-use-window-with-parameter'
   and use it in conjunction with the previous as

   (pop-to-buffer
     buf '(display-buffer-use-window-with-parameter (parameter . image-dired)))

   Probably the most universal approach but people have to learn about a
   new action function + alist entry.

Adding Juri to the discussion.

martin

bug#74246: [PATCH] Reuse display windows in image-dired

[Juri Linkov]

> - Use the existing 'category' alist entry like
>
>   (pop-to-buffer buf '(nil (category . image-dired)))
>
>   This was my original idea.  The downside of this approach is that if
>   "category" were used for its already existing purpose (whatever that
>   is), the new purpose we'd gave it here would side-effect the behavior
>   for the existing purpose.

The drawback of reusing 'category' for other purposes is that
all current display-buffer calls that provide a category,
will set the window parameter 'category' unnecessarily.

OTOH, the drawback of using a separate name is that many calls
will have to duplicate the same name as e.g.

  (display-buffer buffer '(nil (category . image-dired)
                               (parameter . image-dired)))

> - Allow for the value of the existing 'some-window' alist entry to be
>   specified as a string like
>
>   (pop-to-buffer buf '(nil (some-window . "image-dired")))
>
>   Slightly unnatural, but I see no harm with it.

This makes such sense that if it has to find some window
then let it be the same some-window from previous calls.

But it has the same problem as above that many calls should
duplicate the name

  (display-buffer buffer '(nil (category . image-dired)
                               (some-window . "image-dired")))

> - Use a new alist entry, say "parameter" like
>
>   (pop-to-buffer buf '(nil (parameter . image-dired)))
>
>   More intuitive maybe.  People would have to learn about it.

An alternative name would be '(group . image-dired)'  Still
the same problem as above:

  (display-buffer buffer '(nil (category . image-dired)
                               (group . image-dired)))

> - Write a new action function 'display-buffer-use-window-with-parameter'
>   and use it in conjunction with the previous as
>
>   (pop-to-buffer
>     buf '(display-buffer-use-window-with-parameter (parameter . image-dired)))
>
>   Probably the most universal approach but people have to learn about a
>   new action function + alist entry.

This is the explicit and easy to understand.  But too limiting.
display-buffer/pop-to-buffer calls should still use the nil action.

So in conclusion it seems better to reuse 'category' in
display-buffer-use-some-window.  But not to set the window parameter
'category' in window--display-buffer unnecessarily.  Instead
this window parameter could be set only in display-buffer-use-some-window
when failing to find a window with a category.  I mean something like this
in display-buffer-use-some-window

  (if (get-window-with-category category 'visible nil not-this-window)
      (use window with category)
    ;; otherwise
    (use lru window by default)
    (set-window-parameter window 'category (cons category parameter)))

bug#74246: [PATCH] Reuse display windows in image-dired

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

 > The drawback of reusing 'category' for other purposes is that
 > all current display-buffer calls that provide a category,
 > will set the window parameter 'category' unnecessarily.

Correct.  And affect the 'lru' behavior.

 > OTOH, the drawback of using a separate name is that many calls
 > will have to duplicate the same name as e.g.
 >
 >    (display-buffer buffer '(nil (category . image-dired)
 >                                 (parameter . image-dired)))

We could make 'parameter' do both such that it has the matching behavior
of 'category' and also adds a parameter.  It's a question of describing
it in a way people understand.

 >> - Allow for the value of the existing 'some-window' alist entry to be
 >>    specified as a string like
 >>
 >>    (pop-to-buffer buf '(nil (some-window . "image-dired")))
 >>
 >>    Slightly unnatural, but I see no harm with it.
 >
 > This makes such sense that if it has to find some window
 > then let it be the same some-window from previous calls.
 >
 > But it has the same problem as above that many calls should
 > duplicate the name
 >
 >    (display-buffer buffer '(nil (category . image-dired)
 >                                 (some-window . "image-dired")))
 >

Right.

 >> - Use a new alist entry, say "parameter" like
 >>
 >>    (pop-to-buffer buf '(nil (parameter . image-dired)))
 >>
 >>    More intuitive maybe.  People would have to learn about it.
 >
 > An alternative name would be '(group . image-dired)'  Still
 > the same problem as above:
 >
 >    (display-buffer buffer '(nil (category . image-dired)
 >                                 (group . image-dired)))

Right.

 >> - Write a new action function 'display-buffer-use-window-with-parameter'
 >>    and use it in conjunction with the previous as
 >>
 >>    (pop-to-buffer
 >>      buf '(display-buffer-use-window-with-parameter (parameter . image-dired)))
 >>
 >>    Probably the most universal approach but people have to learn about a
 >>    new action function + alist entry.
 >
 > This is the explicit and easy to understand.  But too limiting.
 > display-buffer/pop-to-buffer calls should still use the nil action.

The nil action doesn't come without its pitfalls either.  If, as a
caller, I use an explicit action, I at least live in the (possibly
false) hope that my alist entry affects that function and only that
function.  With nil I have to study all action functions in order to
understand whether my alist entry could have any effect and where.

 > So in conclusion it seems better to reuse 'category' in
 > display-buffer-use-some-window.  But not to set the window parameter
 > 'category' in window--display-buffer unnecessarily.  Instead
 > this window parameter could be set only in display-buffer-use-some-window
 > when failing to find a window with a category.  I mean something like this
 > in display-buffer-use-some-window
 >
 >    (if (get-window-with-category category 'visible nil not-this-window)
 >        (use window with category)
 >      ;; otherwise
 >      (use lru window by default)
 >      (set-window-parameter window 'category (cons category parameter)))

And who would set up the 'category' parameter for the first window used
by 'image-dired'?  In my initial proposal, the presence of a 'category'
parameter would mean to _always_ set the parameter for the window used.
And I even would have the value of the parameter made a list like
'(category . (tex-shell comint)) if that window would have been used for
both.

Another question: Would it make sense to have 'image-dired' do

(display-buffer buf '(nil ((some-window . mru))))

in a first phase before embarking on a more complex solution?
Or better

(display-buffer
   buf '(nil ((some-window . mru) (inhibit-same-window . t))))

to make sure the selected window doesn't get used?

martin

bug#74246: [PATCH] Reuse display windows in image-dired

[Juri Linkov]

>> So in conclusion it seems better to reuse 'category' in
>> display-buffer-use-some-window.  But not to set the window parameter
>> 'category' in window--display-buffer unnecessarily.  Instead
>> this window parameter could be set only in display-buffer-use-some-window
>> when failing to find a window with a category.  I mean something like this
>> in display-buffer-use-some-window
>>
>>    (if (get-window-with-category category 'visible nil not-this-window)
>>        (use window with category)
>>      ;; otherwise
>>      (use lru window by default)
>>      (set-window-parameter window 'category (cons category parameter)))
>
> And who would set up the 'category' parameter for the first window used
> by 'image-dired'?  In my initial proposal, the presence of a 'category'
> parameter would mean to _always_ set the parameter for the window used.
> And I even would have the value of the parameter made a list like
> '(category . (tex-shell comint)) if that window would have been used for
> both.

I agree with the need to set the window parameter always.
For example, I often use commands that override the default action
with e.g. windmove-display-up/down, etc.  Therefore needed
to add an advice that remembers the last window in the
buffer-local variable:

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

(define-advice display-buffer-record-window (:after (type window buffer) set-last-window)
  (with-current-buffer (window-buffer)
    ;; TODO: maybe later turn cons into alist ((COMMAND1 . WINDOW1) (COMMAND2 . WINDOW2))
    (setq-local display-buffer-last-window (cons this-command window))))

(setq display-buffer-base-action
      '(nil . ((some-window
                . (lambda (_buffer alist)
                    (let ((last-window (buffer-local-value
                                        'display-buffer-last-window
                                        (window-buffer))))
                      (or (and (eq this-command (car last-window))
                               (window-live-p (cdr last-window))
                               (cdr last-window))
                          (get-mru-window nil nil t))))))))

This uses the buffer-local variable in the original buffer
because IIUC setting a window parameter like in your patch
can create such a situation that more than one window
will have the same window parameter.  But need to keep
only one unique parameter in the last window that displayed
the buffer.

> Another question: Would it make sense to have 'image-dired' do
>
> (display-buffer buf '(nil ((some-window . mru))))
>
> in a first phase before embarking on a more complex solution?

This looks like the best immediate solution.

However, I still like your proposal to use a category instead of lru,
that could be used here later as well.

> Or better
>
> (display-buffer
>   buf '(nil ((some-window . mru) (inhibit-same-window . t))))
>
> to make sure the selected window doesn't get used?

But get-mru-window already prevents selecting the selected window
by the arg NOT-SELECTED of get-mru-window in display-buffer-use-some-window:

  ((eq some-window-method 'mru)
   (get-mru-window nil nil t))
                           =

bug#74246: [PATCH] Reuse display windows in image-dired

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

 > I agree with the need to set the window parameter always.
 > For example, I often use commands that override the default action
 > with e.g. windmove-display-up/down, etc.  Therefore needed
 > to add an advice that remembers the last window in the
 > buffer-local variable:
 >
 > (defvar-local display-buffer-last-window nil)

Let's define it via DEFVAR_PER_BUFFER in buffer.c and have
'window--display-buffer' always set it.
'display-buffer-use-some-window' then could use it either by default or
if it finds a 'some-window' entry whose cdr is nil or t or whatever we
want.

 > This uses the buffer-local variable in the original buffer
 > because IIUC setting a window parameter like in your patch
 > can create such a situation that more than one window
 > will have the same window parameter.

Which is no harm per se.  But it should be possible to exploit it
somehow.

 >> Another question: Would it make sense to have 'image-dired' do
 >>
 >> (display-buffer buf '(nil ((some-window . mru))))
 >>
 >> in a first phase before embarking on a more complex solution?
 >
 > This looks like the best immediate solution.

Morgan, can you try that?

 > However, I still like your proposal to use a category instead of lru,
 > that could be used here later as well.

With 'image-dired' 'display-buffer-last-window' wouldn't help.  But
'image-dired' could define a mode-local variable, say
'image-dired-last-window', and propose that as 'some-window'.  We could
make 'some-window-method' accept a live window for that purpose.

 > But get-mru-window already prevents selecting the selected window
 > by the arg NOT-SELECTED of get-mru-window in display-buffer-use-some-window:
 >
 >    ((eq some-window-method 'mru)
 >     (get-mru-window nil nil t))
 >                             =

I thought so but was too lazy to countercheck.

martin

bug#74246: [PATCH] Reuse display windows in image-dired

[Juri Linkov]

>> I agree with the need to set the window parameter always.
>> For example, I often use commands that override the default action
>> with e.g. windmove-display-up/down, etc.  Therefore needed
>> to add an advice that remembers the last window in the
>> buffer-local variable:
>>
>> (defvar-local display-buffer-last-window nil)
>
> Let's define it via DEFVAR_PER_BUFFER in buffer.c and have
> 'window--display-buffer' always set it.
> 'display-buffer-use-some-window' then could use it either by default or
> if it finds a 'some-window' entry whose cdr is nil or t or whatever we
> want.
>
>> However, I still like your proposal to use a category instead of lru,
>> that could be used here later as well.
>
> With 'image-dired' 'display-buffer-last-window' wouldn't help.  But
> 'image-dired' could define a mode-local variable, say
> 'image-dired-last-window', and propose that as 'some-window'.  We could
> make 'some-window-method' accept a live window for that purpose.

Please explain why 'display-buffer-last-window' wouldn't help
for 'image-dired'?  IIUC, 'image-dired' uses one source buffer
that could use the buffer-local variable to remember the last
window it used to display an image buffer.

bug#74246: [PATCH] Reuse display windows in image-dired

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

 > Please explain why 'display-buffer-last-window' wouldn't help
 > for 'image-dired'?  IIUC, 'image-dired' uses one source buffer
 > that could use the buffer-local variable to remember the last
 > window it used to display an image buffer.

Hmm...  Currently 'image-dired-display-image' does

   (let ((buf (get-buffer image-dired-display-image-buffer))
         (cur-win (selected-window)))
     (when buf
       (kill-buffer buf))
     (when-let ((buf (find-file-noselect file nil t)))
       (pop-to-buffer buf)
       (rename-buffer image-dired-display-image-buffer)

so it kills that buffer and its local variables are gone.  Hence we have
another motivation to use 'find-file-noselect-1' directly (or maybe
something like

(defun find-file-noselect-in-buffer (filename buffer &optional nowarn rawfile truename)
   "Visit file FILENAME in live buffer BUFFER.
Replace the contents of BUFFER with the contents of file FILENAME and
make BUFFER visiting file FILENAME.

The file FILENAME must not be visited by another buffer.  BUFFER must
not have been be modified.  Optional arguments NOWARN and RAWFILE are as
for `find-file-noselect'."
   (setq filename
	(abbreviate-file-name
	 (expand-file-name filename)))
   (let* ((truename (abbreviate-file-name (file-truename filename)))
	 (attributes (file-attributes truename))
	 (number (file-attribute-file-identifier attributes)))
     (cond
      ((find-buffer-visiting filename)
       (error "A buffer is already visting %s" filename))
      ((not (buffer-live-p buffer))
       (error "%s is not a live buffer" buffer))
      ((buffer-modified-p buffer)
       (error "Buffer %s has been modified" buffer)))

     (find-file-noselect-1 buffer filename nowarn rawfile truename number)))

to avoid that people overwrite a modified buffer).

Otherwise, you're right.  The question is now whether

- 'display-buffer-use-some-window' should use the buffer-local value of
   'display-buffer-last-window' autonomously, or

- get it via a (some-window . display-buffer-last-window) alist entry.

And obviously whether 'display-buffer' should set the value of
'display-buffer-last-window' itself or leave that to the caller.

Maybe something like (some-window . t) could be used to incite
'display-buffer-use-some-window' to go for the buffer-local value of
that variable and 'window--display-buffer' to set it.

martin

bug#74246: [PATCH] Reuse display windows in image-dired

[Juri Linkov]

>> Please explain why 'display-buffer-last-window' wouldn't help
>> for 'image-dired'?  IIUC, 'image-dired' uses one source buffer
>> that could use the buffer-local variable to remember the last
>> window it used to display an image buffer.
>
> Hmm...  Currently 'image-dired-display-image' does
>
>   (let ((buf (get-buffer image-dired-display-image-buffer))
>         (cur-win (selected-window)))
>     (when buf
>       (kill-buffer buf))
>     (when-let ((buf (find-file-noselect file nil t)))
>       (pop-to-buffer buf)
>       (rename-buffer image-dired-display-image-buffer)
>
> so it kills that buffer and its local variables are gone.

It kills the target buffer, not the source buffer?
The idea was to set a buffer-local variable in the source buffer.

> Otherwise, you're right.  The question is now whether
>
> - 'display-buffer-use-some-window' should use the buffer-local value of
>   'display-buffer-last-window' autonomously, or
>
> - get it via a (some-window . display-buffer-last-window) alist entry.
>
> And obviously whether 'display-buffer' should set the value of
> 'display-buffer-last-window' itself or leave that to the caller.
>
> Maybe something like (some-window . t) could be used to incite
> 'display-buffer-use-some-window' to go for the buffer-local value of
> that variable and 'window--display-buffer' to set it.

Or some more meaningful value e.g. (some-window . reuse)

bug#74246: [PATCH] Reuse display windows in image-dired

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

 > It kills the target buffer, not the source buffer?
 > The idea was to set a buffer-local variable in the source buffer.

But you want to display the target buffer.  How would 'display-buffer'
find the buffer-local value of the source buffer?

 > Or some more meaningful value e.g. (some-window . reuse)

Yes.

martin

bug#74246: [PATCH] Reuse display windows in image-dired

[Juri Linkov]

>> It kills the target buffer, not the source buffer?
>> The idea was to set a buffer-local variable in the source buffer.
>
> But you want to display the target buffer.  How would 'display-buffer'
> find the buffer-local value of the source buffer?

Here is how I do this in the snippet I posted 3 days ago:

(setq display-buffer-base-action
      '(nil . ((some-window
                . (lambda (_buffer alist)
                    (let ((last-window (buffer-local-value
                                        'display-buffer-last-window
                                        (window-buffer))))

bug#74246: [PATCH] Reuse display windows in image-dired

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

 > Here is how I do this in the snippet I posted 3 days ago:
 >
 > (setq display-buffer-base-action
 >        '(nil . ((some-window
 >                  . (lambda (_buffer alist)
 >                      (let ((last-window (buffer-local-value
 >                                          'display-buffer-last-window
 >                                          (window-buffer))))

This would imply that

- the window showing the buffer from where to read the value of
   'display-buffer-last-window' must be selected at the time
   'display-buffer' is called,

- as a rule, 'display-buffer' cannot set the buffer-local value of
   'display-buffer-last-window' in the window it uses for showing the
   buffer.

Both implications seem harmful to me.

martin

bug#74246: [PATCH] Reuse display windows in image-dired

[Juri Linkov]

>> Here is how I do this in the snippet I posted 3 days ago:
>>
>> (setq display-buffer-base-action
>>        '(nil . ((some-window
>>                  . (lambda (_buffer alist)
>>                      (let ((last-window (buffer-local-value
>>                                          'display-buffer-last-window
>>                                          (window-buffer))))
>
> This would imply that
>
> - the window showing the buffer from where to read the value of
>   'display-buffer-last-window' must be selected at the time
>   'display-buffer' is called,
>
> - as a rule, 'display-buffer' cannot set the buffer-local value of
>   'display-buffer-last-window' in the window it uses for showing the
>   buffer.
>
> Both implications seem harmful to me.

In all customizations I relied on the assumption that the source buffer
was current and its window was selected.  And indeed I see the uses
of '(eq window (selected-window))' in display-buffer functions.

bug#74246: [PATCH] Reuse display windows in image-dired

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

 > In all customizations I relied on the assumption that the source buffer
 > was current and its window was selected.  And indeed I see the uses
 > of '(eq window (selected-window))' in display-buffer functions.

Let's assume a "source buffer" containing a list of objects (usually
links to files possibly enhanced with positions in them) a user might
want to display in a buffer.  These files could be source files
containing definitions or compiler errors, images or simply all files in
a directory.  A user might want to navigate that list to display a
"target buffer" showing the next or previous object with respect to the
current object of that list.  Do we agree on that?

If the function for showing the next of previous object in the list is
`display-buffer', then if 'display-buffer-use-some-window' is called to
find a suitable window and there are several windows on the selected
frame, it would be nice if always the same window were chosen for
displaying the target buffer instead of using the lru window with the
consequence that the next object in the list is always shown in another
window.  Hence some way to remember the last window chosen and use it
again in the next call would be a nice idea.  The proposed way to do
that is by using a buffer-local variable.  Do we agree on that as well?

Then let's store the last window chosen in a buffer-local variable say
'last-window'.  This can be either done in the target buffer as

   DEFVAR_PER_BUFFER ("last-window", &BVAR (current_buffer, last_window),
		     Qnil,
		     doc: /* Last window showing this buffer via `display-buffer'..
This is the last window used by `display-buffer' for showing this buffer.
It is used by `display-buffer-use-some-window' for displaying this
buffer in that window.  */);

or in the source buffer as

   DEFVAR_PER_BUFFER ("last-window", &BVAR (current_buffer, last_window),
		     Qnil,
		     doc: /* Last window showing a buffer via `display-buffer'.
This is the last window used by `display-buffer' for showing a buffer
invoked by a function with this buffer current.  It is used by
`display-buffer-use-some-window' for displaying its buffer argument in
that window.  */);

The first specification has the following two drawbacks.  The caller of
'display-buffer' would have to initialize it in a target buffer that has
not been shown before to the window that has previously shown an element
of the list.  Also, it's somehow redundant since 'window-prev-buffers'
called on all windows could find that window as well if the target
buffer has already been displayed anywhere.

The second specification has the drawback that _any_ 'display-buffer'
call that relies on 'display-buffer-use-some-window' may use that
window.  Just think of an error occurring during that call: The
*backtrace* buffer will pop up in the window specified by that variable
although it is by no means related to it.

Moreover, in the implementations proposed for setting it so far,
'window--display-buffer' would set that variable locally in the buffer
of the window selected before calling 'display-buffer'.  This implies
that the source buffer must appear in the selected window and would
preclude the use of a key binding that works even if the source buffer
is currently not displayed in any window.

Obviously, we could also ask for the caller to pass the window to use in
a 'previous-window' or 'some-window' alist entry and set the window
previously used in some non-local variable pertinent to the caller.  But
then why use a buffer-local variable in the first place?  What am I
missing?

martin

bug#74246: [PATCH] Reuse display windows in image-dired

[Juri Linkov]

>> In all customizations I relied on the assumption that the source buffer
>> was current and its window was selected.  And indeed I see the uses
>> of '(eq window (selected-window))' in display-buffer functions.
>
> Let's assume a "source buffer" containing a list of objects (usually
> links to files possibly enhanced with positions in them) a user might
> want to display in a buffer.  These files could be source files
> containing definitions or compiler errors, images or simply all files in
> a directory.  A user might want to navigate that list to display a
> "target buffer" showing the next or previous object with respect to the
> current object of that list.  Do we agree on that?

Agree.

> If the function for showing the next of previous object in the list is
> `display-buffer', then if 'display-buffer-use-some-window' is called to
> find a suitable window and there are several windows on the selected
> frame, it would be nice if always the same window were chosen for
> displaying the target buffer instead of using the lru window with the
> consequence that the next object in the list is always shown in another
> window.  Hence some way to remember the last window chosen and use it
> again in the next call would be a nice idea.  The proposed way to do
> that is by using a buffer-local variable.  Do we agree on that as well?

Agree.

>   DEFVAR_PER_BUFFER ("last-window", &BVAR (current_buffer, last_window),
> 		     Qnil,
> 		     doc: /* Last window showing a buffer via `display-buffer'.
> This is the last window used by `display-buffer' for showing a buffer
> invoked by a function with this buffer current.  It is used by
> `display-buffer-use-some-window' for displaying its buffer argument in
> that window.  */);
>
> The second specification has the drawback that _any_ 'display-buffer'
> call that relies on 'display-buffer-use-some-window' may use that
> window.  Just think of an error occurring during that call: The
> *backtrace* buffer will pop up in the window specified by that variable
> although it is by no means related to it.

Indeed, this is not good.  So only a category can ensure that
the same display-buffer call is used?

> Moreover, in the implementations proposed for setting it so far,
> 'window--display-buffer' would set that variable locally in the buffer
> of the window selected before calling 'display-buffer'.  This implies
> that the source buffer must appear in the selected window and would
> preclude the use of a key binding that works even if the source buffer
> is currently not displayed in any window.

It's fine to set a buffer-local variable in the buffer where the user
types a key that displays the target buffer from another source buffer.
As long as the same buffer is used to get the value of this variable.

> Obviously, we could also ask for the caller to pass the window to use in
> a 'previous-window' or 'some-window' alist entry and set the window
> previously used in some non-local variable pertinent to the caller.  But
> then why use a buffer-local variable in the first place?  What am I
> missing?

There are two goals:

1. replace the current lru with another default that reuses a previous window.
   But not complicating all exiting display-buffer calls by requiring
   each of them to set a buffer-local variable.  When a standard variable
   will be set, then it can be shared by different calls.

2. make user customization easy by using a special symbol like
   '(some-window . reuse).  But directly using the variable
   is also fine, e.g. `(some-window . ,last-window)

bug#74246: [PATCH] Reuse display windows in image-dired

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

 >> The second specification has the drawback that _any_ 'display-buffer'
 >> call that relies on 'display-buffer-use-some-window' may use that
 >> window.  Just think of an error occurring during that call: The
 >> *backtrace* buffer will pop up in the window specified by that variable
 >> although it is by no means related to it.
 >
 > Indeed, this is not good.  So only a category can ensure that
 > the same display-buffer call is used?

You mean that is uses the same window?  There cannot be such a
guarantee.  That window might have been (de-)selected, (temporarily)
deleted, made dedicated and for any such reason may have become
unsuitable for 'display-buffer'.  In the course of years people have
added more and more conditions why 'display-buffer' should avoid certain
windows.  Think of 'display-buffer-avoid-small-windows' - a global
option that completely sidesteps the internal alist concept.

 > It's fine to set a buffer-local variable in the buffer where the user
 > types a key that displays the target buffer from another source buffer.
 > As long as the same buffer is used to get the value of this variable.

I have no idea of a secure way to retrieve that buffer.

 > There are two goals:
 >
 > 1. replace the current lru with another default that reuses a previous window.
 >     But not complicating all exiting display-buffer calls by requiring
 >     each of them to set a buffer-local variable.  When a standard variable
 >     will be set, then it can be shared by different calls.

Whatever we do: The buffer-local variable would have to be an option to
fix the existing misbehavior of lru.  And it should be used only in
cases where 'display-buffer-reuse-window' can't find a suitable windows.

'image-dired' could safely use 'display-buffer-reuse-window' because it
eventually displays its images always in the same buffer.  It cannot do
that right away in its present version because that one does ...

   (let ((buf (get-buffer image-dired-display-image-buffer))
         (cur-win (selected-window)))
     (when buf
       (kill-buffer buf))
     (when-let ((buf (find-file-noselect file nil t)))
       (pop-to-buffer buf)

... first pop up a buffer showing 'file' which means that
'display-buffer-reuse-window' usually won't find such a window ...

       (rename-buffer image-dired-display-image-buffer)

... and then renames that buffer to 'image-dired-display-image-buffer' -
a buffer that 'display-buffer-reuse-window' would have found if it has
been displayed at least once.  Note that the present lru mischief
doesn't happen for the first image displayed.  It happens for successive
image displays only, where 'image-dired-display-image-buffer' has been
already displayed at least once.

And note the 'kill-buffer' killing 'image-dired-display-image-buffer'.
That one defies any attempt to set a buffer-local variable in that
buffer.

 > 2. make user customization easy by using a special symbol like
 >     '(some-window . reuse).  But directly using the variable
 >     is also fine, e.g. `(some-window . ,last-window)

I would have to understand the semantics of 'reuse' first.  What
'display-buffer' cannot handle currently is something like the
'image-dired' scenario above where the buffer would not be renamed.  In
that case it would be nice if the caller could set a 'category' or a
'some-window' alist entry and 'display-buffer' could use that to find a
window with the same 'category' or 'some-window' value and display the
buffer there.

martin

bug#74246: [PATCH] Reuse display windows in image-dired

[Juri Linkov]

>>> The second specification has the drawback that _any_ 'display-buffer'
>>> call that relies on 'display-buffer-use-some-window' may use that
>>> window.  Just think of an error occurring during that call: The
>>> *backtrace* buffer will pop up in the window specified by that variable
>>> although it is by no means related to it.
>>
>> Indeed, this is not good.  So only a category can ensure that
>> the same display-buffer call is used?
>
> You mean that is uses the same window?

I meant that only the same command (and therefore the same
display-buffer call) should reuse the same window.
This is why I checked for 'this-command' in the posted snippet.
But instead of associating with a command name an alternative approach
would be to use a category.

The *backtrace* buffer is not a problem because it uses own
display-buffer alist that overrides display-buffer-use-some-window.

> There cannot be such a guarantee.
> That window might have been (de-)selected, (temporarily)
> deleted, made dedicated and for any such reason may have become
> unsuitable for 'display-buffer'.  In the course of years people have
> added more and more conditions why 'display-buffer' should avoid certain
> windows.  Think of 'display-buffer-avoid-small-windows' - a global
> option that completely sidesteps the internal alist concept.

display-buffer-use-some-window could recheck if the window is still suitable
for every use of the same command and display-buffer call.

>> It's fine to set a buffer-local variable in the buffer where the user
>> types a key that displays the target buffer from another source buffer.
>> As long as the same buffer is used to get the value of this variable.
>
> I have no idea of a secure way to retrieve that buffer.

It's always the current buffer, even if another buffer is used
as the source buffer.

>> There are two goals:
>>
>> 1. replace the current lru with another default that reuses a previous window.
>>     But not complicating all exiting display-buffer calls by requiring
>>     each of them to set a buffer-local variable.  When a standard variable
>>     will be set, then it can be shared by different calls.
>
> Whatever we do: The buffer-local variable would have to be an option to
> fix the existing misbehavior of lru.  And it should be used only in
> cases where 'display-buffer-reuse-window' can't find a suitable windows.
>
> 'image-dired' could safely use 'display-buffer-reuse-window' because it
> eventually displays its images always in the same buffer.  It cannot do
> that right away in its present version because that one does ...

What if the user wants to display an image buffer in another window?
Then 'display-buffer-reuse-window' can't be used.

>   (let ((buf (get-buffer image-dired-display-image-buffer))
>         (cur-win (selected-window)))
>     (when buf
>       (kill-buffer buf))
>     (when-let ((buf (find-file-noselect file nil t)))
>       (pop-to-buffer buf)
>
> ... first pop up a buffer showing 'file' which means that
> 'display-buffer-reuse-window' usually won't find such a window ...
>
>       (rename-buffer image-dired-display-image-buffer)
>
> ... and then renames that buffer to 'image-dired-display-image-buffer' -
> a buffer that 'display-buffer-reuse-window' would have found if it has
> been displayed at least once.  Note that the present lru mischief
> doesn't happen for the first image displayed.  It happens for successive
> image displays only, where 'image-dired-display-image-buffer' has been
> already displayed at least once.
>
> And note the 'kill-buffer' killing 'image-dired-display-image-buffer'.
> That one defies any attempt to set a buffer-local variable in that
> buffer.

A buffer-local variable should be set in the source buffer only,
not in the image buffer.

>> 2. make user customization easy by using a special symbol like
>>     '(some-window . reuse).  But directly using the variable
>>     is also fine, e.g. `(some-window . ,last-window)
>
> I would have to understand the semantics of 'reuse' first.  What
> 'display-buffer' cannot handle currently is something like the
> 'image-dired' scenario above where the buffer would not be renamed.  In
> that case it would be nice if the caller could set a 'category' or a
> 'some-window' alist entry and 'display-buffer' could use that to find a
> window with the same 'category' or 'some-window' value and display the
> buffer there.

I have absolutely no problems with existing code
in 'image-dired-display-image' because of using:

(setq display-buffer-base-action
      '(nil . ((some-window
                . (lambda (_buffer alist)
                    (let ((last-window (buffer-local-value
                                        'display-buffer-last-window
                                        (window-buffer))))
                      (or (and (eq this-command (car last-window))
                               (window-live-p (cdr last-window))
                               (cdr last-window))
                          (get-mru-window nil nil t))))))))

bug#74246: [PATCH] Reuse display windows in image-dired

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

 > I meant that only the same command (and therefore the same
 > display-buffer call) should reuse the same window.
 > This is why I checked for 'this-command' in the posted snippet.
 > But instead of associating with a command name an alternative approach
 > would be to use a category.

One and the same command may issue an arbitrary number of
'display-buffer' calls.  Checking for 'this-command' may work for you in
a specific context but is certainly not a safe approach for general use.

 > The *backtrace* buffer is not a problem because it uses own
 > display-buffer alist that overrides display-buffer-use-some-window.

Unless a user has customized it or 'display-buffer-below-selected' fails
for some reason.

 > display-buffer-use-some-window could recheck if the window is still suitable
 > for every use of the same command and display-buffer call.

As I said above this is not reliable.  The only reliable thing is to
pass the symbol of the function calling 'display-buffer' with some
unique number identifying the nth call of 'display-buffer' within that
function.  Everything else is guesswork.

 >>> It's fine to set a buffer-local variable in the buffer where the user
 >>> types a key that displays the target buffer from another source buffer.
 >>> As long as the same buffer is used to get the value of this variable.
 >>
 >> I have no idea of a secure way to retrieve that buffer.
 >
 > It's always the current buffer, even if another buffer is used
 > as the source buffer.

But the current buffer may vary continuously, it might even be the
minibuffer if I issue the call from there.

 > What if the user wants to display an image buffer in another window?
 > Then 'display-buffer-reuse-window' can't be used.

If a user issues the command to display an image in a window that
already shows an image and insists on using another window, an arbitrary
other window can be chosen.  Users who want that just get the usual
chaotic behavior lru provides.  They asked for it.

 > A buffer-local variable should be set in the source buffer only,
 > not in the image buffer.

With 'image-dired' it can be set in the image buffer because that buffer
is always the same.

 > I have absolutely no problems with existing code
 > in 'image-dired-display-image' because of using:
 >
 > (setq display-buffer-base-action
 >        '(nil . ((some-window
 >                  . (lambda (_buffer alist)
 >                      (let ((last-window (buffer-local-value
 >                                          'display-buffer-last-window
 >                                          (window-buffer))))
 >                        (or (and (eq this-command (car last-window))
 >                                 (window-live-p (cdr last-window))
 >                                 (cdr last-window))
 >                            (get-mru-window nil nil t))))))))

Imagine users who once they displayed an image want it to fill their
frame entirely and want to use a key combination to display the next or
previous image there.  Web pages usually display a < on the left and a >
on the right to achieve that behavior.  Image viewers put similar icons
in a toolbar for that.  There is no visible source buffer or current
buffer around for that in Emacs.  All you have is the target buffer.

martin

bug#74246: [PATCH] Reuse display windows in image-dired

[Juri Linkov]

>> I meant that only the same command (and therefore the same
>> display-buffer call) should reuse the same window.
>> This is why I checked for 'this-command' in the posted snippet.
>> But instead of associating with a command name an alternative approach
>> would be to use a category.
>
> One and the same command may issue an arbitrary number of
> 'display-buffer' calls.  Checking for 'this-command' may work for you in
> a specific context but is certainly not a safe approach for general use.

What would be the safest approach to detect the same 'display-buffer' call?
A category?

>> The *backtrace* buffer is not a problem because it uses own
>> display-buffer alist that overrides display-buffer-use-some-window.
>
> Unless a user has customized it or 'display-buffer-below-selected' fails
> for some reason.

Then displaying it by some-window in the same window instead of lru
looks as a nice thing to do.

>> display-buffer-use-some-window could recheck if the window is still suitable
>> for every use of the same command and display-buffer call.
>
> As I said above this is not reliable.  The only reliable thing is to
> pass the symbol of the function calling 'display-buffer' with some
> unique number identifying the nth call of 'display-buffer' within that
> function.  Everything else is guesswork.

There is already such a symbol: 'category'.

>>>> It's fine to set a buffer-local variable in the buffer where the user
>>>> types a key that displays the target buffer from another source buffer.
>>>> As long as the same buffer is used to get the value of this variable.
>>>
>>> I have no idea of a secure way to retrieve that buffer.
>>
>> It's always the current buffer, even if another buffer is used
>> as the source buffer.
>
> But the current buffer may vary continuously, it might even be the
> minibuffer if I issue the call from there.
>
>> What if the user wants to display an image buffer in another window?
>> Then 'display-buffer-reuse-window' can't be used.
>
> If a user issues the command to display an image in a window that
> already shows an image and insists on using another window, an arbitrary
> other window can be chosen.  Users who want that just get the usual
> chaotic behavior lru provides.  They asked for it.

The users might want to switch displaying to another window,
and continue displaying other images in the same other window.

>> A buffer-local variable should be set in the source buffer only,
>> not in the image buffer.
>
> With 'image-dired' it can be set in the image buffer because that buffer
> is always the same.

This is an exception, not a general rule such as for navigating
grep/xref results.  I see no reason for image-dired be different
from grep/xref.

bug#74246: [PATCH] Reuse display windows in image-dired

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

 > What would be the safest approach to detect the same 'display-buffer' call?
 > A category?

As I already mentioned: The calling function would have to reserve a
separate alist entry for it.  In my initial proposal I had even a
separate argument for that purpose.  Alternatively, one could reserve a
local variable in the target buffer for that purpose ('display-buffer'
would have to reset it).

 >> Unless a user has customized it or 'display-buffer-below-selected' fails
 >> for some reason.
 >
 > Then displaying it by some-window in the same window instead of lru
 > looks as a nice thing to do.

Would you like that?  I think displaying *backtrace* in the same window
is always a bad idea.

 >> As I said above this is not reliable.  The only reliable thing is to
 >> pass the symbol of the function calling 'display-buffer' with some
 >> unique number identifying the nth call of 'display-buffer' within that
 >> function.  Everything else is guesswork.
 >
 > There is already such a symbol: 'category'.

But this one is already handled by 'buffer-match-p'.  We can't set it
willy-nilly to some arbitrary value.  Otherwise, that function might
match it in an unexpected way.

 >> If a user issues the command to display an image in a window that
 >> already shows an image and insists on using another window, an arbitrary
 >> other window can be chosen.  Users who want that just get the usual
 >> chaotic behavior lru provides.  They asked for it.
 >
 > The users might want to switch displaying to another window,
 > and continue displaying other images in the same other window.

Yes.  But then either of the windows could be chosen by the next call
(if that window still exists).

 >> With 'image-dired' it can be set in the image buffer because that buffer
 >> is always the same.
 >
 > This is an exception, not a general rule such as for navigating
 > grep/xref results.  I see no reason for image-dired be different
 > from grep/xref.

'image-dired' _is_ different because it always uses the same buffer for
showing images stored in different files.  I know of no other function
doing that.  Do you?

martin

bug#74246: [PATCH] Reuse display windows in image-dired

[Juri Linkov]

>> What would be the safest approach to detect the same 'display-buffer' call?
>> A category?
>
> As I already mentioned: The calling function would have to reserve a
> separate alist entry for it.  In my initial proposal I had even a
> separate argument for that purpose.  Alternatively, one could reserve a
> local variable in the target buffer for that purpose ('display-buffer'
> would have to reset it).

I still don't understand how a local variable in one target buffer could
help to display another buffer in the same window from grep/xref list.

>>> Unless a user has customized it or 'display-buffer-below-selected' fails
>>> for some reason.
>>
>> Then displaying it by some-window in the same window instead of lru
>> looks as a nice thing to do.
>
> Would you like that?  I think displaying *backtrace* in the same window
> is always a bad idea.

Only when 'display-buffer-below-selected' fails that is extremely rare.

>>> As I said above this is not reliable.  The only reliable thing is to
>>> pass the symbol of the function calling 'display-buffer' with some
>>> unique number identifying the nth call of 'display-buffer' within that
>>> function.  Everything else is guesswork.
>>
>> There is already such a symbol: 'category'.
>
> But this one is already handled by 'buffer-match-p'.  We can't set it
> willy-nilly to some arbitrary value.  Otherwise, that function might
> match it in an unexpected way.

'display-buffer-reuse-category-window' could reuse the 'category' symbol.
Or '(some-window . reuse-category)'.

>>> If a user issues the command to display an image in a window that
>>> already shows an image and insists on using another window, an arbitrary
>>> other window can be chosen.  Users who want that just get the usual
>>> chaotic behavior lru provides.  They asked for it.
>>
>> The users might want to switch displaying to another window,
>> and continue displaying other images in the same other window.
>
> Yes.  But then either of the windows could be chosen by the next call
> (if that window still exists).

Not either, but preferably the last used window.

>>> With 'image-dired' it can be set in the image buffer because that buffer
>>> is always the same.
>>
>> This is an exception, not a general rule such as for navigating
>> grep/xref results.  I see no reason for image-dired be different
>> from grep/xref.
>
> 'image-dired' _is_ different because it always uses the same buffer for
> showing images stored in different files.  I know of no other function
> doing that.  Do you?

I don't remember any other function doing such non-standard things.

bug#74246: [PATCH] Reuse display windows in image-dired

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

 > I still don't understand how a local variable in one target buffer could
 > help to display another buffer in the same window from grep/xref list.

By having grep do the following:

- Issue a first request to display a buffer from that list,

- remember the window used,

- set the local variable in the target buffer to the remembered value
   before calling 'display-buffer' again.

It could do that if it wanted to communicate the window to use in a
buffer local variable thus avoiding to pass an extra value via the
alist.

 >> Would you like that?  I think displaying *backtrace* in the same window
 >> is always a bad idea.
 >
 > Only when 'display-buffer-below-selected' fails that is extremely rare.

I assumed that the user had customized it.

 >>>> pass the symbol of the function calling 'display-buffer' with some
 >>>> unique number identifying the nth call of 'display-buffer' within that
 >>>> function.  Everything else is guesswork.
 >>>
 >>> There is already such a symbol: 'category'.

'category' is much broader.

 >> But this one is already handled by 'buffer-match-p'.  We can't set it
 >> willy-nilly to some arbitrary value.  Otherwise, that function might
 >> match it in an unexpected way.
 >
 > 'display-buffer-reuse-category-window' could reuse the 'category' symbol.
 > Or '(some-window . reuse-category)'.

Let me turn the table and ask you: Both grep/xref know very well which
window was used for displaying the last match.  What speaks again to
have them just remember that window after each call and propose it via a
(some-window . ,window) alist entry in the next call together with
'display-buffer-use-some-window'?  I think the main advantage of such an
approach is that grep/xref would be in complete control of a good
proposal for such a window, something most users could hardly resist.

 > Not either, but preferably the last used window.

Not preferably, IMHO.

 >> 'image-dired' _is_ different because it always uses the same buffer for
 >> showing images stored in different files.  I know of no other function
 >> doing that.  Do you?
 >
 > I don't remember any other function doing such non-standard things.

So IMHO we should exploit that fact in its 'display-buffer' call.

martin

bug#74246: [PATCH] Reuse display windows in image-dired

[Juri Linkov]

>> I still don't understand how a local variable in one target buffer could
>> help to display another buffer in the same window from grep/xref list.
>
> By having grep do the following:
>
> - Issue a first request to display a buffer from that list,
>
> - remember the window used,
>
> - set the local variable in the target buffer to the remembered value
>   before calling 'display-buffer' again.

What if the user switches buffers in that window, but still wants
to continue visiting next buffers from grep in the same window?

>>>>> pass the symbol of the function calling 'display-buffer' with some
>>>>> unique number identifying the nth call of 'display-buffer' within that
>>>>> function.  Everything else is guesswork.
>>>>
>>>> There is already such a symbol: 'category'.
>
> 'category' is much broader.
>
>>> But this one is already handled by 'buffer-match-p'.  We can't set it
>>> willy-nilly to some arbitrary value.  Otherwise, that function might
>>> match it in an unexpected way.
>>
>> 'display-buffer-reuse-category-window' could reuse the 'category' symbol.
>> Or '(some-window . reuse-category)'.
>
> Let me turn the table and ask you: Both grep/xref know very well which
> window was used for displaying the last match.  What speaks again to
> have them just remember that window after each call

grep/xref should remember that window in a buffer-local variable?

> and propose it via a (some-window . ,window) alist entry in the next
> call together with 'display-buffer-use-some-window'?

Such providing that window in the display-buffer call would be very nice
to do, e.g. with a more descriptive name like (previous-window . ,window).
This will make easier for users to customize by using different actions
that handle such alist entry: 'display-buffer-in-previous-window' or
'display-buffer-use-some-window'.

Then a category will be continued to be used only for matching,
e.g. the call

  (display-buffer buffer `((nil (category . grep) (previous-window . ,window))))

could be customized to match a category and to use the previous window:

  ((category . grep)
   (display-buffer-in-previous-window))

> I think the main advantage of such an
> approach is that grep/xref would be in complete control of a good
> proposal for such a window, something most users could hardly resist.

Agreed.

bug#74246: [PATCH] Reuse display windows in image-dired

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

 >> By having grep do the following:
 >>
 >> - Issue a first request to display a buffer from that list,
 >>
 >> - remember the window used,
 >>
 >> - set the local variable in the target buffer to the remembered value
 >>    before calling 'display-buffer' again.
 >
 > What if the user switches buffers in that window, but still wants
 > to continue visiting next buffers from grep in the same window?

It would have been the buffer-local value in the last target buffer
displayed.  But it's inconvenient and obviously fails if that target
buffer has been deleted in the meantime.  So it's not a good solution
either.  It's only advantage would have been that such a local variable
could have been shared between applications - grep and xref say - to
make sure one and the same buffer would always go to the same window.

 > grep/xref should remember that window in a buffer-local variable?

As I'd envision it, xref would use a conceptually global variable called
'xref-target-window' or 'xref-last-window-used' and grep would do
something similar.

 >> and propose it via a (some-window . ,window) alist entry in the next
 >> call together with 'display-buffer-use-some-window'?
 >
 > Such providing that window in the display-buffer call would be very nice
 > to do, e.g. with a more descriptive name like (previous-window . ,window).
 > This will make easier for users to customize by using different actions
 > that handle such alist entry: 'display-buffer-in-previous-window' or
 > 'display-buffer-use-some-window'.

No objections; just that the semantics of "previous-window" was so far a
"window that was showing that buffer" while here we would mean a "window
that was showing a buffer from the same caller".

 > Then a category will be continued to be used only for matching,
 > e.g. the call
 >
 >    (display-buffer buffer `((nil (category . grep) (previous-window . ,window))))
 >
 > could be customized to match a category and to use the previous window:
 >
 >    ((category . grep)
 >     (display-buffer-in-previous-window))

Wouldn't that be overkill?  The user's customization would have no
effect IIUC.  I'd say

     (display-buffer buffer `((nil (category . grep) (some-window . ,window))))

would be grep's proposal and a user's

     ((category . grep)
      (display-buffer-in-previous-window))

would try to find a window that previously displayed the buffer first.

martin

bug#74246: [PATCH] Reuse display windows in image-dired

[Juri Linkov]

>> grep/xref should remember that window in a buffer-local variable?
>
> As I'd envision it, xref would use a conceptually global variable called
> 'xref-target-window' or 'xref-last-window-used' and grep would do
> something similar.

Please not a global variable.  I often use many pairs of
source/target windows at the same time for different grep/xref results.

>>    (display-buffer buffer `((nil (category . grep) (previous-window . ,window))))
>>
>> could be customized to match a category and to use the previous window:
>>
>>    ((category . grep)
>>     (display-buffer-in-previous-window))
>
> Wouldn't that be overkill?  The user's customization would have no
> effect IIUC.  I'd say
>
>     (display-buffer buffer `((nil (category . grep) (some-window . ,window))))
>
> would be grep's proposal and a user's
>
>     ((category . grep)
>      (display-buffer-in-previous-window))
>
> would try to find a window that previously displayed the buffer first.

Still too limited solution when the users can't store the last window
for other commands.  Better would be to add a new alist entry that
will force 'display-buffer' to remember the last window from any call:

  (display-buffer buffer `((nil (category . compilation)
                                (remember-last-window . t))))

or by user customization of display-buffer-alist:

  ((category . compilation)
   (nil (remember-last-window . t)))

And a function that stores/restores the last window could be customizable as well.
So the users could decide whether to save it in a buffer-local variable,
or in the window parameter.  This option could be like 'xref-history-storage'
that provides the choice of 'xref-global-history' or 'xref-window-local-history':

  ((category . compilation)
   (nil (remember-last-window . global)))

or

  ((category . compilation)
   (nil (remember-last-window . window-local)))

or

  ((category . compilation)
   (nil (remember-last-window . buffer-local)))

bug#74246: [PATCH] Reuse display windows in image-dired

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

 > Please not a global variable.  I often use many pairs of
 > source/target windows at the same time for different grep/xref
 > results.

I do that too.  Just that my "source window" is a sidebar of the target
window.

 > Still too limited solution when the users can't store the last window
 > for other commands.  Better would be to add a new alist entry that
 > will force 'display-buffer' to remember the last window from any call:
 >
 >    (display-buffer buffer `((nil (category . compilation)
 >                                  (remember-last-window . t))))
 >
 > or by user customization of display-buffer-alist:
 >
 >    ((category . compilation)
 >     (nil (remember-last-window . t)))

In that case the 'last-window' parameter maintained by 'display-buffer'
would have to become a list of categories.  Then what about two grep
operations running in parallel as you mention above: Would they share
the same target window?

 > And a function that stores/restores the last window could be customizable as well.
 > So the users could decide whether to save it in a buffer-local variable,

... buffer-local where - in the source or the target buffer?

 > or in the window parameter.  This option could be like 'xref-history-storage'
 > that provides the choice of 'xref-global-history' or 'xref-window-local-history':
 >
 >    ((category . compilation)
 >     (nil (remember-last-window . global)))
 >
 > or
 >
 >    ((category . compilation)
 >     (nil (remember-last-window . window-local)))
 >
 > or
 >
 >    ((category . compilation)
 >     (nil (remember-last-window . buffer-local)))

Just think of describing this in the manual.

But I already see: We'll have to maintain a buffer-local variable, say
'xref-last-window' in the source buffer, have xref consult that variable
and pass it as (last-window . ,xref-last-window) to 'display-buffer'.
The value returned by 'display-buffer' would then become the new value
of 'xref-last-window' in the source buffer.  Would that be OK with you?

martin

bug#74246: [PATCH] Reuse display windows in image-dired

[Juri Linkov]

>> Still too limited solution when the users can't store the last window
>> for other commands.  Better would be to add a new alist entry that
>> will force 'display-buffer' to remember the last window from any call:
>>
>>    (display-buffer buffer `((nil (category . compilation)
>>                                  (remember-last-window . t))))
>>
>> or by user customization of display-buffer-alist:
>>
>>    ((category . compilation)
>>     (nil (remember-last-window . t)))
>
> In that case the 'last-window' parameter maintained by 'display-buffer'
> would have to become a list of categories.  Then what about two grep
> operations running in parallel as you mention above: Would they share
> the same target window?

The only different things two grep operations don't share are their
source buffers.  Therefore need to save the last window in the buffer-local
variable in each grep buffer.  They can share the same target window
if the user wants.

>> And a function that stores/restores the last window could be customizable as well.
>> So the users could decide whether to save it in a buffer-local variable,
>
> ... buffer-local where - in the source or the target buffer?

In the source buffer.  But it should be possible to redefine the function
to use the target buffer, e.g.:

  (remember-last-window .
    (lambda (source-buffer target-buffer source-window target-window)
      (with-current-buffer target-buffer
        (setq-local last-window target-window))))

or

  (remember-last-window .
    (lambda (source-buffer target-buffer source-window target-window)
      (set-window-parameter source-window 'last-window target-window)))

>> or in the window parameter.  This option could be like 'xref-history-storage'
>> that provides the choice of 'xref-global-history' or 'xref-window-local-history':
>>
>>    ((category . compilation)
>>     (nil (remember-last-window . global)))
>>
>> or
>>
>>    ((category . compilation)
>>     (nil (remember-last-window . window-local)))
>>
>> or
>>
>>    ((category . compilation)
>>     (nil (remember-last-window . buffer-local)))
>
> Just think of describing this in the manual.

No problem.

> But I already see: We'll have to maintain a buffer-local variable, say
> 'xref-last-window' in the source buffer, have xref consult that variable
> and pass it as (last-window . ,xref-last-window) to 'display-buffer'.
> The value returned by 'display-buffer' would then become the new value
> of 'xref-last-window' in the source buffer.  Would that be OK with you?

This should not be specific to packages.  It should be general to be usable
for any display-buffer call.

bug#74246: [PATCH] Reuse display windows in image-dired

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

 > This should not be specific to packages.  It should be general to be usable
 > for any display-buffer call.

Then for a *Compile-Log* buffer 'compile-goto-error' and 'next-error'
would both try to use the same last window something they here currently
don't.  And if clicking on a button in a *Help* buffer invokes
'display-buffer', it might use the same window for unrelated purposes.

martin

bug#74246: [PATCH] Reuse display windows in image-dired

[Juri Linkov]

>> This should not be specific to packages.  It should be general to be usable
>> for any display-buffer call.
>
> Then for a *Compile-Log* buffer 'compile-goto-error' and 'next-error'
> would both try to use the same last window something they here currently
> don't.  And if clicking on a button in a *Help* buffer invokes
> 'display-buffer', it might use the same window for unrelated purposes.

Only if it was configured to use the same window.

Also the same window could be used by display-buffer-use-some-window
instead of lru.