Fix a crash when setting DirectoryTree.show_root before DOM is ready
#3002
Conversation
There are occasions where the Tree may want to get the label (to make size decisions, it seems) that happen *before* a mount has finished. In DirectoryTree component classes are being accessed but they don't come into play until the DOM is up and running. This change builds allows for the building of a directory tree label, while also *not* trying to style the label, if mount hasn't finished yet. See Textualize#2363.
| node_label.stylize_before( | ||
| self.get_component_rich_style("directory-tree--folder", partial=True) | ||
| ) | ||
| if self.app.is_mounted(self): |
There was a problem hiding this comment.
I have a feeling this would be better done before render_label.
It doesn't make much sense for render_label to be called prior to mount. Does the user even see the label rendered pre-mount?
Can we bailed out earlier than this?
There was a problem hiding this comment.
As mentioned in standup yesterday, that was my initial reaction but the issue here is that setting show_root calls Tree.watch_show_root, which in turn sets Tree.cursor_line, which in turn calls Tree.watch_cursor_line, which in turn calls Tree._get_node, which in turn makes access to Tree._tree_lines, which in turn calls Tree._build, which in turn builds the tree, which causes the label to be needed for get_label_width.
Short version being: the size of the label is needed before mounting happens, and the styling doesn't impact the size; hence this approach.
There was a problem hiding this comment.
The things is, setting show_root prematurely invokes this code, but it will run again post mount. The change here makes the error go away but I think the issue is that this code runs at all.
It's also an annoying precedent, in that we may get the same issue with other renderables and watches. And I don't want to establish this as a pattern.
Can you investigate a solution, which doesn't place the burden on the widget developer?
There was a problem hiding this comment.
Do you mean investigate a different approach to how Tree works, or how watches work?
There was a problem hiding this comment.
Honestly, could be either. Whatever is the most elegant solution.
|
Having discussed this some more in the 2023-07-25 standup, we've decided that there's a more general class of issue here with watchers (and friends) and doing things before the DOM is fully mounted. Given this I'm going to close this PR, return the motivating issue back to TODO (actually perhaps place in backlog for now), and raise a fresh issue relating to the more general problem. |
|
Hey Dave, given what we discussed in today's standup and the way issue 3011 was resolved, do you want to reopen this PR & change the mounted checks to make use of I couldn't find a better place to add the |
|
Sure. Weirdly I'd forgotten about this and the fact that there is an is_mounted kicking about anyway(?). |
|
Or not...
|
|
@davep I think you'll need to restore the branch before you can re-open. Hopefully there's a button to do it on the GitHub web UI for you. I think it should be next to the "deleted the branch" text here: 
|
|
Right, that seems to have worked. @rodrigogiraoserrao did you want to snarf this and tidy it up then? (obvious thing to note: the changelog entry location will be waaaaaaaaaaay out of date now). |
|
@davep snarfed, updated, and committed. |
|
@rodrigogiraoserrao I left a comment last year, can you please take a look. |
Sorry, it must have slipped past me. I replied. |
Fixes #2363.
Because labels can sometimes be needed by the tree before the DOM is ready, this changes
DirectoryTree.render_labelso that it only attempts to style the labels if the DOM is up and running.