Add support for a method of expanding/collapsing all tree nodes from a given node down #1644
Conversation
This commit moves the bulk of the work of each action into an internal method that does everything *apart* from invalidating the tree. The idea being that all of the expanded states get updated, all of the update counts get updated, and then finally one single tree invalidation takes place (the latter taking place in the public method, which calls the related internal method). See Textualize#1430.
It might seem excessive for just a single argument, but I feel it's
worthwhile doing it here. It's a single boolean parameter on each of the
methods that, left bare, will always end up reading badly. Consider:
tree.toggle( True )
vs:
tree.toggle( toggle_all=True )
the former looks awkward at best and ugly at worst; toggle True? What does
that even mean? The latter, while a touch more verbose, makes it really
clear what's going on.
Trying this on for size.
No need to set the keyword to True when I can just pass the parameter's value in anyway. This reads a bit nicer.
This got added while I was experimenting with something earlier, and I forgot to remove it and didn't notice it slip in with a commit.
There was a problem hiding this comment.
The changes look good to me.
I have two questions, though:
- I'd expect the
collapse_alldefault value to also beFalseso that, by default, collapsing a node will not mess with the collapsed/expanded state of children. E.g., like VS Code does in the built-in file explorer. - What's the
self._tree._invalidate()? Is there a "local" version of that that we can call when we only change the state of a single node, i.e. when the recursive calls are skipped?
Not sure that's a question. :-P But, yes, you're right, that (and toggle) is a typo, so good catch.
I'm not sure I understand this question though, could you elaborate? |
It's not, I started typing with one formulation in mind and ended up writing something different 😆
I'm assuming the call to |
|
The "toggle children" behaviour seems weird to me - if I have some children open those will be closed, and the children that are closed will be open? Would it make more sense to just toggle the parent, then walk the children and make sure their state matches that of the parent? |
Sorry, I'm still not sure I'm following the question. |
Somehow I'd managed to typo them as True when they obviously should be False to maintain backward-compatibility (and generally this is the only sensible default).
|
Taking this back to a draft PR for a wee while as I can see there's questions about user experience and expectation to address. |
See Textualize#1644 (comment) where Darren raises the excellent point that while the "technically correct" approach that I had was... well, technically correct I guess (it toggled all the nodes from the target node down), it didn't have what was likely the desired effect. So this commit does away with the previous logic for doing the toggle and instead simply calls on expand or collapse depending on the state of the source node.
|
Right, a365836 takes the toggle from being "technically correct" to, erm... actually useful? 😄 |
See Textualize#1644 (comment) -- not exactly my preference but it's been decided it makes for a nicer interface.
|
Todo: we've decided to break these out into different methods; |
While descriptive keywords tend to be a preference within the Textual
codebase for many things, this was one of those times where a developer's
code using the library was likely going to read better if there's a switch
to using dedicated methods; this approach means we can just go with "all"
rather than "{action}_all" without needing to shadow a Python builtin.
This also does mirror mount/mount_all too.
Nowt optional about the new methods!
|
Cool, now we're happy with this I'll add them. |
|
Tests for all the |
Until now the Tree.NodeExpanded and Tree.NodeCollapsed messages were only sent out when changes were made to the tree by user interaction. This meant that if any changes were made with the TreeNode expand, expand_all, collapse, collapse_all, toggle or toggle_all API calls no messages would be sent. This PR corrects this. The work here is, in part, required for Textualize#2456 (DirectoryTree lazy-loads directory information on node expansion so if someone is expanding nodes under code control the DirectoryTree never gets to know that it should load a directory's content) and will build on Textualize#1644, essentially adding a missing aspect to the latter PR.
Adds optional
expand_all,collapse_allandtoggle_allparameters to theexpand,collapseandtogglemethods ofTreeNode. In doing so I move the bulk of the work of those methods inside "private" methods that do the setting of the expanded status (and in turn calling on any children to do the same) without ever invalidating the display of the owningTree. The "public" methods then simply call on those "private" methods and then invalidate theTree(the intention being that there's no need to get the tree to redraw until after all the nodes have done their thing_.See #1430.