Implement Tree Tabs #4602
Implement Tree Tabs #4602
Conversation
The underlying structures always stay enabled as the overhead is very small; this option only toggles show of the tree
This is almost a hack. Since tabs do not have unique identifiers that can be saved by SessionManager, their indexes are used as identifier instead. This relies on tabs being saved (and loaded) in the right order. Then a list of indexes is saved to memorize the tree.
Was there for debug but useless now
:open qute://treegroup/Name%20of%20group will open a tab that can be used as user-defined group. Furthermore, command :tree-tab-create-group wraps this open action (mainly to properly quote spaces and special characters so they can be part of the title)
Now there are no more idiotically complex checks. "collapsed" is a property of Node objects. This also allowed correct tree rendering for subtrees which contain collapsed descendents
It makes more sense as value is not generally a string --- especially when it's a tab
First step towards memoization
It's not used and it never will be; if the user wants that, they'll just un-collapse their tabs
Man, I should really test even stupid changes before pushing
Indexes are not that hard.
To reflect arguments to other tree-tab commands
When a collapsed tab group was being deleted the nodes were being removed from the tree by a sneaky line in `_add_undo_entry()`, which made it difficult to reason about the code. The nodes are removed from the tree by setting `node.parent = None`, the setting goes and calls `parent.disown()`, which is a bit magical but that's a job for another day. This commit moves the line where the node is removed from the tree to be up into the same block as where the tab is deleted and adjusts the undo entry to remove the child uids from it instead. The problem there is that `UndoEntry.from_node()` iterates over a nodes children, previously children were being removed from the tree first so they weren't showing up here. We don't want children uids in the undo nodes because the restore code will look for an existing child to re-attach when re-creating a node. But tabs are restored parent first. And since we do have a parent uid in the undo entry that is enough to restore the full structure.
Little bit of tidy up. This was already being used in the conditional branch for collapsed nodes, I have no idea why it wasn't being used for the un-collapsed case.
Two issues here:
1. transplanted trees could get rendered upside down depending on the value of the
tab.new_position.tree.new_child setting, I think. The parent and child
attributes of the nodes looked correct but it was being rendered upside
down in the tab bar, weird. Anyway, we are adding new tabs at the top level
and them setting their parents based on the mapping. We shouldn't be
opening them as related to the currently focused tab
2. moving a subtree was crashing because the parent of the top node wasn't in
the mapping. Which is fine because that's not being moved over. I changed
it to not explicitly change the parent of such nodes (so they remain under
the tree root).
Two other tidy ups:
* the initialization of "uid_map" as it didn't seem to need the 1:1 in there.
* remove tabs explicitly in reverse order rather than repeatedly
removing the current tab. This avoids re-work in the tree because it
does a bunch of stuff to re-parent children if you delete the parent.
Also I have debug logging in notree currently warning of children being
"orphaned" that was going off
TODO items around tab moving:
* collapsed state is lost
* moved tab isn't focused in destination window (I think that's a thing that
happens normally? Could be wrong)
* this method might be a bit simpler and consistent with other methods
(like undo) if it did more stuff in one go using traverse order to
keep things consistent, instead of the map. It seems to work fine as
is but not re-using patterns makes it harder to see opportunities for
refactoring
The structure I was testing with was:
1. one
2. two
3. three
4. four
5. five
1. six
Then focusing "three" and `:tab-give -r 1`.
Hopefully it saves someone else from having to interpret it in the future. Also change the last line into an assert because is should be redundant. This method seems more complex than it should be. Should, or is, some of this functionality be available in the notree data structure?
I think when removing nodes it's less work to remove leaves first, so that children don't have to be reparented. We also don't need to save everything to a list. I don't know what the difference out of POST and POST_R, maybe if you remove the last sibling first it's less work? Not sure. Also take care of a fixme comment which is fixed (the comment it refers to is still there, but add_undo_entry is no longer modifying the tree).
The root node never is associated with a tab.
We shouldn't really be closing tabs in multiple places[^1], but while we are, lets not promulgate copypasta we don't need. layout.unwrap() was previously called as a workaround for some upstream behaviour in Qt < 5.12. That workaround was removed in c067a96 [^1]: I want to make it so nodes know how to close tabs, that we we only have to worry about either widget stuff or tree stuff, not both
Seems a step from the existing code path wasn't applied to the new tree-specific code. I've pulled it out to be applied to both paths. Previously if you ran gD (tab-give without a positional argument) it would open a new window but you couldn't see it!
When recursive closing a tree group you will be prompted if you want to close any tabs that are pinned. Except we passed the topmost tab to the prompt method every time instead of the tab we were going to close. This fixes that. It does mean that if there are multiple pinned tabs in a tree it gives you an identical prompt for each one, which no indication that they are separate prompts. It looks like nothing is happening when you answer it. But that's a separate problem (probably the solution is to either prompt once or to put URLs and/or tab IDs in the prompt).
I was looking through some commit messages from last week and noticed I
was having trouble keeping track of what these traverse orders meant. So
I tried to adjust the descriptions to be more clear. Hopefully it
worked?
Probably could still use a visual aid though, or the enum value names
could even be changed to be more clear, like "parent-first",
"child-first", "child-first-reverse/-right".
The test I used to verify this description was:
simple_tree = Node('parent')
left = Node('left', simple_tree)
Node('grandleft-left', left)
Node('grandleft-right', left)
Node('middle', simple_tree)
right = Node('right', simple_tree)
Node('grandright', right)
# parent
# ├─ left
# │ ├─ grandleft-left
# │ └─ grandleft-right
# ├─ middle
# └─ right
# └─ grandright
@pytest.mark.parametrize('order, expected', [
(TraverseOrder.PRE, "parent left grandleft-left grandleft-right middle right grandright",),
(TraverseOrder.POST, "grandleft-left grandleft-right left middle grandright right parent",),
(TraverseOrder.POST_R, "grandright right middle grandleft-right grandleft-left left parent",),
])
def test_simple(order, expected):
assert expected == " ".join(
[n.value for n in simple_tree.traverse(order)]
)
TODO: * convert to asciidoc or make scripts/asciidoc2html.py support markdown input too. * for now I'm using `pandoc -f markdown -t html -o qutebrowser/html/doc/treetabs.html doc/treetabs.md` * should be able to asciidoc with pandoc too * the last section and the rest of the second to last * review and refactor * post on the PR and figure out how we can contribute to it collectively (put on main branch so people can open PRs? create new integration repo where people can open issues, discussions and PRs?) * maybe add more examples showing the effect of various operations * (when including in a release) consider removing the implementation section, or refactoring it to be more high level broad strokes and less low level stuff that will inevitably drift from the actual implementation These docs are not supposed to be in their final state. My plan for them so far is to provide a guide to the changes for PR contributors. I think it's currently difficult to hold the scope of the changes in your head the description on the PR is currently very limited. I would like to encourage discussion of usability and feature set as well as highlighting some implementation details to point out upsides, downsides, implications and alternatives. I've modified asciidoc2html to copy all images over. It was only doing specific ones before. But there are more images in there, why are they in doc/img if they aren't images used in the documentation? Anyway, storage is cheap, none of the images are sensitive and not having to hardcode individual files names is good.
|
I've pushed a few fixes and cleanups I made while working to understand this PR over the last couple of weeks. I started out getting rid of the "tree_prefix is empty!" error, that led me into a bit of a rabbit hole where I observed situations where the tree structure and the tab widget got out of sync. It was only for small amounts of time, but if it can happen then any bit of code that deals with those two structure might have to account for it, which could be problematic. I think there's the possibility of a more general fix if we move to using signals to keep things in sync instead of calling I did leave an assert in there in case there are any more cases where they get out of sync, hopefully I've manually tested all the functionality enough and caught everything! Apart from that I did a few cleanups and fixed |
|
Thanks for the excellent work @toofar! I have been meaning to pick up this PR again and get it into mergeable shape for a very long time; starting in the second half of January I should be able to start doing some actual work, in terms of review, discussing plans, and actually refactoring and fixing what's broken. I honestly have forgotten how most of the code works, but I do remember having broadly similar worries regarding duplication and overridden classes. I was young and inexperienced (maybe I still am), and this was probably too big a PR for me! So I really appreciate the notes, and I would love to keep discussing how to move forward with this feature. To be clear, I absolutely do not mind relinquishing what little "control" I had before, if you want to take charge; but I would like to still be involved, assuming I will be able to put in time and effort starting a couple weeks from now. |
|
@pinusc it's good to see that the architect is still around and engaged! Sorry for springing this on you, I've been wanting to get a larger change merged for a while and I've been eyeing up this PR. I hope we can keep momentum up and get this merged without any more breaks but I don't think there will be a huge amount of progress in the next couple of weeks, so I'm sure there will be lots for you to dig into when you have time. Do you have any thoughts about how we could discuss things like outstanding tasks, their priorities and implementation ideas? We could do it on this PR but I think it's a bit linear for that and it would be hard to have multiple discussion going? |
The command was renamed a while back.
|
I had a suggestion of using a project board and labels to track outstanding tasks related to this feature, I've started one off here: https://github.com/orgs/qutebrowser/projects/3/views/1 For now I've put emphasis on fleshing out the larger "blocker" tasks because while there are other valuable contributions we could be doing now, lots of them could also be done after the bulk of the work is merged into the main branch. I've put some words on those blocker issues and a size and priority rating. The sizes didn't turn out to be very granular, maybe I should have used a larger range. Or maybe it's just because I've only gone through the larger ones so far. For me I'll try to continue with the documentation and fleshing out the project board over the next week. @pinusc if it's okay with you I'll create a new |
|
@toofar everything sounds great; feel free to close this PR and move development to an actual branch. The project board is also the perfect solution to the organization problem. |
|
@toofar Any chance you could make me a collaborator / give me write access on the tree-tab github project? I'd actually like to start working on re-standardizing the session format, as it's a small enough issue that will help me refamiliarize myself with my own code. Also, once we move development to a qutebrowser branch, would I be able to push to it or would I need to make PRs towards that branch? |
|
If someone is using this PR daily: the last commit (a2f98aa) will break existing tree tab sessions, as it changes the format. |
Check your mailbox, you should have one invite from the 15th (invite to the org) and one from today (to the repo, because I couldn't find the org level thing again and thought I imagined it).
;_;
You will need to raise PRs. And that's a good thing! Gives us a space to discuss individual changes in isolation. |
I got the invite to the repo, but it didn't give me write access to the project board. Which makes some sense, as the project is at the org level. I hadn't seen the org invite from the 15th and it expired. I think you can give me access to the project directly from the project settings -> manage access; that's all I should need, anyway.
I hope that's okay! I agreed that it would be better if the format was compatible with 'vanilla', and I figured we'd have to break compatibiliity with my previous implementation at some point.
Yeah, that absolutely makes sense |
|
I'm closing this PR now because development is moving to this new one where the branch is hosted in this repo: #8082 In case it wasn't clear tree tabs isn't merged yet, nor is it forgotten. It's being actively worked on, we aim to reflect progress on this project board. It's our first time doing that through so please be patient.
It lists you there now, so hopefully it worked. It didn't let me add people who weren't members or contributors of the org. Hence the invites. Should all be sorted now. |
As discussed in #927.
This pull request implements Tree Tabs in qutebrowser in a similar fashion to Firefox's Tree-Style-Tabs plugin. It is stable enough for everyday use, but it needs testing and review.
Implemented features:
notree.pymodule to replaceanytreedependencytabs.tree_tabsqute://tabgroup/title):tab-close --recursive:tree-tab-promoteand:tree-tab-demote:tree-tab-toggle-hideand:tree-tab-cycle-hide:tab-movemoves entire subtrees correctly:tree-focus parentand:tab-prev/tab-next --sibling:session-savepreserves tree structuresset tabs.new_position.unrelatedundorestores correct parent & children of the tab, and restores the entrire tree closed by:tab-close -rtabs.new_position.tree.{demote,new_child,new_sibling,new_toplevel,promote}Known issues:
:tree-tab-toggle-hidetree_prefix is empty!errors at startup:tab-unload --recursivemay be implemented. With Lazier load #4037 suspended tabs' titles are lost at collapse.:buffer, and it probably won't be implemented in this pull request.