bug#24240: 25.1.50; window-state-put, image-mode and window scrolling

bug#24240: 25.1.50; window-state-put, image-mode and window scrolling

[Andreas Politz]

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


There is a conflict between window-state-put and image-mode and
maybe other modes.  

The function image-mode-reapply-winprops gets called as soon as
window-state-put displays the image buffer in some window via
window-configuration-change-hook and applies the image's
previously stored scroll-values.

But window-state-put is not done yet and it has it's own
understanding of how the window should be scrolled, thereby
overriding images-mode's scrolling.

I guess ideally window-state-put should be atomic in the sense
that it delays these kinds of hooks until the window is
completely restored.  Below is a recipe and I attached a
workaround.

-ap

--

Let's start with

emacs -Q

and find some image file:

C-x C-f foo.png RET

Maybe enlarge the image, such that it can be scrolled.  Move the
image (C-n), such that vscroll is greater 0.

(window-vscroll) => 37

Save the window state,

(setq wst (window-state-get))

and switch to the *scratch* buffer.

(current-buffer) => #<buffer *scratch*>

Now restore the previously saved window state

(window-state-put wst)

,which should bring back the image buffer.  But notice, how
vscroll seems to be 0, i.e. display starts at the top of image.
Finally evaluate any expression in the mini-buffer

M-: t RET

which triggers image-mode-reapply-winprops scrolling the image as
expected.



[-- Attachment #2: window-state-put-workaround.el --]
[-- Type: application/emacs-lisp, Size: 505 bytes --]

bug#24240: 25.1.50; window-state-put, image-mode and window scrolling

[martin rudalics]

 > I guess ideally window-state-put should be atomic in the sense
 > that it delays these kinds of hooks until the window is
 > completely restored.

We'd need a variable, say ‘window-inhibit-configuration-change-hook’.

 > ;; Workaround 1 (does not work, why ?)
 >
 > (defun window-state-put-workaround (fn &rest args)
 >   (let (window-configuration-change-hook)
 >     (apply fn args))
 >   (run-window-configuration-change-hook))

Can you run this under the debugger in order to see what happens?

martin

bug#24240: 25.1.50; window-state-put, image-mode and window scrolling

[Andreas Politz]

martin rudalics <rudalics@gmx.at> writes:

> We'd need a variable, say ‘window-inhibit-configuration-change-hook’.

inhibit-window-configuration-change-hook ?

>> ;; Workaround 1 (does not work, why ?)
>
> Can you run this under the debugger in order to see what happens?
>

The problem is, that it does work when edebugging it.  Anyway, I think I've
found the problem: It's the call to set-window-start, which in my
understanding forces the next redraw to scroll the window, thus
negating image-modes work.

-ap

bug#24240: 25.1.50; window-state-put, image-mode and window scrolling

[martin rudalics]

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

 >> We'd need a variable, say ‘window-inhibit-configuration-change-hook’.
 >
 > inhibit-window-configuration-change-hook ?

We can abuse ‘frame-after-make-frame’.  Try the attached, completely
untested patch.

BTW I just noticed that we still run ‘window-configuration-change-hook’
in far too many cases.  I'll have to remove it from ‘window-resize’,
‘window--resize-mini-window’, ‘adjust-window-trailing-edge’ and
‘balance-windows’.

 > The problem is, that it does work when edebugging it.  Anyway, I think I've
 > found the problem: It's the call to set-window-start, which in my
 > understanding forces the next redraw to scroll the window, thus
 > negating image-modes work.

Would my patch work around that?

martin

[-- Attachment #2: inhibit-run-wcchh.diff --]
[-- Type: text/plain, Size: 2156 bytes --]

--- a/lisp/window.el
+++ b/lisp/window.el
@@ -5620,21 +5620,29 @@ window-state-put
 	;; minimum possible state.  But such configurations hardly make
 	;; sense anyway.
 	(error "Window %s too small to accommodate state" window)
-      (setq state (cdr state))
-      (setq window-state-put-list nil)
-      ;; Work on the windows of a temporary buffer to make sure that
-      ;; splitting proceeds regardless of any buffer local values of
-      ;; `window-size-fixed'.  Release that buffer after the buffers of
-      ;; all live windows have been set by `window--state-put-2'.
-      (with-temp-buffer
-	(set-window-buffer window (current-buffer))
-	(window--state-put-1 state window nil totals pixelwise)
-	(window--state-put-2 ignore pixelwise))
-      (while window-state-put-stale-windows
-	(let ((window (pop window-state-put-stale-windows)))
-	  (when (eq (window-deletable-p window) t)
-	    (delete-window window))))
-      (window--check frame))))
+
+
+      (unwind-protect
+          (progn
+            (frame-after-make-frame frame nil)
+            (setq state (cdr state))
+            (setq window-state-put-list nil)
+            ;; Work on the windows of a temporary buffer to make sure that
+            ;; splitting proceeds regardless of any buffer local values of
+            ;; `window-size-fixed'.  Release that buffer after the buffers of
+            ;; all live windows have been set by `window--state-put-2'.
+            (with-temp-buffer
+              (set-window-buffer window (current-buffer))
+              (window--state-put-1 state window nil totals pixelwise)
+              (window--state-put-2 ignore pixelwise))
+            (while window-state-put-stale-windows
+              (let ((window (pop window-state-put-stale-windows)))
+                (when (eq (window-deletable-p window) t)
+                  (delete-window window)))))
+        (frame-after-make-frame frame t)
+        (prog1
+            (window--check frame)
+          (run-window-configuration-change-hook frame))))))
 \f
 (defun display-buffer-record-window (type window buffer)
   "Record information for window used by `display-buffer'.

bug#24240: 25.1.50; window-state-put, image-mode and window scrolling

[Andreas Politz]

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

martin rudalics <rudalics@gmx.at> writes:

> We can abuse ‘frame-after-make-frame’.  Try the attached, completely
> untested patch.
>
> BTW I just noticed that we still run ‘window-configuration-change-hook’
> in far too many cases.

(defmacro with-inhibit-window-configuration-change-hook (frame &rest body)
  "Inhibit `window-configuration-change-hook' on FRAME in BODY."
  (declare (indent 1) (debug t))
  (let ((frame-var (make-symbol "frame")))
    `(let ((,frame-var (window-normalize-frame ,frame)))
       (unwind-protect
           (progn
             (frame-after-make-frame ,frame-var nil)
             ,@body)
         (frame-after-make-frame ,frame-var t)))))

> Would my patch work around that?

It seems to inhibit running the hook, but there is still the case of
set-window-start in window--state-put-2.  Using the NOFORCE works, but I
don't know the implications of this in other cases.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: set-window-start.diff --]
[-- Type: text/x-diff, Size: 545 bytes --]

diff --git a/lisp/window.el b/lisp/window.el
index 11d7a4e..d30819d 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -5497,7 +5497,7 @@ window--state-put-2
 		;; Install positions (maybe we should do this after all
 		;; windows have been created and sized).
 		(ignore-errors
-		  (set-window-start window (cdr (assq 'start state)))
+		  (set-window-start window (cdr (assq 'start state)) 'noforce)
 		  (set-window-point window (cdr (assq 'point state))))
 		;; Select window if it's the selected one.
 		(when (cdr (assq 'selected state))

[-- Attachment #3: Type: text/plain, Size: 5 bytes --]


-ap

bug#24240: 25.1.50; window-state-put, image-mode and window scrolling

[martin rudalics]

 > (defmacro with-inhibit-window-configuration-change-hook (frame &rest body)
 >    "Inhibit `window-configuration-change-hook' on FRAME in BODY."
 >    (declare (indent 1) (debug t))
 >    (let ((frame-var (make-symbol "frame")))
 >      `(let ((,frame-var (window-normalize-frame ,frame)))
 >         (unwind-protect
 >             (progn
 >               (frame-after-make-frame ,frame-var nil)
 >               ,@body)
 >           (frame-after-make-frame ,frame-var t)))))

Good.  We can use that (if we still need it).

 >> Would my patch work around that?
 >
 > It seems to inhibit running the hook, but there is still the case of
 > set-window-start in window--state-put-2.  Using the NOFORCE works, but I
 > don't know the implications of this in other cases.

It _should_ use NOFORCE anyway.  Otherwise, when, for example, the size
of the window we got the state from is much larger than the size of the
window we put the state in, window-point will end up in the wrong place.
And ‘window-point’ is definitively more important than ‘window-start’.

It should be fairly easy to construct such an example.

But do you mean that setting NOFORCE alone will handle your bug already
and we don't need ‘frame-after-make-frame’ after all?

martin

bug#24240: 25.1.50; window-state-put, image-mode and window scrolling

[Andreas Politz]

martin rudalics <rudalics@gmx.at> writes:

> It _should_ use NOFORCE anyway.

Ok.

> But do you mean that setting NOFORCE alone will handle your bug already
> and we don't need ‘frame-after-make-frame’ after all?

Yes, it would.

At least in the case of image-mode, it would actually be preferable to
let window-state-put override image-mode-reapply-winprops's scrolling,
because it has the correct value.  I don't know how familiar you are
with image-mode, but if it has no record for a particular window, it
restores the value last set in any window.  The difference would be
visible in case the buffer is shown in more than one window (with
different scroll values).

-ap

bug#24240: 25.1.50; window-state-put, image-mode and window scrolling

[martin rudalics]

 >> But do you mean that setting NOFORCE alone will handle your bug already
 >> and we don't need ‘frame-after-make-frame’ after all?
 >
 > Yes, it would.

OK.  Pushed to master.  In any case it fixes a scenario like the following:

(let (window state point start)
   (find-file "../lisp/window.el")
   (forward-line 30)
   (setq window (selected-window))
   (setq state (window-state-get window))
   (setq start (window-start window))
   (setq point (window-point window))
   (message "Before split - %s  ... start: %s ... point: %s" window start point) (sit-for 3)
   (split-window window)
   (message "After split - %s  ... start: %s ... point: %s" window start point) (sit-for 3)
   (window-state-put state window)
   (sit-for 0)
   (message
    "Final - %s ... start: %s -> %s ... point: %s -> %s"
    window start (window-start window) point (window-point window)))

 > At least in the case of image-mode, it would actually be preferable to
 > let window-state-put override image-mode-reapply-winprops's scrolling,
 > because it has the correct value.  I don't know how familiar you are
 > with image-mode,

Not at all.

 > but if it has no record for a particular window, it
 > restores the value last set in any window.  The difference would be
 > visible in case the buffer is shown in more than one window (with
 > different scroll values).

And you mean that the situation after your fix is as expected and the
single, final call of ‘window-configuration-change-hook’ I proposed
earlier would spoil that?  Technically spoken, it would fail because
‘window-state-put’ creates windows anew and for a new window image-mode
has no record of the previous position of the image in that window?  Do
you agree with that interpretation?

martin

bug#24240: 25.1.50; window-state-put, image-mode and window scrolling

[martin rudalics]

Bug closed.  Please reopen it when any of the issues discussed
here become virulent again.

martin