Fix loading collapsed tabs and legacy session support #8084
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
toofar
changed the title
pinusc
reviewed
Jan 29, 2024
There was a problem hiding this comment.
Tests always sound like a good idea.
I don't like the idea of .widgets() returning things that aren't visible. Maybe adding a method .tabs() to TabbedBrowser that is just an alias of widgets, overridden in TreeTabbedBrowser to return all tabs, including collapsed. I don't think there's any places in treetabs where .widgets() explicitly expects only visible tabs, but this half-rename would also ensure compatibility. Maybe that's overcomplicating things.
pinusc
reviewed
Jan 29, 2024
There was a problem hiding this comment.
I did think people might be unhappy with the breaking change 😟
I didn't think legacy code would be great to lug around, but I'm not the maintainer here... that being said, we could keep this legacy code for now, adding a deprecation prompt, and then removing it altogether before merging into main. Loading the session with this code and then immediately saving it should "update" the old session file. This gives users a chance to update their sessions, essentially serving as a conversion script of sorts; but then we don't have to keep legacy code in.
Yeah, good point. And thanks for engagement with a comment buried in a commit message! I wanted to have a look at what existing places in the code would do if So the majority of places where tabs are iterated over or counted at the moment are specifically to do with moving or focusing tabs and wouldn't deal well with tabs that weren't in the tab widget as things are. Full notes from those greps:So if there are valid use cases for iterating over all tabs (including hidden ones) and for iterating only over visible (or focusable?) tabs, what would APIs look like that make it clear to callers clear how to do each without making them deal with concepts they don't need to? As a thought experiment: when we put tabs and windows into the extension API what should that look like? If we add a What about something like A related problem is that once we have an API that can return a mix of tabs which are in the tab widget and hidden tabs, how can the callers know which of those tabs was hidden? There are isHidden() and isVisible() QWidget methods but those are set the same for hidden and background tabs. I kinda want to add a tab.idx() which will return the index within the window, or None. That would also be useful for all the places that call browser.widget.indexOf(tab) to not have to get a reference to the browser. Anyway, that's probably a problem for another day.
I'm thinking we just remove it before merging to main and provide a userscript that'll update any sessions people have that they haven't bothered to upgrade yet. Maybe even point to the userscript in an error message if we can detect those session easy enough. Or since no-one has complained all week maybe no-one is using it 🤷 |
toofar
force-pushed
the
tree/8076_fix_loading_collapsed_tabs_and_legacy_session_support
branch
from
855a2cb
to
dea01de
Compare
|
Alright, I made some more changes:
Anyway, I think this PR has grown in scope enough. The main thing was fixes around session loading and I've managed to cover three of them. I've squeezed in legacy session support and a new API method in here too, the latter was particularly interesting. Lemme know if you have time to review in the next couple of days. Otherwise I'll go ahead and merge into the integration branch, you are of course free to comment on the PR after I've merged it to raise things that could be better! |
Tabs in collapsed tree groups aren't currently listed in tabbed_browser.widgets(), so they weren't getting saved. The tabs are still referenced by the tree structure though so grab them from there if we are dealing with a tree tabs window. I'm not too happy that we are now checking `if tabbed_browser.is_treetabbedbrowser` twice in the same methods. Seems a bit smelly, but oh well. Things we could do to help makes iterating tabs less error prone: * Make TreeTabbedBrowser.widgets() return widgets from the tree instead of the tab widget (would that break other use cases that called that that expected them to always be in the tab widget?) * Have tests to make sure this functionality doesn't break again in the future
In a2f98aa the session format for tree tab windows was changed to be backwards compatible with the prior format. After that change we can load non-tree tab windows fine but the browser crashes on startup when trying to load session in the now-legacy tree tab session format. Since people have been using the now-legacy session for years and there is no way for them to easily migrate I figure we have a few options: * provide a script to help them migrate * handle the error encountered (`KeyError: 'tabs`) when trying to load a legacy session better, eg give them a nice apologetic message * support loading the old style of sessions It didn't take much effort to support the prior format, so I propose we do that. The data is all the same, we just changed it to nodes are childs of tabs instead of the other way around.
We are now iteration over all tabs, including hidden ones that aren't in the tab widget. So we can't use the index of the current tab within the tab widget as a key into that list. Check the actual tab value instead. I made `TabbedBrowser._current_tab()` public because it's useful, so why not! I could have used `tabbed_browser.widget.currentWidget()` like some other places do but if we where doing proper type checking in session.py we would also have to do the None check that is already being done in TabbedBrowser. Also accessing `some_other_object.widget` feels a little dirty, like it should be private?
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.
toofar
force-pushed
the
tree/8076_fix_loading_collapsed_tabs_and_legacy_session_support
branch
from
1da06bf
to
3c06459
Compare
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.
toofar
force-pushed
the
tree/8076_fix_loading_collapsed_tabs_and_legacy_session_support
branch
from
3c06459
to
810088e
Compare
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.
|
Almighty, merging now. To close this out I had another look at the previous changes to session loading and have a couple more notes on that (see the diff --git a/qutebrowser/misc/sessions.py b/qutebrowser/misc/sessions.py
index dd63904cdc4..fe8e7ba24d8 100644
--- a/qutebrowser/misc/sessions.py
+++ b/qutebrowser/misc/sessions.py
@@ -281,13 +317,32 @@ def _save_all(self, *, only_window=None, with_private=False, with_history=True):
if getattr(active_window, 'win_id', None) == win_id:
win_data['active'] = True
win_data['geometry'] = bytes(main_window.saveGeometry())
- win_data['tabs'] = []
if tabbed_browser.is_private:
win_data['private'] = True
+
+ win_data['tabs'] = []
??? ^ not clear why this was moved
for i, tab in enumerate(tabbed_browser.widgets()):
active = i == tabbed_browser.widget.currentIndex()
- win_data['tabs'].append(self._save_tab(tab, active,
- with_history=with_history))
+ tab_data = self._save_tab(tab,
+ active,
+ with_history=with_history)
??? I wonder if we could have _save_tab() fill in the tree data if tab.node was set?
??? For the distant future when refactoring the sessions code (there should be an issue for it somewhere) it might be nice if we could make AbstractTab and MainWindow handle their own serialization
+ 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,
+ 'uid': node.uid
+ }
+ tab_data['treetab_node_data'] = node_data
+ win_data['tabs'].append(tab_data)
+ if tabbed_browser.is_treetabbedbrowser:
+ root = tabbed_browser.widget.tree_root
+ win_data['treetab_root'] = {
+ 'children': [c.uid for c in root.children],
+ 'uid': root.uid
+ }
data['windows'].append(win_data)
return data
@@ -455,6 +510,45 @@ def _load_tab(self, new_tab, data): # noqa: C901
except ValueError as e:
raise SessionError(e)
+ def _load_tree(self, tabbed_browser, tree_data):
+ tree_keys = list(tree_data.keys())
+ if not tree_keys:
+ return None
??? ^ in what situation could this happen?
+
+ root_data = tree_data.get(tree_keys[0])
+ if root_data is None:
+ return None
??? ^ same here, shouldn't this be an error? If this is something we don't expect to happen I would rather complain about it loudly if it doesn't instead of silently ignoring it
+
+ root_node = tabbed_browser.widget.tree_root
+ tab_to_focus = None
+ index = -1
+
+ def recursive_load_node(uid):
+ nonlocal tab_to_focus
+ nonlocal index
+ index += 1
+ 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)
+ self._load_tab(new_tab, tab_data)
+
+ new_tab.node.parent = root_node
??? ^ I assume this is being done to avoid an error in the tree walking code further down somewhere. Probably could pass the parent as an arg into recursive_load_node() and then set the real parent here rather than setting a placeholder one and then setting the real one after the function returns
+ children = [recursive_load_node(uid) for uid in children_uids]
+ new_tab.node.children = children
+ new_tab.node.collapsed = node_data['collapsed']
+ return new_tab.node
+
+ for child_uid in root_data['children']:
+ child = recursive_load_node(child_uid)
+ child.parent = root_node
+
+ return tab_to_focus
+
def _load_window(self, win):
"""Turn yaml data into windows."""
window = mainwindow.MainWindow(geometry=win['geometry'], |
toofar
deleted the
tree/8076_fix_loading_collapsed_tabs_and_legacy_session_support
branch
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.
Relating to #8076 and following on from a2f98aa this PR proposes a fix and an improvement:
fix loading of tab in collapsed tab groups
Tabs in collapsed tab groups were being dropped because the code was iterating over the tabs loaded into the tab widget instead of all those in the tree. Easy spot, easy fix.
support loading legacy tree tab session formats
There are some long time community members who have been using this PR for a while and may have built up complicated session files which they are dreading having to rebuild. Currently if they pull the latest on the
tree-tab-integrationbranch the browser will just crash on startup.I had a look at how hard it would be to continue supporting the previous session format and it turned out to be pretty easy by copying some old code and adding a couple of conditionals. A few two many conditionals for my tastes but at least they are tiny.
bonus: remove unused attribute
Tree tabs were ending up in session files with their uid in both the
nodeanduidattribute. It looks like only theuidone is being used.PS: I haven't reviewed a2f98aa yet, but I've tested it! Everything seems to be working as it was. And from a brief look the diff sure looks smaller. I think
_save_all()looks much cleaner iterating on tabs like normal and then taking a quick diversion to do add tab dataPPS: I've seen another issue where if you load a non-tree session while sessions are enabled every tab will be opened as "related" so instead of a flat set of tabs loaded you get a staircase. This might depend on what your
new_positionsettings are set to. This could be fixed by settingrelated=Truein the call totabopen()but I think this is exposing a problem you can run into with the existing session loading too. Eg tabopen hasrelated=Truefalse and that isn't overriden by the session code and the session code passesbackground=False. Both of which seem incorrect, so it seems the session code is using knowledge of the positioning code in tabbedbrowser instead of overriding it completely (egbackground=False, related=False, idx=i). So since I think it's a preexisting thing I would prefer to wait until we fix up the tests around sessions (lots of them are disabled on webengine) before fixing it "properly". But if someone complains I suppooose fixing it in an isolated manner is okay.TODO: raise a ticket to track thisresolved in this PREdit: this also causes issues with:
If you load the session tab five is also collapsed. If you toggle collapse it gets restored to the correct visibility. This seems to be happening in
_position_tab()becauserelatedis set to True. Not sure why it's only happening when there is a collapsed tab, or why thechild.parent = root_nodeat the end of_load_tree()isn't fixing it. But it gets fixed with related=False anyhow...Hmm, had a look at the e2e tests, which do work on webengine, they don't reproduce this issue because they test against a saved session and the session saves fine, it's the tabs in the browser that are borked (how!). Well, session-load does have some coverage so might just have to be a "it doesn't make things worse" change. Either that or we add a
:debug-dump-tree $winidcommand.Anyway, you can reproduce manually with
python3 -m qutebrowser -T -s tabs.tree_tabs true -s tabs.position left ":open about:blank?one" ":open -tr about:blank?two" ":open -tr about:blank?three" ":open -t about:blank?four" ":tab-focus 2" ":tree-tab-toggle-hide" ":cmd-later 300 session-save asdf" ":cmd-later 350 session-load asdf"then look how one window looks like it has less tabs than the other one!