GNU bug report logs - #24240
25.1.50; window-state-put, image-mode and window scrolling

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 24240 in the body.
You can then email your comments to 24240 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Andreas Politz <politza <at> hochschule-trier.de>
To: bug-gnu-emacs <at> gnu.org
Subject: 25.1.50; window-state-put, image-mode and window scrolling
Date: Tue, 16 Aug 2016 01:05:11 +0200

[Message part 1 (text/plain, inline)]

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.

[window-state-put-workaround.el (application/emacs-lisp, attachment)]

Message #8 received at 24240 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Andreas Politz <politza <at> hochschule-trier.de>, 24240 <at> debbugs.gnu.org
Subject: Re: bug#24240: 25.1.50;
 window-state-put, image-mode and window scrolling
Date: Tue, 16 Aug 2016 13:00:23 +0200

> 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

Message #11 received at 24240 <at> debbugs.gnu.org (full text, mbox):

From: Andreas Politz <politza <at> hochschule-trier.de>
To: martin rudalics <rudalics <at> gmx.at>
Cc: 24240 <at> debbugs.gnu.org
Subject: Re: bug#24240: 25.1.50;
 window-state-put, image-mode and window scrolling
Date: Tue, 16 Aug 2016 15:51:09 +0200

martin rudalics <rudalics <at> 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

Message #14 received at 24240 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Andreas Politz <politza <at> hochschule-trier.de>
Cc: 24240 <at> debbugs.gnu.org
Subject: Re: bug#24240: 25.1.50;
 window-state-put, image-mode and window scrolling
Date: Wed, 17 Aug 2016 10:30:26 +0200

[Message part 1 (text/plain, inline)]

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

[inhibit-run-wcchh.diff (text/plain, attachment)]

Message #17 received at 24240 <at> debbugs.gnu.org (full text, mbox):

From: Andreas Politz <politza <at> hochschule-trier.de>
To: martin rudalics <rudalics <at> gmx.at>
Cc: 24240 <at> debbugs.gnu.org
Subject: Re: bug#24240: 25.1.50;
 window-state-put, image-mode and window scrolling
Date: Wed, 17 Aug 2016 12:33:19 +0200

[Message part 1 (text/plain, inline)]

martin rudalics <rudalics <at> 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.

[Message part 2 (text/x-diff, inline)]

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

[Message part 3 (text/plain, inline)]

-ap

Message #20 received at 24240 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Andreas Politz <politza <at> hochschule-trier.de>
Cc: 24240 <at> debbugs.gnu.org
Subject: Re: bug#24240: 25.1.50;
 window-state-put, image-mode and window scrolling
Date: Wed, 17 Aug 2016 17:47:32 +0200

> (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

Message #23 received at 24240 <at> debbugs.gnu.org (full text, mbox):

From: Andreas Politz <politza <at> hochschule-trier.de>
To: martin rudalics <rudalics <at> gmx.at>
Cc: 24240 <at> debbugs.gnu.org
Subject: Re: bug#24240: 25.1.50;
 window-state-put, image-mode and window scrolling
Date: Wed, 17 Aug 2016 18:12:31 +0200

martin rudalics <rudalics <at> 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

Message #26 received at 24240 <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Andreas Politz <politza <at> hochschule-trier.de>
Cc: 24240 <at> debbugs.gnu.org
Subject: Re: bug#24240: 25.1.50;
 window-state-put, image-mode and window scrolling
Date: Thu, 18 Aug 2016 10:42:06 +0200

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

Message #31 received at 24240-done <at> debbugs.gnu.org (full text, mbox):

From: martin rudalics <rudalics <at> gmx.at>
To: Andreas Politz <politza <at> hochschule-trier.de>
Cc: 24240-done <at> debbugs.gnu.org
Subject: Re: bug#24240: 25.1.50;
 window-state-put, image-mode and window scrolling
Date: Sun, 30 Oct 2016 09:47:13 +0100

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

martin

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.