From 88cfc1c3edbfcadec35ba9653e2d9b7608bdb5a2 Mon Sep 17 00:00:00 2001 From: toofar Date: Mon, 29 Jan 2024 23:28:29 +1300 Subject: Fix tab widget benchmark tests In 8aa88b83e I added a dependency in the TabWidget in `parent.is_shutting_down` (assuming parent is a TabbedBrowser, but without updating the type hints...). Since the benchmark test wasn't setting a parent it's been breaking since then, oops. That's going to be breaking all the branches based off of the tree tabs branch, so we should probably cherry pick this back to the tree-tabs-integration branch. And consider updating the TabWidget parent kwarg type hint (and make the dummpy parent a mock/spy/double of the TabbedBrowser I guess). Not sure where the size of 640x480 came from before hand but when I added a parent it went to 100x30. --- tests/unit/mainwindow/test_tabwidget.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/unit/mainwindow/test_tabwidget.py b/tests/unit/mainwindow/test_tabwidget.py index a249a6d1c..59defeb50 100644 --- a/tests/unit/mainwindow/test_tabwidget.py +++ b/tests/unit/mainwindow/test_tabwidget.py @@ -11,6 +11,7 @@ import pytest from unittest.mock import Mock from qutebrowser.qt.gui import QIcon, QPixmap +from qutebrowser.qt.widgets import QWidget from qutebrowser.mainwindow import tabwidget from qutebrowser.utils import usertypes @@ -21,7 +22,14 @@ class TestTabWidget: @pytest.fixture def widget(self, qtbot, monkeypatch, config_stub): - w = tabwidget.TabWidget(0) + class DummyParent(QWidget): + def __init__(self): + super().__init__() + self.is_shutting_down = False + self.show() + + w = tabwidget.TabWidget(0, parent=DummyParent()) + w.resize(640, 480) qtbot.add_widget(w) monkeypatch.setattr(tabwidget.objects, 'backend', usertypes.Backend.QtWebKit) -- cgit v1.2.3-54-g00ecf From 2c149c2faa69bf15c0f5a9cd53562e3335157f38 Mon Sep 17 00:00:00 2001 From: toofar Date: Tue, 6 Feb 2024 13:20:11 +1300 Subject: Fix lint: Various lint fixes. The ones we will likely revisit later are: * unused sibling arg in parent TabbedBrowser class * seperate-but-similar UndoEntry implementations * variables with two leading underscores in notree * `_tree_tab_give()` implementation qutebrowser/browser/commands.py:499:9: F841 local variable 'oldroot' is assigned to but never used In `_tree_tab_give()` there is a check to see if a node being moved needs to be re parented. That used to use `is not oldroot` and I changed it in c2bc549605a3c to be `parent.uid in uid_map`, eg is this a child node of the tree being moved. Now we don't need oldroot anymore but I forgot to clean it up. PS: not sure why that check is so complicated. Won't it only ever be false for the top node being moved? So we can just say `if node != current_widget.node:`? I guess that's irrelevant if that's all going to be re-written anyhow. qutebrowser/mainwindow/treetabbedbrowser.py:83:1: D202 No blank lines allowed after function docstring qutebrowser/mainwindow/treetabwidget.py:77:22: C419 Unnecessary list comprehension passed to all() prevents short-circuiting - rewrite as a generator. qutebrowser/mainwindow/treetabwidget.py:78:13: C1804: "self.widget(idx).url().toString() == ''" can be simplified to "not self.widget(idx).url().toString()", if it is striclty a string, as an empty string is falsey (use-implicit-booleaness-not-comparison-to-string) qutebrowser/mainwindow/treetabwidget.py:132:16: R5501: Consider using "elif" instead of "else" then "if" to remove one indentation level (else-if-used) Ran out of energy at the double negative. Hope that's okay. qutebrowser/mainwindow/treetabbedbrowser.py:131:35: W0613: Unused argument 'idx' (unused-argument) Add an unused-argument ignore line because, while we aren't using index (tab undo entries remember their parents instead of their index) I think we'll be pulling this method up to the parent class later which does use it. qutebrowser/mainwindow/treetabbedbrowser.py:198:4: W0237: Parameter 'idx' has been renamed to 'sibling' in overriding 'TreeTabbedBrowser.tabopen' method (arguments-renamed) Urghgh, I have no idea why this would be like this. I looked through the codebase both for `sibling` and `tabopen` and didn't see this being called with positional arguments beyond the url. So seems safe to change it. Probably some linter will be complaining that it still doesn't match the signature of the parent... qutebrowser/mainwindow/treetabbedbrowser.py:19:0: W0611: Unused log imported from qutebrowser.utils (unused-import) qutebrowser/mainwindow/treetabbedbrowser.py:214:0: I0021: Useless suppression of 'arguments-differ' (useless-suppression) And I guess this is the complaint about the arguments differing. But it says we don't need the suppression anymore? We'll, I'll add it to the parent and see what that does. qutebrowser/misc/notree.py:60:4: C0103: Class constant name "PRE" doesn't conform to snake_case naming style (invalid-name) qutebrowser/misc/notree.py:61:4: C0103: Class constant name "POST" doesn't conform to snake_case naming style (invalid-name) qutebrowser/misc/notree.py:62:4: C0103: Class constant name "POST_R" doesn't conform to snake_case naming style (invalid-name) I assume these are copied from anytree. Not a huge fan, why are the variables names so much less readable than the values? qutebrowser/misc/notree.py:221:16: R5501: Consider using "elif" instead of "else" then "if" to remove one indentation level (else-if-used) qutebrowser/misc/notree.py:289:12: W0719: Raising too general exception: Exception (broad-exception-raised) `promote()` is only called from one place in commands.py currently. So we are essentially validating config, so this error "shouldn't happen". If this was going to be called from more places we should probably make an enum in notree and maybe generate the config from that? Or validate it against the config (in a test?) or something. qutebrowser/misc/notree.py:256:4: W0238: Unused private member `Node.__add_child(self, node: 'Node[T]')` (unused-private-member) qutebrowser/misc/notree.py:260:4: W0238: Unused private member `Node.__disown(self, value: 'Node[T]')` (unused-private-member) qutebrowser/misc/notree.py:190:12: W0238: Unused private member `Node.__modified` (unused-private-member) I assume these are copied from anytree. The internet says variables with two leading underscores are mangled by the interpreter to be `__classname__variable` so that you don't have to worry about child and parent classes fighting over variable names. We don't have any examples of this pattern in the codebase currently and this Node class seems designed for composition, not inheritance. So I think I would prefer to change them to have a single underscore. That seems like something that people might want to discuss though, so for now silence pylint as it's presumably comparing some mangled vs unmangled name (eg if I rename them it's happy). qutebrowser/misc/notree.py:68:0: I0021: Useless suppression of 'invalid-name' (useless-suppression) --- qutebrowser/browser/commands.py | 1 - qutebrowser/mainwindow/tabbedbrowser.py | 1 + qutebrowser/mainwindow/treetabbedbrowser.py | 12 +++++---- qutebrowser/mainwindow/treetabwidget.py | 39 +++++++++++++++-------------- qutebrowser/misc/notree.py | 29 ++++++++++++--------- 5 files changed, 45 insertions(+), 37 deletions(-) diff --git a/qutebrowser/browser/commands.py b/qutebrowser/browser/commands.py index d7212d6ce..6784f0c1d 100644 --- a/qutebrowser/browser/commands.py +++ b/qutebrowser/browser/commands.py @@ -496,7 +496,6 @@ class CommandDispatcher: # second pass: copy tree structure over newroot = tabbed_browser.widget.tree_root - oldroot = self._tabbed_browser.widget.tree_root for node in traversed: if node.parent.uid in uid_map: uid = uid_map[node.uid] diff --git a/qutebrowser/mainwindow/tabbedbrowser.py b/qutebrowser/mainwindow/tabbedbrowser.py index 89b43285d..c07b9e1ad 100644 --- a/qutebrowser/mainwindow/tabbedbrowser.py +++ b/qutebrowser/mainwindow/tabbedbrowser.py @@ -616,6 +616,7 @@ class TabbedBrowser(QWidget): background: bool = None, related: bool = True, idx: int = None, + sibling: bool = False, # pylint: disable=unused-argument ) -> browsertab.AbstractTab: """Open a new tab with a given URL. diff --git a/qutebrowser/mainwindow/treetabbedbrowser.py b/qutebrowser/mainwindow/treetabbedbrowser.py index 5e2f796f1..75a3c55f8 100644 --- a/qutebrowser/mainwindow/treetabbedbrowser.py +++ b/qutebrowser/mainwindow/treetabbedbrowser.py @@ -16,7 +16,6 @@ from qutebrowser.mainwindow.tabbedbrowser import TabbedBrowser from qutebrowser.mainwindow.treetabwidget import TreeTabWidget from qutebrowser.browser import browsertab from qutebrowser.misc import notree -from qutebrowser.utils import log @dataclasses.dataclass @@ -81,7 +80,6 @@ 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) @@ -128,7 +126,12 @@ class TreeTabbedBrowser(TabbedBrowser): self.widget.tree_tab_update() - def _add_undo_entry(self, tab, idx, new_undo): + 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 @@ -199,8 +202,8 @@ class TreeTabbedBrowser(TabbedBrowser): self, url: QUrl = None, background: bool = None, related: bool = True, - sibling: bool = False, idx: int = None, + sibling: bool = False, ) -> browsertab.AbstractTab: """Open a new tab with a given url. @@ -211,7 +214,6 @@ class TreeTabbedBrowser(TabbedBrowser): focused tab. Follows `tabs.new_position.tree.sibling`. """ - # pylint: disable=arguments-differ # we save this now because super.tabopen also resets the focus cur_tab = self.widget.currentWidget() tab = super().tabopen(url, background, related, idx) diff --git a/qutebrowser/mainwindow/treetabwidget.py b/qutebrowser/mainwindow/treetabwidget.py index 082b7a079..92e18ae77 100644 --- a/qutebrowser/mainwindow/treetabwidget.py +++ b/qutebrowser/mainwindow/treetabwidget.py @@ -75,7 +75,7 @@ class TreeTabWidget(TabWidget): # empty_urls here is a proxy for "there is a session being loaded into # this window" empty_urls = all( - [self.widget(idx).url().toString() == "" for idx in range(self.count())] + not self.widget(idx).url().toString() for idx in range(self.count()) ) if empty_urls and is_hidden: # All tabs will be added to the tab widget during session load @@ -121,24 +121,25 @@ class TreeTabWidget(TabWidget): for node in self.tree_root.traverse(): if node.value is None: continue - if any(ancestor.collapsed for ancestor in node.path[:-1]): - if self.indexOf(node.value) != -1: - # node should be hidden but is shown - cur_tab = node.value - idx = self.indexOf(cur_tab) - if idx != -1: - self.removeTab(idx) - else: - if self.indexOf(node.value) == -1: - # node should be shown but is hidden - parent = node.parent - tab = node.value - name = tab.title() - icon = tab.icon() - if node.parent is not None: - parent_idx = self.indexOf(node.parent.value) - self.insertTab(parent_idx + 1, tab, icon, name) - tab.node.parent = parent # insertTab resets node + + should_be_hidden = any(ancestor.collapsed for ancestor in node.path[:-1]) + is_shown = self.indexOf(node.value) != -1 + if should_be_hidden and is_shown: + # node should be hidden but is shown + cur_tab = node.value + idx = self.indexOf(cur_tab) + if idx != -1: + self.removeTab(idx) + elif not should_be_hidden and not is_shown: + # node should be shown but is hidden + parent = node.parent + tab = node.value + name = tab.title() + icon = tab.icon() + if node.parent is not None: + parent_idx = self.indexOf(node.parent.value) + self.insertTab(parent_idx + 1, tab, icon, name) + tab.node.parent = parent # insertTab resets node def tree_tab_update(self): """Update titles and positions.""" diff --git a/qutebrowser/misc/notree.py b/qutebrowser/misc/notree.py index 297673bfb..e12a8bf3f 100644 --- a/qutebrowser/misc/notree.py +++ b/qutebrowser/misc/notree.py @@ -57,15 +57,15 @@ class TraverseOrder(enum.Enum): POST_R: post-order-reverse: like POST but rightmost nodes first. """ - PRE = 'pre-order' - POST = 'post-order' - POST_R = 'post-order-reverse' + PRE = 'pre-order' # pylint: disable=invalid-name + POST = 'post-order' # pylint: disable=invalid-name + POST_R = 'post-order-reverse' # pylint: disable=invalid-name uid_gen = itertools.count(0) # generic type of value held by Node -T = TypeVar('T') # pylint: disable=invalid-name +T = TypeVar('T') class Node(Generic[T]): @@ -187,7 +187,7 @@ class Node(Generic[T]): def __set_modified(self) -> None: """If self is modified, every ancestor is modified as well.""" for node in self.path: - node.__modified = True # pylint: disable=protected-access + node.__modified = True # pylint: disable=protected-access,unused-private-member def render(self) -> List[Tuple[str, 'Node[T]']]: """Render a tree with ascii symbols. @@ -217,11 +217,10 @@ class Node(Generic[T]): result += [subtree[0]] else: result += subtree + elif child is self.children[-1]: + result.append((CORNER, child)) else: - if child is self.children[-1]: - result.append((CORNER, child)) - else: - result.append((INTERSECTION, child)) + result.append((INTERSECTION, child)) self.__modified = False self.__rendered = list(result) return list(result) @@ -253,11 +252,17 @@ class Node(Generic[T]): if order in [TraverseOrder.POST, TraverseOrder.POST_R]: yield self - def __add_child(self, node: 'Node[T]') -> None: + def __add_child( # pylint: disable=unused-private-member + self, + node: 'Node[T]', + ) -> None: if node not in self.__children: self.__children.append(node) - def __disown(self, value: 'Node[T]') -> None: + def __disown( # pylint: disable=unused-private-member + self, + value: 'Node[T]', + ) -> None: self.__set_modified() if value in self.__children: self.__children.remove(value) @@ -286,7 +291,7 @@ class Node(Generic[T]): """ if to not in ['first', 'last', 'next', 'prev']: - raise Exception("Invalid value supplied for 'to': " + to) + raise ValueError("Invalid value supplied for 'to': " + to) position = {'first': 0, 'last': -1}.get(to, None) diff = {'next': 1, 'prev': 0}.get(to, 1) count = times -- cgit v1.2.3-54-g00ecf From 5c9bfeffe4490e99d56fa53bc401cc6bacfe626e Mon Sep 17 00:00:00 2001 From: toofar Date: Tue, 6 Feb 2024 14:34:55 +1300 Subject: treetabs: minor doc proof read --- doc/treetabs.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/doc/treetabs.md b/doc/treetabs.md index 319997efb..f32ee5449 100644 --- a/doc/treetabs.md +++ b/doc/treetabs.md @@ -5,7 +5,7 @@ Tree style tabs allow you to group and manage related tabs together. Related tabs will be shown in a hierarchical fashion in the tab bar when it is on the left or right side of the browser window. It can be enabled by setting -`tabs.tree_tabs` to `true`. That setting only applies to new windows create +`tabs.tree_tabs` to `true`. That setting only applies to new windows created after it is enabled (including via saving and loading a session or `:restart`). @@ -38,10 +38,10 @@ todo: add animated illustrations? You can change how tabs relate to each other after they are created too. -* `:open`, as described in the into, has picked up some new behaviour to +* `:open`, as described in the intro, has picked up some new behavior to decide where in relation to the current tab a new one should go. It has a - new `--sibling` argument and the `--related` argument, as well as `--tab` and - `-background`, has some additional meaning. + new `--sibling` argument and the existing arguments `--related`, `--tab` and + `--background` have picked up some additional meaning to help with that. * `:tab-move` will move a tab and its children within the tree * With a `+` or `-` argument tabs will only move within their siblings (wrapping at the top or bottom) @@ -90,7 +90,7 @@ commands introduced to take advantage of the tab grouping feature. ## Settings -There are some existing settings who's behaviour will be modified when tree +There are some existing settings that will have modified behavior when tree tabs are enabled: * `tabs.new_position.related`: this is essentially replaced by @@ -144,7 +144,7 @@ either the parent or children of a node via property setters. Beyond those two setters nodes have `promote()` and `demote()` helper functions used by the corresponding commands. -Beyond those four methods to manipulate tree the tree structures nodes have +Beyond those four methods to manipulate the tree structure nodes have methods for: * traversing the tree: @@ -153,9 +153,9 @@ methods for: * `depth()` return depth in tree * collapsing a node * this just sets an attribute on a node, the traversal function respects it - * but beyond that it's up to callers to know that an un-collapsed node may + but beyond that it's up to callers to know that an un-collapsed node may be hidden if a parent node is collapsed, there are a few pieces of - calling code which do implement different behaviour for collapsed nodes + calling code which do implement different behavior for collapsed nodes * rendering unicode tree segments to be used in tab titles * our tab bar itself doesn't understand the tree structure for now, it's just being represented by drawing unicode line and angle characters to -- cgit v1.2.3-54-g00ecf