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