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