From 30a20ac8daeb8a95800cdcf1ffbbadb890f18d50 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 9 May 2023 16:41:17 +0100 Subject: [PATCH 01/33] Break iterdir out into a method of its own for easy testing As I work on what's to come (loading DirectoryTree with a worker), I'm going to want to try and construct slow loads so I can test the effectiveness of the changes. This means a desire to fake a very slow source of directory information. So let's drop this into its own method so we can then do silly things like add a sleep to really show stuff down. --- src/textual/widgets/_directory_tree.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index fb6d7c5c5..7c0dc6ba0 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -2,7 +2,7 @@ from __future__ import annotations from dataclasses import dataclass from pathlib import Path -from typing import ClassVar, Iterable +from typing import ClassVar, Iterable, Iterator from rich.style import Style from rich.text import Text, TextType @@ -229,6 +229,19 @@ class DirectoryTree(Tree[DirEntry]): """ return paths + def _directory_content(self, directory: Path) -> Iterator[Path]: + """Get the entries within a given directory. + + Args: + directory: The directory to get the content of. + + Returns: + An iterator of `Path` objects. + """ + # TODO: Place this in a loop with a sleep to slow things down for + # testing. + return directory.iterdir() + def _load_directory(self, node: TreeNode[DirEntry]) -> None: """Load the directory contents for a given node. @@ -238,7 +251,7 @@ class DirectoryTree(Tree[DirEntry]): assert node.data is not None node.data.loaded = True directory = sorted( - self.filter_paths(node.data.path.iterdir()), + self.filter_paths(self._directory_content(node.data.path)), key=lambda path: (not path.is_dir(), path.name.lower()), ) for path in directory: From d673175e621f2c289207fac0ee79bfa51e5a99f9 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 10 May 2023 11:17:02 +0100 Subject: [PATCH 02/33] Experimenting with placing _load_directory in a worker 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 #2456. --- src/textual/widgets/_directory_tree.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 7c0dc6ba0..3cab29220 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -7,6 +7,7 @@ from typing import ClassVar, Iterable, Iterator from rich.style import Style from rich.text import Text, TextType +from .. import work from ..events import Mount from ..message import Message from ..reactive import var @@ -238,10 +239,18 @@ class DirectoryTree(Tree[DirEntry]): Returns: An iterator of `Path` objects. """ - # TODO: Place this in a loop with a sleep to slow things down for - # testing. - return directory.iterdir() + # TODO: Not like this. Oh so very not like this. This is here to + # slow things down on purpose, to emulate loading directory + # information from a slow source. + # + # REMOVE BEFORE FLIGHT! + import time + for entry in directory.iterdir(): + yield entry + time.sleep(0.05) + + @work def _load_directory(self, node: TreeNode[DirEntry]) -> None: """Load the directory contents for a given node. @@ -255,12 +264,13 @@ class DirectoryTree(Tree[DirEntry]): key=lambda path: (not path.is_dir(), path.name.lower()), ) for path in directory: - node.add( + self.app.call_from_thread( + node.add, path.name, data=DirEntry(path), allow_expand=path.is_dir(), ) - node.expand() + self.app.call_from_thread(node.expand) def _on_mount(self, _: Mount) -> None: self._load_directory(self.root) From 8b9a8e417415860cd4726dc50122a6c210ac177b Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 10 May 2023 12:06:11 +0100 Subject: [PATCH 03/33] Simplify _load_directory Move the node population code into its own method, the idea here being that the update happens in one call to call_from_thread rather than spawning lots of calls to it. --- src/textual/widgets/_directory_tree.py | 35 ++++++++++++++++++-------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 3cab29220..97294f8bc 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -250,6 +250,23 @@ class DirectoryTree(Tree[DirEntry]): yield entry time.sleep(0.05) + def _populate_node( + self, node: TreeNode[DirEntry], directory: Iterable[Path] + ) -> None: + """Populate the given node with the contents of a directory. + + Args: + node: The node to populate. + directory: The directory contents to populate it with. + """ + for path in directory: + node.add( + path.name, + data=DirEntry(path), + allow_expand=path.is_dir(), + ) + node.expand() + @work def _load_directory(self, node: TreeNode[DirEntry]) -> None: """Load the directory contents for a given node. @@ -259,18 +276,14 @@ class DirectoryTree(Tree[DirEntry]): """ assert node.data is not None node.data.loaded = True - directory = sorted( - self.filter_paths(self._directory_content(node.data.path)), - key=lambda path: (not path.is_dir(), path.name.lower()), + self.app.call_from_thread( + self._populate_node, + node, + sorted( + self.filter_paths(self._directory_content(node.data.path)), + key=lambda path: (not path.is_dir(), path.name.lower()), + ), ) - for path in directory: - self.app.call_from_thread( - node.add, - path.name, - data=DirEntry(path), - allow_expand=path.is_dir(), - ) - self.app.call_from_thread(node.expand) def _on_mount(self, _: Mount) -> None: self._load_directory(self.root) From 39971876d06c30fd5c5db5ee6fb2fe47a9fe5b13 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 10 May 2023 16:26:36 +0100 Subject: [PATCH 04/33] WiP: Throttle back the number of concurrent loads of a DirectoryTree Having got the initial version of background loading of nodes in the directory tree working, this moves to a gentler approach where only so many loads run at once, and a queue of jobs that need to be completed is kept. This is an end-of-coding-session WiP commit; there's more to come on this. But at the moment I'm happy with the way it's going. --- src/textual/widgets/_directory_tree.py | 67 +++++++++++++++++++++++++- 1 file changed, 65 insertions(+), 2 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 97294f8bc..0f7629bc7 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -2,10 +2,12 @@ from __future__ import annotations from dataclasses import dataclass from pathlib import Path +from queue import Empty, Queue from typing import ClassVar, Iterable, Iterator from rich.style import Style from rich.text import Text, TextType +from typing_extensions import Final from .. import work from ..events import Mount @@ -126,6 +128,8 @@ class DirectoryTree(Tree[DirEntry]): disabled=disabled, ) self.path = path + self._waiting_load_jobs: Queue[TreeNode[DirEntry]] = Queue() + self._running_load_jobs: set[int] = set() def reload(self) -> None: """Reload the `DirectoryTree` contents.""" @@ -267,6 +271,13 @@ class DirectoryTree(Tree[DirEntry]): ) node.expand() + @dataclass + class _LoadFinished(Message): + """Internal message to mark when a load of a node is finished.""" + + node: TreeNode[DirEntry] + """The node that has finished loading.""" + @work def _load_directory(self, node: TreeNode[DirEntry]) -> None: """Load the directory contents for a given node. @@ -276,6 +287,7 @@ class DirectoryTree(Tree[DirEntry]): """ assert node.data is not None node.data.loaded = True + # TODO: Perhaps move this out of here and... self.app.call_from_thread( self._populate_node, node, @@ -284,9 +296,60 @@ class DirectoryTree(Tree[DirEntry]): key=lambda path: (not path.is_dir(), path.name.lower()), ), ) + # TODO: ...attach it to this and have the receiver update the tree? + self.post_message(self._LoadFinished(node)) + + _MAX_CONCURRENT_JOBS: Final[int] = 5 + """The maximum number of load jobs to run at the same time.""" + + # TODO: Remove debug logging before going to an actual PR. + + def _process_load_jobs(self) -> None: + """Process the incoming load request queue.""" + # While we still have spare capacity... + self.log.debug( + f"LOAD QUEUE: Running job count is {len(self._running_load_jobs)}" + ) + while len(self._running_load_jobs) <= self._MAX_CONCURRENT_JOBS: + try: + # ...pull a load job off the queue. + new_job = self._waiting_load_jobs.get(block=False) + except Empty: + # Queue is empty; our work here is done. + self.log.debug("LOAD QUEUE: New job queue is empty") + return + # We've got a new directory load job, add it to the collection + # of running jobs and kick off the load. + self.log.debug(f"LOAD QUEUE: Staring a new job running {new_job.id}") + self._running_load_jobs.add(new_job.id) + self._load_directory(new_job) + self.log.debug(f"LOAD QUEUE: Running job queue is full") + + def _on_directory_tree__load_finished( + self, event: DirectoryTree._LoadFinished + ) -> None: + """Act on a signal that a node has finished loading. + + Args: + event: The event to process. + """ + event.stop() + self.log.debug(f"LOAD QUEUE: Done {event.node.id} - REMOVING") + self._running_load_jobs.remove(event.node.id) + self._process_load_jobs() + + def _add_load_job(self, node: TreeNode[DirEntry]) -> None: + """Add a directory loading job to the queue. + + Args: + node: The node that needs loading. + """ + self.log.debug(f"LOAD QUEUE: New {node.id} - ADDING") + self._waiting_load_jobs.put(node) + self._process_load_jobs() def _on_mount(self, _: Mount) -> None: - self._load_directory(self.root) + self._add_load_job(self.root) def _on_tree_node_expanded(self, event: Tree.NodeExpanded) -> None: event.stop() @@ -295,7 +358,7 @@ class DirectoryTree(Tree[DirEntry]): return if dir_entry.path.is_dir(): if not dir_entry.loaded: - self._load_directory(event.node) + self._add_load_job(event.node) else: self.post_message(self.FileSelected(self, event.node, dir_entry.path)) From 05dc877a24e95dc7a82a32be97fc255b16a74ccc Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 11 May 2023 08:31:01 +0100 Subject: [PATCH 05/33] Check for the worker being cancelled So far this is working fine, but there was an issue where, if the load of a very large directory was started, and then the application was cancelled right away, the application would close down but there would be a long pause until the shell prompt came back, the cause presumably being that we were waiting for that particular thread to end. So here I make sure I check the cancelled state of the worker. This would also suggest that, while I turned the use of iterdir into a loop so I could throw the sleep in to emulate a slow directory load, I *actually* want to do this in a loop so I can test the cancelled state as we stream in the directory content. --- src/textual/widgets/_directory_tree.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 0f7629bc7..fc60ab500 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -13,6 +13,7 @@ from .. import work from ..events import Mount from ..message import Message from ..reactive import var +from ..worker import get_current_worker from ._tree import TOGGLE_STYLE, Tree, TreeNode @@ -250,7 +251,10 @@ class DirectoryTree(Tree[DirEntry]): # REMOVE BEFORE FLIGHT! import time + worker = get_current_worker() for entry in directory.iterdir(): + if worker.is_cancelled: + return yield entry time.sleep(0.05) From df0f73ba3bec55f89f48a126c82132b2509663c7 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 11 May 2023 08:35:36 +0100 Subject: [PATCH 06/33] Remove debug logging --- src/textual/widgets/_directory_tree.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index fc60ab500..8ae59581b 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -306,28 +306,20 @@ class DirectoryTree(Tree[DirEntry]): _MAX_CONCURRENT_JOBS: Final[int] = 5 """The maximum number of load jobs to run at the same time.""" - # TODO: Remove debug logging before going to an actual PR. - def _process_load_jobs(self) -> None: """Process the incoming load request queue.""" # While we still have spare capacity... - self.log.debug( - f"LOAD QUEUE: Running job count is {len(self._running_load_jobs)}" - ) while len(self._running_load_jobs) <= self._MAX_CONCURRENT_JOBS: try: # ...pull a load job off the queue. new_job = self._waiting_load_jobs.get(block=False) except Empty: # Queue is empty; our work here is done. - self.log.debug("LOAD QUEUE: New job queue is empty") return # We've got a new directory load job, add it to the collection # of running jobs and kick off the load. - self.log.debug(f"LOAD QUEUE: Staring a new job running {new_job.id}") self._running_load_jobs.add(new_job.id) self._load_directory(new_job) - self.log.debug(f"LOAD QUEUE: Running job queue is full") def _on_directory_tree__load_finished( self, event: DirectoryTree._LoadFinished @@ -338,7 +330,6 @@ class DirectoryTree(Tree[DirEntry]): event: The event to process. """ event.stop() - self.log.debug(f"LOAD QUEUE: Done {event.node.id} - REMOVING") self._running_load_jobs.remove(event.node.id) self._process_load_jobs() @@ -348,7 +339,6 @@ class DirectoryTree(Tree[DirEntry]): Args: node: The node that needs loading. """ - self.log.debug(f"LOAD QUEUE: New {node.id} - ADDING") self._waiting_load_jobs.put(node) self._process_load_jobs() From 4ead43c149fae6027c2217261a45d028177dca59 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 11 May 2023 09:10:55 +0100 Subject: [PATCH 07/33] Set up the job tracking before setting the path Setting the path to anything other than "." is going to result in a reset happening, so we need the tracking support in place first. --- src/textual/widgets/_directory_tree.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 8ae59581b..86478063c 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -128,9 +128,9 @@ class DirectoryTree(Tree[DirEntry]): classes=classes, disabled=disabled, ) - self.path = path self._waiting_load_jobs: Queue[TreeNode[DirEntry]] = Queue() self._running_load_jobs: set[int] = set() + self.path = path def reload(self) -> None: """Reload the `DirectoryTree` contents.""" From ce7a78db69fc0366463006386fcb8cd8d36bba03 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 11 May 2023 09:12:10 +0100 Subject: [PATCH 08/33] Have the reset method take part in background loading One instance of a call to _load_directory that I missed. --- src/textual/widgets/_directory_tree.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 86478063c..030ed8857 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -135,7 +135,7 @@ class DirectoryTree(Tree[DirEntry]): def reload(self) -> None: """Reload the `DirectoryTree` contents.""" self.reset(str(self.path), DirEntry(Path(self.path))) - self._load_directory(self.root) + self._add_load_job(self.root) def validate_path(self, path: str | Path) -> Path: """Ensure that the path is of the `Path` type. From 791f2ea1890dc494ce2fee577ba38ac74c7a2ff8 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 11 May 2023 09:16:54 +0100 Subject: [PATCH 09/33] Ensure we don't create a job for a node that's already being loaded --- src/textual/widgets/_directory_tree.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 030ed8857..50b03dd2f 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -316,10 +316,12 @@ class DirectoryTree(Tree[DirEntry]): except Empty: # Queue is empty; our work here is done. return - # We've got a new directory load job, add it to the collection - # of running jobs and kick off the load. - self._running_load_jobs.add(new_job.id) - self._load_directory(new_job) + # At this point we've got a new directory load job; add it to + # the collection of running jobs and kick off the load, but only + # if there isn't already a job for it. + if not new_job.id in self._running_load_jobs: + self._running_load_jobs.add(new_job.id) + self._load_directory(new_job) def _on_directory_tree__load_finished( self, event: DirectoryTree._LoadFinished From 08246d84ac8e9928dcd949e2b77a115991a15137 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 11 May 2023 09:57:30 +0100 Subject: [PATCH 10/33] Don't post the finished message if we've been cancelled --- src/textual/widgets/_directory_tree.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 50b03dd2f..9c60aff94 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -301,7 +301,8 @@ class DirectoryTree(Tree[DirEntry]): ), ) # TODO: ...attach it to this and have the receiver update the tree? - self.post_message(self._LoadFinished(node)) + if not get_current_worker().is_cancelled: + self.post_message(self._LoadFinished(node)) _MAX_CONCURRENT_JOBS: Final[int] = 5 """The maximum number of load jobs to run at the same time.""" From 9ae8e47c6ce7bf97df977c87a6974811ef57229a Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 11 May 2023 09:59:18 +0100 Subject: [PATCH 11/33] Add a method for cancelling all of the load jobs This marks all current jobs as cancelled and also removes all pending jobs. --- src/textual/widgets/_directory_tree.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 9c60aff94..b1fea1a0e 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -307,6 +307,16 @@ class DirectoryTree(Tree[DirEntry]): _MAX_CONCURRENT_JOBS: Final[int] = 5 """The maximum number of load jobs to run at the same time.""" + def _cancel_all_jobs(self) -> None: + """Cancel all running load jobs.""" + self._waiting_load_jobs = Queue() + self._running_load_jobs = set() + # TODO: Check if there's an Textual-API-way to say "get all workers + # in this DOM node", or "cancel all of the works I made", or + # something. This seems fine, but I want to be 100% sure. + for job in (worker for worker in self.app.workers if worker.node == self): + job.cancel() + def _process_load_jobs(self) -> None: """Process the incoming load request queue.""" # While we still have spare capacity... From 82a08177af299c12c92f6c5d313e119902562ed4 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 11 May 2023 09:59:57 +0100 Subject: [PATCH 12/33] Cancel any loads when resetting the tree --- src/textual/widgets/_directory_tree.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index b1fea1a0e..7dbbf789f 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -134,6 +134,13 @@ class DirectoryTree(Tree[DirEntry]): def reload(self) -> None: """Reload the `DirectoryTree` contents.""" + # We're about to nuke the whole tree and start over, so we don't + # want any dangling load jobs. Before we do anything else, ensure + # they're all marked as cancelled and that the queue of pending jobs + # has been emptied. + self._cancel_all_jobs() + # That out of the way, we can reset the tree and start loading the + # root's content. self.reset(str(self.path), DirEntry(Path(self.path))) self._add_load_job(self.root) From 05eeaa767984a15ce4af23d079dc505b5d5e0068 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 11 May 2023 10:53:07 +0100 Subject: [PATCH 13/33] Tidy up _load_directory Explain some about the decisions made, and also throw in a bit of over-cautious worker cancellation checking. --- src/textual/widgets/_directory_tree.py | 44 ++++++++++++++++++++------ 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 7dbbf789f..e3a9eb33a 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -296,19 +296,43 @@ class DirectoryTree(Tree[DirEntry]): Args: node: The node to load the directory contents for. """ + + # We should not ever be asked to load a directory for a node that + # has no directory information. assert node.data is not None + + # Mark the node as loaded; we do this as soon as possible. node.data.loaded = True - # TODO: Perhaps move this out of here and... - self.app.call_from_thread( - self._populate_node, - node, - sorted( - self.filter_paths(self._directory_content(node.data.path)), - key=lambda path: (not path.is_dir(), path.name.lower()), - ), + + # From now on we get the directory content and populate the node in + # simple steps, checking that the worker hasn't been cancelled at + # every step of the way. We /could/ just run to the end, but we + # might as well drop out of here as soon as we can tell we've been + # asked to stop. + worker = get_current_worker() + + # Load up the content of the directly. + content = self.filter_paths(self._directory_content(node.data.path)) + if worker.is_cancelled: + return + + # We're still going, sort the content, case-insensitive, placing + # directory entries up front. + content = sorted( + content, + key=lambda path: (not path.is_dir(), path.name.lower()), ) - # TODO: ...attach it to this and have the receiver update the tree? - if not get_current_worker().is_cancelled: + if worker.is_cancelled: + return + + # We have directory content, it's filtered, it's sorted, we're still + # working, so now let's update the actual node in the tree. + self.app.call_from_thread(self._populate_node, node, content) + + # Finally, if we're 100% sure we've not been cancelled, post a + # message to say the load has finished. Our caller should not be + # told we finished fine if they've cancelled us. + if not worker.is_cancelled: self.post_message(self._LoadFinished(node)) _MAX_CONCURRENT_JOBS: Final[int] = 5 From 9b41b743fee554fd614af29b569802e0f4b99680 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 11 May 2023 11:13:19 +0100 Subject: [PATCH 14/33] Remove the artificial slowdown --- src/textual/widgets/_directory_tree.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index e3a9eb33a..0c8f390e1 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -251,19 +251,11 @@ class DirectoryTree(Tree[DirEntry]): Returns: An iterator of `Path` objects. """ - # TODO: Not like this. Oh so very not like this. This is here to - # slow things down on purpose, to emulate loading directory - # information from a slow source. - # - # REMOVE BEFORE FLIGHT! - import time - worker = get_current_worker() for entry in directory.iterdir(): if worker.is_cancelled: return yield entry - time.sleep(0.05) def _populate_node( self, node: TreeNode[DirEntry], directory: Iterable[Path] From c45126b21cbed4e2c007d22093bfe2ade4ff9f68 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 11 May 2023 11:19:30 +0100 Subject: [PATCH 15/33] Update the ChangeLog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 21b136f51..5e37712ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed - App `title` and `sub_title` attributes can be set to any type https://github.com/Textualize/textual/issues/2521 +- `DirectoryTree` now loads directory contents in a worker https://github.com/Textualize/textual/issues/2456 ### Fixed From 4d225b8ebb5648f6bdb65214cbae49799ad8932d Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 11 May 2023 15:22:19 +0100 Subject: [PATCH 16/33] Correct a comment typo --- src/textual/widgets/_directory_tree.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 0c8f390e1..9ba82d6df 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -303,7 +303,7 @@ class DirectoryTree(Tree[DirEntry]): # asked to stop. worker = get_current_worker() - # Load up the content of the directly. + # Load up the content of the directory. content = self.filter_paths(self._directory_content(node.data.path)) if worker.is_cancelled: return From 926c0a2b4f143b1bf2eb7739eaa58205ae346c56 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 16 May 2023 15:29:36 +0100 Subject: [PATCH 17/33] Reset all DirectoryTree worker changes After deciding https://github.com/Textualize/textual/pull/2545#issuecomment-1547544057 it makes more sense to roll back to the state of `main` than to try and get to where I want to be from where we've decided we didn't want to be. Can't get there from here, so let's go rogue-like on this PR... --- src/textual/widgets/_directory_tree.py | 155 +++---------------------- 1 file changed, 13 insertions(+), 142 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 9ba82d6df..fb6d7c5c5 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -2,18 +2,14 @@ from __future__ import annotations from dataclasses import dataclass from pathlib import Path -from queue import Empty, Queue -from typing import ClassVar, Iterable, Iterator +from typing import ClassVar, Iterable from rich.style import Style from rich.text import Text, TextType -from typing_extensions import Final -from .. import work from ..events import Mount from ..message import Message from ..reactive import var -from ..worker import get_current_worker from ._tree import TOGGLE_STYLE, Tree, TreeNode @@ -128,21 +124,12 @@ class DirectoryTree(Tree[DirEntry]): classes=classes, disabled=disabled, ) - self._waiting_load_jobs: Queue[TreeNode[DirEntry]] = Queue() - self._running_load_jobs: set[int] = set() self.path = path def reload(self) -> None: """Reload the `DirectoryTree` contents.""" - # We're about to nuke the whole tree and start over, so we don't - # want any dangling load jobs. Before we do anything else, ensure - # they're all marked as cancelled and that the queue of pending jobs - # has been emptied. - self._cancel_all_jobs() - # That out of the way, we can reset the tree and start loading the - # root's content. self.reset(str(self.path), DirEntry(Path(self.path))) - self._add_load_job(self.root) + self._load_directory(self.root) def validate_path(self, path: str | Path) -> Path: """Ensure that the path is of the `Path` type. @@ -242,30 +229,18 @@ class DirectoryTree(Tree[DirEntry]): """ return paths - def _directory_content(self, directory: Path) -> Iterator[Path]: - """Get the entries within a given directory. + def _load_directory(self, node: TreeNode[DirEntry]) -> None: + """Load the directory contents for a given node. Args: - directory: The directory to get the content of. - - Returns: - An iterator of `Path` objects. - """ - worker = get_current_worker() - for entry in directory.iterdir(): - if worker.is_cancelled: - return - yield entry - - def _populate_node( - self, node: TreeNode[DirEntry], directory: Iterable[Path] - ) -> None: - """Populate the given node with the contents of a directory. - - Args: - node: The node to populate. - directory: The directory contents to populate it with. + node: The node to load the directory contents for. """ + assert node.data is not None + node.data.loaded = True + directory = sorted( + self.filter_paths(node.data.path.iterdir()), + key=lambda path: (not path.is_dir(), path.name.lower()), + ) for path in directory: node.add( path.name, @@ -274,112 +249,8 @@ class DirectoryTree(Tree[DirEntry]): ) node.expand() - @dataclass - class _LoadFinished(Message): - """Internal message to mark when a load of a node is finished.""" - - node: TreeNode[DirEntry] - """The node that has finished loading.""" - - @work - def _load_directory(self, node: TreeNode[DirEntry]) -> None: - """Load the directory contents for a given node. - - Args: - node: The node to load the directory contents for. - """ - - # We should not ever be asked to load a directory for a node that - # has no directory information. - assert node.data is not None - - # Mark the node as loaded; we do this as soon as possible. - node.data.loaded = True - - # From now on we get the directory content and populate the node in - # simple steps, checking that the worker hasn't been cancelled at - # every step of the way. We /could/ just run to the end, but we - # might as well drop out of here as soon as we can tell we've been - # asked to stop. - worker = get_current_worker() - - # Load up the content of the directory. - content = self.filter_paths(self._directory_content(node.data.path)) - if worker.is_cancelled: - return - - # We're still going, sort the content, case-insensitive, placing - # directory entries up front. - content = sorted( - content, - key=lambda path: (not path.is_dir(), path.name.lower()), - ) - if worker.is_cancelled: - return - - # We have directory content, it's filtered, it's sorted, we're still - # working, so now let's update the actual node in the tree. - self.app.call_from_thread(self._populate_node, node, content) - - # Finally, if we're 100% sure we've not been cancelled, post a - # message to say the load has finished. Our caller should not be - # told we finished fine if they've cancelled us. - if not worker.is_cancelled: - self.post_message(self._LoadFinished(node)) - - _MAX_CONCURRENT_JOBS: Final[int] = 5 - """The maximum number of load jobs to run at the same time.""" - - def _cancel_all_jobs(self) -> None: - """Cancel all running load jobs.""" - self._waiting_load_jobs = Queue() - self._running_load_jobs = set() - # TODO: Check if there's an Textual-API-way to say "get all workers - # in this DOM node", or "cancel all of the works I made", or - # something. This seems fine, but I want to be 100% sure. - for job in (worker for worker in self.app.workers if worker.node == self): - job.cancel() - - def _process_load_jobs(self) -> None: - """Process the incoming load request queue.""" - # While we still have spare capacity... - while len(self._running_load_jobs) <= self._MAX_CONCURRENT_JOBS: - try: - # ...pull a load job off the queue. - new_job = self._waiting_load_jobs.get(block=False) - except Empty: - # Queue is empty; our work here is done. - return - # At this point we've got a new directory load job; add it to - # the collection of running jobs and kick off the load, but only - # if there isn't already a job for it. - if not new_job.id in self._running_load_jobs: - self._running_load_jobs.add(new_job.id) - self._load_directory(new_job) - - def _on_directory_tree__load_finished( - self, event: DirectoryTree._LoadFinished - ) -> None: - """Act on a signal that a node has finished loading. - - Args: - event: The event to process. - """ - event.stop() - self._running_load_jobs.remove(event.node.id) - self._process_load_jobs() - - def _add_load_job(self, node: TreeNode[DirEntry]) -> None: - """Add a directory loading job to the queue. - - Args: - node: The node that needs loading. - """ - self._waiting_load_jobs.put(node) - self._process_load_jobs() - def _on_mount(self, _: Mount) -> None: - self._add_load_job(self.root) + self._load_directory(self.root) def _on_tree_node_expanded(self, event: Tree.NodeExpanded) -> None: event.stop() @@ -388,7 +259,7 @@ class DirectoryTree(Tree[DirEntry]): return if dir_entry.path.is_dir(): if not dir_entry.loaded: - self._add_load_job(event.node) + self._load_directory(event.node) else: self.post_message(self.FileSelected(self, event.node, dir_entry.path)) From 58f0d11a93d6b1398f2c21850217acc0f3fcd2d8 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 16 May 2023 16:41:36 +0100 Subject: [PATCH 18/33] Change to a single loader thread with a queue --- src/textual/widgets/_directory_tree.py | 80 +++++++++++++++++++++----- 1 file changed, 65 insertions(+), 15 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index fb6d7c5c5..be7fe9794 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -2,14 +2,18 @@ from __future__ import annotations from dataclasses import dataclass from pathlib import Path -from typing import ClassVar, Iterable +from queue import Empty, Queue +from typing import ClassVar, Iterable, Iterator from rich.style import Style from rich.text import Text, TextType +from typing_extensions import Final +from .. import work from ..events import Mount from ..message import Message from ..reactive import var +from ..worker import Worker, get_current_worker from ._tree import TOGGLE_STYLE, Tree, TreeNode @@ -116,6 +120,7 @@ class DirectoryTree(Tree[DirEntry]): classes: A space-separated list of classes, or None for no classes. disabled: Whether the directory tree is disabled or not. """ + self._to_load: Queue[TreeNode[DirEntry]] = Queue() super().__init__( str(path), data=DirEntry(Path(path)), @@ -129,7 +134,13 @@ class DirectoryTree(Tree[DirEntry]): def reload(self) -> None: """Reload the `DirectoryTree` contents.""" self.reset(str(self.path), DirEntry(Path(self.path))) - self._load_directory(self.root) + # Orphan the old queue... + self._to_load = Queue() + # ...and replace the old load with a new one. + self._loader() + # We have a fresh queue, we have a fresh loader, get the fresh root + # loading up. + self._to_load.put_nowait(self.root) def validate_path(self, path: str | Path) -> Path: """Ensure that the path is of the `Path` type. @@ -229,19 +240,14 @@ class DirectoryTree(Tree[DirEntry]): """ return paths - def _load_directory(self, node: TreeNode[DirEntry]) -> None: - """Load the directory contents for a given node. + def _populate_node(self, node: TreeNode[DirEntry], content: Iterable[Path]) -> None: + """Populate the given tree node with the given directory content. Args: - node: The node to load the directory contents for. + node: The Tree node to populate. + content: The collection of `Path` objects to populate the node with. """ - assert node.data is not None - node.data.loaded = True - directory = sorted( - self.filter_paths(node.data.path.iterdir()), - key=lambda path: (not path.is_dir(), path.name.lower()), - ) - for path in directory: + for path in content: node.add( path.name, data=DirEntry(path), @@ -249,8 +255,52 @@ class DirectoryTree(Tree[DirEntry]): ) node.expand() - def _on_mount(self, _: Mount) -> None: - self._load_directory(self.root) + def _directory_content(self, location: Path, worker: Worker) -> Iterator[Path]: + """Load the content of a given directory. + + Args: + location: The location to load from. + worker: The worker that the loading is taking place in. + + Yields: + Path: A entry within the location. + """ + for entry in location.iterdir(): + if worker.is_cancelled: + break + yield entry + + def _load_directory(self, node: TreeNode[DirEntry], worker: Worker) -> None: + """Load the directory contents for a given node. + + Args: + node: The node to load the directory contents for. + """ + assert node.data is not None + node.data.loaded = True + self.app.call_from_thread( + self._populate_node, + node, + sorted( + self.filter_paths(self._directory_content(node.data.path, worker)), + key=lambda path: (not path.is_dir(), path.name.lower()), + ), + ) + + _LOADER_INTERVAL: Final[float] = 0.2 + """How long the loader should block while waiting for queue content.""" + + @work(exclusive=True) + def _loader(self) -> None: + """Background loading queue processor.""" + worker = get_current_worker() + while not worker.is_cancelled: + try: + self._load_directory( + self._to_load.get(timeout=self._LOADER_INTERVAL), worker + ) + except Empty: + pass def _on_tree_node_expanded(self, event: Tree.NodeExpanded) -> None: event.stop() @@ -259,7 +309,7 @@ class DirectoryTree(Tree[DirEntry]): return if dir_entry.path.is_dir(): if not dir_entry.loaded: - self._load_directory(event.node) + self._to_load.put_nowait(event.node) else: self.post_message(self.FileSelected(self, event.node, dir_entry.path)) From e69e57d7c02e02da706845200439127c45a855b3 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 16 May 2023 20:53:18 +0100 Subject: [PATCH 19/33] Remove unused import Recent changes meant it wasn't needed any more. --- src/textual/widgets/_directory_tree.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index be7fe9794..437811758 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -10,7 +10,6 @@ from rich.text import Text, TextType from typing_extensions import Final from .. import work -from ..events import Mount from ..message import Message from ..reactive import var from ..worker import Worker, get_current_worker From 80d00ce4bf8bbcbbdb83e190f697b688c35394ca Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 17 May 2023 10:49:46 +0100 Subject: [PATCH 20/33] Logging and experimenting for Windows --- src/textual/widgets/_directory_tree.py | 32 +++++++++++++++++--------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 437811758..0c615f26e 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -3,6 +3,7 @@ from __future__ import annotations from dataclasses import dataclass from pathlib import Path from queue import Empty, Queue +from threading import RLock from typing import ClassVar, Iterable, Iterator from rich.style import Style @@ -15,6 +16,8 @@ from ..reactive import var from ..worker import Worker, get_current_worker from ._tree import TOGGLE_STYLE, Tree, TreeNode +read_dir = RLock() + @dataclass class DirEntry: @@ -239,6 +242,9 @@ class DirectoryTree(Tree[DirEntry]): """ return paths + def _tlog(self, message: str) -> None: + self.app.call_from_thread(self.log.debug, f"{self.id} - {message}") + def _populate_node(self, node: TreeNode[DirEntry], content: Iterable[Path]) -> None: """Populate the given tree node with the given directory content. @@ -268,6 +274,7 @@ class DirectoryTree(Tree[DirEntry]): if worker.is_cancelled: break yield entry + self._tlog(f"Loaded entry {entry} from {location}") def _load_directory(self, node: TreeNode[DirEntry], worker: Worker) -> None: """Load the directory contents for a given node. @@ -277,14 +284,15 @@ class DirectoryTree(Tree[DirEntry]): """ assert node.data is not None node.data.loaded = True - self.app.call_from_thread( - self._populate_node, - node, - sorted( - self.filter_paths(self._directory_content(node.data.path, worker)), - key=lambda path: (not path.is_dir(), path.name.lower()), - ), - ) + with read_dir: + self.app.call_from_thread( + self._populate_node, + node, + sorted( + self.filter_paths(self._directory_content(node.data.path, worker)), + key=lambda path: (not path.is_dir(), path.name.lower()), + ), + ) _LOADER_INTERVAL: Final[float] = 0.2 """How long the loader should block while waiting for queue content.""" @@ -292,12 +300,14 @@ class DirectoryTree(Tree[DirEntry]): @work(exclusive=True) def _loader(self) -> None: """Background loading queue processor.""" + self._tlog("_loader started") worker = get_current_worker() while not worker.is_cancelled: try: - self._load_directory( - self._to_load.get(timeout=self._LOADER_INTERVAL), worker - ) + next_node = self._to_load.get(timeout=self._LOADER_INTERVAL) + self._tlog(f"Received {next_node} for loading") + self._load_directory(next_node, worker) + self._tlog(f"Loaded {next_node}") except Empty: pass From 6876a041a4954607bfa4ada6f781a5b3fe97c3cc Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 17 May 2023 11:02:04 +0100 Subject: [PATCH 21/33] More Windows thread oddness experimenting --- src/textual/widgets/_directory_tree.py | 34 +++++++++++++------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 0c615f26e..d6aec5aaa 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -3,7 +3,6 @@ from __future__ import annotations from dataclasses import dataclass from pathlib import Path from queue import Empty, Queue -from threading import RLock from typing import ClassVar, Iterable, Iterator from rich.style import Style @@ -16,8 +15,6 @@ from ..reactive import var from ..worker import Worker, get_current_worker from ._tree import TOGGLE_STYLE, Tree, TreeNode -read_dir = RLock() - @dataclass class DirEntry: @@ -270,11 +267,15 @@ class DirectoryTree(Tree[DirEntry]): Yields: Path: A entry within the location. """ - for entry in location.iterdir(): - if worker.is_cancelled: - break - yield entry - self._tlog(f"Loaded entry {entry} from {location}") + yield Path("Foo") + yield Path("Bar") + yield Path("Baz") + yield Path("Wibble") + # for entry in location.iterdir(): + # if worker.is_cancelled: + # break + # yield entry + # self._tlog(f"Loaded entry {entry} from {location}") def _load_directory(self, node: TreeNode[DirEntry], worker: Worker) -> None: """Load the directory contents for a given node. @@ -284,15 +285,14 @@ class DirectoryTree(Tree[DirEntry]): """ assert node.data is not None node.data.loaded = True - with read_dir: - self.app.call_from_thread( - self._populate_node, - node, - sorted( - self.filter_paths(self._directory_content(node.data.path, worker)), - key=lambda path: (not path.is_dir(), path.name.lower()), - ), - ) + self.app.call_from_thread( + self._populate_node, + node, + sorted( + self.filter_paths(self._directory_content(node.data.path, worker)), + key=lambda path: (not path.is_dir(), path.name.lower()), + ), + ) _LOADER_INTERVAL: Final[float] = 0.2 """How long the loader should block while waiting for queue content.""" From 64d9c602670616db8c310740619a90d5e2dd189b Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 17 May 2023 11:26:33 +0100 Subject: [PATCH 22/33] Revert experimental code --- src/textual/widgets/_directory_tree.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index d6aec5aaa..7fe50c3f1 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -267,15 +267,11 @@ class DirectoryTree(Tree[DirEntry]): Yields: Path: A entry within the location. """ - yield Path("Foo") - yield Path("Bar") - yield Path("Baz") - yield Path("Wibble") - # for entry in location.iterdir(): - # if worker.is_cancelled: - # break - # yield entry - # self._tlog(f"Loaded entry {entry} from {location}") + for entry in location.iterdir(): + if worker.is_cancelled: + break + yield entry + self._tlog(f"Loaded entry {entry} from {location}") def _load_directory(self, node: TreeNode[DirEntry], worker: Worker) -> None: """Load the directory contents for a given node. From 82924c2d7c69e35fa748b081ee159e41f90c69c3 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 17 May 2023 11:34:05 +0100 Subject: [PATCH 23/33] Make the main load worker into a asyncio task Turns out, there's a maximum number of threads you can have going in the underlying pool, that's tied to the number of CPUs. As such, there was a limit on how many directory trees you could have up and running before it would start to block all sorts of operations in the surrounding application (in Parallels on macOS, with the Windows VM appearing to have just the one CPU, it would give up after 8 directory trees). So here we move to a slightly different approach: have the main loader still run "forever", but be an async task; it then in turn farms the loading out to threads which close once the loading is done. So far tested on macOS and behaves as expected. Next to test on Windows. --- src/textual/widgets/_directory_tree.py | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 7fe50c3f1..2bd41e230 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -1,8 +1,8 @@ from __future__ import annotations +from asyncio import Queue, QueueEmpty from dataclasses import dataclass from pathlib import Path -from queue import Empty, Queue from typing import ClassVar, Iterable, Iterator from rich.style import Style @@ -239,9 +239,6 @@ class DirectoryTree(Tree[DirEntry]): """ return paths - def _tlog(self, message: str) -> None: - self.app.call_from_thread(self.log.debug, f"{self.id} - {message}") - def _populate_node(self, node: TreeNode[DirEntry], content: Iterable[Path]) -> None: """Populate the given tree node with the given directory content. @@ -271,9 +268,9 @@ class DirectoryTree(Tree[DirEntry]): if worker.is_cancelled: break yield entry - self._tlog(f"Loaded entry {entry} from {location}") - def _load_directory(self, node: TreeNode[DirEntry], worker: Worker) -> None: + @work + def _load_directory(self, node: TreeNode[DirEntry]) -> None: """Load the directory contents for a given node. Args: @@ -281,6 +278,7 @@ class DirectoryTree(Tree[DirEntry]): """ assert node.data is not None node.data.loaded = True + worker = get_current_worker() self.app.call_from_thread( self._populate_node, node, @@ -290,21 +288,14 @@ class DirectoryTree(Tree[DirEntry]): ), ) - _LOADER_INTERVAL: Final[float] = 0.2 - """How long the loader should block while waiting for queue content.""" - @work(exclusive=True) - def _loader(self) -> None: + async def _loader(self) -> None: """Background loading queue processor.""" - self._tlog("_loader started") worker = get_current_worker() while not worker.is_cancelled: try: - next_node = self._to_load.get(timeout=self._LOADER_INTERVAL) - self._tlog(f"Received {next_node} for loading") - self._load_directory(next_node, worker) - self._tlog(f"Loaded {next_node}") - except Empty: + self._load_directory(await self._to_load.get()) + except QueueEmpty: pass def _on_tree_node_expanded(self, event: Tree.NodeExpanded) -> None: From a42250daa3137db501f00c0a59906c61e66def2f Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 17 May 2023 12:29:03 +0100 Subject: [PATCH 24/33] async Queue get blocks when empty, so don't handle empty exception --- src/textual/widgets/_directory_tree.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 2bd41e230..3e88a4a2d 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -1,6 +1,6 @@ from __future__ import annotations -from asyncio import Queue, QueueEmpty +from asyncio import Queue from dataclasses import dataclass from pathlib import Path from typing import ClassVar, Iterable, Iterator @@ -293,10 +293,7 @@ class DirectoryTree(Tree[DirEntry]): """Background loading queue processor.""" worker = get_current_worker() while not worker.is_cancelled: - try: - self._load_directory(await self._to_load.get()) - except QueueEmpty: - pass + self._load_directory(await self._to_load.get()) def _on_tree_node_expanded(self, event: Tree.NodeExpanded) -> None: event.stop() From ecde90b1c31d814646a0d2d444ffb3893d0e8b20 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 17 May 2023 12:29:36 +0100 Subject: [PATCH 25/33] Remove unused import The code that was using this was removed earlier. --- src/textual/widgets/_directory_tree.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 3e88a4a2d..ba28c7f0f 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -7,7 +7,6 @@ from typing import ClassVar, Iterable, Iterator from rich.style import Style from rich.text import Text, TextType -from typing_extensions import Final from .. import work from ..message import Message From 26e6dbbfa35c19dff841c5eaf025a0b626cec5ce Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 17 May 2023 13:28:07 +0100 Subject: [PATCH 26/33] Swap to a dual-working approach Plan C; or is it plan D? Something like that. Anyway... in this approach we keep a single "forever" async task worker per directory tree, which in turn looks at the async Queue, and when a new node appears on it it starts a short-lived thread to load the directory data. This seems to be working fine on macOS. Next up is testing on Windows. --- src/textual/widgets/_directory_tree.py | 39 +++++++++++++++++++------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index ba28c7f0f..c87ab42a3 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -11,7 +11,7 @@ from rich.text import Text, TextType from .. import work from ..message import Message from ..reactive import var -from ..worker import Worker, get_current_worker +from ..worker import Worker, WorkerCancelled, WorkerFailed, get_current_worker from ._tree import TOGGLE_STYLE, Tree, TreeNode @@ -269,22 +269,22 @@ class DirectoryTree(Tree[DirEntry]): yield entry @work - def _load_directory(self, node: TreeNode[DirEntry]) -> None: + def _load_directory(self, node: TreeNode[DirEntry]) -> list[Path]: """Load the directory contents for a given node. Args: node: The node to load the directory contents for. + + Returns: + The list of entries within the directory associated with the node. """ assert node.data is not None node.data.loaded = True - worker = get_current_worker() - self.app.call_from_thread( - self._populate_node, - node, - sorted( - self.filter_paths(self._directory_content(node.data.path, worker)), - key=lambda path: (not path.is_dir(), path.name.lower()), + return sorted( + self.filter_paths( + self._directory_content(node.data.path, get_current_worker()) ), + key=lambda path: (not path.is_dir(), path.name.lower()), ) @work(exclusive=True) @@ -292,7 +292,26 @@ class DirectoryTree(Tree[DirEntry]): """Background loading queue processor.""" worker = get_current_worker() while not worker.is_cancelled: - self._load_directory(await self._to_load.get()) + # Get the next node that needs loading off the queue. Note that + # this blocks if the queue is empty. + node = await self._to_load.get() + content: list[Path] = [] + try: + # Spin up a short-lived thread that will load the content of + # the directory associated with that node. + content = await self._load_directory(node).wait() + except WorkerCancelled: + # The worker was cancelled, that would suggest we're all + # done here and we should get out of the loader in general. + break + except WorkerFailed: + # This particular worker failed to start. We don't know the + # reason so let's no-op that (for now anyway). + pass + # We're still here and we have directory content, get it into + # the tree. + if content: + self._populate_node(node, content) def _on_tree_node_expanded(self, event: Tree.NodeExpanded) -> None: event.stop() From c04bbd1e2eb55c73418ee74223d80a6df5b59e7d Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 17 May 2023 13:41:58 +0100 Subject: [PATCH 27/33] Ensure the loader kicks off when starting up with . as the directory --- src/textual/widgets/_directory_tree.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index c87ab42a3..6423454db 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -92,7 +92,7 @@ class DirectoryTree(Tree[DirEntry]): """ return self.tree - path: var[str | Path] = var["str | Path"](Path("."), init=False) + path: var[str | Path] = var["str | Path"](Path("."), init=False, always_update=True) """The path that is the root of the directory tree. Note: From dadd7c0a145d1dc342c552f45db6ab963287a6d6 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 17 May 2023 14:13:52 +0100 Subject: [PATCH 28/33] Guard against PermissionError Normally it's not a great idea to eat and hide exceptions within library code; but I think it makes sense to make an exception here. This is a UI element that lets the user navigate about a filesystem. If there is something they don't have permission for, that should not cause an exception, it should just give up with the best possible outcome. If actually doing something with the exception is important, the developer using this could use the filter to do tests and act accordingly. See #2564. --- src/textual/widgets/_directory_tree.py | 40 ++++++++++++++++++++------ 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 6423454db..6efef7e9b 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -238,6 +238,27 @@ class DirectoryTree(Tree[DirEntry]): """ return paths + @staticmethod + def _safe_is_dir(path: Path) -> bool: + """Safely check if a path is a directory. + + Args: + path: The path to check. + + Returns: + `True` if the path is for a directory, `False` if not. + """ + try: + return path.is_dir() + except PermissionError: + # We may or may not have been looking at a directory, but we + # don't have the rights or permissions to even know that. Best + # we can do, short of letting the error blow up, is assume it's + # not a directory. A possible improvement in here could be to + # have a third state which is "unknown", and reflect that in the + # tree. + return False + def _populate_node(self, node: TreeNode[DirEntry], content: Iterable[Path]) -> None: """Populate the given tree node with the given directory content. @@ -249,7 +270,7 @@ class DirectoryTree(Tree[DirEntry]): node.add( path.name, data=DirEntry(path), - allow_expand=path.is_dir(), + allow_expand=self._safe_is_dir(path), ) node.expand() @@ -263,10 +284,13 @@ class DirectoryTree(Tree[DirEntry]): Yields: Path: A entry within the location. """ - for entry in location.iterdir(): - if worker.is_cancelled: - break - yield entry + try: + for entry in location.iterdir(): + if worker.is_cancelled: + break + yield entry + except PermissionError: + pass @work def _load_directory(self, node: TreeNode[DirEntry]) -> list[Path]: @@ -284,7 +308,7 @@ class DirectoryTree(Tree[DirEntry]): self.filter_paths( self._directory_content(node.data.path, get_current_worker()) ), - key=lambda path: (not path.is_dir(), path.name.lower()), + key=lambda path: (not self._safe_is_dir(path), path.name.lower()), ) @work(exclusive=True) @@ -318,7 +342,7 @@ class DirectoryTree(Tree[DirEntry]): dir_entry = event.node.data if dir_entry is None: return - if dir_entry.path.is_dir(): + if self._safe_is_dir(dir_entry.path): if not dir_entry.loaded: self._to_load.put_nowait(event.node) else: @@ -329,5 +353,5 @@ class DirectoryTree(Tree[DirEntry]): dir_entry = event.node.data if dir_entry is None: return - if not dir_entry.path.is_dir(): + if not self._safe_is_dir(dir_entry.path): self.post_message(self.FileSelected(self, event.node, dir_entry.path)) From 2a91e13ca3bbcc24787db79d477cc3f76b317019 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 17 May 2023 14:45:08 +0100 Subject: [PATCH 29/33] Mark each load task as done when it's done --- src/textual/widgets/_directory_tree.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 6efef7e9b..b85af246c 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -336,6 +336,8 @@ class DirectoryTree(Tree[DirEntry]): # the tree. if content: self._populate_node(node, content) + # Mark this iteration as done. + self._to_load.task_done() def _on_tree_node_expanded(self, event: Tree.NodeExpanded) -> None: event.stop() From 86bee6c49573df7cb45846096f5040d18d65efc4 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 17 May 2023 15:17:05 +0100 Subject: [PATCH 30/33] Rename _to_load to _load_queue As per https://github.com/Textualize/textual/pull/2545#discussion_r1196580316 --- src/textual/widgets/_directory_tree.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index b85af246c..4c8ff7490 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -118,7 +118,7 @@ class DirectoryTree(Tree[DirEntry]): classes: A space-separated list of classes, or None for no classes. disabled: Whether the directory tree is disabled or not. """ - self._to_load: Queue[TreeNode[DirEntry]] = Queue() + self._load_queue: Queue[TreeNode[DirEntry]] = Queue() super().__init__( str(path), data=DirEntry(Path(path)), @@ -133,12 +133,12 @@ class DirectoryTree(Tree[DirEntry]): """Reload the `DirectoryTree` contents.""" self.reset(str(self.path), DirEntry(Path(self.path))) # Orphan the old queue... - self._to_load = Queue() + self._load_queue = Queue() # ...and replace the old load with a new one. self._loader() # We have a fresh queue, we have a fresh loader, get the fresh root # loading up. - self._to_load.put_nowait(self.root) + self._load_queue.put_nowait(self.root) def validate_path(self, path: str | Path) -> Path: """Ensure that the path is of the `Path` type. @@ -318,7 +318,7 @@ class DirectoryTree(Tree[DirEntry]): while not worker.is_cancelled: # Get the next node that needs loading off the queue. Note that # this blocks if the queue is empty. - node = await self._to_load.get() + node = await self._load_queue.get() content: list[Path] = [] try: # Spin up a short-lived thread that will load the content of @@ -337,7 +337,7 @@ class DirectoryTree(Tree[DirEntry]): if content: self._populate_node(node, content) # Mark this iteration as done. - self._to_load.task_done() + self._load_queue.task_done() def _on_tree_node_expanded(self, event: Tree.NodeExpanded) -> None: event.stop() @@ -346,7 +346,7 @@ class DirectoryTree(Tree[DirEntry]): return if self._safe_is_dir(dir_entry.path): if not dir_entry.loaded: - self._to_load.put_nowait(event.node) + self._load_queue.put_nowait(event.node) else: self.post_message(self.FileSelected(self, event.node, dir_entry.path)) From 522d56c601f53235784af59f5dc7ea95c1db077b Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 17 May 2023 15:21:38 +0100 Subject: [PATCH 31/33] Be more optimistic about when the node content is loaded As per https://github.com/Textualize/textual/pull/2545#discussion_r1196589864 --- src/textual/widgets/_directory_tree.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index 4c8ff7490..ebebccb1c 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -303,7 +303,6 @@ class DirectoryTree(Tree[DirEntry]): The list of entries within the directory associated with the node. """ assert node.data is not None - node.data.loaded = True return sorted( self.filter_paths( self._directory_content(node.data.path, get_current_worker()) @@ -346,6 +345,7 @@ class DirectoryTree(Tree[DirEntry]): return if self._safe_is_dir(dir_entry.path): if not dir_entry.loaded: + dir_entry.loaded = True self._load_queue.put_nowait(event.node) else: self.post_message(self.FileSelected(self, event.node, dir_entry.path)) From e381c26165ca85afdff731fee6550d4750768a6a Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 17 May 2023 15:27:01 +0100 Subject: [PATCH 32/33] Create a single method for adding a node to the load queue In doing so fix an issue where, after the previous change, I wasn't marking the root of the tree as loaded. --- src/textual/widgets/_directory_tree.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index ebebccb1c..b914bd7bc 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -129,6 +129,16 @@ class DirectoryTree(Tree[DirEntry]): ) self.path = path + def _add_to_load_queue(self, node: TreeNode[DirEntry]) -> None: + """Add the given node to the load queue. + + Args: + node: The node to add to the load queue. + """ + assert node.data is not None + node.data.loaded = True + self._load_queue.put_nowait(node) + def reload(self) -> None: """Reload the `DirectoryTree` contents.""" self.reset(str(self.path), DirEntry(Path(self.path))) @@ -138,7 +148,7 @@ class DirectoryTree(Tree[DirEntry]): self._loader() # We have a fresh queue, we have a fresh loader, get the fresh root # loading up. - self._load_queue.put_nowait(self.root) + self._add_to_load_queue(self.root) def validate_path(self, path: str | Path) -> Path: """Ensure that the path is of the `Path` type. @@ -345,8 +355,7 @@ class DirectoryTree(Tree[DirEntry]): return if self._safe_is_dir(dir_entry.path): if not dir_entry.loaded: - dir_entry.loaded = True - self._load_queue.put_nowait(event.node) + self._add_to_load_queue(event.node) else: self.post_message(self.FileSelected(self, event.node, dir_entry.path)) From abbffbfa6a28808677bbb4335a9feefebde08fc4 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 17 May 2023 15:30:13 +0100 Subject: [PATCH 33/33] Code tidy As per https://github.com/Textualize/textual/pull/2545#discussion_r1196591147 --- src/textual/widgets/_directory_tree.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/textual/widgets/_directory_tree.py b/src/textual/widgets/_directory_tree.py index b914bd7bc..72a19ddf1 100644 --- a/src/textual/widgets/_directory_tree.py +++ b/src/textual/widgets/_directory_tree.py @@ -341,10 +341,11 @@ class DirectoryTree(Tree[DirEntry]): # This particular worker failed to start. We don't know the # reason so let's no-op that (for now anyway). pass - # We're still here and we have directory content, get it into - # the tree. - if content: - self._populate_node(node, content) + else: + # We're still here and we have directory content, get it into + # the tree. + if content: + self._populate_node(node, content) # Mark this iteration as done. self._load_queue.task_done()