From 2342ea08e03185136324e906193d7ce2f523a292 Mon Sep 17 00:00:00 2001
From: toofar <toofar@spalge.com>
Date: Sun, 8 Sep 2024 17:27:44 +1200
Subject: [PATCH 01/12] Simplify :tab-close --recursive

This piece of code was traversing all node of the tab tree, including
hidden ones, and then it had special handling for closing hidden tabs.
`TreeTabbedBrowser._remove_tab()` already handles hidden tabs though so
there was some duplication.

This changes the tree traversal to only including visible tabs and then
calls the normal close method on each of them.

Potential follow ons:
* make `TabbedBrowser.tab_close_prompt_if_pinned()` pass `tab` to
  `yes_action()` so that we don't have to create a new partial function
  every time
* pass `recursive` down to `TabbedBrowser.close_tab()` and
  `TabbedBrowser._remove_tab()` so that callers don't have to go through
  the command interface to get this behavior.
---
 qutebrowser/browser/commands.py         | 35 +++++++++++--------------
 qutebrowser/misc/notree.py              |  4 +--
 tests/end2end/features/treetabs.feature | 25 ++++++++++++++++++
 3 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py
index 3b950602b..2ee905047 100644
--- a/qutebrowser/browser/commands.py
+++ b/qutebrowser/browser/commands.py
@@ -255,33 +255,28 @@ class CommandDispatcher:
             opposite: Force selecting the tab in the opposite direction of
                       what's configured in 'tabs.select_on_remove'.
             force: Avoid confirmation for pinned tabs.
-            recursive: Close all descendents (tree-tabs) as well as current tab
+            recursive: Close all descendants (tree-tabs) as well as current tab
             count: The tab index to close, or None
         """
         tab = self._cntwidget(count)
         tabbed_browser = self._tabbed_browser
         if tab is None:
             return
-        if (tabbed_browser.is_treetabbedbrowser and recursive and not
-                tab.node.collapsed):
-            # if collapsed, recursive is the same as normal close
-            new_undo = True  # only for first one
-            for descendent in tab.node.traverse(notree.TraverseOrder.POST_R,
-                                                True):
-                if self._tabbed_browser.widget.indexOf(descendent.value) > -1:
-                    close = functools.partial(self._tab_close,
-                                              descendent.value, prev, next_,
-                                              opposite, new_undo)
-                    tabbed_browser.tab_close_prompt_if_pinned(descendent.value, force,
-                                                              close)
-                    new_undo = False
-                else:
-                    tab = descendent.value
-                    tab.private_api.shutdown()
-                    tab.deleteLater()
+
+        if (recursive and tabbed_browser.is_treetabbedbrowser):
+            new_undo = True
+            for descendent in tab.node.traverse(
+                notree.TraverseOrder.POST_R,
+                render_collapsed=False,
+            ):
+                close = functools.partial(self._tab_close,
+                                          descendent.value, prev, next_,
+                                          opposite, new_undo)
+                tabbed_browser.tab_close_prompt_if_pinned(descendent.value, force,
+                                                          close)
+                new_undo = False
+
         else:
-            # this also applied to closing collapsed tabs
-            # logic for that is in TreeTabbedBrowser
             close = functools.partial(self._tab_close, tab, prev,
                                       next_, opposite)
             tabbed_browser.tab_close_prompt_if_pinned(tab, force, close)
diff --git a/qutebrowser/misc/notree.py b/qutebrowser/misc/notree.py
index 3fd3cfde3..318215cb7 100644
--- a/qutebrowser/misc/notree.py
+++ b/qutebrowser/misc/notree.py
@@ -228,13 +228,11 @@ class Node(Generic[T]):
 
     def traverse(self, order: TraverseOrder = TraverseOrder.PRE,
                  render_collapsed: bool = True) -> Iterable['Node']:
-        """Generator for all descendants of `self`.
+        """Generator for `self` and all descendants.
 
         Args:
             order: a TraverseOrder object. See TraverseOrder documentation.
             render_collapsed: whether to yield children of collapsed nodes
-            Even if render_collapsed is False, collapsed nodes are be rendered.
-            It's their children that won't.
         """
         if order == TraverseOrder.PRE:
             yield self
diff --git a/tests/end2end/features/treetabs.feature b/tests/end2end/features/treetabs.feature
index 1e8412092..dbc82c4f4 100644
--- a/tests/end2end/features/treetabs.feature
+++ b/tests/end2end/features/treetabs.feature
@@ -72,6 +72,31 @@ Feature: Tree tab management
             - data/numbers/4.txt
             """
 
+    Scenario: :tab-close --recursive with pinned tab
+        When I open data/numbers/1.txt
+        And I open data/numbers/2.txt in a new related tab
+        And I open data/numbers/3.txt in a new related tab
+        And I open data/numbers/4.txt in a new tab
+        And I run :tab-focus 1
+        And I run :cmd-run-with-count 2 tab-pin
+        And I run :tab-close --recursive
+        And I wait for "Asking question *" in the log
+        And I run :prompt-accept yes
+        Then the following tabs should be open:
+            - data/numbers/4.txt
+
+    Scenario: :tab-close --recursive with collapsed subtree
+        When I open data/numbers/1.txt
+        And I open data/numbers/2.txt in a new related tab
+        And I open data/numbers/3.txt in a new related tab
+        And I open data/numbers/4.txt in a new tab
+        And I run :tab-focus 2
+        And I run :tree-tab-toggle-hide
+        And I run :tab-focus 1
+        And I run :tab-close --recursive
+        Then the following tabs should be open:
+            - data/numbers/4.txt
+
     Scenario: Open a child tab
         When I open data/numbers/1.txt
         And I open data/numbers/2.txt in a new related tab

From ed58779690507a92cdb99ba7196d39ed27d2d1cb Mon Sep 17 00:00:00 2001
From: toofar <toofar@spalge.com>
Date: Sun, 8 Sep 2024 18:53:06 +1200
Subject: [PATCH 02/12] Move recursive tab-close code to treetabbedbrowser

This commit moves any tree tab specific behavior out of `tab_close()`
into TreeTabbedBrowser. I would like calling code not to have to deal
with tree specific logic if it doesn't have to. In this case all we
should need to do is tell the tabbed browser to close the tab and let
that handle the details. (An even better API might be to ask the tab to
close itself, maybe in the future.)

This makes `TabbedBrowser.close_tab()` more powerful but now some logic
is a bit awkwardly mixed between commands.py and treetabbedbrowser. Eg
`tab_close_prompt()` is now being called in two places.

One shortfall of this commit is that `--force` is no longer supported to
close pinned tabs without prompting. It should be able to be supported
by ... plumbing through yet another keyword argument! I'm not sure why
the tab selection logic is in commands.py, I feel like all of that
should be in TabbedBrowser too, and then the passed through kwargs would
hopefully be a bit cleaner because they wouldn't have to go through so
many layers.
---
 qutebrowser/browser/commands.py             | 26 +++++----------------
 qutebrowser/mainwindow/tabbedbrowser.py     | 14 ++++++++---
 qutebrowser/mainwindow/treetabbedbrowser.py | 23 +++++++++++++++++-
 3 files changed, 39 insertions(+), 24 deletions(-)

diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py
index 2ee905047..9bf4e580f 100644
--- a/qutebrowser/browser/commands.py
+++ b/qutebrowser/browser/commands.py
@@ -220,7 +220,7 @@ class CommandDispatcher:
         return None
 
     def _tab_close(self, tab, prev=False, next_=False,
-                   opposite=False, new_undo=True):
+                   opposite=False, new_undo=True, recursive=False):
         """Helper function for tab_close be able to handle message.async.
 
         Args:
@@ -236,11 +236,11 @@ class CommandDispatcher:
                                                           opposite)
 
         if selection_override is None:
-            self._tabbed_browser.close_tab(tab, new_undo=new_undo)
+            self._tabbed_browser.close_tab(tab, new_undo=new_undo, recursive=recursive)
         else:
             old_selection_behavior = tabbar.selectionBehaviorOnRemove()
             tabbar.setSelectionBehaviorOnRemove(selection_override)
-            self._tabbed_browser.close_tab(tab, new_undo=new_undo)
+            self._tabbed_browser.close_tab(tab, new_undo=new_undo, recursive=recursive)
             tabbar.setSelectionBehaviorOnRemove(old_selection_behavior)
 
     @cmdutils.register(instance='command-dispatcher', scope='window')
@@ -263,23 +263,9 @@ class CommandDispatcher:
         if tab is None:
             return
 
-        if (recursive and tabbed_browser.is_treetabbedbrowser):
-            new_undo = True
-            for descendent in tab.node.traverse(
-                notree.TraverseOrder.POST_R,
-                render_collapsed=False,
-            ):
-                close = functools.partial(self._tab_close,
-                                          descendent.value, prev, next_,
-                                          opposite, new_undo)
-                tabbed_browser.tab_close_prompt_if_pinned(descendent.value, force,
-                                                          close)
-                new_undo = False
-
-        else:
-            close = functools.partial(self._tab_close, tab, prev,
-                                      next_, opposite)
-            tabbed_browser.tab_close_prompt_if_pinned(tab, force, close)
+        close = functools.partial(self._tab_close, tab, prev,
+                                  next_, opposite, True, recursive)
+        tabbed_browser.tab_close_prompt_if_pinned(tab, force, close)
 
     @cmdutils.register(instance='command-dispatcher', scope='window',
                        name='tab-pin')
diff --git a/qutebrowser/mainwindow/tabbedbrowser.py b/qutebrowser/mainwindow/tabbedbrowser.py
index f20a59c60..836e1a7de 100644
--- a/qutebrowser/mainwindow/tabbedbrowser.py
+++ b/qutebrowser/mainwindow/tabbedbrowser.py
@@ -488,7 +488,7 @@ class TabbedBrowser(QWidget):
         else:
             yes_action()
 
-    def close_tab(self, tab, *, add_undo=True, new_undo=True, transfer=False):
+    def close_tab(self, tab, *, add_undo=True, new_undo=True, transfer=False, recursive=False):
         """Close a tab.
 
         Args:
@@ -507,7 +507,7 @@ class TabbedBrowser(QWidget):
         if last_close == 'ignore' and count == 1:
             return
 
-        self._remove_tab(tab, add_undo=add_undo, new_undo=new_undo)
+        self._remove_tab(tab, add_undo=add_undo, new_undo=new_undo, recursive=recursive)
 
         if count == 1:  # We just closed the last tab above.
             if last_close == 'close':
@@ -520,7 +520,15 @@ class TabbedBrowser(QWidget):
             elif last_close == 'default-page':
                 self.load_url(config.val.url.default_page, newtab=True)
 
-    def _remove_tab(self, tab, *, add_undo=True, new_undo=True, crashed=False):
+    def _remove_tab(
+        self,
+        tab,
+        *,
+        add_undo=True,
+        new_undo=True,
+        crashed=False,
+        recursive=False,  # pylint: disable=unused-argument
+    ):
         """Remove a tab from the tab list and delete it properly.
 
         Args:
diff --git a/qutebrowser/mainwindow/treetabbedbrowser.py b/qutebrowser/mainwindow/treetabbedbrowser.py
index e46abd14e..31c2b6996 100644
--- a/qutebrowser/mainwindow/treetabbedbrowser.py
+++ b/qutebrowser/mainwindow/treetabbedbrowser.py
@@ -6,6 +6,7 @@
 
 import collections
 import dataclasses
+import functools
 from typing import Union
 from qutebrowser.qt.core import pyqtSlot, QUrl
 
@@ -124,11 +125,31 @@ class TreeTabbedBrowser(TabbedBrowser):
         """Return the tab widget that can display a tree structure."""
         return TreeTabWidget(self._win_id, parent=self)
 
-    def _remove_tab(self, tab, *, add_undo=True, new_undo=True, crashed=False):
+    def _remove_tab(self, tab, *, add_undo=True, new_undo=True, crashed=False, recursive=False):
         """Handle children positioning after a tab is removed."""
         if not tab.url().isEmpty() and tab.url().isValid() and add_undo:
             self._add_undo_entry(tab, new_undo)
 
+        if recursive:
+            for descendent in tab.node.traverse(
+                order=notree.TraverseOrder.POST_R,
+                render_collapsed=False
+            ):
+                self.tab_close_prompt_if_pinned(
+                    descendent.value,
+                    False,
+                    functools.partial(
+                        self._remove_tab,
+                        descendent.value,
+                        add_undo=add_undo,
+                        new_undo=new_undo,
+                        crashed=crashed,
+                        recursive=False,
+                    )
+                )
+                new_undo = False
+            return
+
         node = tab.node
         parent = node.parent
 

From 6b706836542aebf7f3090066c19d7be1b897c2fb Mon Sep 17 00:00:00 2001
From: toofar <toofar@spalge.com>
Date: Sun, 15 Sep 2024 11:51:05 +1200
Subject: [PATCH 03/12] Support fixed index in `TreeTabbedBrowser.tabopen()`

There are various settings that the user can change to dictate where a
new tab will be opened (eg, first, next, last, prev). The non-tree
tabbed browser supports passing in an index to `tabopen()` to bypass
those settings and use absolute positioning for new tabs. This is used
for example when undoing a tab close or loading a session. So far for
tree tab enabled windows callers have had to manually manipulate the
tree to get absolute positioning. This is an attempt to provide absolute
positing with the existing interface, at least as much as we can.

How it works is that we still use the `related` and `sibling` flags to
tell where in the tree hierarchy the tab should be placed (represented
by selecting the new parent node in the code) but then use the index
passed in to figure out where the tab should go in the list of siblings.
We find the tab bar index of the sibling tabs and try to place the index
passed in relative to them.

This is a very loose interpretation of "absolute" positioning. We are
relying on the position of nodes in the tree having the same ordering as
the tabs in tha tab bar, which
`TreeTabWidget.update_tree_tab_positions()` tries to ensure, and I've
added an assert as a bonus. Callers could also just pass in `0` or
`math.inf` to force the tabs to the first
or last positions without having to mess with the current config.

Seems to be working for the test suite for now. We can always remove it
again if gaps in the logic come up.
---
 qutebrowser/mainwindow/treetabbedbrowser.py | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/qutebrowser/mainwindow/treetabbedbrowser.py b/qutebrowser/mainwindow/treetabbedbrowser.py
index 31c2b6996..b46087690 100644
--- a/qutebrowser/mainwindow/treetabbedbrowser.py
+++ b/qutebrowser/mainwindow/treetabbedbrowser.py
@@ -283,7 +283,8 @@ class TreeTabbedBrowser(TabbedBrowser):
             pos = config.val.tabs.new_position.tree.new_toplevel
             parent = self.widget.tree_root
 
-        self._position_tab(cur_tab.node, tab.node, pos, parent, sibling, related, background)
+        self._position_tab(cur_tab.node, tab.node, pos, parent, sibling,
+                           related, background, idx)
 
         return tab
 
@@ -296,6 +297,7 @@ class TreeTabbedBrowser(TabbedBrowser):
         sibling: bool = False,
         related: bool = True,
         background: bool = None,
+        idx: int = None,
     ) -> None:
         toplevel = not sibling and not related
         siblings = list(parent.children)
@@ -304,7 +306,14 @@ class TreeTabbedBrowser(TabbedBrowser):
             # potentially adding it as a duplicate later.
             siblings.remove(new_node)
 
-        if pos == 'first':
+        if idx:
+            sibling_indices = [self.widget.indexOf(node.value) for node in siblings]
+            assert sibling_indices == sorted(sibling_indices)
+            sibling_indices.append(idx)
+            sibling_indices = sorted(sibling_indices)
+            rel_idx = sibling_indices.index(idx)
+            siblings.insert(rel_idx, new_node)
+        elif pos == 'first':
             rel_idx = 0
             if config.val.tabs.new_position.stacking and related:
                 rel_idx += self._tree_tab_child_rel_idx

From 1a43c8ff05cb4c2bc8aaa33ed968cf35468ad860 Mon Sep 17 00:00:00 2001
From: toofar <toofar@spalge.com>
Date: Sat, 14 Sep 2024 16:15:08 +1200
Subject: [PATCH 04/12] tab-give --recursive without direct tree manipulation

The existing `_tree_tab_give()` method manipulates the tree manually and
has a two setp process to create tabs and then position them.
I wanted to see if I could create and position tabs in a single step,
assuming it would be simpler.

This did not turn out as simple as I had hoped!
It was hoping I could iterate through the tabs to be given and open new
tabs in the destination window in the right order, then they would end
up in the right place and we would be done.

The complication is that I don't think there's any way to traverse the
tree and open new tabs without doing extra work to position tabs. In
this case we have to figure out when we go up and down levels in the
tree, so we can pass the right args to `tabopen()` and have the right
tab focused to have the new one opened relative to. Going down the tree
is pretty easy, going up the tree we have to figure out how far we went,
also not so bad as it turns out.

On balance I'm not sure this is a clear win. The new function is more
lines of code and needs heuristic to understand it's position in the tree
while traversing. On the plus side the new code maintains the collapsed
state of subtrees! For the previous code I tried to add that but it
seemed to blow up on the sanity check in update_tab_titles where it's
gets indexes that are out of range of the visible tabs.
---
 qutebrowser/browser/commands.py         | 57 ++++++++++++++++++++++++-
 tests/end2end/features/treetabs.feature | 18 ++++++++
 tests/unit/misc/test_notree.py          | 11 +++--
 3 files changed, 81 insertions(+), 5 deletions(-)

diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py
index 9bf4e580f..2e931b87e 100644
--- a/qutebrowser/browser/commands.py
+++ b/qutebrowser/browser/commands.py
@@ -466,6 +466,61 @@ class CommandDispatcher:
         if not keep:
             tabbed_browser.close_tab(tab, add_undo=False, transfer=True)
 
+    def _tree_tab_give_onepass(self, tabbed_browser, keep):
+        """Recursive tab-give, move current tab and children to tabbed_browser."""
+        seen_nodes = []
+        new_tabs = []
+        root_idx = None
+        for idx, node in enumerate(
+            self._current_widget().node.traverse(
+                notree.TraverseOrder.PRE,
+                render_collapsed=True
+            )
+        ):
+            related=False
+            sibling=False
+
+            if not seen_nodes:  # first node, top level
+                pass
+            elif node.parent == seen_nodes[-1].parent:  # going through siblings
+                sibling = True
+            elif node.parent == seen_nodes[-1]:  # first child, going down a level
+                related = True
+            else:  # next subtree, walk back up to find the parent
+                related = True
+                for count, parent in enumerate(reversed(seen_nodes), start=1):
+                    if node.parent == parent:
+                        tabbed_browser.widget.setCurrentWidget(new_tabs[-count])
+                        break
+                else:
+                    assert False, f"Unknown tree structure in tab-give: node={node}"
+
+            # Open each new tab in the foreground so that the sibling and
+            # related settings above will apply to the previously opened one.
+            tab = tabbed_browser.tabopen(
+                node.value.url(),
+                background=False,
+                idx=None if root_idx is None else root_idx + idx,
+                related=related,
+                sibling=sibling,
+            )
+            if node.collapsed:
+                tab.node.collapsed = True
+            if root_idx == None:
+                root_idx = tabbed_browser.widget.currentIndex()
+
+            seen_nodes.append(node)
+            new_tabs.append(tab)
+
+        if not keep:
+            self._tabbed_browser.close_tab(
+                self._current_widget(),
+                add_undo=False,
+                transfer=True,
+                recursive=True,
+            )
+        tabbed_browser.widget.setCurrentWidget(new_tabs[0])
+
     def _tree_tab_give(self, tabbed_browser, keep):
         """Helper function to simplify tab-give."""
         # first pass: open tabs and save the uids of the new nodes
@@ -546,7 +601,7 @@ class CommandDispatcher:
                     "The window with id {} is not private".format(win_id))
 
         if recursive and tabbed_browser.is_treetabbedbrowser:
-            self._tree_tab_give(tabbed_browser, keep)
+            self._tree_tab_give_onepass(tabbed_browser, keep)
         else:
             tabbed_browser.tabopen(self._current_url())
             if not keep:
diff --git a/tests/end2end/features/treetabs.feature b/tests/end2end/features/treetabs.feature
index dbc82c4f4..9717b5073 100644
--- a/tests/end2end/features/treetabs.feature
+++ b/tests/end2end/features/treetabs.feature
@@ -97,6 +97,24 @@ Feature: Tree tab management
         Then the following tabs should be open:
             - data/numbers/4.txt
 
+    Scenario: :tab-give --recursive with collapsed subtree
+        When I open data/numbers/1.txt
+        And I open data/numbers/2.txt in a new related tab
+        And I open data/numbers/3.txt in a new sibling tab
+        And I open data/numbers/4.txt in a new related tab
+        And I open data/numbers/5.txt in a new tab
+        And I run :tab-focus 2
+        And I run :tree-tab-toggle-hide
+        And I run :tab-focus 1
+        And I run :tab-give --recursive
+        And I run :window-only
+        And I wait until data/numbers/4.txt is loaded
+        Then the following tabs should be open:
+            - data/numbers/1.txt (active)
+              - data/numbers/3.txt (collapsed)
+                - data/numbers/4.txt
+              - data/numbers/2.txt
+
     Scenario: Open a child tab
         When I open data/numbers/1.txt
         And I open data/numbers/2.txt in a new related tab
diff --git a/tests/unit/misc/test_notree.py b/tests/unit/misc/test_notree.py
index 192dc6ac7..056daf463 100644
--- a/tests/unit/misc/test_notree.py
+++ b/tests/unit/misc/test_notree.py
@@ -172,10 +172,13 @@ def test_demote_to_last(tree):
     assert n6.children[-1] is n11
 
 
-def test_traverse(node):
-    len_traverse = len(list(node.traverse()))
-    len_render = len(node.render())
-    assert len_traverse == len_render
+def test_traverse(tree):
+    n1, n2, n3, n4, n5, n6, n7, n8, n9, n10, n11 = tree
+    actual = list(n1.traverse())
+    rendered = n1.render()
+    assert len(actual) == len(rendered)
+    print("\n".join('\t'.join((str(t[0]), t[1][0] + str(t[1][1]))) for t in zip(actual, rendered)))
+    assert actual == [n1, n2, n4, n5, n3, n6, n7, n8, n9, n10, n11]
 
 
 def test_traverse_postorder(tree):

From 3446cd80e6af3b61e6eae441310b8ae0847eb9cc Mon Sep 17 00:00:00 2001
From: toofar <toofar@spalge.com>
Date: Sat, 14 Sep 2024 17:37:38 +1200
Subject: [PATCH 05/12] Simplify tab-give --recursive implementation

This gets a bit simpler to read if we do things in one pass. Because of the
order we go through the tree we will always process a node's parent before we
get to it. So if we save the new tabs (or just the new nodes) we can set the
correct parent of a new tab right away. Additionally since we are always
appending tabs (same order as we traverse them) then we can just append it to
the list of children of the parent we found.

In this implementation we traverse through the subtree of tabs-to-be given and
create a new tab in the target window for each one. We construct the right
tree structure in the distination window by adding the new tabs to the
children list of the appropriate parent.

For some reason the `setCurrentWidget()` at the end didn't make the test fail,
even though it is checking for a specific active tab. Hmmm.
---
 qutebrowser/browser/commands.py | 47 +++++++++++++++------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py
index 2e931b87e..974365149 100644
--- a/qutebrowser/browser/commands.py
+++ b/qutebrowser/browser/commands.py
@@ -522,37 +522,34 @@ class CommandDispatcher:
         tabbed_browser.widget.setCurrentWidget(new_tabs[0])
 
     def _tree_tab_give(self, tabbed_browser, keep):
-        """Helper function to simplify tab-give."""
-        # first pass: open tabs and save the uids of the new nodes
-        uid_map = {}  # old_uid -> new_uid
-        traversed = list(self._current_widget().node.traverse())
-        for node in traversed:
+        """Recursive tab-give, move current tab and children to tabbed_browser."""
+        new_tab_map = {}  # old_uid -> new tab
+        current_node = self._current_widget().node
+        for node in current_node.traverse(
+            notree.TraverseOrder.PRE,
+            render_collapsed=True
+        ):
             tab = tabbed_browser.tabopen(
                 node.value.url(),
                 related=False,
+                background=True,
             )
-            uid_map[node.uid] = tab.node.uid
+            new_tab_map[node.uid] = tab
 
-        # second pass: copy tree structure over
-        newroot = tabbed_browser.widget.tree_root
-        for node in traversed:
-            if node.parent.uid in uid_map:
-                uid = uid_map[node.uid]
-                new_node = newroot.get_descendent_by_uid(uid)
-                parent_uid = uid_map[node.parent.uid]
-                new_parent = newroot.get_descendent_by_uid(parent_uid)
-                new_node.parent = new_parent
+            if node.collapsed:
+                tab.node.collapsed = True
+            if node != current_node:  # top level node has no parent
+                parent = new_tab_map[node.parent.uid].node
+                parent.children += (tab.node,)
 
-        # third pass: remove tabs from old window, children first this time to
-        # avoid having to re-parent things when traversing.
+        tabbed_browser.widget.setCurrentWidget(new_tab_map[current_node.uid])
         if not keep:
-            for node in self._current_widget().node.traverse(
-                notree.TraverseOrder.POST_R,
-                render_collapsed=False,
-            ):
-                self._tabbed_browser.close_tab(node.value,
-                                               add_undo=False,
-                                               transfer=True)
+            self._tabbed_browser.close_tab(
+                node.value,
+                add_undo=False,
+                transfer=True,
+                recursive=True,
+            )
 
     @cmdutils.register(instance='command-dispatcher', scope='window')
     @cmdutils.argument('win_id', completion=miscmodels.window)
@@ -601,7 +598,7 @@ class CommandDispatcher:
                     "The window with id {} is not private".format(win_id))
 
         if recursive and tabbed_browser.is_treetabbedbrowser:
-            self._tree_tab_give_onepass(tabbed_browser, keep)
+            self._tree_tab_give(tabbed_browser, keep)
         else:
             tabbed_browser.tabopen(self._current_url())
             if not keep:

From 8a102d6452737a8192e4064be2555a7aa1279b42 Mon Sep 17 00:00:00 2001
From: toofar <toofar@spalge.com>
Date: Sun, 15 Sep 2024 14:06:29 +1200
Subject: [PATCH 06/12] Remove longer tab-give --recursive implementation

Remove the alternate "onepass" implementation that uses
`tabopen(idx=idx)` for positioning instead of manipulating tree nodes
directly. Because:

    1. it was way longer
    2. it had logic to discern the tree structure anyway, the value in
       keeping tree manipulation code isolated is that there isn't as many
       place people have to reason about trees.
    3. it ended up touch nodes directly to set the collapsed state anyway,
       so some amount of tree knowledge is require
---
 qutebrowser/browser/commands.py | 55 ---------------------------------
 1 file changed, 55 deletions(-)

diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py
index 974365149..1150305d5 100644
--- a/qutebrowser/browser/commands.py
+++ b/qutebrowser/browser/commands.py
@@ -466,61 +466,6 @@ class CommandDispatcher:
         if not keep:
             tabbed_browser.close_tab(tab, add_undo=False, transfer=True)
 
-    def _tree_tab_give_onepass(self, tabbed_browser, keep):
-        """Recursive tab-give, move current tab and children to tabbed_browser."""
-        seen_nodes = []
-        new_tabs = []
-        root_idx = None
-        for idx, node in enumerate(
-            self._current_widget().node.traverse(
-                notree.TraverseOrder.PRE,
-                render_collapsed=True
-            )
-        ):
-            related=False
-            sibling=False
-
-            if not seen_nodes:  # first node, top level
-                pass
-            elif node.parent == seen_nodes[-1].parent:  # going through siblings
-                sibling = True
-            elif node.parent == seen_nodes[-1]:  # first child, going down a level
-                related = True
-            else:  # next subtree, walk back up to find the parent
-                related = True
-                for count, parent in enumerate(reversed(seen_nodes), start=1):
-                    if node.parent == parent:
-                        tabbed_browser.widget.setCurrentWidget(new_tabs[-count])
-                        break
-                else:
-                    assert False, f"Unknown tree structure in tab-give: node={node}"
-
-            # Open each new tab in the foreground so that the sibling and
-            # related settings above will apply to the previously opened one.
-            tab = tabbed_browser.tabopen(
-                node.value.url(),
-                background=False,
-                idx=None if root_idx is None else root_idx + idx,
-                related=related,
-                sibling=sibling,
-            )
-            if node.collapsed:
-                tab.node.collapsed = True
-            if root_idx == None:
-                root_idx = tabbed_browser.widget.currentIndex()
-
-            seen_nodes.append(node)
-            new_tabs.append(tab)
-
-        if not keep:
-            self._tabbed_browser.close_tab(
-                self._current_widget(),
-                add_undo=False,
-                transfer=True,
-                recursive=True,
-            )
-        tabbed_browser.widget.setCurrentWidget(new_tabs[0])
-
     def _tree_tab_give(self, tabbed_browser, keep):
         """Recursive tab-give, move current tab and children to tabbed_browser."""
         new_tab_map = {}  # old_uid -> new tab

From 961a7cc457d28d51b105778ffe7331616763d1e1 Mon Sep 17 00:00:00 2001
From: toofar <toofar@spalge.com>
Date: Tue, 17 Sep 2024 17:34:28 +1200
Subject: [PATCH 07/12] Fix `tab-move end` for tree tabs

Previously moving a tab to a higher tab number ended up putting it right
before the target node. Since the target node stayed in the same place that
actually meant that the tab being moved didn't make it to the indicated index.
The `new_idx_relative += 1` bit in this change accounts for that.

I also somewhat re-wrote the block of code to read a bit more clearly to me.
Previously it called nodes "tabs", and it looked up `tabs[new_idx]` twice.
It made it a bit difficult to spot logic issues.

What this block of code is doing is:
1. find target node by indexing into the list of tabs as per the tab bar
2. remove the current node from the tree
3. insert the current node into the sibling list of the target node

It's important that we look up the target node before removing the current.
Otherwise everything could shift across and we could end up putting the tab in
the next subtree.

Also added a bunch of tests that let me make sure I was making changes
correctly.
---
 qutebrowser/browser/commands.py         | 28 +++++++-------
 tests/end2end/features/treetabs.feature | 49 ++++++++++++++++++++++++-
 2 files changed, 61 insertions(+), 16 deletions(-)

diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py
index 1150305d5..29e1892b8 100644
--- a/qutebrowser/browser/commands.py
+++ b/qutebrowser/browser/commands.py
@@ -1196,28 +1196,26 @@ class CommandDispatcher:
         cmdutils.check_overflow(new_idx, 'int')
 
         if self._tabbed_browser.is_treetabbedbrowser:
-            # self._tree_tab_move(new_idx)
-            new_idx += 1  # tree-tabs indexes start at 1 (0 is hidden root tab)
             tab = self._current_widget()
-
-            # traverse order is the same as display order
-            # so indexing works correctly
             tree_root = self._tabbed_browser.widget.tree_root
-            tabs = list(tree_root.traverse(render_collapsed=False))
-            target_node = tabs[new_idx]
+            # Lookup target nodes from display order list to match what the
+            # user sees in the tab bar.
+            nodes = list(tree_root.traverse(render_collapsed=False))[1:]
+            target_node = nodes[new_idx]
             if tab.node in target_node.path:
                 raise cmdutils.CommandError("Can't move tab to a descendent"
                                             " of itself")
 
-            new_parent = target_node.parent
-            # we need index relative to parent for correct placement
-            dest_tab = tabs[new_idx]
-            new_idx_relative = new_parent.children.index(dest_tab)
+            tab.node.parent = None  # detach the node now to avoid duplicate errors
+            target_siblings = list(target_node.parent.children)
+            new_idx_relative = target_siblings.index(target_node)
+            if cur_idx < new_idx:
+                # If moving the tab to a higher number, insert if after the
+                # target node to account for all the tabs shifting down.
+                new_idx_relative += 1
 
-            tab.node.parent = None  # avoid duplicate errors
-            siblings = list(new_parent.children)
-            siblings.insert(new_idx_relative, tab.node)
-            new_parent.children = siblings
+            target_siblings.insert(new_idx_relative, tab.node)
+            target_node.parent.children = target_siblings
 
             self._tabbed_browser.widget.tree_tab_update()
         else:
diff --git a/tests/end2end/features/treetabs.feature b/tests/end2end/features/treetabs.feature
index 9717b5073..4d2b7ceb3 100644
--- a/tests/end2end/features/treetabs.feature
+++ b/tests/end2end/features/treetabs.feature
@@ -124,7 +124,7 @@ Feature: Tree tab management
               - data/numbers/2.txt (active)
             """
 
-    Scenario: Move a tab to the given index
+    Scenario: Move a tab down to the given index
         When I open data/numbers/1.txt
         And I open data/numbers/2.txt in a new related tab
         And I open data/numbers/3.txt in a new tab
@@ -139,6 +139,53 @@ Feature: Tree tab management
               - data/numbers/2.txt
             """
 
+    Scenario: Move a tab up to given index
+        When I open data/numbers/1.txt
+        And I open data/numbers/2.txt in a new related tab
+        And I open data/numbers/3.txt in a new tab
+        And I open data/numbers/4.txt in a new related tab
+        And I run :tab-move 2
+        Then the following tabs should be open:
+            - data/numbers/1.txt
+              - data/numbers/4.txt
+              - data/numbers/2.txt
+            - data/numbers/3.txt
+
+    Scenario: Move a tab within siblings
+        When I open data/numbers/1.txt
+        And I open data/numbers/2.txt in a new related tab
+        And I open data/numbers/3.txt in a new sibling tab
+        And I run :tab-move +
+        Then the following tabs should be open:
+            - data/numbers/1.txt
+              - data/numbers/2.txt
+              - data/numbers/3.txt
+
+    Scenario: Move a tab to end
+        When I open data/numbers/1.txt
+        And I open data/numbers/2.txt in a new related tab
+        And I open data/numbers/3.txt in a new tab
+        And I open data/numbers/4.txt in a new related tab
+        And I run :tab-focus 2
+        And I run :tab-move end
+        Then the following tabs should be open:
+            - data/numbers/1.txt
+            - data/numbers/3.txt
+              - data/numbers/4.txt
+              - data/numbers/2.txt
+
+    Scenario: Move a tab to start
+        When I open data/numbers/1.txt
+        And I open data/numbers/2.txt in a new related tab
+        And I open data/numbers/3.txt in a new tab
+        And I open data/numbers/4.txt in a new related tab
+        And I run :tab-move start
+        Then the following tabs should be open:
+            - data/numbers/4.txt
+            - data/numbers/1.txt
+              - data/numbers/2.txt
+            - data/numbers/3.txt
+
     Scenario: Collapse a subtree
         When I open data/numbers/1.txt
         And I open data/numbers/2.txt in a new related tab

From a3e34bc03c6c7b1990e6edc65102c018fabf031a Mon Sep 17 00:00:00 2001
From: toofar <toofar@spalge.com>
Date: Tue, 17 Sep 2024 18:56:10 +1200
Subject: [PATCH 08/12] Add common "ensure tree tab" method in commands.py

There was a few checks of `is_treetabbedbrowser` and repeated errors messages.
I wanted to move them to a common method, but might have overestimated how
many places could use it. There are several tree tab specific arguments that
aren't treated as fatal, for example `tab-give --recursive` and `tab-prev
--sibling`. I'm not sure if they should be fatal or not. There is some value
in defaulting to the non-tree tab behavior if someone wants to use the same
bindings across tree tab and non-tree tab basedirs. There is also value in
being clear that what you've asked for can't be delivered.

If indeed there is only one or two places where we need to complain about an
argument, and not a whole command, then we could move the magic command name
finding bit into the Command object and just hardcode the command name in the
two places where we want to warn about a specific argument.

There is also the `tree-tab-create-group` command which doesn't bother
checking that yo have a tree tab enabled windows because it doesn't actually
depend on any tree tab features, it's just an alternate to about:blank.
---
 qutebrowser/browser/commands.py | 60 +++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 18 deletions(-)

diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py
index 29e1892b8..00fc89b18 100644
--- a/qutebrowser/browser/commands.py
+++ b/qutebrowser/browser/commands.py
@@ -10,6 +10,7 @@ import os.path
 import shlex
 import functools
 import urllib.parse
+import inspect
 from typing import cast, Union, Optional
 from collections.abc import Callable
 
@@ -129,15 +130,11 @@ class CommandDispatcher:
             tabbed_browser.tabopen(url)
             tabbed_browser.window().show()
         elif tab or background:
-            if tabbed_browser.is_treetabbedbrowser:
-                tabbed_browser.tabopen(url, background=background,
-                                       related=related, sibling=sibling)
-            elif sibling:
-                raise cmdutils.CommandError("--sibling flag only works with \
-                                            tree-tab enabled")
-            else:
-                tabbed_browser.tabopen(url, background=background,
-                                       related=related)
+            if sibling:
+                self._ensure_tree_tabs("--sibling")
+
+            tabbed_browser.tabopen(url, background=background,
+                                   related=related, sibling=sibling)
         else:
             widget = self._current_widget()
             widget.load_url(url)
@@ -1094,7 +1091,8 @@ class CommandDispatcher:
             assert isinstance(index, str)
             self._tab_focus_stack(index)
             return
-        elif index == 'parent' and self._tabbed_browser.is_treetabbedbrowser:
+        elif index == 'parent':
+            self._ensure_tree_tabs("parent")
             node = self._current_widget().node
             path = node.path
             if count:
@@ -2035,6 +2033,35 @@ class CommandDispatcher:
         log.misc.debug('state before fullscreen: {}'.format(
             debug.qflags_key(Qt, window.state_before_fullscreen)))
 
+    def _ensure_tree_tabs(self, arg_name: Optional[str] = None):
+        """Check if we are on a tree tabs enabled browser."""
+        if not self._tabbed_browser.is_treetabbedbrowser:
+            # Potentially fragile code to get the name of the command the user
+            # called. Get the calling functions via inspect, lookup the
+            # command object by looking for a command with the related unbound
+            # functions as its handler.
+            # Alternate options:
+            # 1. stash the cmd object on the function
+            # 2. duplicate the slugification of the function name (it's just _->-)
+            # 3. move this check into the Command object somehow (easy for
+            #    disallowed commands, hard for disallowed args)
+            # 4. save the currently executing command somewhere
+            bound_func = getattr(self, inspect.stack()[1].function)
+            cmds = [
+                name
+                for name, cmd
+                in objects.commands.items()
+                if cmd.handler == bound_func.__func__
+            ]
+            assert len(cmds) == 1
+            cmd_name = cmds[0]
+
+            arg_part = ""
+            if arg_name:
+                arg_part = f"argument `{arg_name}` "
+            msg = f"{cmd_name}: {arg_part}requires a window with tree tabs"
+            raise cmdutils.CommandError(msg)
+
     @cmdutils.register(instance='command-dispatcher', scope='window',
                        tree_tab=True)
     @cmdutils.argument('count', value=cmdutils.Value.count)
@@ -2047,8 +2074,7 @@ class CommandDispatcher:
         Args:
             count: How many levels the tabs should be promoted to
         """
-        if not self._tabbed_browser.is_treetabbedbrowser:
-            raise cmdutils.CommandError('Tree-tabs are disabled')
+        self._ensure_tree_tabs()
         config_position = config.val.tabs.new_position.tree.promote
         try:
             self._current_widget().node.promote(count, config_position)
@@ -2065,8 +2091,7 @@ class CommandDispatcher:
         Observes tabs.new_position.tree.demote in positioning the tab among new
         siblings.
         """
-        if not self._tabbed_browser.is_treetabbedbrowser:
-            raise cmdutils.CommandError('Tree-tabs are disabled')
+        self._ensure_tree_tabs()
         cur_node = self._current_widget().node
 
         config_position = config.val.tabs.new_position.tree.demote
@@ -2088,8 +2113,7 @@ class CommandDispatcher:
         Args:
             count: Which tab to collapse
         """
-        if not self._tabbed_browser.is_treetabbedbrowser:
-            raise cmdutils.CommandError('Tree-tabs are disabled')
+        self._ensure_tree_tabs()
         tab = self._cntwidget(count)
         if not tab.node.children:
             return
@@ -2106,8 +2130,7 @@ class CommandDispatcher:
         Args:
             count: How many levels to hide.
         """
-        if not self._tabbed_browser.is_treetabbedbrowser:
-            raise cmdutils.CommandError('Tree-tabs are disabled')
+        self._ensure_tree_tabs()
         while count > 0:
             tab = self._current_widget()
             self._tabbed_browser.cycle_hide_tab(tab.node)
@@ -2145,6 +2168,7 @@ class CommandDispatcher:
         Args:
             count: Target tab.
         """
+        self._ensure_tree_tabs()
         tab = self._cntwidget(count)
         for descendent in tab.node.traverse():
             cur_tab = descendent.value

From 707cca09f9ed13e7af14dfb91765a6572d1c5a2f Mon Sep 17 00:00:00 2001
From: toofar <toofar@spalge.com>
Date: Tue, 1 Apr 2025 06:48:52 +1300
Subject: [PATCH 09/12] quote multiline gherkin strings

---
 tests/end2end/features/treetabs.feature | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tests/end2end/features/treetabs.feature b/tests/end2end/features/treetabs.feature
index 4d2b7ceb3..02fff95c1 100644
--- a/tests/end2end/features/treetabs.feature
+++ b/tests/end2end/features/treetabs.feature
@@ -83,7 +83,9 @@ Feature: Tree tab management
         And I wait for "Asking question *" in the log
         And I run :prompt-accept yes
         Then the following tabs should be open:
+            """
             - data/numbers/4.txt
+            """
 
     Scenario: :tab-close --recursive with collapsed subtree
         When I open data/numbers/1.txt
@@ -95,7 +97,9 @@ Feature: Tree tab management
         And I run :tab-focus 1
         And I run :tab-close --recursive
         Then the following tabs should be open:
+            """
             - data/numbers/4.txt
+            """
 
     Scenario: :tab-give --recursive with collapsed subtree
         When I open data/numbers/1.txt
@@ -110,10 +114,12 @@ Feature: Tree tab management
         And I run :window-only
         And I wait until data/numbers/4.txt is loaded
         Then the following tabs should be open:
+            """
             - data/numbers/1.txt (active)
               - data/numbers/3.txt (collapsed)
                 - data/numbers/4.txt
               - data/numbers/2.txt
+            """
 
     Scenario: Open a child tab
         When I open data/numbers/1.txt
@@ -146,10 +152,12 @@ Feature: Tree tab management
         And I open data/numbers/4.txt in a new related tab
         And I run :tab-move 2
         Then the following tabs should be open:
+            """
             - data/numbers/1.txt
               - data/numbers/4.txt
               - data/numbers/2.txt
             - data/numbers/3.txt
+            """
 
     Scenario: Move a tab within siblings
         When I open data/numbers/1.txt
@@ -157,9 +165,11 @@ Feature: Tree tab management
         And I open data/numbers/3.txt in a new sibling tab
         And I run :tab-move +
         Then the following tabs should be open:
+            """
             - data/numbers/1.txt
               - data/numbers/2.txt
               - data/numbers/3.txt
+            """
 
     Scenario: Move a tab to end
         When I open data/numbers/1.txt
@@ -169,10 +179,12 @@ Feature: Tree tab management
         And I run :tab-focus 2
         And I run :tab-move end
         Then the following tabs should be open:
+            """
             - data/numbers/1.txt
             - data/numbers/3.txt
               - data/numbers/4.txt
               - data/numbers/2.txt
+            """
 
     Scenario: Move a tab to start
         When I open data/numbers/1.txt
@@ -181,10 +193,12 @@ Feature: Tree tab management
         And I open data/numbers/4.txt in a new related tab
         And I run :tab-move start
         Then the following tabs should be open:
+            """
             - data/numbers/4.txt
             - data/numbers/1.txt
               - data/numbers/2.txt
             - data/numbers/3.txt
+            """
 
     Scenario: Collapse a subtree
         When I open data/numbers/1.txt

From f31d1e0fe3c9b87eaf9da89ed69b32ce9f46d2f8 Mon Sep 17 00:00:00 2001
From: toofar <toofar@spalge.com>
Date: Tue, 1 Apr 2025 06:56:40 +1300
Subject: [PATCH 10/12] Make :tab-give --recursive test more thorough

It was broken so that only one tab was being removed from the donor
window and this wasn't being caught in the test that was only looking at
the target window.

Using the "session should look like" test to make sure all the right
tabs are open, then the "tabs should be open" one to check the tab
hierarchy is right.
---
 qutebrowser/browser/commands.py         |  2 +-
 tests/end2end/features/treetabs.feature | 20 ++++++++++++++++++--
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py
index 00fc89b18..32bec5c85 100644
--- a/qutebrowser/browser/commands.py
+++ b/qutebrowser/browser/commands.py
@@ -487,7 +487,7 @@ class CommandDispatcher:
         tabbed_browser.widget.setCurrentWidget(new_tab_map[current_node.uid])
         if not keep:
             self._tabbed_browser.close_tab(
-                node.value,
+                current_node.value,
                 add_undo=False,
                 transfer=True,
                 recursive=True,
diff --git a/tests/end2end/features/treetabs.feature b/tests/end2end/features/treetabs.feature
index 02fff95c1..0292818a5 100644
--- a/tests/end2end/features/treetabs.feature
+++ b/tests/end2end/features/treetabs.feature
@@ -111,9 +111,25 @@ Feature: Tree tab management
         And I run :tree-tab-toggle-hide
         And I run :tab-focus 1
         And I run :tab-give --recursive
-        And I run :window-only
         And I wait until data/numbers/4.txt is loaded
-        Then the following tabs should be open:
+        Then the session should look like:
+            """
+            windows:
+            - tabs:
+              - history:
+                - url: http://localhost:*/data/numbers/5.txt
+            - tabs:
+              - history:
+                - url: http://localhost:*/data/numbers/1.txt
+              - history:
+                - url: http://localhost:*/data/numbers/3.txt
+              - history:
+                - url: http://localhost:*/data/numbers/4.txt
+              - history:
+                - url: http://localhost:*/data/numbers/2.txt
+            """
+        And I run :window-only
+        And the following tabs should be open:
             """
             - data/numbers/1.txt (active)
               - data/numbers/3.txt (collapsed)

From 7c1c54da424863599dc455ca9cee0ec27397e94f Mon Sep 17 00:00:00 2001
From: toofar <toofar@spalge.com>
Date: Tue, 1 Apr 2025 17:18:50 +1300
Subject: [PATCH 11/12] "fix" lint

---
 qutebrowser/browser/commands.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py
index 32bec5c85..61574b0fb 100644
--- a/qutebrowser/browser/commands.py
+++ b/qutebrowser/browser/commands.py
@@ -1133,7 +1133,11 @@ class CommandDispatcher:
     @cmdutils.register(instance="command-dispatcher", scope="window")
     @cmdutils.argument("index", choices=["+", "-", "start", "end"])
     @cmdutils.argument("count", value=cmdutils.Value.count)
-    def tab_move(self, index: Union[str, int] = None, count: int = None) -> None:
+    def tab_move(  # noqa: C901
+        self,
+        index: Union[str, int] = None,
+        count: int = None,
+    ) -> None:
         """Move the current tab according to the argument and [count].
 
         If neither is given, move it to the first position.

From 2f3885a77a0c51adb9562b73e397d9b676f336fe Mon Sep 17 00:00:00 2001
From: toofar <toofar@spalge.com>
Date: Sun, 6 Apr 2025 15:56:33 +1200
Subject: [PATCH 12/12] Make `select_on_remove=prev` select previous sibling

Updates `tabs.select_on_remove=prev` for tree tab windows so that the
previous sibling is selected, instead of the previous tab in the tab
bar. I've found that the selected tab descending to the rightmost leaf
node of a tab group when I close a top level tab is never what I want.

It may have been more respectful of people's preferences and habits to
add a "previous-sibling" option for the `tabs.select_on_remove` option,
but that would be a fair bit of work. Turning that option into strings
and mapping them to QTabBar enum values at the only place they are used
wouldn't be so bad, but then there would be another place where non-tree
code would have to deal with a tree specific option. So I figured I
would be bold and change the default behaviour and see if anyone
complained.
---
 qutebrowser/mainwindow/treetabbedbrowser.py | 26 ++++++++++++++
 tests/end2end/features/treetabs.feature     | 40 +++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/qutebrowser/mainwindow/treetabbedbrowser.py b/qutebrowser/mainwindow/treetabbedbrowser.py
index b46087690..756e4a2f2 100644
--- a/qutebrowser/mainwindow/treetabbedbrowser.py
+++ b/qutebrowser/mainwindow/treetabbedbrowser.py
@@ -15,6 +15,7 @@ from qutebrowser.mainwindow.tabbedbrowser import TabbedBrowser, _UndoEntry
 from qutebrowser.mainwindow.treetabwidget import TreeTabWidget
 from qutebrowser.browser import browsertab
 from qutebrowser.misc import notree
+from qutebrowser.qt.widgets import QTabBar
 
 
 @dataclasses.dataclass
@@ -152,6 +153,31 @@ class TreeTabbedBrowser(TabbedBrowser):
 
         node = tab.node
         parent = node.parent
+        current_tab = self.current_tab()
+
+        # Override tabs.select_on_remove behavior to be tree aware.
+        # The default behavior is in QTabBar.removeTab(), by way of
+        # QTabWidget.removeTab(). But here we are detaching the tab from the
+        # tree before those methods get called, so if we want to have a tree
+        # aware behavior we need to implement that here by selecting the new
+        # tab before the closing the current one.
+        if tab == current_tab:
+            selection_behavior = self.widget.tabBar().selectionBehaviorOnRemove()
+            # Given a tree structure like:
+            # - one
+            #   - two
+            # - three (active)
+            # If the setting is "prev" (aka left) we want to end up with tab
+            # "one" selected after closing tab "three". Switch to either the
+            # current tab's previous sibling or its parent.
+            if selection_behavior == QTabBar.SelectionBehavior.SelectLeftTab:
+                siblings = parent.children
+                rel_index = siblings.index(node)
+                if rel_index == 0:
+                    next_tab = parent.value
+                else:
+                    next_tab = siblings[rel_index-1].value
+                self.widget.setCurrentWidget(next_tab)
 
         if node.collapsed:
             # Collapsed nodes have already been removed from the TabWidget so
diff --git a/tests/end2end/features/treetabs.feature b/tests/end2end/features/treetabs.feature
index 0292818a5..1591a8359 100644
--- a/tests/end2end/features/treetabs.feature
+++ b/tests/end2end/features/treetabs.feature
@@ -356,3 +356,43 @@ Feature: Tree tab management
           - about:blank?grandparent
             - about:blank?parent (active)
           """
+
+    Scenario: Tabs.select_on_remove prev selects previous sibling
+        When I open about:blank?one
+        And I open about:blank?two in a new related tab
+        And I open about:blank?three in a new tab
+        And I run :set tabs.select_on_remove prev
+        And I run :tab-close
+        And I run :config-unset tabs.select_on_remove
+        Then the following tabs should be open:
+          """
+          - about:blank?one (active)
+            - about:blank?two
+          """
+
+    Scenario: Tabs.select_on_remove prev selects parent
+        When I open about:blank?one
+        And I open about:blank?two in a new related tab
+        And I open about:blank?three in a new sibling tab
+        And I run :set tabs.select_on_remove prev
+        And I run :tab-close
+        And I run :config-unset tabs.select_on_remove
+        Then the following tabs should be open:
+          """
+          - about:blank?one (active)
+            - about:blank?two
+          """
+
+    Scenario: Tabs.select_on_remove prev can be overridden
+        When I open about:blank?one
+        And I open about:blank?two in a new related tab
+        And I open about:blank?three in a new tab
+        And I run :tab-select ?two
+        And I run :set tabs.select_on_remove prev
+        And I run :tab-close --next
+        And I run :config-unset tabs.select_on_remove
+        Then the following tabs should be open:
+          """
+          - about:blank?one
+          - about:blank?three (active)
+          """