diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index 3b950602b..61574b0fb 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) @@ -220,7 +217,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 +233,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') @@ -255,36 +252,17 @@ 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() - 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) + + 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') @@ -486,37 +464,34 @@ class CommandDispatcher: tabbed_browser.close_tab(tab, add_undo=False, transfer=True) 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( + current_node.value, + add_undo=False, + transfer=True, + recursive=True, + ) @cmdutils.register(instance='command-dispatcher', scope='window') @cmdutils.argument('win_id', completion=miscmodels.window) @@ -1116,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: @@ -1157,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. @@ -1218,28 +1198,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: @@ -2059,6 +2037,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) @@ -2071,8 +2078,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) @@ -2089,8 +2095,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 @@ -2112,8 +2117,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 @@ -2130,8 +2134,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) @@ -2169,6 +2172,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 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..756e4a2f2 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 @@ -14,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 @@ -124,13 +126,58 @@ 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 + 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 @@ -262,7 +309,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 @@ -275,6 +323,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) @@ -283,7 +332,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 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..1591a8359 100644 --- a/tests/end2end/features/treetabs.feature +++ b/tests/end2end/features/treetabs.feature @@ -72,6 +72,71 @@ 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: :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 wait until data/numbers/4.txt is loaded + 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) + - 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 @@ -81,7 +146,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 @@ -96,6 +161,61 @@ 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 @@ -236,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) + """ 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):