Tree tabs integration #8082
Draft
Tree tabs integration #8082
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix a crash happening when tree tabs are disabled and some tabs are closed
This was a line from kepi's code that I never touched nor understood. The original comment was "this was removed otherwise it was making a hell of a mess in tree-tabs". There seems to be no problem now, however.
Useful for recursively closing tabs preserving correct order.
This happened when :tab-close -r was called right after a normal :tab-close. Then :undo would restore both the last subtree and the previous single tab. This commit fixes this issue.
And... make flake8 happy
Now correctly handles case where not all branches have same depth
We have one (1) use case where a caller wants to get all tabs from a
tabbed browser, without caring whether they are mounted in the tab bar
or not. I was wondering what an API would look like that let callers ask
that without having to worry about whether the tabbed browser had tree
tabs enabled or not, while still maintaining the more common use case of
only getting widgets that are visible, or focusable, to the user.
Right now the best option I can come up with is this
`tabs(include_hidden=True)` method. This provides a clear API that makes
it obvious how to achieve both use cases (apart from the overloaded
meaning of "hidden" regarding widget visibility). I think referring
to "tabs" is better than "widgets" too, it requires you to hold less
information in your head about what these widgets in particular are.
Am I going to put any effort into attempting to deprecate `.widgets()`?
Nah.
Despite what I think is a fine API I do have some concerns:
*extensibility* One of the goals of this attempt at tree tabs and
productionising it is to see what extensions to core logic would look
like without having to change core classes. I'm not sure if this is a
good example or not because the core class just has the `.widgets()` which
is very much tied to the QTabBar. It doesn't have the concept of tabs
that exist and are children of a TabbedBrowser but aren't in the
QTabBar. So it's a fundamentally new piece of information to express and
we can't just unilaterally return tabs that aren't in the tab bar
because I can see it'll break some things.
So perhaps it would be more fitting with the "extensibility case study"
refactor goal to require the caller to have more knowledge. But I still
don't want callers iterating trees if they don't have to be, so maybe
something like:
class TabbedBrowser:
def widgets():
...
class TreeTabbedBrowser:
def widgets(include_hidden=False):
...
tabbed_browser = ...
if isinstance(tabbed_browser, TreeTabbedBrowser):
tabs = tabbed_browser.widgets(include_hidden=True)
else:
tabs = tabbed_browser.widgets()
Since we are going to be merging this into the core codebase I don't
want to be inflicting that on users of tabs in the extensions API (when
we add them), but perhaps it's a pattern we can recommend for actual
extensions, when they show up.
*ownership and hidden tabs* For tabs associated with a window but not
visible in the tab bar, who should they be owned by? Currently
TabbedBrowser doesn't attempt to keep track of tabs at all, that's all
handled by the TabWidget. But now we have tabs that the TabWidget
doesn't know about. They are only referenced by the tree structure under
TabbedBrowser. Should the browser be managing tabs and just telling the
tab bar what to show? (Which is essentially what is happening now but
only for the tree case.) Or should we be pushing the tab hiding
functionality into the TabWidget?
Having tabs not in the tab bar can cause issues, mainly because it
causes violates some assumptions in the code that all tabs have an index
in the tab bar. But putting the tabs in the tab bar that you are not
showing can cause usability issues with, for example, navigating to a
tab via count.
When we implement our own tab bar we are probably going to re-do the
displaying of the tree structure to be a bit more "native". When we do
that should we move the tab hiding into the tab bar too? I guess the API
would end up looking like it does with the tree case today, hidden tabs
wouldn't get indices and you would want ways to both iterate through
visible tabs and all tabs (and maybe hidden tabs too, but we don't have
that currently without traversing the tree yourself). I imagine in that
case the tree structure would be part of the tab bar and in the "flat"
case it would just always put stuff at the top level, and complain if
you tried to demote.
That rambling was mostly driven by so many callers going
`tabbed_browser.widget.currentTab()` etc. Which I don't think is a great
API. Beyond an attribute called "widget" being not very meaningful have
tab accessing and manipulating commands spread between and parent and a
child seems like it could be confusing. So I settled on the future logic
being in the tab widget (or in a tree in the tab widget), does that
change anything or dictate what they should be owned by? No. Oh well. I
kinda want to say tabs should be owned by the tabbed browser but I don't
know what practical implications that would have. The interface to
callers would remain predominantly the tabbed browser and the logic
would probably continue to be mixed between that and the tab widget.
Looks like when we refactored the session support to be backwards compatible we forgot to assign tab_to_focus so it was ending up focusing the last tab in the session for some reason. I guess that was just the last one added to the tab.
With, presumably, rare combinations of settings tab order in windows loaded from session can be different from the window the session was saved from. In particular with `tabs.new_position.related=prev` the tabs would be loaded in reverse order! This commit changes tabs loaded from sessions to 1) override positioning related kwargs to tabopen so that it doesn't look at user sessions 2) provide an explicit index to use for new tabs. This particular test passes with either one of the newly passed kwargs. We don't strictly need both of them, but while being explicit we may as well override everything to be extra clear. While looking at this code you may be asking why background is set to True for all the tabs when only one of them is going to be focused? Good question, I don't think it should be, see #6983 (comment) But since that is explicitly set it should probably be a separate PR to change it. Looks like the original PR to fix that got caught up in wider scope and trying to do a new style of e2e-but-python tests. It should be easy enough to write an e2e that loads a page with JS that reports that visibility state. TODO: * add a similar test for tree tabs when the scaffolding is there (with `new_position.tree.new_toplevel=prev` too) * override sibling too?
The first three tests make sure that the "collapsed" attribute makes it through the session machinery and behaves as expected. Third one is a bit sneaky because it tests a case that was causing issues, but this test couldn't actually reproduce. The problem was that before we were passing `related=False` to `tabopen()` the tab after a collapsed group would be loaded hidden when it wasn't supposed to be. This was only in the UI though, if you saved a session it would be correct, so we can't test it in these e2e tests currently...
In 3ec2000 I identified a few cases where we could be asked to call into the tree structure based on a widget index with evidence that the tree and widget where out of sync. In particular where there were a different amount of items in each. Previously I had ignored desyncs during session loads but now I've found a case where the desync remains after the session was loaded, for a short time. It can be reproduced (most of the time) with: python3 -m qutebrowser -T -s tabs.tree_tabs true ":open about:blank?1" ":open -tr about:blank?2" ":open -tr about:blank?3" ":tab-focus 2" ":tree-tab-toggle-hide" ":cmd-later 200 session-save foo" ":cmd-later 300 session-load -c foo" This only happens if the last set of tabs we open is a collapsed group. There would have been no update event fired since we set the collapsed attribute. If this issue shows up again a more robust approach might be to not set the collapsed attribute in the loop, but load all the tabs visible then go and set the collapsed attribute on all the nodes that need it and fire an update. I initially reproduced this with the "Load a collapsed subtree" end2end test.
I got a failure in CI on windows for the "Load a collapsed subtree" where it got an index error when trying to load the active history entry for a tab in a session. This is likely because the session was saved before the tab was loaded. I can reproduce it locally by changing the last test to wait for only the first tab, instead of the last.
…psed_tabs_and_legacy_session_support Fix loading collapsed tabs and legacy session support
I don't believe this call is required, and disconnecting something that was connected in the parent class could be surprising. This was added in commit b8d9622 which says it was done for performance reasons because `update_tab_titles()` many time when `:tree-tab-collapse` was called on large trees. I agree that `update_tab_titles()` is called lots. The tree operations could be a bit more atomic. I did some work previously to prevent title updates during update (for consistency reasons, not performance reasons) in dcee69f that helps with the `:tree-tab-collapse` case. I can see that `update_tab_titles()` is called once for each tab being hidden or show but it now doesn't do any work (`self._tab_title_update_disabled` is `True`) for any of the calls except that last one. Surprisingly the `tabMoved` signal doesn't actually seem to be fired when hiding or showing a tab group. Possibly the refactoring in late 2019 (e795eac and friends) changed it so we aren't trigging that case anymore.
The method in this function where added in: 411d39a "Add config option to enable tree-tabs". The `update_tab_titles()` line has the comment "must also be called when deactivating" because back then it was right before something gated on `if tabs.tree_tabs:`. But: 1. tree_tab_update() already calls update_tab_titles() 2. we don't currently support toggling tree tab off and on on an existing window So I don't believe the calls in this override are needed with the current design. `_init_config()` is called on `__init__()` and `tabs.*` config items changing. To double check if these is needed for anything I've tried a bunch of things like: 1. change tab format 2. show/hide tab group 3. change tabs.position 4. show/hide tab bar - huh, when the tab bar is re-shown it's scrolled down so on the title of the active tab and below are shown, is this new? But that happens with a flat tabs window too. It only happens sometimes though. Oh, it's not happening on 6.4.2 but is on 6.6.0 and I don't remember it happending on 6.7.0. Must be a Qt issue that hopefully got fixed. And it all seemed to behave normally. I'm not sure what else to check as everything else tree tab specific that I can think of relates to moving tabs, which should be updating things already.
These two values are already defaulted in the parent class, so we don't need
to set them in the child (and weren't doing it consistently anyway).
I'm trying to identify places where we can't keep the tree tabs
knowledge self contained and speculate how we could handle that for
actually external code like extensions. So, thinking about how we would
gracefully handle some non-core code adding tab title format
placeholders, especially when that code could be disabled (eg, an
extension):
TabWidget will throw a KeyError if you have a format specifier which isn't
filled in. So we would have to decide how to handle that. Log a warning and
ignore it? Silently set it to `''`? Leave it as the literal `{thing}`, curly
braces and all? Presumable that API would look something like
`register_title_format_placeholder(placeholder: str, getter:
Callable[AbstractTab])`.
The `FormatString` config type used by `tabs.title.format` would have to
change as well to pull allowed placeholders from some global registry instead
of having the specified ahead of time in configdata.yml.
Even then, if you disabled an extension so it didn't register its placeholder
and you have the placeholder configured in your format string, what should
happen?
PS: jinja defaults to `''` for placeholders without values.
ref: #30
…c_cleanups TreeTabWidget: duplication reduction and misc cleanup
The only thing that the overridden init was doing was creating a different widget then duplicating the signal setup. I've moved the widget creation out to a helper function so that we only need to override that. This has the benefit of not needlessly creating the non-tree TabWidget. An alternative to moving the tab widget out to a helper would be to add a `_set_tab_widget(widget)` method that did the signal connecting and then keep the widget creating in init and pass the widget to the setter. But then 1) there would be a bigger diff 2) I would have to think about what to do if a widget was already set, complain? do nothing? disconnect and dispose of the previous one?
This suffix will reduce some boilerplate of setting up a tree tab structure in end2end tests.
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.
Not sure why these just started showing up now, they aren't related to what I've changed. Pylint update maybe? qutebrowser/mainwindow/treetabbedbrowser.py:292:8: W0201: Attribute '_tree_tab_child_rel_idx' defined outside __init__ (attribute-defined-outside-init) qutebrowser/mainwindow/treetabbedbrowser.py:293:8: W0201: Attribute '_tree_tab_sibling_rel_idx' defined outside __init__ (attribute-defined-outside-init) qutebrowser/mainwindow/treetabbedbrowser.py:294:8: W0201: Attribute '_tree_tab_toplevel_rel_idx' defined outside __init__ (attribute-defined-outside-init) qutebrowser/mainwindow/treetabbedbrowser.py:11:0: W0611: Unused QSizePolicy imported from qutebrowser.qt.widgets (unused-import)
If we are restoring a tab whos old child has since been removed then `root.get_descendent_by_uid(child_uid)` can return None and we should skip that child uid. Also added some tests around undoing tree tab closes, one happy path and a few odd cases. We could theoretically be a little cleverer about where we put tabs back, if if the parent is gone but the children are still there maybe we could insert it above the children instead of dragging them with us up to the top of the tree? Oh well, these are edge cases, they can be improved later. Minor changes: * remove redundant `list()` call * add explanatory comment on why we are worrying about non-TreeTabUndo type undo entries. (Actually all of the entries in the list at that point we'll be ignoring so we could skip earlier.)
Following from the previous commit that added an `UndoEntry.from_tab()` so that the TabbedBrowser could have a generic way of creating an undo entry, this commit adds an `UndoEntry.restore_into_tab()` that lets the parent TabbedBrowser handle restoring a tab without the TreeTabbedBrowser class overriding the `undo()` method. Mostly this just moves code around so that TreeTabbedBrowser doesn't have to store the undo entries before calling the super method, and we don't have to change the parent method to return something just for a child class. The core to that is being able to call a method per undo entry instead of going through all of them at once. We could have added a new helper function in TabbedBrowser to handle that and overridden it in TreeTabbedBrowser. But I think it is nice to tuck the undo related code into the undo related object anyhow. The code doesn't look like it has moved because I changed the variable names: newtab -> tab and entry -> self.
This has two benefits: 1. removes tree lines of duplication 2. means that the entries in `AbstractTab.undo_entry` are always of the same type. It's a bit odd that the undo entry classes are private but that attribute they are stored in is public. I think the queue was made public just recently so perhaps an oversight. The `kw_only` arg to dataclass is because the parent class sets `created_at` with a default argument and dataclass enumerates the attributes of the child class after the parent. If you tell it all attributes need to be passed as keywords it doesn't care about that.
I've made TreeUndoEntry a subclass of UndoEntry. But UndoEntry sets a default value for the `created_at` attribute, which means if any of the extra attributes defined on the child class don't define a default an error is thrown complaining about non-keyword arguments after a keyword only one. Python 3.10 adds a kw_only argument you can pass to the dataclass decorator but it looks like we currently support python 3.8+. See https://www.trueblade.com/blogs/news/python-3-10-new-dataclass-features So a more compatible way to do it is to set `init=False` on the attribute with a default, if you don't need to pass it in the constructor. Which we don't because it's only set by the caller in tests. Hence changing the tests to set it a different way. A bit uglier but should be fine. Also fix my "retains_sort_order" test from #5964 that wasn't actually being run...
browsertab.py: bool(TabWidget) is False is len(TabWidget) is 0. Here I'm pretty sure we are only wanting to see if the parent is None or not, so fix that check. I'm not sure in which case parent would be None, I suspect never, but that's what the type hint says... I suspect that setting `node = Node(self, parent=None)` is not the correct way to signal that this isn't a usable node. We should be setting `node = None`, but I wait till we have correct type hints to do that because it'll probably be a pain to deal with downstream. treetabbedbrowser.py: Now that a node's parent should always be set we don't have to see it to tree_root in tabopen. It was hard to reason about why this code was here and in what situation it would be needed. Turns out it was a bug somewhere else. Short circuit positioning code if tabs_are_windows too. Particularly because we get called twice in that case! Don't call tree_tab_update() when short circuiting positioning code. I hope that's okay, I'm not sure why this would be needed. We are either updating an existing tab (no change in position) or we are opening a single tab in a new window (no extra tree related positioning required). It's a persistent issue that `tree_tab_update()` is sprinkled through the code without any justification, it's not a great pattern for maintenance where maintainers can't reason about why a call is made and if it's required.
I went in to tabopen feeling a bit suspicious that the positioning code was in some way duplicating `_get_new_tab_idx` in the parent class, or if this code could be moved out to a common helper method or called by a signal. When a new tab is opened we need to position it in the tree structure. For related and sibling tabs we need to know the tab that the new one is opened "from". The tab related code deals with three new "new_position" settings that the parent method doesn't know about. For pulling it out behind a signal, we have the exiting `new_tab` signal, but that doesn't say what the previous tab was. We could maybe use the `TabbedBrowser._now_focused` or `TabbedBrowser.tab_dequeue` for that, but that seems like it might be unfairly adding some new responsibility onto those attributes? Even then we still would have no way to know whether the user had requested a `related` or `sibling` tab. So it seems all the code around tab positioning left here is specific to tree tabs, assuming all the new "new_position" settings are needed, and it's integrating with the existing code in a fine way, assuming we are aiming for keeping the new code in subclasses. But that all begs the question of how would an extension proper do it? I think it would do it much the same way, but instead of subclassing TabbedBrowser it would define a new :open command and ask uses to switch to that (or replace it if we allow that). Then that new command would call TabbedBrowser to get a tab and then do it's own extra stuff. And hook into the `new_tab` signal to handle a new one being created via some other means. Although that would not let if know whether a tab was opened as related or not (eg created from clicking a link vs loading a session). So maybe something to work on there for the extension API. tabopen: * add comment explaining necessity of the method * add some early exits - I think this is fairly important to limit the number of possible logic paths maintainers have to keep in their heads when working with the more logic heavy code down below * didn't add an early exit for `idx is not None` because I'm not 100% sure when that is set and I'm not confident enough in our test coverage to change it right now position_tab: * change to work with just Nodes instead of tabs - makes testing easier * change the initial duplicate avoidance code to be more clear to me, probably it would be even more clear is `parent` was called `new_parent` test_treetabbedbrowser * add some tests for position_tab - not too happy with the mocking, should probably see how it looks with a proper tab, they just feel a bit heavy weight with the amount of mocking they bring with them
probably, as mypy isn't passing on this branch anyhow. Also the whole tabbedbrowser class looks pretty lightly type hinted.
…_misc_cleanups Tree/8078 treetabbedbrowser misc cleanups
Add more end2end tests for tree tabs feature
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR tracks the in-development Tree Tabs feature. It replaces a prior PR for the purpose of bringing development into the main qutebrowser repo.
TODO: put a nice animated image here showing how awesome tree tabs are
PRs meant to add to the Tree Tabs feature should have the
tree-tabs-integrationbranch set as their base branch.The project board tracking outstanding work and progress toward the whole feature being merged is here,. see the right hand side bar for instructions on contributing: https://github.com/orgs/qutebrowser/projects/3/views/1
The previous PR where the initial version of the feature was developed is here, it has many valid comments on it still: #4602
Closes: #927