Stop modifying the tree in `_add_undo_entry()`

When a collapsed tab group was being deleted the nodes were being removed
from the tree by a sneaky line in `_add_undo_entry()`, which made it difficult
to reason about the code. The nodes are removed from the tree by setting
`node.parent = None`, the setting goes and calls `parent.disown()`, which is a
bit magical but that's a job for another day.

This commit moves the line where the node is removed from the tree to be up into
the same block as where the tab is deleted and adjusts the undo entry to
remove the child uids from it instead. The problem there is that
`UndoEntry.from_node()` iterates over a nodes children, previously
children were being removed from the tree first so they weren't showing
up here. We don't want children uids in the undo nodes because the
restore code will look for an existing child to re-attach when
re-creating a node. But tabs are restored parent first. And since we do
have a parent uid in the undo entry that is enough to restore the full
structure.

1 files changed, 11 insertions, 7 deletions

diff --git a/qutebrowser/mainwindow/treetabbedbrowser.py b/qutebrowser/mainwindow/treetabbedbrowser.py
index a9f134bd2..f93e7c98d 100644
--- a/qutebrowser/mainwindow/treetabbedbrowser.py
+++ b/qutebrowser/mainwindow/treetabbedbrowser.py
@@ -94,11 +94,10 @@ class TreeTabbedBrowser(TabbedBrowser):
 
         if node.collapsed:
             # Collapsed nodes have already been removed from the TabWidget so
-            # we can't ask super() to disposed of them and need to do it
+            # we can't ask super() to dispose of them and need to do it
             # ourselves.
-            # The node is removed from the tree in _add_undo_entry() currently
-            # (see fixme comment).
             for descendent in descendents:
+                descendent.parent = None
                 descendent_tab = descendent.value
                 descendent_tab.private_api.shutdown()
                 descendent_tab.deleteLater()
@@ -161,10 +160,15 @@ class TreeTabbedBrowser(TabbedBrowser):
             else:
                 entries = []
                 for descendent in node.traverse(notree.TraverseOrder.POST_R):
-                    entries.append(_TreeUndoEntry.from_node(descendent, 0))
-                    # ensure descendent is not later saved as child as well
-                    descendent.parent = None  # FIXME: Find a way not to change
-                    # the tree
+                    entry = _TreeUndoEntry.from_node(descendent, 0)
+                    # Recursively removed nodes will never have any children
+                    # in the tree they are being added into. Children will
+                    # always be added later as the undo stack is worked
+                    # through.
+                    # UndoEntry.from_node() is not clever enough enough to
+                    # handle this case on its own currently.
+                    entry.children_node_uids = []
+                    entries.append(entry)
                 if new_undo:
                     self.undo_stack.append(entries)
                 else: