Tree tabs integration #8082
Draft
Tree tabs integration #8082
+3,285
−250
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
And... make flake8 happy
Now correctly handles case where not all branches have same depth
Calls to render_traverse not accounting for hidden tabs caused crashes or unexpected behaviour
No one found much use in it
In particular, TreeTabWidget and TreeTabbedBrowser
If render_collapsed was False and self was collapsed, it would still show all the children or at least the first level
Following fix in notree.Node.traverse
|
@artromone thanks for your interest! It's not merged to the default branch yet so you need to run qutebrowser from source on the |
|
Just checking in to ask how this is progressing. Been using your branch for a while now (wrapped in a MacOS .App) and really happy with it so far. I use it like I used Microsoft Edge/Brave's tab groups, so just top-level category 'labels' (created using Is there a way to limit/force the tree to be only 1 level deep? I created some bindings to always create sibling tabs (so I always stay on the first indented level within a 'tab group'), but sometimes I still end up creating deeper level tabs accidentally. I'm fine to keep using the branch for now, but I look forward to it being merged in the main branch for easy installation/updates. Once again, thank you for your work, this is amazing and exactly what I was looking for! In the past I had to stick with other browsers because of tab groups and it's nice I can use my vim workflow in the browser too now. |
|
Thanks for the feedback @Kuret! Progress has been glacially slow over the last 6 months. This is still my top priority in terms of feature work, but I've been spending what time I have available for this project helping adapt to new Qt releases and attempting to improve the maintenance experience. It's interesting to see your use case for tab groups! Personally I've struggled to incorporate tree tabs into my day-to-day because I'm not very rigorous about making sure new tabs end up at the right place, and it just makes the resulting hierarchy a bit useless. I should probably practise some more strict hierarchy hygiene. I actually use separate basedirs for the use case you describe: to separate out a bunch of domains without necessarily having a tree structure. That also lets me have separate cookies where I use a site for personal and work reasons. I believe we've also had requests to use pinned tabs to demarc different tab groups at some point, or maybe I'm just thinking of #2898 (comment). I suppose the advantage of tab groups in this case is that you can collapse them to get them out of the way. You could also use sessions and windows I guess.
No, I don't think so. If only we had some kind of tab API and an extension mechanism that would probably be one of the easier tab positioning extensions to do: get a new tab signal and loop over tabs to flatten. But we don't have those things. Looking at the positioning code, we could maybe add a |
|
Thanks for your reply! The reason i'm not using multiple windows/basedirs is because:
If it helps someone, these are some of the bindings that make my specific workflow work: As long as I don't use the mouse (cmd+click) or a tab gets triggered some other way (js etc) this will make sure I always open stuff in the current tab group. |
|
Is this essentially the same as tab folders? If so, is there any update? 👀 |
Main conflicts were with: * 97104b2 Use builtin list/dict/set/... types for annotations * 4d069b8 Use str.removeprefix() and str.removesuffix() * #8345 use dosctrings for multiline strings in gherkin (still need to migrate the new tree tabs test file) Conflicts: qutebrowser/browser/commands.py qutebrowser/mainwindow/mainwindow.py qutebrowser/mainwindow/tabbedbrowser.py tests/end2end/features/conftest.py tests/end2end/features/sessions.feature
Most of the files where adapted on the main branch in #8345 This commit adds proper multiline string syntax to this new treetabs.feature file. I chose to not indent the contents of the new multiline strings, but in the above PR some did get extra indentation, some didn't. Here is the script I used to make this change, it doesn't do in-place editing currently because I was testing against a file outside of get and it was easier to iterate without having to change the file back. As it is you can run it like `gherkin.py $file | sponge $file`: #!/usr/bin/env python3 import sys, itertools # Set to True to indent the content of the multiline strings. Currently we # don't have a consistent style and personally I don't think the extra # horizontal space is needed. INDENT_STRING_CONTENT = False with open(sys.argv[1]) as f: inlines = f.readlines() def indent_len(s): return len(list(itertools.takewhile(lambda c: c.isspace(), s))) inmultiline = False # I'm a state machine indent_to_add = "" # For indenting multiline string content starting_indent = 0 # For detecting when we have come out of a string block outlines = [] for idx, line in enumerate(inlines): if idx == 0: outlines.append(line) continue previous_line = inlines[idx-1] try: previous_keyword = previous_line.split()[0] except IndexError: previous_keyword = "" previous_indent = indent_len(previous_line) indent = indent_len(line) if ( previous_keyword in ["And", "Then"] and indent > previous_indent and line.split()[0] != '"""' ): inmultiline = True starting_indent = indent outlines.append((indent * " ") + '"""\n') if INDENT_STRING_CONTENT: # Copy the amount of indent between this and the previous line indent_to_add = " " * (indent - previous_indent) if inmultiline and indent < starting_indent: outlines.append((" " * starting_indent) + '"""\n') inmultiline = False starting_indent = 0 indent_to_add = "" outlines.append(indent_to_add + line) if inmultiline: outlines.append((" " * starting_indent) + '"""\n') print("".join(outlines), end="")
This commit (aceef82) is on the main branch already, not sure why this loop is only showing up on the tree tab branch. The stack trace is: py311-pyqt67: commands[0]> .tox/py311-pyqt67/bin/python -bb -m pytest ImportError while loading conftest '/home/runner/work/qutebrowser/qutebrowser/tests/conftest.py'. tests/conftest.py:22: in <module> from helpers.fixtures import * # noqa: F403 tests/helpers/fixtures.py:29: in <module> import helpers.stubs as stubsmod tests/helpers/stubs.py:25: in <module> from qutebrowser.browser import browsertab, downloads qutebrowser/browser/browsertab.py:29: in <module> from qutebrowser.keyinput import modeman qutebrowser/keyinput/modeman.py:16: in <module> from qutebrowser.commands import runners qutebrowser/commands/runners.py:15: in <module> from qutebrowser.api import cmdutils qutebrowser/api/cmdutils.py:42: in <module> from qutebrowser.commands import command, cmdexc qutebrowser/commands/command.py:20: in <module> from qutebrowser.completion.models import completionmodel qutebrowser/completion/models/__init__.py:9: in <module> from qutebrowser.completion.models.util import DeleteFuncType qutebrowser/completion/models/util.py:11: in <module> from qutebrowser.config import config qutebrowser/config/config.py:15: in <module> from qutebrowser.commands import cmdexc, parser qutebrowser/commands/parser.py:16: in <module> class ParseResult: qutebrowser/commands/parser.py:20: in ParseResult cmd: command.Command E AttributeError: partially initialized module 'qutebrowser.commands.command' has no attribute 'Command' (most likely due to a circular import)
In 77591f3 'Support modifying suffixes in "Given I open ..."' I modified the parsing of the "Then I open {path}" lines to match suffixes in the loop instead of with a big if/else block. When I was pulling the suffixes out to a map I dropped the leading space on them all for simplicity, but then when stripping the suffix from the path I forgot to move the separating space. That ended up with the path being opened fine by the browser under test, but the log line matching was looking for the path with a trailing space, which was never matching, oops!
pylint says `Possible using variable 'parent_idx' before assignment. Which is true. Git blame shows that this conditional was added in a commit that just says "lint fixes". Looking at the logic, the only tab that doesn't have a parent should be the tree root, and we shouldn't be acting on that because of the `if node.value is None` guard at the top of the loop. So hopefully this conditional isn't needed at all. I'm not actually sure the logic is complete here anyway, this codeblock says it's inserting a hidden tab back into the tab widget. But it's just inserting it at parent_idx+1, shouldn't it be putting it in the correct place in the siblings list too? At least there are lots of other places in the codebase that do that.
The correct fix is to put an `*` before the first keyword arg and make all the args keyword only. But this method is only called once from immediately above it.
This reverts commit aee81c0. Not sure which line that ignore applies to, but I get we changed that at some point in the tab tree journey.
This piece of code was traversing all node of the tab tree, including hidden ones, and then it had special handling for closing hidden tabs. `TreeTabbedBrowser._remove_tab()` already handles hidden tabs though so there was some duplication. This changes the tree traversal to only including visible tabs and then calls the normal close method on each of them. Potential follow ons: * make `TabbedBrowser.tab_close_prompt_if_pinned()` pass `tab` to `yes_action()` so that we don't have to create a new partial function every time * pass `recursive` down to `TabbedBrowser.close_tab()` and `TabbedBrowser._remove_tab()` so that callers don't have to go through the command interface to get this behavior.
This commit moves any tree tab specific behavior out of `tab_close()` into TreeTabbedBrowser. I would like calling code not to have to deal with tree specific logic if it doesn't have to. In this case all we should need to do is tell the tabbed browser to close the tab and let that handle the details. (An even better API might be to ask the tab to close itself, maybe in the future.) This makes `TabbedBrowser.close_tab()` more powerful but now some logic is a bit awkwardly mixed between commands.py and treetabbedbrowser. Eg `tab_close_prompt()` is now being called in two places. One shortfall of this commit is that `--force` is no longer supported to close pinned tabs without prompting. It should be able to be supported by ... plumbing through yet another keyword argument! I'm not sure why the tab selection logic is in commands.py, I feel like all of that should be in TabbedBrowser too, and then the passed through kwargs would hopefully be a bit cleaner because they wouldn't have to go through so many layers.
There are various settings that the user can change to dictate where a new tab will be opened (eg, first, next, last, prev). The non-tree tabbed browser supports passing in an index to `tabopen()` to bypass those settings and use absolute positioning for new tabs. This is used for example when undoing a tab close or loading a session. So far for tree tab enabled windows callers have had to manually manipulate the tree to get absolute positioning. This is an attempt to provide absolute positing with the existing interface, at least as much as we can. How it works is that we still use the `related` and `sibling` flags to tell where in the tree hierarchy the tab should be placed (represented by selecting the new parent node in the code) but then use the index passed in to figure out where the tab should go in the list of siblings. We find the tab bar index of the sibling tabs and try to place the index passed in relative to them. This is a very loose interpretation of "absolute" positioning. We are relying on the position of nodes in the tree having the same ordering as the tabs in tha tab bar, which `TreeTabWidget.update_tree_tab_positions()` tries to ensure, and I've added an assert as a bonus. Callers could also just pass in `0` or `math.inf` to force the tabs to the first or last positions without having to mess with the current config. Seems to be working for the test suite for now. We can always remove it again if gaps in the logic come up.
The existing `_tree_tab_give()` method manipulates the tree manually and has a two setp process to create tabs and then position them. I wanted to see if I could create and position tabs in a single step, assuming it would be simpler. This did not turn out as simple as I had hoped! It was hoping I could iterate through the tabs to be given and open new tabs in the destination window in the right order, then they would end up in the right place and we would be done. The complication is that I don't think there's any way to traverse the tree and open new tabs without doing extra work to position tabs. In this case we have to figure out when we go up and down levels in the tree, so we can pass the right args to `tabopen()` and have the right tab focused to have the new one opened relative to. Going down the tree is pretty easy, going up the tree we have to figure out how far we went, also not so bad as it turns out. On balance I'm not sure this is a clear win. The new function is more lines of code and needs heuristic to understand it's position in the tree while traversing. On the plus side the new code maintains the collapsed state of subtrees! For the previous code I tried to add that but it seemed to blow up on the sanity check in update_tab_titles where it's gets indexes that are out of range of the visible tabs.
This gets a bit simpler to read if we do things in one pass. Because of the order we go through the tree we will always process a node's parent before we get to it. So if we save the new tabs (or just the new nodes) we can set the correct parent of a new tab right away. Additionally since we are always appending tabs (same order as we traverse them) then we can just append it to the list of children of the parent we found. In this implementation we traverse through the subtree of tabs-to-be given and create a new tab in the target window for each one. We construct the right tree structure in the distination window by adding the new tabs to the children list of the appropriate parent. For some reason the `setCurrentWidget()` at the end didn't make the test fail, even though it is checking for a specific active tab. Hmmm.
Remove the alternate "onepass" implementation that uses
`tabopen(idx=idx)` for positioning instead of manipulating tree nodes
directly. Because:
1. it was way longer
2. it had logic to discern the tree structure anyway, the value in
keeping tree manipulation code isolated is that there isn't as many
place people have to reason about trees.
3. it ended up touch nodes directly to set the collapsed state anyway,
so some amount of tree knowledge is require
Previously moving a tab to a higher tab number ended up putting it right before the target node. Since the target node stayed in the same place that actually meant that the tab being moved didn't make it to the indicated index. The `new_idx_relative += 1` bit in this change accounts for that. I also somewhat re-wrote the block of code to read a bit more clearly to me. Previously it called nodes "tabs", and it looked up `tabs[new_idx]` twice. It made it a bit difficult to spot logic issues. What this block of code is doing is: 1. find target node by indexing into the list of tabs as per the tab bar 2. remove the current node from the tree 3. insert the current node into the sibling list of the target node It's important that we look up the target node before removing the current. Otherwise everything could shift across and we could end up putting the tab in the next subtree. Also added a bunch of tests that let me make sure I was making changes correctly.
There was a few checks of `is_treetabbedbrowser` and repeated errors messages. I wanted to move them to a common method, but might have overestimated how many places could use it. There are several tree tab specific arguments that aren't treated as fatal, for example `tab-give --recursive` and `tab-prev --sibling`. I'm not sure if they should be fatal or not. There is some value in defaulting to the non-tree tab behavior if someone wants to use the same bindings across tree tab and non-tree tab basedirs. There is also value in being clear that what you've asked for can't be delivered. If indeed there is only one or two places where we need to complain about an argument, and not a whole command, then we could move the magic command name finding bit into the Command object and just hardcode the command name in the two places where we want to warn about a specific argument. There is also the `tree-tab-create-group` command which doesn't bother checking that yo have a tree tab enabled windows because it doesn't actually depend on any tree tab features, it's just an alternate to about:blank.
It was broken so that only one tab was being removed from the donor window and this wasn't being caught in the test that was only looking at the target window. Using the "session should look like" test to make sure all the right tabs are open, then the "tabs should be open" one to check the tab hierarchy is right.
Updates `tabs.select_on_remove=prev` for tree tab windows so that the previous sibling is selected, instead of the previous tab in the tab bar. I've found that the selected tab descending to the rightmost leaf node of a tab group when I close a top level tab is never what I want. It may have been more respectful of people's preferences and habits to add a "previous-sibling" option for the `tabs.select_on_remove` option, but that would be a fair bit of work. Turning that option into strings and mapping them to QTabBar enum values at the only place they are used wouldn't be so bad, but then there would be another place where non-tree code would have to deal with a tree specific option. So I figured I would be bold and change the default behaviour and see if anyone complained.
…ng_on_close Tree tabs: Make `select_on_remove=prev` select previous sibling
…from_commands.py Feat/8077 remove tree code from commands.py
|
I've changed the behavior of closing a tab with I did think about adding a separate option for this behavior, but I'm convinced the previous behavior wasn't intentional. Let me know if you prefer the old behaviour. (Next up I'm planning to look at mouse dragging (#8540), then type hints (#8073) then reviewing either changed/new commands (#8074) or changed/new settings (#8075)). |
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: Tree Tabs Integration (view)
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