bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Benson Chu]

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

Hello,

I've noticed that when I call #'tab-bar-new-tab while I'm in a
side-window that has siblings, I get an error from
#'window--sides-check, which happens when #'tab-bar-new-tab calls
#'delete-other-windows. Here's an example of my problem:

(progn
  (display-buffer-in-side-window (get-buffer-create "*hello*")
                                 '((side . right)
                                   (slot . 1)))
  (select-window
   (display-buffer-in-side-window (get-buffer-create "*world*")
                                  '((side . right)
                                    (slot . 2))))
  (tab-bar-new-tab))

The attached patch fixes this issue. Could it be applied to the emacs-29
branch?

Thanks!

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-tab-bar-new-tab-to-now-handles-cases-with-multiple-s.patch --]
[-- Type: text/x-patch, Size: 2105 bytes --]

From cc8974b45d5726a0f6a8d9ab6566411c118a0769 Mon Sep 17 00:00:00 2001
From: Benson Chu <bensonchu457@gmail.com>
Date: Fri, 24 Mar 2023 15:38:03 -0500
Subject: [PATCH] tab-bar-new-tab-to now handles cases with multiple
 side-windows

Previously, calling tab-bar-new-tab-to only removes the 'window-side
property on the currently selected window, and then a call to
delete-other-windows was made to ensure that the selected window was
the only window.

However, if there are other side-windows (with the same side) present,
the call to delete-other-windows will fail on the
window--sides-check. This is because according to the check, all
windows on the same side should have the same 'window-side, and
because we only removed the 'window-side parameters on one of the
windows, there will be inconsistencies with that window and its parent
and siblings. Because of this, the call to delete-other-windows to
fail.

This patch makes sure that all windows on a given side will have
'window-side set to nil, so that the call to delete-other-windows will
not fail.

* lisp/tab-bar.el: remove 'window-side from selected window, window's
parent, and all window's siblings.
---
 lisp/tab-bar.el | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lisp/tab-bar.el b/lisp/tab-bar.el
index dce6fa735fc..f05abffbcdb 100644
--- a/lisp/tab-bar.el
+++ b/lisp/tab-bar.el
@@ -1556,7 +1556,13 @@ tab-bar-new-tab-to
       ;; with `delete-other-windows' and `split-window'.
       (unless (eq tab-bar-new-tab-choice 'clone)
         (set-window-parameter nil 'window-atom nil)
-        (set-window-parameter nil 'window-side nil))
+        (let ((side (window-parameter nil 'window-side)))
+          (when side
+            (walk-window-tree
+             (lambda (window)
+               (when (eq side (window-parameter window 'window-side))
+                 (set-window-parameter window 'window-side nil)))
+             nil t))))
       (let ((ignore-window-parameters t))
         (if (eq tab-bar-new-tab-choice 'clone)
             ;; Create new unique windows with the same layout
-- 
2.40.0

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

> I've noticed that when I call #'tab-bar-new-tab while I'm in a
> side-window that has siblings, I get an error from
> #'window--sides-check, which happens when #'tab-bar-new-tab calls
> #'delete-other-windows. Here's an example of my problem:
> ...
> The attached patch fixes this issue. Could it be applied to the emacs-29
> branch?

Thanks.  Unless Eli has objections, I'd like to push the patch
to the emacs-29 branch since it correctly fixes the problem.

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Eli Zaretskii]

> Cc: 62427@debbugs.gnu.org
> From: Juri Linkov <juri@linkov.net>
> Date: Sat, 25 Mar 2023 21:14:24 +0200
> 
> > I've noticed that when I call #'tab-bar-new-tab while I'm in a
> > side-window that has siblings, I get an error from
> > #'window--sides-check, which happens when #'tab-bar-new-tab calls
> > #'delete-other-windows. Here's an example of my problem:
> > ...
> > The attached patch fixes this issue. Could it be applied to the emacs-29
> > branch?
> 
> Thanks.  Unless Eli has objections, I'd like to push the patch
> to the emacs-29 branch since it correctly fixes the problem.

I'd like first to understand better the code involved and the
suggested change in it.  Just reading the code of tab-bar-new-tab-to,
it looks strange.  Why does tab-bar.el care about "window parameters
that can cause problems with 'delete-other-windows' and
'split-window'" to begin with?  And why removing these parameters from
windows is TRT for tab-bar.el to resolve such problems, when it
doesn't really "own" those windows?

(Also, the patch's commit log message is not according to our
conventions, but that's a minor issue.)

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

>> > I've noticed that when I call #'tab-bar-new-tab while I'm in a
>> > side-window that has siblings, I get an error from
>> > #'window--sides-check, which happens when #'tab-bar-new-tab calls
>> > #'delete-other-windows. Here's an example of my problem:
>> > ...
>> > The attached patch fixes this issue. Could it be applied to the emacs-29
>> > branch?
>>
>> Thanks.  Unless Eli has objections, I'd like to push the patch
>> to the emacs-29 branch since it correctly fixes the problem.
>
> I'd like first to understand better the code involved and the
> suggested change in it.  Just reading the code of tab-bar-new-tab-to,
> it looks strange.  Why does tab-bar.el care about "window parameters
> that can cause problems with 'delete-other-windows' and
> 'split-window'" to begin with?  And why removing these parameters from
> windows is TRT for tab-bar.el to resolve such problems, when it
> doesn't really "own" those windows?

Indeed, maybe 'delete-other-windows' and `split-window'
should handle these cases.  So since the patch fixes the bug,
it could be pushed to emacs-29.  Then in master
'delete-other-windows' and `split-window' could be rewritten
to handle the 'window-side' parameter.

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: bensonchu457@fastmail.com,  62427@debbugs.gnu.org
> Date: Mon, 27 Mar 2023 10:05:40 +0300
> 
> >> > I've noticed that when I call #'tab-bar-new-tab while I'm in a
> >> > side-window that has siblings, I get an error from
> >> > #'window--sides-check, which happens when #'tab-bar-new-tab calls
> >> > #'delete-other-windows. Here's an example of my problem:
> >> > ...
> >> > The attached patch fixes this issue. Could it be applied to the emacs-29
> >> > branch?
> >>
> >> Thanks.  Unless Eli has objections, I'd like to push the patch
> >> to the emacs-29 branch since it correctly fixes the problem.
> >
> > I'd like first to understand better the code involved and the
> > suggested change in it.  Just reading the code of tab-bar-new-tab-to,
> > it looks strange.  Why does tab-bar.el care about "window parameters
> > that can cause problems with 'delete-other-windows' and
> > 'split-window'" to begin with?  And why removing these parameters from
> > windows is TRT for tab-bar.el to resolve such problems, when it
> > doesn't really "own" those windows?
> 
> Indeed, maybe 'delete-other-windows' and `split-window'
> should handle these cases.  So since the patch fixes the bug,
> it could be pushed to emacs-29.  Then in master
> 'delete-other-windows' and `split-window' could be rewritten
> to handle the 'window-side' parameter.

Maybe I'll agree, but I still don't understand the problem well
enough.  Would you please explain the original problem that led
tab-bar.el to care about these window parameters?

TIA

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

>> >> > I've noticed that when I call #'tab-bar-new-tab while I'm in a
>> >> > side-window that has siblings, I get an error from
>> >> > #'window--sides-check, which happens when #'tab-bar-new-tab calls
>> >> > #'delete-other-windows. Here's an example of my problem:
>> >> > ...
>> >> > The attached patch fixes this issue. Could it be applied to the emacs-29
>> >> > branch?
>> >>
>> >> Thanks.  Unless Eli has objections, I'd like to push the patch
>> >> to the emacs-29 branch since it correctly fixes the problem.
>> >
>> > I'd like first to understand better the code involved and the
>> > suggested change in it.  Just reading the code of tab-bar-new-tab-to,
>> > it looks strange.  Why does tab-bar.el care about "window parameters
>> > that can cause problems with 'delete-other-windows' and
>> > 'split-window'" to begin with?  And why removing these parameters from
>> > windows is TRT for tab-bar.el to resolve such problems, when it
>> > doesn't really "own" those windows?
>>
>> Indeed, maybe 'delete-other-windows' and `split-window'
>> should handle these cases.  So since the patch fixes the bug,
>> it could be pushed to emacs-29.  Then in master
>> 'delete-other-windows' and `split-window' could be rewritten
>> to handle the 'window-side' parameter.
>
> Maybe I'll agree, but I still don't understand the problem well
> enough.  Would you please explain the original problem that led
> tab-bar.el to care about these window parameters?

Sorry, I can't explain.  I just did that Martin said to do
in bug#53662.

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: bensonchu457@fastmail.com,  62427@debbugs.gnu.org
> Date: Mon, 27 Mar 2023 19:39:25 +0300
> 
> > Maybe I'll agree, but I still don't understand the problem well
> > enough.  Would you please explain the original problem that led
> > tab-bar.el to care about these window parameters?
> 
> Sorry, I can't explain.  I just did that Martin said to do
> in bug#53662.

That's okay, Benson Chu explained it.

Let me think about this.

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Benson Chu]

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

Oops, I only replied to Eli. I'll send my explanation, and new patch. 

> When the variable tab-bar-new-tab-choice is set to t, the intended
> behavior is to create a new tab with a single window, and that single
> window displaying the current buffer of the currently selected window,
> and the new window should have a fresh set of window parameters.

> Typically, this is done by first calling delete-other-windows, so the
> currently selected window is the only window. The call to
> delete-other-windows also ignores window-parameters, so even windows
> that have the no-delete-other-windows parameter still get deleted. Then,
> the current window is split, to create a fresh new window with fresh
> window parameters, and then delete-window is called to delete the
> currently selected window.

> This strategy doesn't work when the current window is a side-window,
> because delete-other-windows has a check which says that a side-window
> cannot be the only window in a frame. So, to work around this, we just
> remove the window-side parameter beforehand, so the above strategy still
> works.

> Another way we could do this was to get the current-buffer, then delete
> all side-windows. After deleting all side-windows, we know the current
> selected window is NOT a side-window, and then we can call
> delete-other-windows, split-window, and delete-window.

On Mon, Mar 27, 2023, at 12:06 PM, Eli Zaretskii wrote:
>> From: Juri Linkov <juri@linkov.net>
>> Cc: bensonchu457@fastmail.com,  62427@debbugs.gnu.org
>> Date: Mon, 27 Mar 2023 19:39:25 +0300
>> 
>> > Maybe I'll agree, but I still don't understand the problem well
>> > enough.  Would you please explain the original problem that led
>> > tab-bar.el to care about these window parameters?
>> 
>> Sorry, I can't explain.  I just did that Martin said to do
>> in bug#53662.
>
> That's okay, Benson Chu explained it.
>
> Let me think about this.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-tab-bar-new-tab-now-handles-multiple-side-windows.patch --]
[-- Type: text/x-patch; name="0001-tab-bar-new-tab-now-handles-multiple-side-windows.patch", Size: 2176 bytes --]

From 9596fe47f899c77567f4051990add7a31c8718aa Mon Sep 17 00:00:00 2001
From: Benson Chu <bensonchu457@gmail.com>
Date: Fri, 24 Mar 2023 15:38:03 -0500
Subject: [PATCH] ; tab-bar-new-tab now handles multiple side-windows

; Previously, calling tab-bar-new-tab-to only removes the 'window-side
; property on the currently selected window, and then a call to
; delete-other-windows was made to ensure that the selected window was
; the only window.

; However, if there are other side-windows (with the same side)
; present, the call to delete-other-windows will fail on the
; window--sides-check. This is because according to the check, all
; windows on the same side should have the same 'window-side, and
; because we only removed the 'window-side parameters on one of the
; windows, there will be inconsistencies with that window and its
; parent and siblings. Because of this, the call to
; delete-other-windows to fail.

; This patch makes sure that all windows on a given side will have
; 'window-side set to nil, so that the call to delete-other-windows
; will not fail.

* lisp/tab-bar.el (tab-bar-new-tab-to): remove 'window-side
from selected window, window's parent, and all window's
siblings.

Copyright-paperwork-exempt: yes
---
 lisp/tab-bar.el | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lisp/tab-bar.el b/lisp/tab-bar.el
index dce6fa735fc..f05abffbcdb 100644
--- a/lisp/tab-bar.el
+++ b/lisp/tab-bar.el
@@ -1556,7 +1556,13 @@ tab-bar-new-tab-to
       ;; with `delete-other-windows' and `split-window'.
       (unless (eq tab-bar-new-tab-choice 'clone)
         (set-window-parameter nil 'window-atom nil)
-        (set-window-parameter nil 'window-side nil))
+        (let ((side (window-parameter nil 'window-side)))
+          (when side
+            (walk-window-tree
+             (lambda (window)
+               (when (eq side (window-parameter window 'window-side))
+                 (set-window-parameter window 'window-side nil)))
+             nil t))))
       (let ((ignore-window-parameters t))
         (if (eq tab-bar-new-tab-choice 'clone)
             ;; Create new unique windows with the same layout
-- 
2.40.0

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Eli Zaretskii]

> Date: Mon, 27 Mar 2023 12:43:31 -0500
> From: "Benson Chu" <bensonchu457@fastmail.com>
> Cc: 62427@debbugs.gnu.org
> 
> > When the variable tab-bar-new-tab-choice is set to t, the intended
> > behavior is to create a new tab with a single window, and that single
> > window displaying the current buffer of the currently selected window,
> > and the new window should have a fresh set of window parameters.
> 
> > Typically, this is done by first calling delete-other-windows, so the
> > currently selected window is the only window. The call to
> > delete-other-windows also ignores window-parameters, so even windows
> > that have the no-delete-other-windows parameter still get deleted. Then,
> > the current window is split, to create a fresh new window with fresh
> > window parameters, and then delete-window is called to delete the
> > currently selected window.
> 
> > This strategy doesn't work when the current window is a side-window,
> > because delete-other-windows has a check which says that a side-window
> > cannot be the only window in a frame. So, to work around this, we just
> > remove the window-side parameter beforehand, so the above strategy still
> > works.
> 
> > Another way we could do this was to get the current-buffer, then delete
> > all side-windows. After deleting all side-windows, we know the current
> > selected window is NOT a side-window, and then we can call
> > delete-other-windows, split-window, and delete-window.
> 
> On Mon, Mar 27, 2023, at 12:06 PM, Eli Zaretskii wrote:
> >> From: Juri Linkov <juri@linkov.net>
> >> Cc: bensonchu457@fastmail.com,  62427@debbugs.gnu.org
> >> Date: Mon, 27 Mar 2023 19:39:25 +0300
> >> 
> >> > Maybe I'll agree, but I still don't understand the problem well
> >> > enough.  Would you please explain the original problem that led
> >> > tab-bar.el to care about these window parameters?
> >> 
> >> Sorry, I can't explain.  I just did that Martin said to do
> >> in bug#53662.
> >
> > That's okay, Benson Chu explained it.
> >
> > Let me think about this.

After thinking about this, I'm very uncomfortable with removing window
parameters like that.  These windows don't belong to us, right?  They
are windows that just happen to be there when the user creates a new
tab.  So arbitrary removal of their parameters behind the back of the
user and possibly some Lisp program which set these parameters is not
TRT.

Can't we create a completely new window and show the buffer in it?
That new window then can have any parameters we want, since it's new.

Or am I missing something?

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Benson Chu]

> Can't we create a completely new window and show the buffer in it?

I'm not sure how easy this is. Typically new windows come from calls to
#'split-window, and you can't do that for a side-window.

>                            I'm very uncomfortable with removing window
> parameters like that.  These windows don't belong to us, right?

Let me know if this is wrong, but I am interpreting this statement as:

"We shouldn't be modifying the window parameters of the windows that
belong to the old tab."

Because if that's the case, we're not /really/ modifying the old tab's
window-parameters. They're only "removed" "temporarily", for the
purposes of creating the new tab. You can see right before we modify any
window parameters, we make a call to (tab-bar--tab), which returns a tab
data structure, which contain a representation of the current wconf
(window configuration) - effectively saving the wconf - including the
paramaters. It's kind of like a save excursion:

(let ((old-tab-num (tab-bar--current-tab-index tabs))
      (old-window-configuration (tab-bar--tab)))
  ;; modify window-parameters
  ;; do appropriate splitting
  ;; Now we have the window layout for the new tab

  ;; The old tab should have the old-window-configuration
  (setf (nth old-tab-num tabs) old-window-configuration)

  ;; The rest of the function
  )

Maybe this function would read better if the (setf ...) came first.

So, we capture the current window configuration at the start of the
function, transition the current window configuration into the window
configuration for the new tab (which involves mangling window
parameters and destroying windows), then we make sure that the old tab
has an unmodified window-configuration.

When the user switches back to the tab they left, all the
window-parameters are still present, untouched. Are you still
uncomfortable with doing things this way?

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Mon, 27 Mar 2023 12:43:31 -0500
>> From: "Benson Chu" <bensonchu457@fastmail.com>
>> Cc: 62427@debbugs.gnu.org
>>
>> > When the variable tab-bar-new-tab-choice is set to t, the intended
>> > behavior is to create a new tab with a single window, and that single
>> > window displaying the current buffer of the currently selected window,
>> > and the new window should have a fresh set of window parameters.
>>
>> > Typically, this is done by first calling delete-other-windows, so the
>> > currently selected window is the only window. The call to
>> > delete-other-windows also ignores window-parameters, so even windows
>> > that have the no-delete-other-windows parameter still get deleted. Then,
>> > the current window is split, to create a fresh new window with fresh
>> > window parameters, and then delete-window is called to delete the
>> > currently selected window.
>>
>> > This strategy doesn't work when the current window is a side-window,
>> > because delete-other-windows has a check which says that a side-window
>> > cannot be the only window in a frame. So, to work around this, we just
>> > remove the window-side parameter beforehand, so the above strategy still
>> > works.
>>
>> > Another way we could do this was to get the current-buffer, then delete
>> > all side-windows. After deleting all side-windows, we know the current
>> > selected window is NOT a side-window, and then we can call
>> > delete-other-windows, split-window, and delete-window.
>>
>> On Mon, Mar 27, 2023, at 12:06 PM, Eli Zaretskii wrote:
>> >> From: Juri Linkov <juri@linkov.net>
>> >> Cc: bensonchu457@fastmail.com,  62427@debbugs.gnu.org
>> >> Date: Mon, 27 Mar 2023 19:39:25 +0300
>> >>
>> >> > Maybe I'll agree, but I still don't understand the problem well
>> >> > enough.  Would you please explain the original problem that led
>> >> > tab-bar.el to care about these window parameters?
>> >>
>> >> Sorry, I can't explain.  I just did that Martin said to do
>> >> in bug#53662.
>> >
>> > That's okay, Benson Chu explained it.
>> >
>> > Let me think about this.
>
> After thinking about this, I'm very uncomfortable with removing window
> parameters like that.  These windows don't belong to us, right?  They
> are windows that just happen to be there when the user creates a new
> tab.  So arbitrary removal of their parameters behind the back of the
> user and possibly some Lisp program which set these parameters is not
> TRT.
>
> Can't we create a completely new window and show the buffer in it?
> That new window then can have any parameters we want, since it's new.
>
> Or am I missing something?

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Eli Zaretskii]

> From: Benson Chu <bensonchu457@fastmail.com>
> Cc: juri@linkov.net, 62427@debbugs.gnu.org
> Date: Tue, 28 Mar 2023 11:17:29 -0500
> 
> > Can't we create a completely new window and show the buffer in it?
> 
> I'm not sure how easy this is. Typically new windows come from calls to
> #'split-window, and you can't do that for a side-window.

What does display-buffer do in this case? reuse the same window
regardless of any action alists?

> >                            I'm very uncomfortable with removing window
> > parameters like that.  These windows don't belong to us, right?
> 
> Let me know if this is wrong, but I am interpreting this statement as:
> 
> "We shouldn't be modifying the window parameters of the windows that
> belong to the old tab."

There doesn't have to be any "old tab".  AFAIU, this option's default
value is t, so even when you create the first tab, the code you
suggest changing will run and mess with the window parameters of the
windows that happen to exist at that point.  Right?

> Because if that's the case, we're not /really/ modifying the old tab's
> window-parameters. They're only "removed" "temporarily", for the
> purposes of creating the new tab. You can see right before we modify any
> window parameters, we make a call to (tab-bar--tab), which returns a tab
> data structure, which contain a representation of the current wconf
> (window configuration) - effectively saving the wconf - including the
> paramaters. It's kind of like a save excursion:
> 
> (let ((old-tab-num (tab-bar--current-tab-index tabs))
>       (old-window-configuration (tab-bar--tab)))
>   ;; modify window-parameters
>   ;; do appropriate splitting
>   ;; Now we have the window layout for the new tab
> 
>   ;; The old tab should have the old-window-configuration
>   (setf (nth old-tab-num tabs) old-window-configuration)
> 
>   ;; The rest of the function
>   )

Is this inside unwind-protect, so that any error or quit or throw is
caught and the parameters restored?  If so, it might be semi-okay.

> So, we capture the current window configuration at the start of the
> function, transition the current window configuration into the window
> configuration for the new tab (which involves mangling window
> parameters and destroying windows), then we make sure that the old tab
> has an unmodified window-configuration.

Ugh!  That is _soooo_ inelegant...  Also inefficient, and quite
fragile (as any code which uses seve/restore window-configuration is).
But I guess that ship has sailed long ago.

> When the user switches back to the tab they left, all the
> window-parameters are still present, untouched. Are you still
> uncomfortable with doing things this way?

What happens with all kinds of hooks that run due to this saving and
restoring window-configuration? they still will see windows without
their parameters, right?

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Benson Chu]

> Ugh!  That is _soooo_ inelegant...  Also inefficient

Hehe, I see what you mean, but creating frames is so expensive. These
tabs are like lightweight frames for me. What would you suggest be done
instead?

> Is this inside unwind-protect, so that any error or quit or throw is
> caught and the parameters restored?  If so, it might be semi-okay.

Ahhh, that is a good point. The reason I even noticed this error was
that there wasn't an unwind-protect around it, so not only would Emacs
not switch to a new tab, but my window paramters for the current tab
would be all messed up.

> What does display-buffer do in this case? reuse the same window
> regardless of any action alists?

Huh, that is a gooood question. I will look into what #'display-buffer
does.

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Benson Chu <bensonchu457@fastmail.com>
>> Cc: juri@linkov.net, 62427@debbugs.gnu.org
>> Date: Tue, 28 Mar 2023 11:17:29 -0500
>>
>> > Can't we create a completely new window and show the buffer in it?
>>
>> I'm not sure how easy this is. Typically new windows come from calls to
>> #'split-window, and you can't do that for a side-window.
>
> What does display-buffer do in this case? reuse the same window
> regardless of any action alists?
>
>> >                            I'm very uncomfortable with removing window
>> > parameters like that.  These windows don't belong to us, right?
>>
>> Let me know if this is wrong, but I am interpreting this statement as:
>>
>> "We shouldn't be modifying the window parameters of the windows that
>> belong to the old tab."
>
> There doesn't have to be any "old tab".  AFAIU, this option's default
> value is t, so even when you create the first tab, the code you
> suggest changing will run and mess with the window parameters of the
> windows that happen to exist at that point.  Right?
>
>> Because if that's the case, we're not /really/ modifying the old tab's
>> window-parameters. They're only "removed" "temporarily", for the
>> purposes of creating the new tab. You can see right before we modify any
>> window parameters, we make a call to (tab-bar--tab), which returns a tab
>> data structure, which contain a representation of the current wconf
>> (window configuration) - effectively saving the wconf - including the
>> paramaters. It's kind of like a save excursion:
>>
>> (let ((old-tab-num (tab-bar--current-tab-index tabs))
>>       (old-window-configuration (tab-bar--tab)))
>>   ;; modify window-parameters
>>   ;; do appropriate splitting
>>   ;; Now we have the window layout for the new tab
>>
>>   ;; The old tab should have the old-window-configuration
>>   (setf (nth old-tab-num tabs) old-window-configuration)
>>
>>   ;; The rest of the function
>>   )
>
> Is this inside unwind-protect, so that any error or quit or throw is
> caught and the parameters restored?  If so, it might be semi-okay.
>
>> So, we capture the current window configuration at the start of the
>> function, transition the current window configuration into the window
>> configuration for the new tab (which involves mangling window
>> parameters and destroying windows), then we make sure that the old tab
>> has an unmodified window-configuration.
>
> Ugh!  That is _soooo_ inelegant...  Also inefficient, and quite
> fragile (as any code which uses seve/restore window-configuration is).
> But I guess that ship has sailed long ago.
>
>> When the user switches back to the tab they left, all the
>> window-parameters are still present, untouched. Are you still
>> uncomfortable with doing things this way?
>
> What happens with all kinds of hooks that run due to this saving and
> restoring window-configuration? they still will see windows without
> their parameters, right?

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

> After thinking about this, I'm very uncomfortable with removing window
> parameters like that.  These windows don't belong to us, right?  They
> are windows that just happen to be there when the user creates a new
> tab.  So arbitrary removal of their parameters behind the back of the
> user and possibly some Lisp program which set these parameters is not
> TRT.
>
> Can't we create a completely new window and show the buffer in it?
> That new window then can have any parameters we want, since it's new.
>
> Or am I missing something?

Sometimes the window configuration gets into such inconsistent state
that the minibuffer window remains on the window list while the
minibuffer is not active.  Then creating a new tab fails with
such error:

Debugger entered--Lisp error: (error "Specified root is not an ancestor of specified window")
  delete-other-windows-internal(#<window 4 on  *Minibuf-1*> #<window 5 on *scratch*>)
  delete-other-windows()
  tab-bar-new-tab-to()
  tab-bar-new-tab()

Please fix this bug as well.

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Benson Chu]

>                            Then creating a new tab fails with
> such error:
>
> Debugger entered--Lisp error: (error "Specified root is not an ancestor of specified window")

Sure, I would be happy to investigate the problem! Is there a way to
reproduce this bug? I've personally never seen it before.

Juri Linkov <juri@linkov.net> writes:

>> After thinking about this, I'm very uncomfortable with removing window
>> parameters like that.  These windows don't belong to us, right?  They
>> are windows that just happen to be there when the user creates a new
>> tab.  So arbitrary removal of their parameters behind the back of the
>> user and possibly some Lisp program which set these parameters is not
>> TRT.
>>
>> Can't we create a completely new window and show the buffer in it?
>> That new window then can have any parameters we want, since it's new.
>>
>> Or am I missing something?
>
> Sometimes the window configuration gets into such inconsistent state
> that the minibuffer window remains on the window list while the
> minibuffer is not active.  Then creating a new tab fails with
> such error:
>
> Debugger entered--Lisp error: (error "Specified root is not an ancestor of specified window")
>   delete-other-windows-internal(#<window 4 on  *Minibuf-1*> #<window 5 on *scratch*>)
>   delete-other-windows()
>   tab-bar-new-tab-to()
>   tab-bar-new-tab()
>
> Please fix this bug as well.

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

>> Then creating a new tab fails with such error:
>>
>> Debugger entered--Lisp error: (error "Specified root is not an ancestor of specified window")
>
> Sure, I would be happy to investigate the problem! Is there a way to
> reproduce this bug? I've personally never seen it before.

Sorry, this bug is not reproducible.  It comes out of the blue.
And has such consequences that there is no recursive edit,
but you still can switch to the minibuffer window that has no prompt
with 'C-x o'.

>> Sometimes the window configuration gets into such inconsistent state
>> that the minibuffer window remains on the window list while the
>> minibuffer is not active.  Then creating a new tab fails with
>> such error:
>>
>> Debugger entered--Lisp error: (error "Specified root is not an ancestor of specified window")
>>   delete-other-windows-internal(#<window 4 on  *Minibuf-1*> #<window 5 on *scratch*>)
>>   delete-other-windows()
>>   tab-bar-new-tab-to()
>>   tab-bar-new-tab()

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

>>> Debugger entered--Lisp error: (error "Specified root is not an ancestor of specified window")
>>
>> Sure, I would be happy to investigate the problem! Is there a way to
>> reproduce this bug? I've personally never seen it before.
>
> Sorry, this bug is not reproducible.  It comes out of the blue.
> And has such consequences that there is no recursive edit,
> but you still can switch to the minibuffer window that has no prompt
> with 'C-x o'.

Ok, now here is 100% reproducible case:

0. emacs-29 -Q
1. (setq debug-on-error t enable-recursive-minibuffers t)
2. M-:   (or any other prompt like 'M-x')
3. Resize the minibuffer window e.g. with the mouse to at least 4 lines high
4. M-x windmove-swap-states-up RET
5. Click on the bottom window
6. C-x t 2
7. Click on the bottom window
8. C-x t 2

Debugger entered--Lisp error: (error "Specified root is not an ancestor of specified win...")
  delete-other-windows-internal(#<window 4 on *scratch*> #<window 7 on  *Minibuf-1*>)
  delete-other-windows()
  tab-bar-new-tab-to()
  tab-new(nil)

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

> 1. (setq debug-on-error t enable-recursive-minibuffers t)
> 2. M-:   (or any other prompt like 'M-x')
> 3. Resize the minibuffer window e.g. with the mouse to at least 4 lines high
> 4. M-x windmove-swap-states-up RET
> 5. Click on the bottom window
> 6. C-x t 2
> 7. Click on the bottom window
> 8. C-x t 2
>
> Debugger entered--Lisp error: (error "Specified root is not an ancestor of specified win...")
>   delete-other-windows-internal(#<window 4 on *scratch*> #<window 7 on  *Minibuf-1*>)
>   delete-other-windows()
>   tab-bar-new-tab-to()
>   tab-new(nil)

Maybe the simplest fix is just not to create such a broken
window configuration:

diff --git a/lisp/windmove.el b/lisp/windmove.el
index 06ce16c0d42..57511291588 100644
--- a/lisp/windmove.el
+++ b/lisp/windmove.el
@@ -724,6 +724,8 @@ windmove-swap-states-in-direction
                                            nil windmove-wrap-around 'nomini)))
     (cond ((or (null other-window) (window-minibuffer-p other-window))
            (user-error "No window %s from selected window" dir))
+          ((window-minibuffer-p (selected-window))
+           (user-error "Selected window is the minibuffer"))
           (t
            (window-swap-states nil other-window)))))

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Benson Chu]

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

I've done some digging into whether we can mimic OR use display-buffer
when creating a new tab, and while we could do this, we also advertise
two other behaviors for tab-bar-new-tab-choice that would make the
function needlessly complex, as use/mimicry of display-buffer would only
help in the case of when tab-bar-new-tab-choice is t.

On a side-note (side-window? haha), I've got a patch that side-steps
(hehe) the issue of modifying the 'window-side parameter completely, by
binding to window--sides-inhibit-check to t.

Having bound this, the issues wrt split-window and delete-other-windows
is no longer present, and thus we no longer have to touch the
'window-side parameter at all.

The same can't be said for the 'window-atom parameter (there's no
variable to bind to ignore checks), so it still needs to be removed in
order to ensure a safe call to delete-other-windows and split-window.

Would this be a better patch? It's now less destructive on existing
window parameters.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-tab-bar-new-tab-inhibit-side-window-checks.patch --]
[-- Type: text/x-patch, Size: 1947 bytes --]

From 2eaeea1f815b5e6c75947e9373c1b2f8fc539344 Mon Sep 17 00:00:00 2001
From: Benson Chu <bensonchu457@gmail.com>
Date: Fri, 24 Mar 2023 15:38:03 -0500
Subject: [PATCH] ; tab-bar-new-tab inhibit side-window checks

; Previously, calling tab-bar-new-tab-to only removes the 'window-side
; property on the currently selected window, and then a call to
; delete-other-windows was made to ensure that the selected window was
; the only window. We can skip this check by shadowing
; window--sides-inhibit-check to t.

* lisp/tab-bar.el (tab-bar-new-tab-to): inhibit side-window checks

Copyright-paperwork-exempt: yes
---
 lisp/tab-bar.el | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/lisp/tab-bar.el b/lisp/tab-bar.el
index dce6fa735fc..c7983146bf9 100644
--- a/lisp/tab-bar.el
+++ b/lisp/tab-bar.el
@@ -1552,15 +1552,14 @@ tab-bar-new-tab-to
       ;; Handle the case when it's called in the active minibuffer.
       (when (minibuffer-selected-window)
         (select-window (minibuffer-selected-window)))
-      ;; Remove window parameters that can cause problems
-      ;; with `delete-other-windows' and `split-window'.
-      (unless (eq tab-bar-new-tab-choice 'clone)
-        (set-window-parameter nil 'window-atom nil)
-        (set-window-parameter nil 'window-side nil))
-      (let ((ignore-window-parameters t))
+      (let ((ignore-window-parameters t)
+            (window--sides-inhibit-check t))
         (if (eq tab-bar-new-tab-choice 'clone)
             ;; Create new unique windows with the same layout
             (window-state-put (window-state-get))
+          ;; Remove window parameters that can cause problems
+          ;; with `delete-other-windows' and `split-window'.
+          (set-window-parameter nil 'window-atom nil)
           (delete-other-windows)
           (if (eq tab-bar-new-tab-choice 'window)
               ;; Create new unique window from remaining window
-- 
2.40.0


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


Juri Linkov <juri@linkov.net> writes:

>> 1. (setq debug-on-error t enable-recursive-minibuffers t)
>> 2. M-:   (or any other prompt like 'M-x')
>> 3. Resize the minibuffer window e.g. with the mouse to at least 4 lines high
>> 4. M-x windmove-swap-states-up RET
>> 5. Click on the bottom window
>> 6. C-x t 2
>> 7. Click on the bottom window
>> 8. C-x t 2
>>
>> Debugger entered--Lisp error: (error "Specified root is not an ancestor of specified win...")
>>   delete-other-windows-internal(#<window 4 on *scratch*> #<window 7 on  *Minibuf-1*>)
>>   delete-other-windows()
>>   tab-bar-new-tab-to()
>>   tab-new(nil)
>
> Maybe the simplest fix is just not to create such a broken
> window configuration:
>
> diff --git a/lisp/windmove.el b/lisp/windmove.el
> index 06ce16c0d42..57511291588 100644
> --- a/lisp/windmove.el
> +++ b/lisp/windmove.el
> @@ -724,6 +724,8 @@ windmove-swap-states-in-direction
>                                             nil windmove-wrap-around 'nomini)))
>      (cond ((or (null other-window) (window-minibuffer-p other-window))
>             (user-error "No window %s from selected window" dir))
> +          ((window-minibuffer-p (selected-window))
> +           (user-error "Selected window is the minibuffer"))
>            (t
>             (window-swap-states nil other-window)))))

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

> On a side-note (side-window? haha), I've got a patch that side-steps
> (hehe) the issue of modifying the 'window-side parameter completely, by
> binding to window--sides-inhibit-check to t.

Thanks, window--sides-inhibit-check is a good find.
Let me test it for a while.

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

>> On a side-note (side-window? haha), I've got a patch that side-steps
>> (hehe) the issue of modifying the 'window-side parameter completely, by
>> binding to window--sides-inhibit-check to t.
>
> Thanks, window--sides-inhibit-check is a good find.
> Let me test it for a while.

I still can't find a test case that could be fixed by
window--sides-inhibit-check.  Your original test case is fixed
because you removed (set-window-parameter nil 'window-side nil).
And the test case in bug#53662 doesn't need this line because
it was fixed by adding (ignore-window-parameters t).

But maybe there are cases where window--sides-inhibit-check
would help, so we could add it as a precaution.

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  62427@debbugs.gnu.org
> Date: Tue, 18 Apr 2023 09:58:10 +0300
> 
> >> On a side-note (side-window? haha), I've got a patch that side-steps
> >> (hehe) the issue of modifying the 'window-side parameter completely, by
> >> binding to window--sides-inhibit-check to t.
> >
> > Thanks, window--sides-inhibit-check is a good find.
> > Let me test it for a while.
> 
> I still can't find a test case that could be fixed by
> window--sides-inhibit-check.  Your original test case is fixed
> because you removed (set-window-parameter nil 'window-side nil).
> And the test case in bug#53662 doesn't need this line because
> it was fixed by adding (ignore-window-parameters t).
> 
> But maybe there are cases where window--sides-inhibit-check
> would help, so we could add it as a precaution.

Should I install the last patch, or should I wait for more testing?

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

>> >> On a side-note (side-window? haha), I've got a patch that side-steps
>> >> (hehe) the issue of modifying the 'window-side parameter completely, by
>> >> binding to window--sides-inhibit-check to t.
>> >
>> > Thanks, window--sides-inhibit-check is a good find.
>> > Let me test it for a while.
>>
>> I still can't find a test case that could be fixed by
>> window--sides-inhibit-check.  Your original test case is fixed
>> because you removed (set-window-parameter nil 'window-side nil).
>> And the test case in bug#53662 doesn't need this line because
>> it was fixed by adding (ignore-window-parameters t).
>>
>> But maybe there are cases where window--sides-inhibit-check
>> would help, so we could add it as a precaution.
>
> Should I install the last patch, or should I wait for more testing?

The patch works in two known test cases, and no more tests were found,
so probably it could be installed.

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: bensonchu457@fastmail.com,  62427@debbugs.gnu.org
> Date: Sun, 23 Apr 2023 19:39:10 +0300
> 
> >> >> On a side-note (side-window? haha), I've got a patch that side-steps
> >> >> (hehe) the issue of modifying the 'window-side parameter completely, by
> >> >> binding to window--sides-inhibit-check to t.
> >> >
> >> > Thanks, window--sides-inhibit-check is a good find.
> >> > Let me test it for a while.
> >>
> >> I still can't find a test case that could be fixed by
> >> window--sides-inhibit-check.  Your original test case is fixed
> >> because you removed (set-window-parameter nil 'window-side nil).
> >> And the test case in bug#53662 doesn't need this line because
> >> it was fixed by adding (ignore-window-parameters t).
> >>
> >> But maybe there are cases where window--sides-inhibit-check
> >> would help, so we could add it as a precaution.
> >
> > Should I install the last patch, or should I wait for more testing?
> 
> The patch works in two known test cases, and no more tests were found,
> so probably it could be installed.

Thanks, installed on emacs-29 branch, and closing the bug.

Benson, any news on your copyright assignment?

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

> Thanks, installed on emacs-29 branch, and closing the bug.

From the docstring of 'tab-bar-new-tab-choice':

  If the value is the symbol `window', then keep the selected
  window as a single window on the new tab, and keep all its
  window parameters except `window-atom' and `window-side'.
                                              ===========

So still need to remove `window-side' in this case:

(progn
  (display-buffer-in-side-window (get-buffer-create "*hello*")
                                 '((side . right)
                                   (slot . 1)))
  (select-window
   (display-buffer-in-side-window (get-buffer-create "*world*")
                                  '((side . right)
                                    (slot . 2))))
  (tab-bar-move-window-to-tab)
  (split-window))

This should handle it:

diff --git a/lisp/tab-bar.el b/lisp/tab-bar.el
index 61e177c051d..42fc5a23990 100644
--- a/lisp/tab-bar.el
+++ b/lisp/tab-bar.el
@@ -1588,7 +1588,9 @@ tab-bar-new-tab-to
           (delete-other-windows)
           (if (eq tab-bar-new-tab-choice 'window)
               ;; Create new unique window from remaining window
-              (window-state-put (window-state-get))
+              (progn
+                (set-window-parameter nil 'window-side nil)
+                (window-state-put (window-state-get)))
             ;; Create a new window to get rid of old window parameters
             ;; (e.g. prev/next buffers) of old window.
             (split-window) (delete-window))))

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

>> 1. (setq debug-on-error t enable-recursive-minibuffers t)
>> 2. M-:   (or any other prompt like 'M-x')
>> 3. Resize the minibuffer window e.g. with the mouse to at least 4 lines high
>> 4. M-x windmove-swap-states-up RET
>> 5. Click on the bottom window
>> 6. C-x t 2
>> 7. Click on the bottom window
>> 8. C-x t 2
>>
>> Debugger entered--Lisp error: (error "Specified root is not an ancestor of specified win...")
>>   delete-other-windows-internal(#<window 4 on *scratch*> #<window 7 on  *Minibuf-1*>)
>>   delete-other-windows()
>>   tab-bar-new-tab-to()
>>   tab-new(nil)
>
> Maybe the simplest fix is just not to create such a broken
> window configuration:
>
> diff --git a/lisp/windmove.el b/lisp/windmove.el
> index 06ce16c0d42..57511291588 100644
> --- a/lisp/windmove.el
> +++ b/lisp/windmove.el
> @@ -724,6 +724,8 @@ windmove-swap-states-in-direction
>                                             nil windmove-wrap-around 'nomini)))
>      (cond ((or (null other-window) (window-minibuffer-p other-window))
>             (user-error "No window %s from selected window" dir))
> +          ((window-minibuffer-p (selected-window))
> +           (user-error "Selected window is the minibuffer"))
>            (t
>             (window-swap-states nil other-window)))))

This is another patch that could be installed either to emacs-29 or master.

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

>> Thanks, installed on emacs-29 branch, and closing the bug.
>
> From the docstring of 'tab-bar-new-tab-choice':
>
>   If the value is the symbol `window', then keep the selected
>   window as a single window on the new tab, and keep all its
>   window parameters except `window-atom' and `window-side'.
>                                               ===========
>
> So still need to remove `window-side' in this case:
>
> diff --git a/lisp/tab-bar.el b/lisp/tab-bar.el
> index 61e177c051d..42fc5a23990 100644
> --- a/lisp/tab-bar.el
> +++ b/lisp/tab-bar.el
> @@ -1588,7 +1588,9 @@ tab-bar-new-tab-to
>            (delete-other-windows)
>            (if (eq tab-bar-new-tab-choice 'window)
>                ;; Create new unique window from remaining window
> -              (window-state-put (window-state-get))
> +              (progn
> +                (set-window-parameter nil 'window-side nil)
> +                (window-state-put (window-state-get)))
>              ;; Create a new window to get rid of old window parameters
>              ;; (e.g. prev/next buffers) of old window.
>              (split-window) (delete-window))))

Now installed on emacs-29 branch.

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

>>> 1. (setq debug-on-error t enable-recursive-minibuffers t)
>>> 2. M-:   (or any other prompt like 'M-x')
>>> 3. Resize the minibuffer window e.g. with the mouse to at least 4 lines high
>>> 4. M-x windmove-swap-states-up RET
>>> 5. Click on the bottom window
>>> 6. C-x t 2
>>> 7. Click on the bottom window
>>> 8. C-x t 2
>>>
>>> Debugger entered--Lisp error: (error "Specified root is not an ancestor of specified win...")
>>>   delete-other-windows-internal(#<window 4 on *scratch*> #<window 7 on  *Minibuf-1*>)
>>>   delete-other-windows()
>>>   tab-bar-new-tab-to()
>>>   tab-new(nil)
>>
>> Maybe the simplest fix is just not to create such a broken
>> window configuration:
>>
>> diff --git a/lisp/windmove.el b/lisp/windmove.el
>> index 06ce16c0d42..57511291588 100644
>> --- a/lisp/windmove.el
>> +++ b/lisp/windmove.el
>> @@ -724,6 +724,8 @@ windmove-swap-states-in-direction
>>                                             nil windmove-wrap-around 'nomini)))
>>      (cond ((or (null other-window) (window-minibuffer-p other-window))
>>             (user-error "No window %s from selected window" dir))
>> +          ((window-minibuffer-p (selected-window))
>> +           (user-error "Selected window is the minibuffer"))
>>            (t
>>             (window-swap-states nil other-window)))))
>
> This is another patch that could be installed either to emacs-29 or master.

This guard is pushed to master.  But the root problem still persists.
It can be reproduced in emacs-28/29/30:

0. emacs -Q
1. M-x
2. C-x t 2
3. C-x o     ;; switches to the minibuffer
4. C-x t 2

Debugger entered--Lisp error: (error "Specified root is not an ancestor of specified win...")
  delete-other-windows-internal(#<window 4 on  *Minibuf-1*> #<window 8 on *scratch*>)
  delete-other-windows()
  tab-bar-new-tab-to()
  tab-new(nil)
  funcall-interactively(tab-new nil)
  command-execute(tab-new)

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

> But the root problem still persists.
> It can be reproduced in emacs-28/29/30:
>
> 0. emacs -Q
> 1. M-x
> 2. C-x t 2
> 3. C-x o     ;; switches to the minibuffer
> 4. C-x t 2
>
> Debugger entered--Lisp error: (error "Specified root is not an ancestor of specified win...")
>   delete-other-windows-internal(#<window 4 on  *Minibuf-1*> #<window 8 on *scratch*>)
>   delete-other-windows()
>   tab-bar-new-tab-to()
>   tab-new(nil)
>   funcall-interactively(tab-new nil)
>   command-execute(tab-new)

Martin, could you please help to debug this problem.

Does the error occur because the window configuration is broken?

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Date: Tue, 16 May 2023 20:32:56 +0300
> 
> This guard is pushed to master.  But the root problem still persists.
> It can be reproduced in emacs-28/29/30:
> 
> 0. emacs -Q
> 1. M-x
> 2. C-x t 2
> 3. C-x o     ;; switches to the minibuffer
> 4. C-x t 2

Why does the last command in this scenario make sense?

The below also signals an error:

  emacs -Q
  M-x
  C-x 2

Looks appropriate to me.

If you are arguing for a better error message in your case, I'm okay
with diagnosing that specific situation and emitting a more focused,
user-friendly error.

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[martin rudalics]

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

 >> 0. emacs -Q
 >> 1. M-x
 >> 2. C-x t 2
 >> 3. C-x o     ;; switches to the minibuffer
 >> 4. C-x t 2
 >>
 >> Debugger entered--Lisp error: (error "Specified root is not an ancestor of specified win...")
 >>    delete-other-windows-internal(#<window 4 on  *Minibuf-1*> #<window 8 on *scratch*>)
 >>    delete-other-windows()
 >>    tab-bar-new-tab-to()
 >>    tab-new(nil)
 >>    funcall-interactively(tab-new nil)
 >>    command-execute(tab-new)
 >
 > Martin, could you please help to debug this problem.
 >
 > Does the error occur because the window configuration is broken?

No.  Binding 'ignore-window-parameters' to t skips the usual error
message that you "Can't expand minibuffer to full frame".  Does the
attached patch fix it?

martin

[-- Attachment #2: delete-other-windows.diff --]
[-- Type: text/x-patch, Size: 1742 bytes --]

diff --git a/lisp/window.el b/lisp/window.el
index 016d53ffbdd..5994b6816ef 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -4356,10 +4356,7 @@ delete-other-windows
 	    (error "Root of atomic window is root window of its frame")
 	  (throw 'done (delete-other-windows atom-root))))
        ((window-parameter window 'window-side)
-	(error "Cannot make side window the only window"))
-       ((and (window-minibuffer-p window)
-	     (not (eq window (frame-root-window window))))
-	(error "Can't expand minibuffer to full frame")))
+	(error "Cannot make side window the only window")))
 
       (cond
        ((or ignore-window-parameters
@@ -4395,15 +4392,21 @@ delete-other-windows
               (error nil))))
         (throw 'done nil)))
 
-      ;; If WINDOW is the main window of its frame do nothing.
-      (if (eq window main)
-          ;; Give a message to the user if this has been called as a
-          ;; command.
-          (when (and interactive
-                     (not (or executing-kbd-macro noninteractive)))
-            (message "No other windows to delete"))
+      (cond
+       ;; If WINDOW is the main window of its frame do nothing.
+       ((eq window main)
+        ;; Give a message to the user if this has been called as a
+        ;; command.
+        (when (and interactive
+                   (not (or executing-kbd-macro noninteractive)))
+          (message "No other windows to delete")))
+       ((and (window-minibuffer-p window)
+	     (not (eq window (frame-root-window window))))
+	(user-error "Can't expand minibuffer to full frame"))
+       (t
 	(delete-other-windows-internal window main)
-	(window--check frame))
+	(window--check frame)))
+
       ;; Always return nil.
       nil)))
 

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

>> 0. emacs -Q
>> 1. M-x
>> 2. C-x t 2
>> 3. C-x o     ;; switches to the minibuffer
>> 4. C-x t 2
>
> Why does the last command in this scenario make sense?

Because the same works in the step 2 above.

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

>>> 1. M-x
>>> 2. C-x t 2
>>> 3. C-x o     ;; switches to the minibuffer
>>> 4. C-x t 2
>>>
>>> Debugger entered--Lisp error: (error "Specified root is not an ancestor of specified win...")
>>>    delete-other-windows-internal(#<window 4 on  *Minibuf-1*> #<window 8 on *scratch*>)
>>>    delete-other-windows()
>>>    tab-bar-new-tab-to()
>>>    tab-new(nil)
>>>    funcall-interactively(tab-new nil)
>>>    command-execute(tab-new)
>>
>> Does the error occur because the window configuration is broken?
>
> No.  Binding 'ignore-window-parameters' to t skips the usual error
> message that you "Can't expand minibuffer to full frame".  Does the
> attached patch fix it?

This is just a better error message.

After 'M-x C-x t 2', 'minibuffer-selected-window' returns non-nil that
is used in 'tab-bar-new-tab-to' before calling 'delete-other-windows':

      ;; Handle the case when it's called in the active minibuffer.
      (when (minibuffer-selected-window)
        (select-window (minibuffer-selected-window)))

The problem is that after 'M-x C-x t 2 C-x o C-x t 2',
'minibuffer-selected-window' returns nil while
the current buffer is the minibuffer.

This is because after the first 'C-x t 2', a new window is created by:

            ;; Create a new window to get rid of old window parameters
            ;; (e.g. prev/next buffers) of old window.
            (split-window) (delete-window)

so 'minibuffer-selected-window' refers to the deleted window,
therefore nil.

Would it be possible to update 'minibuf_selected_window'
with a new window after deleting the original window?

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> Cc: 62427@debbugs.gnu.org
> Date: Wed, 17 May 2023 19:32:36 +0300
> 
> >> 0. emacs -Q
> >> 1. M-x
> >> 2. C-x t 2
> >> 3. C-x o     ;; switches to the minibuffer
> >> 4. C-x t 2
> >
> > Why does the last command in this scenario make sense?
> 
> Because the same works in the step 2 above.

In step 2 we are not in the minibuffer.

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[martin rudalics]

 > Would it be possible to update 'minibuf_selected_window'
 > with a new window after deleting the original window?

Not really.  We'd change existing behavior that might rely on the fact
that 'minibuffer-selected-window' returns nil once it got deleted.

Besides, it might not make you happy anyway.  With the default of
'minibuffer-follows-selected-frame', a user may correspond with a
minibuffer window that resides on another frame than that returned by
'minibuffer-selected-window' and neither minibuf_selected_window nor
Vminibuf_scroll_window may make sense any more in such context.

Why don't you use 'get-mru-window' on the frame that owns the
'minibuffer-window' instead of 'minibuffer-selected-window'?

martin

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

>> Would it be possible to update 'minibuf_selected_window'
>> with a new window after deleting the original window?
>
> Not really.  We'd change existing behavior that might rely on the fact
> that 'minibuffer-selected-window' returns nil once it got deleted.
>
> Besides, it might not make you happy anyway.  With the default of
> 'minibuffer-follows-selected-frame', a user may correspond with a
> minibuffer window that resides on another frame than that returned by
> 'minibuffer-selected-window' and neither minibuf_selected_window nor
> Vminibuf_scroll_window may make sense any more in such context.
>
> Why don't you use 'get-mru-window' on the frame that owns the
> 'minibuffer-window' instead of 'minibuffer-selected-window'?

Thanks for the idea, this works perfectly:

diff --git a/lisp/tab-bar.el b/lisp/tab-bar.el
index 42fc5a23990..0ae7fb44b27 100644
--- a/lisp/tab-bar.el
+++ b/lisp/tab-bar.el
@@ -1339,8 +1340,7 @@ tab-bar-select-tab
 
          (ws
           ;; `window-state-put' fails when called in the minibuffer
-          (when (minibuffer-selected-window)
-            (select-window (minibuffer-selected-window)))
+          (when (minibufferp) (select-window (get-mru-window)))
           (window-state-put ws nil 'safe)))
 
         ;; Select the minibuffer when it was active before switching tabs
@@ -1575,8 +1575,7 @@ tab-bar-new-tab-to
 
     (when tab-bar-new-tab-choice
       ;; Handle the case when it's called in the active minibuffer.
-      (when (minibuffer-selected-window)
-        (select-window (minibuffer-selected-window)))
+      (when (minibufferp) (select-window (get-mru-window)))
       (let ((ignore-window-parameters t)
             (window--sides-inhibit-check t))
         (if (eq tab-bar-new-tab-choice 'clone)

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[martin rudalics]

 > +          (when (minibufferp) (select-window (get-mru-window)))

Here and below I'd use

(when (window-minibuffer-p)

because it more clearly demonstrates the idea and also handles the
(unlikely) case where a minibuffer is shown in a normal window.

 >             (window-state-put ws nil 'safe)))
 >
 >           ;; Select the minibuffer when it was active before switching tabs
 > @@ -1575,8 +1575,7 @@ tab-bar-new-tab-to
 >
 >       (when tab-bar-new-tab-choice
 >         ;; Handle the case when it's called in the active minibuffer.
 > -      (when (minibuffer-selected-window)
 > -        (select-window (minibuffer-selected-window)))
 > +      (when (minibufferp) (select-window (get-mru-window)))
 >         (let ((ignore-window-parameters t)
 >               (window--sides-inhibit-check t))
 >           (if (eq tab-bar-new-tab-choice 'clone)

You might want to try this with multiple frames (some of them without
their own minibuffer window) jumping from one frame to another within
the scope of 'read-from-minibuffer'.

martin

bug#62427: tab-bar-new-tab-to now handles cases with multiple side-windows

[Juri Linkov]

>> +          (when (minibufferp) (select-window (get-mru-window)))
>
> Here and below I'd use
>
> (when (window-minibuffer-p)
>
> because it more clearly demonstrates the idea and also handles the
> (unlikely) case where a minibuffer is shown in a normal window.

Thanks, now pushed to emacs-29.

>>             (window-state-put ws nil 'safe)))
>>
>>           ;; Select the minibuffer when it was active before switching tabs
>> @@ -1575,8 +1575,7 @@ tab-bar-new-tab-to
>>
>>       (when tab-bar-new-tab-choice
>>         ;; Handle the case when it's called in the active minibuffer.
>> -      (when (minibuffer-selected-window)
>> -        (select-window (minibuffer-selected-window)))
>> +      (when (minibufferp) (select-window (get-mru-window)))
>>         (let ((ignore-window-parameters t)
>>               (window--sides-inhibit-check t))
>>           (if (eq tab-bar-new-tab-choice 'clone)
>
> You might want to try this with multiple frames (some of them without
> their own minibuffer window) jumping from one frame to another within
> the scope of 'read-from-minibuffer'.

I confirm that this works correctly.