Merge pull request #8084 from qutebrowser/tree/8076_fix_loading_collapsed_tabs_and_legacy_session_support

Fix loading collapsed tabs and legacy session support

5 files changed, 181 insertions, 17 deletions

diff --git a/qutebrowser/mainwindow/tabbedbrowser.py b/qutebrowser/mainwindow/tabbedbrowser.py
index c07b9e1ad..cbc1bac78 100644
--- a/qutebrowser/mainwindow/tabbedbrowser.py
+++ b/qutebrowser/mainwindow/tabbedbrowser.py
@@ -281,9 +281,11 @@ class TabbedBrowser(QWidget):
             raise TabDeletedError("index is -1!")
         return idx
 
-    def widgets(self):
+    def widgets(self) -> List[browsertab.AbstractTab]:
         """Get a list of open tab widgets.
 
+        Consider using `tabs()` instead of this method.
+
         We don't implement this as generator so we can delete tabs while
         iterating over the list.
         """
@@ -296,6 +298,18 @@ class TabbedBrowser(QWidget):
                 widgets.append(widget)
         return widgets
 
+    def tabs(
+        self,
+        include_hidden: bool = False,  # pylint: disable=unused-argument
+    ) -> List[browsertab.AbstractTab]:
+        """Get a list of tabs in this browser.
+
+        Args:
+            include_hidden: Include child tabs which are not currently in the
+                            tab bar.
+        """
+        return self.widgets()
+
     def _update_window_title(self, field=None):
         """Change the window title to match the current tab.
 
@@ -377,7 +391,7 @@ class TabbedBrowser(QWidget):
             tab.history_item_triggered.connect(
                 history.web_history.add_from_tab)
 
-    def _current_tab(self) -> browsertab.AbstractTab:
+    def current_tab(self) -> browsertab.AbstractTab:
         """Get the current browser tab.
 
         Note: The assert ensures the current tab is never None.
@@ -586,7 +600,7 @@ class TabbedBrowser(QWidget):
         if newtab or self.widget.currentWidget() is None:
             self.tabopen(url, background=False)
         else:
-            self._current_tab().load_url(url)
+            self.current_tab().load_url(url)
 
     @pyqtSlot(int)
     def on_tab_close_requested(self, idx):
@@ -672,7 +686,7 @@ class TabbedBrowser(QWidget):
             # Make sure the background tab has the correct initial size.
             # With a foreground tab, it's going to be resized correctly by the
             # layout anyways.
-            current_widget = self._current_tab()
+            current_widget = self.current_tab()
             tab.resize(current_widget.size())
             self.widget.tab_index_changed.emit(self.widget.currentIndex(),
                                                self.widget.count())
@@ -1087,7 +1101,7 @@ class TabbedBrowser(QWidget):
             if key != "'":
                 message.error("Failed to set mark: url invalid")
             return
-        point = self._current_tab().scroller.pos_px()
+        point = self.current_tab().scroller.pos_px()
 
         if key.isupper():
             self._global_marks[key] = point, url
@@ -1108,7 +1122,7 @@ class TabbedBrowser(QWidget):
         except qtutils.QtValueError:
             urlkey = None
 
-        tab = self._current_tab()
+        tab = self.current_tab()
 
         if key.isupper():
             if key in self._global_marks:
diff --git a/qutebrowser/mainwindow/treetabbedbrowser.py b/qutebrowser/mainwindow/treetabbedbrowser.py
index 75a3c55f8..2ac18c337 100644
--- a/qutebrowser/mainwindow/treetabbedbrowser.py
+++ b/qutebrowser/mainwindow/treetabbedbrowser.py
@@ -195,6 +195,25 @@ class TreeTabbedBrowser(TabbedBrowser):
 
         self.widget.tree_tab_update()
 
+    def tabs(
+        self,
+        include_hidden: bool = False,
+    ) -> List[browsertab.AbstractTab]:
+        """Get a list of tabs in this browser.
+
+        Args:
+            include_hidden: Include child tabs which are not currently in the
+                            tab bar.
+        """
+        return [
+            node.value
+            for node
+            in self.widget.tree_root.traverse(
+                render_collapsed=include_hidden,
+            )
+            if node.value
+        ]
+
     @pyqtSlot('QUrl')
     @pyqtSlot('QUrl', bool)
     @pyqtSlot('QUrl', bool, bool)
diff --git a/qutebrowser/misc/sessions.py b/qutebrowser/misc/sessions.py
index fe8e7ba24..35c36f203 100644
--- a/qutebrowser/misc/sessions.py
+++ b/qutebrowser/misc/sessions.py
@@ -321,15 +321,14 @@ class SessionManager(QObject):
                 win_data['private'] = True
 
             win_data['tabs'] = []
-            for i, tab in enumerate(tabbed_browser.widgets()):
-                active = i == tabbed_browser.widget.currentIndex()
+            for tab in tabbed_browser.tabs(include_hidden=True):
+                active = tab == tabbed_browser.current_tab()
                 tab_data = self._save_tab(tab,
                                           active,
                                           with_history=with_history)
                 if tabbed_browser.is_treetabbedbrowser:
                     node = tab.node
                     node_data = {
-                        'node': node.uid,
                         'parent': node.parent.uid,
                         'children': [c.uid for c in node.children],
                         'collapsed': node.collapsed,
@@ -510,7 +509,7 @@ class SessionManager(QObject):
         except ValueError as e:
             raise SessionError(e)
 
-    def _load_tree(self, tabbed_browser, tree_data):
+    def _load_tree(self, tabbed_browser, tree_data, legacy=False):
         tree_keys = list(tree_data.keys())
         if not tree_keys:
             return None
@@ -527,14 +526,22 @@ class SessionManager(QObject):
             nonlocal tab_to_focus
             nonlocal index
             index += 1
-            tab_data = tree_data[uid]
-            node_data = tab_data['treetab_node_data']
+            if legacy:
+                node_data = tree_data[uid]
+                tab_data = node_data['tab']
+            else:
+                tab_data = tree_data[uid]
+                node_data = tab_data['treetab_node_data']
             children_uids = node_data['children']
 
             if tab_data.get('active'):
                 tab_to_focus = index
 
-            new_tab = tabbed_browser.tabopen(background=False)
+            new_tab = tabbed_browser.tabopen(
+                background=False,
+                related=False,
+                idx=index,
+            )
             self._load_tab(new_tab, tab_data)
 
             new_tab.node.parent = root_node
@@ -547,8 +554,41 @@ class SessionManager(QObject):
             child = recursive_load_node(child_uid)
             child.parent = root_node
 
+        # Make sure any collapsed tabs are removed from the widget.
+        # Since we only set the "collapsed" attribute after loading the tab,
+        # and the tree only gets updated in the above loop on tab loads. So if
+        # the last set of tabs we load is a collapsed group this children
+        # won't know they are support to be hidden yet.
+        tabbed_browser.widget.tree_tab_update()
+
         return tab_to_focus
 
+    def _load_legacy_tree_tabs(self, win, tabbed_browser):
+        """Load the "legacy" tree session format.
+
+        For a number of years (pull #4602) tree tabs used a session format
+        that wasn't backwards compatible with prior session formats. In that
+        tab data was a child of a tree node. Now it's been switched to the
+        other way around. This method (along with a conditional in
+        `_load_tree()`) handle loading the old format for early adopters who
+        have session they don't want to have to rebuild.
+
+        Returns:
+          a. `(None, None)` if tree tabs where loaded, or
+          b. or a tuple of the tab index to focus and a flat list of tabs that
+             need loading if tree tabs is turned off.
+        """
+        plain_tabs = None
+        tab_to_focus = None
+        tree_data = win.get('tree')
+        if tabbed_browser.is_treetabbedbrowser:
+            tab_to_focus = self._load_tree(tabbed_browser, tree_data, legacy=True)
+            tabbed_browser.widget.tree_tab_update()
+        else:
+            plain_tabs = [tree_data[i]['tab'] for i in tree_data if
+                          tree_data[i]['tab']]
+        return tab_to_focus, plain_tabs
+
     def _load_window(self, win):
         """Turn yaml data into windows."""
         window = mainwindow.MainWindow(geometry=win['geometry'],
@@ -557,7 +597,13 @@ class SessionManager(QObject):
                                     window=window.win_id)
         tab_to_focus = None
 
-        tabs = win['tabs']
+        legacy_tree_loaded = False
+        if win.get('tree'):
+            tab_to_focus, tabs = self._load_legacy_tree_tabs(win, tabbed_browser)
+            legacy_tree_loaded = not tabs
+        else:
+            tabs = win['tabs']
+
         # restore a tab tree only if the session contains treetab
         # data and tree tabs are enabled.
         # Otherwise, restore tabs "flat"
@@ -565,10 +611,14 @@ class SessionManager(QObject):
             tabbed_browser.is_treetabbedbrowser
         if load_tree_tabs:
             tree_data = reconstruct_tree_data(win)
-            self._load_tree(tabbed_browser, tree_data)
-        else:
+            tab_to_focus = self._load_tree(tabbed_browser, tree_data)
+        elif not legacy_tree_loaded:
             for i, tab in enumerate(tabs):
-                new_tab = tabbed_browser.tabopen(background=False)
+                new_tab = tabbed_browser.tabopen(
+                    background=False,
+                    related=False,
+                    idx=i,
+                )
                 self._load_tab(new_tab, tab)
                 if tab.get('active', False):
                     tab_to_focus = i
diff --git a/tests/end2end/features/sessions.feature b/tests/end2end/features/sessions.feature
index 9a61baf61..2f121132f 100644
--- a/tests/end2end/features/sessions.feature
+++ b/tests/end2end/features/sessions.feature
@@ -440,3 +440,18 @@ Feature: Saving and loading sessions
         - data/numbers/2.txt (active) (pinned)
         - data/numbers/4.txt
         - data/numbers/3.txt
+
+  # Make sure the new_position.related setting doesn't change the tab order
+  # when loading from a session.
+  Scenario: Loading a session with tabs.new_position.related=prev
+      When I open data/numbers/1.txt
+      And I open data/numbers/2.txt in a new tab
+      And I open data/numbers/3.txt in a new tab
+      And I run :session-save foo
+      And I set tabs.new_position.related to prev
+      And I run :session-load -c foo
+      And I wait until data/numbers/3.txt is loaded
+      Then the following tabs should be open:
+        - data/numbers/1.txt
+        - data/numbers/2.txt
+        - data/numbers/3.txt (active)
diff --git a/tests/end2end/features/treetabs.feature b/tests/end2end/features/treetabs.feature
index f32ff3d07..27bc72fa4 100644
--- a/tests/end2end/features/treetabs.feature
+++ b/tests/end2end/features/treetabs.feature
@@ -25,3 +25,69 @@ Feature: Tree tab management
         Then the following tabs should be open:
             - data/numbers/1.txt
               - data/numbers/2.txt (active)
+
+    Scenario: Collapse a 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 run :tab-focus 2
+        And I run :tree-tab-toggle-hide
+        Then the following tabs should be open:
+            - data/numbers/1.txt
+              - data/numbers/2.txt (active) (collapsed)
+                - data/numbers/3.txt
+
+    Scenario: Load a collapsed subtree
+        # Same setup as above
+        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 run :tab-focus 2
+        And I run :tree-tab-toggle-hide
+        # Now actually load the saved session
+        And I run :session-save foo
+        And I run :session-load -c foo
+        And I wait until data/numbers/1.txt is loaded
+        And I wait until data/numbers/2.txt is loaded
+        And I wait until data/numbers/3.txt is loaded
+        # And of course the same assertion as above too
+        Then the following tabs should be open:
+            - data/numbers/1.txt
+              - data/numbers/2.txt (active) (collapsed)
+                - data/numbers/3.txt
+
+    Scenario: Uncollapse a 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 run :tab-focus 2
+        And I run :tree-tab-toggle-hide
+        And I run :tree-tab-toggle-hide
+        Then the following tabs should be open:
+            - data/numbers/1.txt
+              - data/numbers/2.txt (active)
+                - data/numbers/3.txt
+
+    # Same as a test in sessions.feature but tree tabs and the related
+    # settings.
+    Scenario: TreeTabs: Loading a session with tabs.new_position.related=prev
+        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 :session-save foo
+        And I set tabs.new_position.related to prev
+        And I set tabs.new_position.tree.new_child to last
+        And I set tabs.new_position.tree.new_toplevel to prev
+        And I run :session-load -c foo
+        And I wait until data/numbers/1.txt is loaded
+        And I wait until data/numbers/2.txt is loaded
+        And I wait until data/numbers/3.txt is loaded
+        And I wait until data/numbers/4.txt is loaded
+        Then the following tabs should be open:
+          - data/numbers/1.txt
+            - data/numbers/2.txt (active) (collapsed)
+              - data/numbers/3.txt
+          - data/numbers/4.txt