Remove `treetabbedbrowser._add_undo_entry()`

This one's not as clear a win as the previous one. The main duplication
here was manipulating `undo_stack`. There was very little in
`_add_undo_entry()` apart from that, and it was already being done in
the parent. Sadly there isn't really a win in terms of line count (there
is slightly more lines now but that's because of comments and formatting
changes).

I pulled out a ` _add_undo_entry()` method in the parent class, even
though it isn't overriden anymore, just because it seemed like a good
idea and makes the `_remove_tab()` method a bit cleaner.

I moved the tree tab specific logic (collapsed tabs and parent and child
data) into the UndoEntry class because I don't really see an alternative
to having a specialised one of them, so may as well make use of it. Also
using it as a dumb data class and having the related class contain logic
related to it _pushes up glasses_ violates one or two OOP principles.

Having both undo classes handle a `browsertab.WebTabError` is a minor
bit of duplication but if the parent class handled it I think it's too
non-specific an error to be passing up to levels (it would seem a bit
"magical" how the parent class inferred the meaning of it) and adding a
`history_data` arg into `from_tab()` seems like pointless toil for the
parent class. There is probably more "pointless toil" that could be
removed from the parent class and pulled into the specialised class that
knows about undo logic, but that isn't the goal of this PR.

I had to teach the parent class about `_undo_class.from_tab()` possibly
returning a list even though it would never do that for a flat
TabbedBrowser, but oh well. That nested conditional could maybe be
arguably optimised but I'm not seeing a clear win.

Regarding next steps around here, I think
`TreeTabbedBrowser._remove_tab()` is the bigger win than the undo logic
itself. It entirely deals with tree stuff which I feel should be owned
by the tree data structure, and moving that there would probably mean
other pieces of code could be switched to use it too. Eg the
re-parenting could be in the notree class and if we taught the notree
nodes how to close tabs the collapsed node handling could be moved there
too.

2 files changed, 87 insertions, 69 deletions

diff --git a/qutebrowser/mainwindow/tabbedbrowser.py b/qutebrowser/mainwindow/tabbedbrowser.py
index 79eae98a5..bd81cc371 100644
--- a/qutebrowser/mainwindow/tabbedbrowser.py
+++ b/qutebrowser/mainwindow/tabbedbrowser.py
@@ -36,6 +36,21 @@ class _UndoEntry:
     created_at: datetime.datetime = dataclasses.field(
         default_factory=datetime.datetime.now)
 
+    @classmethod
+    def from_tab(cls, tab: browsertab.AbstractTab, idx: int):
+        """Generate an undo entry from `tab`."""
+        try:
+            history_data = tab.history.private_api.serialize()
+        except browsertab.WebTabError:
+            return None  # special URL
+
+        return cls(
+            url=tab.url(),
+            history=history_data,
+            index=idx,
+            pinned=tab.data.pinned,
+        )
+
 
 UndoStackType = MutableSequence[MutableSequence[_UndoEntry]]
 
@@ -196,6 +211,7 @@ class TabbedBrowser(QWidget):
     new_tab = pyqtSignal(browsertab.AbstractTab, int)
     is_treetabbedbrowser = False
     shutting_down = pyqtSignal()
+    _undo_class = _UndoEntry
 
     def __init__(self, *, win_id, private, parent=None):
         if private:
@@ -513,36 +529,46 @@ class TabbedBrowser(QWidget):
 
         tab.pending_removal = True
 
+        if add_undo:
+            self._add_undo_entry(tab, new_undo=new_undo)
+
+        tab.private_api.shutdown()
+        self.widget.removeTab(idx)
+
+        tab.deleteLater()
+
+    def _add_undo_entry(self, tab, new_undo):
         if tab.url().isEmpty():
             # There are some good reasons why a URL could be empty
             # (target="_blank" with a download, see [1]), so we silently ignore
             # this.
             # [1] https://github.com/qutebrowser/qutebrowser/issues/163
-            pass
-        elif not tab.url().isValid():
+            return
+
+        if not tab.url().isValid():
             # We display a warning for URLs which are not empty but invalid -
             # but we don't return here because we want the tab to close either
             # way.
             urlutils.invalid_url_error(tab.url(), "saving tab")
-        elif add_undo:
-            try:
-                history_data = tab.history.private_api.serialize()
-            except browsertab.WebTabError:
-                pass  # special URL
-            else:
-                entry = _UndoEntry(url=tab.url(),
-                                   history=history_data,
-                                   index=idx,
-                                   pinned=tab.data.pinned)
-                if new_undo or not self.undo_stack:
-                    self.undo_stack.append([entry])
-                else:
-                    self.undo_stack[-1].append(entry)
+            return
 
-        tab.private_api.shutdown()
-        self.widget.removeTab(idx)
+        idx = self.widget.indexOf(tab)
+        entry = self._undo_class.from_tab(tab, idx)
+        if not entry:
+            return
 
-        tab.deleteLater()
+        if isinstance(entry, self._undo_class):
+            if new_undo or not self.undo_stack:
+                self.undo_stack.append([entry])
+            else:
+                self.undo_stack[-1].append(entry)
+        else:
+            assert len(entry) > 0
+            entries = entry
+            if new_undo:
+                self.undo_stack.append(entries)
+            else:
+                self.undo_stack[-1].extend(entries)
 
     def undo(self, depth=1):
         """Undo removing of a tab or tabs."""
diff --git a/qutebrowser/mainwindow/treetabbedbrowser.py b/qutebrowser/mainwindow/treetabbedbrowser.py
index f88cd7bab..78308ec21 100644
--- a/qutebrowser/mainwindow/treetabbedbrowser.py
+++ b/qutebrowser/mainwindow/treetabbedbrowser.py
@@ -33,27 +33,54 @@ class _TreeUndoEntry():
     created_at: datetime.datetime = dataclasses.field(
         default_factory=datetime.datetime.now)
 
-    @staticmethod
-    def from_node(node, idx):
+    @classmethod
+    def from_tab(
+        cls,
+        tab: browsertab.AbstractTab,
+        idx: int,
+        recursing: bool = False,
+    ):
         """Make a TreeUndoEntry from a Node."""
+        node = tab.node
         url = node.value.url()
         try:
-            history_data = node.value.history.private_api.serialize()
+            history_data = tab.history.private_api.serialize()
         except browsertab.WebTabError:
-            history_data = []
+            return None  # special URL
+
+        if not recursing and node.collapsed:
+            entries = [
+                cls.from_tab(descendent.value, idx+1, recursing=True)
+                for descendent in
+                node.traverse(notree.TraverseOrder.POST_R)
+            ]
+            entries = [entry for entry in entries if entry]
+            return entries
+
         pinned = node.value.data.pinned
         uid = node.uid
         parent_uid = node.parent.uid
-        children = [n.uid for n in node.children]
+        if recursing:
+            # Recursively removed nodes will never have any existing children
+            # to re-parent in the tree they are being added into, children
+            # will always be added later as the undo stack is worked through.
+            # So remove child IDs here so we don't confuse undo() later.
+            children = []
+        else:
+            children = [n.uid for n in node.children]
         local_idx = node.index
-        return _TreeUndoEntry(url=url,
-                              history=history_data,
-                              index=idx,
-                              pinned=pinned,
-                              uid=uid,
-                              parent_node_uid=parent_uid,
-                              children_node_uids=children,
-                              local_index=local_idx)
+        return cls(
+            url=url,
+            history=history_data,
+            # The index argument is redundant given the parent and local index
+            # info, but required by the parent class.
+            index=idx,
+            pinned=pinned,
+            uid=uid,
+            parent_node_uid=parent_uid,
+            children_node_uids=children,
+            local_index=local_idx,
+        )
 
 
 class TreeTabbedBrowser(TabbedBrowser):
@@ -66,6 +93,7 @@ class TreeTabbedBrowser(TabbedBrowser):
     """
 
     is_treetabbedbrowser = True
+    _undo_class = _TreeUndoEntry
 
     def _create_tab_widget(self):
         """Return the tab widget that can display a tree structure."""
@@ -74,8 +102,7 @@ class TreeTabbedBrowser(TabbedBrowser):
     def _remove_tab(self, tab, *, add_undo=True, new_undo=True, crashed=False):
         """Handle children positioning after a tab is removed."""
         if not tab.url().isEmpty() and tab.url().isValid() and add_undo:
-            idx = self.widget.indexOf(tab)
-            self._add_undo_entry(tab, idx, new_undo)
+            self._add_undo_entry(tab, new_undo)
 
         node = tab.node
         parent = node.parent
@@ -119,41 +146,6 @@ class TreeTabbedBrowser(TabbedBrowser):
 
         self.widget.tree_tab_update()
 
-    def _add_undo_entry(
-        self,
-        tab,
-        idx,  # pylint: disable=unused-argument
-        new_undo,
-    ):
-        """Save undo entry with tree information.
-
-        This function was removed in tabbedbrowser, but it is still useful here because
-        the mechanism is quite a bit more complex
-        """
-        node = tab.node
-        if not node.collapsed:
-            entry = _TreeUndoEntry.from_node(node, 0)
-            if new_undo or not self.undo_stack:
-                self.undo_stack.append([entry])
-            else:
-                self.undo_stack[-1].append(entry)
-        else:
-            entries = []
-            for descendent in node.traverse(notree.TraverseOrder.POST_R):
-                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:
-                self.undo_stack[-1] += entries
-
     def undo(self, depth=1):
         """Undo removing of a tab or tabs."""
         # save entries before super().undo() pops them