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.