bug#16793: window-state-put needs a live window

bug#16793: window-state-put needs a live window

[Juanma Barranquero]

Package: emacs
Version: 24.3.50
X-Debbugs-Cc: rudalics@gmx.at

There's a problem when restoring a frameset where "put"ing a window
state into the root window of a frame fails for some windows if the
frame is split (which is common enough).

It's easy to see the problem with

  emacs -Q
  C-x 2
  C-x r f a
  C-x r j a
  => "Error (frameset): #<window 6> is not a live window"

or

  emacs -Q
  ;; eval this in *scratch*
  (let (ws frw)
    (split-window)
    (setq frw (frame-root-window))
    (setq ws (window-state-get frw))
    (window-state-put ws frw))
  => "Lisp error: (error "#<window 6> is not a live window")"

Strictly speaking It's not a bug, because the docstring of
`window-state-put' says "Optional argument WINDOW must specify a live
window". But as the examples above show, it is less than useful to be
able to get the state of a window and not being able to put it again
into the same window.

In framesets, the problem arises because it's often the case that the
preferred frame to restore a window-state is the very same one the
window-state was saved from, particularly for saving/restoring in the
same session (as it happens with frameset-to-register /
jump-to-register). That minimizes flicker and simplifies dealing with
non-reused frames.

Martin suggests that frameset--restore should check that the root
window of the matching frame found is alive, and if not, discard that
frame and create a new one. That's doable, but it seems less than
optimal, from both a coding and UI perspective.

bug#16793: window-state-put needs a live window

[Stefan Monnier]

> Package: emacs
> Version: 24.3.50
> X-Debbugs-Cc: rudalics@gmx.at

[ The X-Debbugs-Cc should be in the email header, not in the here
  pseudo-header.  Welcome to Debbugs ;-)  ]

> Martin suggests that frameset--restore should check that the root
> window of the matching frame found is alive, and if not, discard that
> frame and create a new one.  That's doable, but it seems less than
> optimal, from both a coding and UI perspective.

I don't understand what this means.  Why would the frame's root window
not be alive?  Is it because it's an "internal" window (because of
a split)?  deleting a frame and creating a new one is definitely not
a good idea, since it can have all kinds of unintended side-effects,
depending on the window manager.


        Stefan

bug#16793: window-state-put needs a live window

[Glenn Morris]

Stefan Monnier wrote:

>> X-Debbugs-Cc: rudalics@gmx.at
>
> [ The X-Debbugs-Cc should be in the email header, not in the here
>   pseudo-header.  Welcome to Debbugs ;-)  ]

Either works. Welcome to debbugs.gnu.org...

bug#16793: window-state-put needs a live window

[Glenn Morris]

Glenn Morris wrote:

>> [ The X-Debbugs-Cc should be in the email header, not in the here
>>   pseudo-header.  Welcome to Debbugs ;-)  ]
>
> Either works. Welcome to debbugs.gnu.org...

Actually, that's a standard Debian debbugs feature.

bug#16793: window-state-put needs a live window

[Stefan Monnier]

>>> [ The X-Debbugs-Cc should be in the email header, not in the here
>>> pseudo-header.  Welcome to Debbugs ;-)  ]
>> Either works. Welcome to debbugs.gnu.org...
> Actually, that's a standard Debian debbugs feature.

Hmm... should debbugs turn it into a "Cc:" (when it allocates
a bug-number to the email), then?


        Stefan

bug#16793: window-state-put needs a live window

[Glenn Morris]

Stefan Monnier wrote:

> Hmm... should debbugs turn it into a "Cc:" (when it allocates
> a bug-number to the email), then?

IIRC, it does, but Martin is subscribed to bug-gnu-emacs with
duplication suppression enabled, so bug-gnu-emacs Mailman strips out the
cc in the version of the mail it sends out to the list.

http://debbugs.gnu.org/cgi/bugreport.cgi?msg=2;bug=16793

bug#16793: window-state-put needs a live window

[Stefan Monnier]

> IIRC, it does, but Martin is subscribed to bug-gnu-emacs with
> duplication suppression enabled, so bug-gnu-emacs Mailman strips out the
> cc in the version of the mail it sends out to the list.

Aha!  Live and learn!  I still don't consider that I know what
"duplication suppression" really means in a mailing-list-manager, but at
least I finally saw it in action for the first time.


        Stefan

bug#16793: window-state-put needs a live window

[Juanma Barranquero]

On Tue, Feb 18, 2014 at 6:09 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:

> [ The X-Debbugs-Cc should be in the email header, not in the here
>   pseudo-header.  Welcome to Debbugs ;-)  ]

[ It works here too. At least it worked all other times I've used it ;-) ]

> I don't understand what this means.  Why would the frame's root window
> not be alive?  Is it because it's an "internal" window (because of
> a split)?

Yes.

>  deleting a frame and creating a new one is definitely not
> a good idea, since it can have all kinds of unintended side-effects,
> depending on the window manager.

I just committed a workaround for this issue: deleting all windows
except the first one in the relevant frame if its root window isn't
alive.

But I still think that getting a window-state for a window and putting
it again into the same window should always be allowed, unless Martin
has a good reason why that symmetry is a bad idea.

bug#16793: window-state-put needs a live window

[Stefan Monnier]

> But I still think that getting a window-state for a window and putting
> it again into the same window should always be allowed, unless Martin
> has a good reason why that symmetry is a bad idea.

I tend to agree,


        Stefan

bug#16793: window-state-put needs a live window

[martin rudalics]

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

 > But I still think that getting a window-state for a window and putting
 > it again into the same window should always be allowed, unless Martin
 > has a good reason why that symmetry is a bad idea.

I tend to agree with you.  But the obvious consequence is that the
WINDOW argument of `window-state-put' could then specify an internal
window that is not the root window of its frame.  In this case plain
`delete-other-windows' would be the wrong action.  I attach a patch.

martin

[-- Attachment #2: window-state-put.diff --]
[-- Type: text/plain, Size: 1393 bytes --]

=== modified file 'lisp/window.el'
--- lisp/window.el	2014-02-10 01:34:22 +0000
+++ lisp/window.el	2014-02-18 18:59:17 +0000
@@ -5026,14 +5026,29 @@
   "Put window state STATE into WINDOW.
 STATE should be the state of a window returned by an earlier
 invocation of `window-state-get'.  Optional argument WINDOW must
-specify a live window and defaults to the selected one.
+specify a valid window and defaults to the selected one.  If
+WINDOW is not live, delete all child windows of WINDOW before
+putting STATE.

 Optional argument IGNORE non-nil means ignore minimum window
 sizes and fixed size restrictions.  IGNORE equal `safe' means
 windows can get as small as `window-safe-min-height' and
 `window-safe-min-width'."
   (setq window-state-put-stale-windows nil)
-  (setq window (window-normalize-window window t))
+  (setq window (window-normalize-window window))
+
+  (unless (window-live-p window)
+    (let ((root (frame-root-window)))
+      (if (eq window root)
+	  (setq window (frame-first-window root))
+	(setq root window)
+	(setq window (catch 'live
+		       (walk-window-subtree
+			(lambda (window)
+			  (when (window-live-p window)))
+			root))))
+      (delete-other-windows-internal window root)))
+
   (let* ((frame (window-frame window))
 	 (head (car state))
 	 ;; We check here (1) whether the total sizes of root window of

bug#16793: window-state-put needs a live window

[Juanma Barranquero]

On Tue, Feb 18, 2014 at 8:04 PM, martin rudalics <rudalics@gmx.at> wrote:

> I tend to agree with you.  But the obvious consequence is that the
> WINDOW argument of `window-state-put' could then specify an internal
> window that is not the root window of its frame.  In this case plain
> `delete-other-windows' would be the wrong action.

Yeah. In my workaround that wasn't important because I specifically
got the frame's root.

>  I attach a patch.

Was this:

> +       (setq window (catch 'live
> +                      (walk-window-subtree
> +                       (lambda (window)
> +                         (when (window-live-p window)))
> +                       root))))

intended to be

  (setq window (catch 'live
                 (walk-window-subtree
                  (lambda (window)
                    (when (window-live-p window)
                      (throw 'live window)))
                  root))))

I suppose?

   J

bug#16793: window-state-put needs a live window

[martin rudalics]

> Was this:
> 
>> +       (setq window (catch 'live
>> +                      (walk-window-subtree
>> +                       (lambda (window)
>> +                         (when (window-live-p window)))
>> +                       root))))
> 
> intended to be
> 
>   (setq window (catch 'live
>                  (walk-window-subtree
>                   (lambda (window)
>                     (when (window-live-p window)
>                       (throw 'live window)))
>                   root))))
> 
> I suppose?

You suppose correctly.

martin

bug#16793: window-state-put needs a live window

[Juanma Barranquero]

On Tue, Feb 18, 2014 at 10:26 PM, martin rudalics <rudalics@gmx.at> wrote:

> You suppose correctly.

It works correctly ;-)

   J

bug#16793: window-state-put needs a live window

[martin rudalics]

 > It works correctly ;-)

Installed (with the throw) in revision 116477.  You should be able to
remove your workaround now.

martin

bug#16793: window-state-put needs a live window

[Juanma Barranquero]

> Installed (with the throw) in revision 116477.  You should be able to
> remove your workaround now.

Done. Thanks.

bug#16793: window-state-put needs a live window

[martin rudalics]

 > Martin suggests that frameset--restore should check that the root
 > window of the matching frame found is alive, and if not, discard that
 > frame and create a new one. That's doable, but it seems less than
 > optimal, from both a coding and UI perspective.

That was a misunderstanding.  Your initial scenario was

   emacs -Q --eval "(setq debug-on-error t)"
   C-x 2
   C-x r f a
   C-x 5 2
   C-u C-x r j a

so I thought you wanted to create a new frame anyway ;-)

martin

bug#16793: window-state-put needs a live window

[Juanma Barranquero]

On Tue, Feb 18, 2014 at 8:04 PM, martin rudalics <rudalics@gmx.at> wrote:

> That was a misunderstanding.

Oh, glad to hear it.

>  Your initial scenario was
>
>   emacs -Q --eval "(setq debug-on-error t)"
>
>   C-x 2
>   C-x r f a
>   C-x 5 2
>   C-u C-x r j a
>
> so I thought you wanted to create a new frame anyway ;-)

With or without C-x 5 2, "C-x r f a" followed a time later by "C-x r j
a" is intended to reuse frames if possible, not create new ones. But I
suppose I could've been clearer. Sorry about that.