bug#46904: Non-unique windows produced by window-state-put

bug#46904: Non-unique windows produced by window-state-put

[Juri Linkov]

We have a problem in window-state-put.  For example, in emacs -Q
evaluate this several times:

(progn
  (window-state-put
   (window-state-get
    (frame-root-window (selected-frame))
    'writable)
   (frame-root-window (selected-frame)) 'safe)
  (selected-window))

Every time the selected window remains the same:

  #<window 3 on *scratch*>

But after splitting the window with e.g. 'C-x 2',
evaluating the same every time creates a new window
that it's expected to do even when there is only one window
on the frame.

I don't know how severe are the consequences.  The only side-effect I noticed
that prev-buffers of restored windows are the same on different tabs because
they share the same window.

bug#46904: Non-unique windows produced by window-state-put

[martin rudalics]

 > We have a problem in window-state-put.  For example, in emacs -Q
 > evaluate this several times:
 >
 > (progn
 >    (window-state-put
 >     (window-state-get
 >      (frame-root-window (selected-frame))
 >      'writable)
 >     (frame-root-window (selected-frame)) 'safe)
 >    (selected-window))
 >
 > Every time the selected window remains the same:
 >
 >    #<window 3 on *scratch*>
 >
 > But after splitting the window with e.g. 'C-x 2',
 > evaluating the same every time creates a new window
 > that it's expected to do even when there is only one window
 > on the frame.

You mean we should do that

             ;; Create a new window to replace the existing one.
             (setq window (prog1 (split-window window)
                            (delete-window window)))))

in the one window case too?  This would be certainly more consistent but
I do not remember any more why this is needed in the multiple windows
case.

 > I don't know how severe are the consequences.  The only side-effect I noticed
 > that prev-buffers of restored windows are the same on different tabs because
 > they share the same window.

If that is a problem and the above would help, let's do it.  It couldn't
possibly harm unless the root window is too small to get split and we
want to put only one window there; so we should handle that special case.

martin

bug#46904: Non-unique windows produced by window-state-put

[Juri Linkov]

>>
>> Every time the selected window remains the same:
>>
>>    #<window 3 on *scratch*>
>>
>> But after splitting the window with e.g. 'C-x 2',
>> evaluating the same every time creates a new window
>> that it's expected to do even when there is only one window
>> on the frame.
>
> You mean we should do that
>
>             ;; Create a new window to replace the existing one.
>             (setq window (prog1 (split-window window)
>                            (delete-window window)))))
>
> in the one window case too?  This would be certainly more consistent but

This looks like the right fix.

> I do not remember any more why this is needed in the multiple windows
> case.

And I do not remember it too, maybe the current logic handled some use cases,
maybe not.

>> I don't know how severe are the consequences.  The only side-effect I noticed
>> that prev-buffers of restored windows are the same on different tabs because
>> they share the same window.
>
> If that is a problem and the above would help, let's do it.  It couldn't
> possibly harm unless the root window is too small to get split and we
> want to put only one window there; so we should handle that special case.

I'll try this, thanks.

bug#46904: Non-unique windows produced by window-state-put

[martin rudalics]

 >> It couldn't
 >> possibly harm unless the root window is too small to get split and we
 >> want to put only one window there; so we should handle that special case.
 >
 > I'll try this, thanks.

I'd first try a vertical, then a horizontal split.  Give up if both
fail.

martin

bug#46904: Non-unique windows produced by window-state-put

[Juri Linkov]

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

>>> It couldn't
>>> possibly harm unless the root window is too small to get split and we
>>> want to put only one window there; so we should handle that special case.
>>
>> I'll try this, thanks.
>
> I'd first try a vertical, then a horizontal split.  Give up if both
> fail.

Maybe something like this:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: window-state-put.patch --]
[-- Type: text/x-diff, Size: 897 bytes --]

diff --git a/lisp/window.el b/lisp/window.el
index cfd9876ed0..d750e16626 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -6229,10 +6229,15 @@ window-state-put
                               (throw 'live window)))
                           root))
                      (selected-window)))
-      (delete-other-windows-internal window root)
-      ;; Create a new window to replace the existing one.
-      (setq window (prog1 (split-window window)
-                     (delete-window window)))))
+      (delete-other-windows-internal window root)))
+
+  ;; Create a new window to replace the existing one.
+  (let ((new-window
+         (or (ignore-errors (split-window (selected-window)))
+             (ignore-errors (split-window (selected-window) nil t)))))
+    (when new-window
+      (delete-window window)
+      (setq window new-window)))
 
   (set-window-dedicated-p window nil)
 

bug#46904: Non-unique windows produced by window-state-put

[Juri Linkov]

>> I'd first try a vertical, then a horizontal split.  Give up if both
>> fail.
>
> Maybe something like this:
>
> @@ -6229,10 +6229,15 @@ window-state-put
> +  ;; Create a new window to replace the existing one.
> +  (let ((new-window
> +         (or (ignore-errors (split-window (selected-window)))
> +             (ignore-errors (split-window (selected-window) nil t)))))
> +    (when new-window
> +      (delete-window window)
> +      (setq window new-window)))

Actually with this patch window-swap-states fails with the error
"Wrong type argument: stringp, nil" in:

      ;; Swap basic states.
      (window-state-put state-1 window-2 t)
      (window-state-put state-2 window-1 t)
      ;; Swap overlays with `window' property.
      (with-current-buffer (window-buffer window-1)

Here '(window-buffer window-1)' returns nil because
window-1 doesn't exist anymore after calling window-state-put.

bug#46904: Non-unique windows produced by window-state-put

[martin rudalics]

 > Actually with this patch window-swap-states fails with the error
 > "Wrong type argument: stringp, nil" in:
 >
 >        ;; Swap basic states.
 >        (window-state-put state-1 window-2 t)
 >        (window-state-put state-2 window-1 t)
 >        ;; Swap overlays with `window' property.
 >        (with-current-buffer (window-buffer window-1)
 >
 > Here '(window-buffer window-1)' returns nil because
 > window-1 doesn't exist anymore after calling window-state-put.

We could easily handle that by having `window-state-put' return the
window actually used for putting the state into and use that in
`window-swap-states'.  Yet someone else might use `window-state-put' in
much the same way as `window-swap-states' does and we have a quite
incompatible change: We nowhere say that the window used as second
argument of `window-state-put' will be live if it was so before, but we
neither say that it can be deleted.  So I'm not sure what to do ...

martin

bug#46904: Non-unique windows produced by window-state-put

[Juri Linkov]

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

>> Actually with this patch window-swap-states fails with the error
>> "Wrong type argument: stringp, nil" in:
>>
>>        ;; Swap basic states.
>>        (window-state-put state-1 window-2 t)
>>        (window-state-put state-2 window-1 t)
>>        ;; Swap overlays with `window' property.
>>        (with-current-buffer (window-buffer window-1)
>>
>> Here '(window-buffer window-1)' returns nil because
>> window-1 doesn't exist anymore after calling window-state-put.
>
> We could easily handle that by having `window-state-put' return the
> window actually used for putting the state into and use that in
> `window-swap-states'.  Yet someone else might use `window-state-put' in
> much the same way as `window-swap-states' does and we have a quite
> incompatible change: We nowhere say that the window used as second
> argument of `window-state-put' will be live if it was so before, but we
> neither say that it can be deleted.  So I'm not sure what to do ...

Indeed, this would be an incompatible change.

So better to fix callers not to use frame-root-window
that is unreliable: sometimes frame-root-window returns
an internal window, sometimes a live window:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: window-state-put-without-frame-root-window.patch --]
[-- Type: text/x-diff, Size: 397 bytes --]

diff --git a/lisp/tab-bar.el b/lisp/tab-bar.el
index 917b5e496b..a7d905184b 100644
--- a/lisp/tab-bar.el
+++ b/lisp/tab-bar.el
@@ -766,7 +772,7 @@ tab-bar-select-tab
                      tab-bar-history-forward)))
 
          (ws
-          (window-state-put ws (frame-root-window (selected-frame)) 'safe)))
+          (window-state-put ws nil 'safe)))
 
         (setq tab-bar-history-omit t)
 

bug#46904: Non-unique windows produced by window-state-put

[martin rudalics]

 > So better to fix callers not to use frame-root-window
 > that is unreliable: sometimes frame-root-window returns
 > an internal window, sometimes a live window:

To emulate `set-window-configuration' via `window-state-put' you have
to put the state into the root window.

martin

bug#46904: Non-unique windows produced by window-state-put

[Juri Linkov]

>> So better to fix callers not to use frame-root-window
>> that is unreliable: sometimes frame-root-window returns
>> an internal window, sometimes a live window:
>
> To emulate `set-window-configuration' via `window-state-put' you have
> to put the state into the root window.

When 'frame-root-window' returns an internal window,
then we don't put the state into the root window anyway,
we create a new window.  The patch does the same for the
case when there is only one window too, so both cases
will provide the same consistent result.

bug#46904: Non-unique windows produced by window-state-put

[martin rudalics]

 > When 'frame-root-window' returns an internal window,
 > then we don't put the state into the root window anyway,
 > we create a new window.  The patch does the same for the
 > case when there is only one window too, so both cases
 > will provide the same consistent result.

Yes.  But how would you handle `window-swap-states' now?

martin

bug#46904: Non-unique windows produced by window-state-put

[Juri Linkov]

>> When 'frame-root-window' returns an internal window,
>> then we don't put the state into the root window anyway,
>> we create a new window.  The patch does the same for the
>> case when there is only one window too, so both cases
>> will provide the same consistent result.
>
> Yes.  But how would you handle `window-swap-states' now?

No changes are planned in window.el, only in tab-bar.el.

bug#46904: Non-unique windows produced by window-state-put

[Juri Linkov]

tags 46904 fixed
close 46904 28.0.50
thanks

>>> When 'frame-root-window' returns an internal window,
>>> then we don't put the state into the root window anyway,
>>> we create a new window.  The patch does the same for the
>>> case when there is only one window too, so both cases
>>> will provide the same consistent result.
>>
>> Yes.  But how would you handle `window-swap-states' now?
>
> No changes are planned in window.el, only in tab-bar.el.

Now pushed to master in commit 29458ec7d2.

bug#46904: Non-unique windows produced by window-state-put

[Juri Linkov]

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

>> I don't know how severe are the consequences.  The only side-effect I noticed
>> that prev-buffers of restored windows are the same on different tabs because
>> they share the same window.
>
> If that is a problem and the above would help, let's do it.  It couldn't
> possibly harm unless the root window is too small to get split and we
> want to put only one window there; so we should handle that special case.

The original problem was that prev-buffers of restored windows are the
same on different tabs.  This was fixed by always creating a new window.
However, the same problem still exists, but for a different reason.

Most windows have current buffer duplicated in prev-buffers.  So when
set-window-prev-buffers in window--state-put-2 sets prev-buffers to a list
with only current buffer, this means as if there are no prev-buffers.

But when a window has an empty list of prev-buffers, then the window
parameter of prev-buffers is not cleared, and still contains a random
buffer from some previous state.  This patch fixes it:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: window--state-put-2-prev-buffers.patch --]
[-- Type: text/x-diff, Size: 631 bytes --]

diff --git a/lisp/window.el b/lisp/window.el
index 036eb271ee..fc4ca303b0 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -6140,7 +6140,8 @@ window--state-put-2
                                        (setq buffer (get-buffer buffer))
                                        (when (buffer-live-p buffer) buffer))
                                      next-buffers))))
-                (when prev-buffers
+                (if (not prev-buffers)
+                    (set-window-prev-buffers window nil)
                   (set-window-prev-buffers
                    window
                    (delq nil (mapcar (lambda (entry)

bug#46904: Non-unique windows produced by window-state-put

[Juri Linkov]

> But when a window has an empty list of prev-buffers, then the window
> parameter of prev-buffers is not cleared, and still contains a random
> buffer from some previous state.  This patch fixes it:

Now pushed to master in commit 0e8c862885.