Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DirectoryTree should load a directory in a Worker #2456

Closed
willmcgugan opened this issue May 2, 2023 · 3 comments · Fixed by #2545
Closed

DirectoryTree should load a directory in a Worker #2456

willmcgugan opened this issue May 2, 2023 · 3 comments · Fixed by #2545
Assignees
Labels
enhancement New feature or request Task

Comments

@willmcgugan
Copy link
Collaborator

Getting the current directory is a blocking operation.

It is typically virtually instant, but it can be slow for larger directories. We should load the directory within a thread worker to avoid suspending the UI.

@davep davep added enhancement New feature or request Task labels May 2, 2023
@davep davep self-assigned this May 9, 2023
@davep
Copy link
Contributor

davep commented May 9, 2023

Background requirement for this: the worst case scenario here would be that some code does my_tree.root.expand_all() on the root directory of a volume with a deep directory hierarchy. Very useful for testing. But...

Turns out that this doesn't currently work for DirectoryTree because it lazy-loads the content of directories, loading the content when the folder is expanded. It does this by capturing Tree.NodeExpanded. The problem is that Tree.NodeExpanded is only ever posted if the expansion was done "by hand".

It looks like Tree needs to be updated along with this so that the work done in #1644 is expanded on such that every node expansion results in a message being sent. This could, of course, make for a very busy message queue, but generally it should settle down fairly well.

@davep
Copy link
Contributor

davep commented May 10, 2023

Raised the above as #2535 to highlight this side-issue; this being the main issue and #2535 needing to be done to make sense of this.

davep added a commit to davep/textual that referenced this issue May 10, 2023
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.
davep added a commit to davep/textual that referenced this issue May 10, 2023
This isn't the final form, not even close, this is more to help test out the
idea and how well it will work. Note the very deliberate sleep in the code
that's there to emulate loading from a slow blocking source.

This will be removed and tidied up before a final PR, of course. The main
aim here is to emulate a worst-case scenario so that the use of a worker can
be tried out with some confidence.

See Textualize#2456.
@davep davep linked a pull request May 11, 2023 that will close this issue
@github-actions
Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

1j01 added a commit to 1j01/textual-paint that referenced this issue Jun 11, 2023
- This regressed due to updates in Textual 0.25.0,
  because DirectoryTree now loads directory contents in a worker:
  Textualize/textual#2456
- Directory tree expansion may be more robust now, although it's using
  more internals now, and it still needs timers for whatever reason.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants