bug#36680: 27.0.50; undo-tree visualizer flickering with display-buffer-reuse-frames -> t

bug#36680: 27.0.50; undo-tree visualizer flickering with display-buffer-reuse-frames -> t

[Michael Heerdegen]

Hi,

recipe from emacs -Q, after getting a source of undo-tree:

Copy to scratch and evaluate

(setq display-buffer-reuse-frames t)
(require 'undo-tree)
(undo-tree-mode)

Make some further edits to scratch for testing.  Then

C-x 5 2
C-x u
[up]...

Any time you switch the current node in the visualizer, the first frame
is raised for a very short time, then lowered again.  This is quite
annoying, it makes C-x u quite useless when you have a window in another
frame displaying the current buffer because it distracts me from seeing
what changes in the buffer, I'm unable to see it.  Setting
display-buffer-reuse-frames -> nil fixes this.

The visualizer calls

  (switch-to-buffer-other-window undo-tree-visualizer-buffer-name)

which doesn't seem to behave well in this situation.

TIA,

Michael.


In GNU Emacs 27.0.50 (build 9, x86_64-pc-linux-gnu, GTK+ Version 3.24.5)
 of 2019-07-15 built on drachen
Repository revision: fb725fc0fa320e94daf8e4aa1a3320ba60142449
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux bullseye/sid

bug#36680: 27.0.50; undo-tree visualizer flickering with display-buffer-reuse-frames -> t

[martin rudalics]

 > (setq display-buffer-reuse-frames t)
 > (require 'undo-tree)
 > (undo-tree-mode)
 >
 > Make some further edits to scratch for testing.  Then
 >
 > C-x 5 2
 > C-x u
 > [up]...
 >
 > Any time you switch the current node in the visualizer, the first frame
 > is raised for a very short time, then lowered again.  This is quite
 > annoying, it makes C-x u quite useless when you have a window in another
 > frame displaying the current buffer because it distracts me from seeing
 > what changes in the buffer, I'm unable to see it.  Setting
 > display-buffer-reuse-frames -> nil fixes this.
 >
 > The visualizer calls
 >
 >    (switch-to-buffer-other-window undo-tree-visualizer-buffer-name)
 >
 > which doesn't seem to behave well in this situation.

I don't know why 'undo-tree' apparently insists on switching to its
buffer here but if you put it on another frame you have to live with
the fact that that frame will be raised.  I think you should make a
special rule for 'display-buffer' to not display the undo tree on a
separate frame.  Note also that 'display-buffer-reuse-frames' is an
obsolete variable.

martin

bug#36680: 27.0.50; undo-tree visualizer flickering with display-buffer-reuse-frames -> t

[Michael Heerdegen]

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

martin rudalics <rudalics@gmx.at> writes:

> I don't know why 'undo-tree' apparently insists on switching to its
> buffer here but if you put it on another frame you have to live with
> the fact that that frame will be raised.

I think you didn't understand what I tried to describe.  I attach a
video demonstration (hope that works).

I don't put the visualizer to another frame, that's not the case and not
the problem.

It's the visualizer that looks if the buffer is displayed not only in
the current frame, but also in another frame, and raises this other
frame when there is one.

> I think you should make a special rule for 'display-buffer' to not
> display the undo tree on a separate frame.

As said, this is not the problem.

> Note also that 'display-buffer-reuse-frames' is an obsolete variable.

I think also other settings could lead to this effect.


Thanks,

Michael.


[-- Attachment #2: Emacs Bug#36680.webm --]
[-- Type: video/webm, Size: 225340 bytes --]

bug#36680: 27.0.50; undo-tree visualizer flickering with display-buffer-reuse-frames -> t

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> As said, this is not the problem.

BTW, the call tree looks like

undo-tree-visualize > switch-to-buffer-other-window
                    > pop-to-buffer with t ACTION
                    > display-buffer with t ACTION
                    > display-buffer-reuse-window

and the last blindly uses the first window in

  (get-buffer-window-list buffer 'nomini frames)

which can be a window of a different frame even if there is a suitable
window in the selected frame.  That's not good IMHO.

Michael.

bug#36680: 27.0.50; undo-tree visualizer flickering with display-buffer-reuse-frames -> t

[martin rudalics]

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

 >> As said, this is not the problem.

You probably should have used an 'inhibit-switch-frame' ALIST entry
then.

 > BTW, the call tree looks like
 >
 > undo-tree-visualize > switch-to-buffer-other-window
 >                      > pop-to-buffer with t ACTION
 >                      > display-buffer with t ACTION
 >                      > display-buffer-reuse-window
 >
 > and the last blindly uses the first window in
 >
 >    (get-buffer-window-list buffer 'nomini frames)
 >
 > which can be a window of a different frame even if there is a suitable
 > window in the selected frame.  That's not good IMHO.

OK.  Then please, try the attached patch.  Note that when there's no
suitable window on the selected frame you will still get the annoying
behavior.

Thanks, martin

[-- Attachment #2: display-buffer-reuse-window.diff --]
[-- Type: text/plain, Size: 3453 bytes --]

diff --git a/doc/lispref/windows.texi b/doc/lispref/windows.texi
index b5ca053e46..1035739e2b 100644
--- a/doc/lispref/windows.texi
+++ b/doc/lispref/windows.texi
@@ -2536,7 +2536,8 @@ Buffer Display Action Functions
 
 @defun display-buffer-reuse-window buffer alist
 This function tries to display @var{buffer} by finding a window that
-is already displaying it.
+is already displaying it.  Windows on the selected frame are preferred
+to windows on other frames.
 
 If @var{alist} has a non-@code{nil} @code{inhibit-same-window} entry,
 the selected window is not eligible for reuse.  The set of frames to
diff --git a/lisp/window.el b/lisp/window.el
index 726d022dfe..8cb9670ae4 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -7332,25 +7332,26 @@ display-buffer--maybe-same-window
 
 (defun display-buffer-reuse-window (buffer alist)
   "Return a window that is already displaying BUFFER.
-Return nil if no usable window is found.
+Preferably use a window on the selected frame if such a window
+exists.  Return nil if no usable window is found.
 
-If ALIST has a non-nil `inhibit-same-window' entry, the selected
+If ALIST has a non-nil 'inhibit-same-window' entry, the selected
 window is not eligible for reuse.
 
-If ALIST contains a `reusable-frames' entry, its value determines
+If ALIST contains a 'reusable-frames' entry, its value determines
 which frames to search for a reusable window:
   nil -- the selected frame (actually the last non-minibuffer frame)
   A frame   -- just that frame
-  `visible' -- all visible frames
+  'visible' -- all visible frames
   0   -- all frames on the current terminal
   t   -- all frames.
 
-If ALIST contains no `reusable-frames' entry, search just the
+If ALIST contains no 'reusable-frames' entry, search just the
 selected frame if `display-buffer-reuse-frames' and
 `pop-up-frames' are both nil; search all frames on the current
 terminal if either of those variables is non-nil.
 
-If ALIST has a non-nil `inhibit-switch-frame' entry, then in the
+If ALIST has a non-nil 'inhibit-switch-frame' entry, then in the
 event that a window on another frame is chosen, avoid raising
 that frame."
   (let* ((alist-entry (assq 'reusable-frames alist))
@@ -7364,9 +7365,21 @@ display-buffer-reuse-window
 	 (window (if (and (eq buffer (window-buffer))
 			  (not (cdr (assq 'inhibit-same-window alist))))
 		     (selected-window)
-		   (car (delq (selected-window)
-			      (get-buffer-window-list buffer 'nomini
-						      frames))))))
+                   ;; Preferably use a window on the selected frame,
+                   ;; if such a window exists (Bug#36680).
+                   (let* ((windows (delq (selected-window)
+                                         (get-buffer-window-list
+                                          buffer 'nomini frames)))
+                          (first (car windows))
+                          (this-frame (selected-frame)))
+                     (cond
+                      ((eq (window-frame first) this-frame)
+                       first)
+                      ((catch 'found
+                         (dolist (next (cdr windows))
+                           (when (eq (window-frame next) this-frame)
+                             (throw 'found next)))))
+                      (t first))))))
     (when (window-live-p window)
       (prog1 (window--display-buffer buffer window 'reuse alist)
 	(unless (cdr (assq 'inhibit-switch-frame alist))

bug#36680: 27.0.50; undo-tree visualizer flickering with display-buffer-reuse-frames -> t

[Michael Heerdegen]

martin rudalics <rudalics@gmx.at> writes:

> You probably should have used an 'inhibit-switch-frame' ALIST entry
> then.

Where would you suggest to do that, as a user, without changing code?
Potentially any arbitrary buffer can be affected.

> OK.  Then please, try the attached patch.

Yes, good, that's what I want - thanks.  I hope no existing code relies
on the old behavior.  But I think it's an improvement, also in other
situations.

> Note that when there's no suitable window on the selected frame you
> will still get the annoying behavior.

In this case it's not trivial what could be done better.  Let's say that
this is not part of my report.


Thanks,

Michael.

bug#36680: 27.0.50; undo-tree visualizer flickering with display-buffer-reuse-frames -> t

[martin rudalics]

 >> You probably should have used an 'inhibit-switch-frame' ALIST entry
 >> then.
 >
 > Where would you suggest to do that, as a user, without changing code?
 > Potentially any arbitrary buffer can be affected.

It's not trivial, unfortunately.  As a user, one has to identify the
calling undo-tree function.

 >> OK.  Then please, try the attached patch.
 >
 > Yes, good, that's what I want - thanks.

I pushed the change now.

 > I hope no existing code relies
 > on the old behavior.  But I think it's an improvement, also in other
 > situations.

Strictly speaking, the change should be in 'get-buffer-window-list' to
possibly pick a window before the selected one on the selected frame.
But I can't really change the semantics of that.

 >> Note that when there's no suitable window on the selected frame you
 >> will still get the annoying behavior.
 >
 > In this case it's not trivial what could be done better.  Let's say that
 > this is not part of my report.

OK.  If you see no further problems, please close this bug.

Thanks, martin

bug#36680: 27.0.50; undo-tree visualizer flickering with display-buffer-reuse-frames -> t

[Michael Heerdegen]

martin rudalics <rudalics@gmx.at> writes:

> OK.  If you see no further problems, please close this bug.

I'm happy.  Thanks for working on this.

Michael.