* bug#46904: Non-unique windows produced by window-state-put
@ 2021-03-03 20:13 Juri Linkov
2021-03-04 8:35 ` martin rudalics
0 siblings, 1 reply; 15+ messages in thread
From: Juri Linkov @ 2021-03-03 20:13 UTC (permalink / raw)
To: 46904
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.
^ permalink raw reply [flat|nested] 15+ messages in thread* bug#46904: Non-unique windows produced by window-state-put
2021-03-03 20:13 bug#46904: Non-unique windows produced by window-state-put Juri Linkov
@ 2021-03-04 8:35 ` martin rudalics
2021-03-04 9:39 ` Juri Linkov
2021-04-27 17:19 ` Juri Linkov
0 siblings, 2 replies; 15+ messages in thread
From: martin rudalics @ 2021-03-04 8:35 UTC (permalink / raw)
To: Juri Linkov, 46904
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread* bug#46904: Non-unique windows produced by window-state-put
2021-03-04 8:35 ` martin rudalics
@ 2021-03-04 9:39 ` Juri Linkov
2021-03-04 10:34 ` martin rudalics
2021-04-27 17:19 ` Juri Linkov
1 sibling, 1 reply; 15+ messages in thread
From: Juri Linkov @ 2021-03-04 9:39 UTC (permalink / raw)
To: martin rudalics; +Cc: 46904
>>
>> 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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#46904: Non-unique windows produced by window-state-put
2021-03-04 9:39 ` Juri Linkov
@ 2021-03-04 10:34 ` martin rudalics
2021-03-04 17:57 ` Juri Linkov
0 siblings, 1 reply; 15+ messages in thread
From: martin rudalics @ 2021-03-04 10:34 UTC (permalink / raw)
To: Juri Linkov; +Cc: 46904
>> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#46904: Non-unique windows produced by window-state-put
2021-03-04 10:34 ` martin rudalics
@ 2021-03-04 17:57 ` Juri Linkov
2021-03-04 20:31 ` Juri Linkov
0 siblings, 1 reply; 15+ messages in thread
From: Juri Linkov @ 2021-03-04 17:57 UTC (permalink / raw)
To: martin rudalics; +Cc: 46904
[-- 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)
^ permalink raw reply related [flat|nested] 15+ messages in thread* bug#46904: Non-unique windows produced by window-state-put
2021-03-04 17:57 ` Juri Linkov
@ 2021-03-04 20:31 ` Juri Linkov
2021-03-05 9:10 ` martin rudalics
0 siblings, 1 reply; 15+ messages in thread
From: Juri Linkov @ 2021-03-04 20:31 UTC (permalink / raw)
To: martin rudalics; +Cc: 46904
>> 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.
^ permalink raw reply [flat|nested] 15+ messages in thread* bug#46904: Non-unique windows produced by window-state-put
2021-03-04 20:31 ` Juri Linkov
@ 2021-03-05 9:10 ` martin rudalics
2021-03-07 18:43 ` Juri Linkov
0 siblings, 1 reply; 15+ messages in thread
From: martin rudalics @ 2021-03-05 9:10 UTC (permalink / raw)
To: Juri Linkov; +Cc: 46904
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#46904: Non-unique windows produced by window-state-put
2021-03-05 9:10 ` martin rudalics
@ 2021-03-07 18:43 ` Juri Linkov
2021-03-08 8:26 ` martin rudalics
0 siblings, 1 reply; 15+ messages in thread
From: Juri Linkov @ 2021-03-07 18:43 UTC (permalink / raw)
To: martin rudalics; +Cc: 46904
[-- 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)
^ permalink raw reply related [flat|nested] 15+ messages in thread* bug#46904: Non-unique windows produced by window-state-put
2021-03-07 18:43 ` Juri Linkov
@ 2021-03-08 8:26 ` martin rudalics
2021-03-08 17:18 ` Juri Linkov
0 siblings, 1 reply; 15+ messages in thread
From: martin rudalics @ 2021-03-08 8:26 UTC (permalink / raw)
To: Juri Linkov; +Cc: 46904
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#46904: Non-unique windows produced by window-state-put
2021-03-08 8:26 ` martin rudalics
@ 2021-03-08 17:18 ` Juri Linkov
2021-03-08 18:06 ` martin rudalics
0 siblings, 1 reply; 15+ messages in thread
From: Juri Linkov @ 2021-03-08 17:18 UTC (permalink / raw)
To: martin rudalics; +Cc: 46904
>> 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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#46904: Non-unique windows produced by window-state-put
2021-03-08 17:18 ` Juri Linkov
@ 2021-03-08 18:06 ` martin rudalics
2021-03-08 18:24 ` Juri Linkov
0 siblings, 1 reply; 15+ messages in thread
From: martin rudalics @ 2021-03-08 18:06 UTC (permalink / raw)
To: Juri Linkov; +Cc: 46904
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#46904: Non-unique windows produced by window-state-put
2021-03-08 18:06 ` martin rudalics
@ 2021-03-08 18:24 ` Juri Linkov
2021-03-09 17:28 ` Juri Linkov
0 siblings, 1 reply; 15+ messages in thread
From: Juri Linkov @ 2021-03-08 18:24 UTC (permalink / raw)
To: martin rudalics; +Cc: 46904
>> 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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#46904: Non-unique windows produced by window-state-put
2021-03-08 18:24 ` Juri Linkov
@ 2021-03-09 17:28 ` Juri Linkov
0 siblings, 0 replies; 15+ messages in thread
From: Juri Linkov @ 2021-03-09 17:28 UTC (permalink / raw)
To: martin rudalics; +Cc: 46904
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#46904: Non-unique windows produced by window-state-put
2021-03-04 8:35 ` martin rudalics
2021-03-04 9:39 ` Juri Linkov
@ 2021-04-27 17:19 ` Juri Linkov
2021-04-28 20:25 ` Juri Linkov
1 sibling, 1 reply; 15+ messages in thread
From: Juri Linkov @ 2021-04-27 17:19 UTC (permalink / raw)
To: martin rudalics; +Cc: 46904
[-- 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)
^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-04-28 20:25 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-03 20:13 bug#46904: Non-unique windows produced by window-state-put Juri Linkov
2021-03-04 8:35 ` martin rudalics
2021-03-04 9:39 ` Juri Linkov
2021-03-04 10:34 ` martin rudalics
2021-03-04 17:57 ` Juri Linkov
2021-03-04 20:31 ` Juri Linkov
2021-03-05 9:10 ` martin rudalics
2021-03-07 18:43 ` Juri Linkov
2021-03-08 8:26 ` martin rudalics
2021-03-08 17:18 ` Juri Linkov
2021-03-08 18:06 ` martin rudalics
2021-03-08 18:24 ` Juri Linkov
2021-03-09 17:28 ` Juri Linkov
2021-04-27 17:19 ` Juri Linkov
2021-04-28 20:25 ` Juri Linkov
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).