When reopening a project, restore download tasks correctly

Author: davidfstr

RELEASE_NOTES.md

@@ -12,6 +12,9 @@ Release Notes ⋮
 
 ### main
 
+* Workflow improvements
+    * When reopening a project, restore download tasks correctly.
+
 * Support changes **(Breaking Change)**
     * macOS 14+ is now the minimum macOS version.
       Drop support for macOS 13.

src/crystal/browser/tasktree.py

@@ -160,7 +160,11 @@ def change_root_task(self, new_root_task: Task) -> None:
         # Dispose the old root TaskTreeNode
         self.root.dispose()
 
-        # Create new root TaskTreeNode for the new root task
+        # Dispose all peer TreeItems
+        self.peer.DeleteChildren(self.peer.GetRootItem())
+        
+        # 1. Create new root TaskTreeNode for the new root task
+        # 2. Create new peer TreeItems
         self.root = TaskTreeNode(new_root_task)
         self.tree.root = self.root.tree_node
 
@@ -573,7 +577,7 @@ def fg_task() -> None:
             )
 
             # Commit changes to the children list
-            new_children = []  # type: List[NodeView1]
+            new_children = []  # type: List[NodeView]
             if first_more_node.more_count != 0:
                 new_children.append(first_more_node)
             new_children.extend(intermediate_nodes)

src/crystal/model.py

@@ -472,17 +472,20 @@ def _start_scheduler(self) -> None:
         """
         import crystal.task
 
-        # Capture old root task if it exists (for reopen scenarios)
+        # Recreate the root task
         old_root_task = getattr(self, 'root_task', None)  # capture
+        new_root_task = crystal.task.RootTask()
+        self.root_task = new_root_task  # export
 
-        self.root_task = crystal.task.RootTask()
+        # Notify listeners if this is a root task change (not initial creation)
+        if old_root_task is not None:
+            self._root_task_did_change(old_root_task, new_root_task)
+        
+        # Start the scheduler (after preparing everything else)
         self._scheduler_thread = (
-            crystal.task.start_scheduler_thread(self.root_task)
+            crystal.task.start_scheduler_thread(new_root_task)
         )  # type: threading.Thread | None
 
-        # Notify listeners if this is a root task change (not initial creation)
-        if old_root_task is not None:
-            self._root_task_did_change(old_root_task, self.root_task)
 
     # --- Load: Validity ---
 
@@ -1994,6 +1997,10 @@ def _save_as_coro(self,
             #       Project state it may fail again.
             self.close(capture_crashes=False, _will_reopen=True)
 
+            # Reset session state that could affect how tasks are unhibernated
+            for r in self._materialized_resources:
+                r.already_downloaded_this_session = False
+            
             # Move/copy the project directory to the new path.
             # - If the project is untitled, it will be renamed.
             # - If the project is titled, it will be copied.

src/crystal/task.py

@@ -800,6 +800,30 @@ def task_crash_reason_did_change(self, task: Task) -> None:
 
         if hasattr(self, 'child_task_did_crash'):
             self.child_task_did_crash(task)
+
+    # === Debugging ===
+
+    def print_tree(self) -> None:
+        """Print a formatted view of this task and its descendants."""
+        lines: list[str] = []
+        def format_tree(parent: Task, depth: int) -> None:
+            children_seq = parent.children  # cache
+            for index in range(len(children_seq)):
+                try:
+                    child = children_seq[index]
+                except UnmaterializedItemError:
+                    lines.append(f"{'  ' * depth}{index}: <unmaterialized child>")
+                else:
+                    lines.append(f"{'  ' * depth}{index}: {child.title} -- {child.subtitle}")
+                    format_tree(child, depth + 1)
+        if len(self.children) == 0:
+            lines.append('<empty>')
+        else:
+            format_tree(self, 0)
+        
+        tree_output = '\n'.join(lines)
+        print(tree_output)
+        print()
 
     # === Utility ===
 

src/crystal/tests/test_untitled_projects.py

@@ -1,6 +1,7 @@
 from collections.abc import AsyncIterator, Iterator
 from contextlib import asynccontextmanager, contextmanager
 from crystal.browser import MainWindow as RealMainWindow
+from crystal.browser.tasktree import TaskTreeNode
 from crystal.model import (
     Project, ProjectReadOnlyError, Resource, ResourceGroup, RootResource,
 )
@@ -32,6 +33,7 @@
 from crystal.util.wx_dialog import mocked_show_modal
 from crystal.util.xappdirs import user_untitled_projects_dir
 from crystal.util.xos import is_ci, is_linux, is_mac_os, is_windows
+from crystal.util.xtyping import not_none
 from dataclasses import dataclass
 import errno
 from functools import cache, wraps
@@ -137,6 +139,8 @@ async def test_when_project_properties_changed_then_untitled_project_becomes_dir
 
 @awith_subtests
 async def test_when_untitled_project_saved_then_becomes_clean_and_titled(subtests: SubtestsContext) -> None:
+    DEBUG_TASK_TREE = False
+    
     with scheduler_disabled() as disabled_scheduler, \
             served_project('testdata_xkcd.crystalproj.zip') as sp:
         atom_feed_url = sp.get_request_url('https://xkcd.com/atom.xml')
@@ -256,40 +260,78 @@ async def test_when_untitled_project_saved_then_becomes_clean_and_titled(subtest
         # Test saving an untitled project while downloads are in progress
         # AKA: test_when_save_as_project_with_active_tasks_then_hibernates_and_restores_tasks
         with subtests.test(tasks_running=True), \
-                _untitled_project() as project, \
                 xtempfile.TemporaryDirectory() as new_container_dirpath:
-            # Download a resource revision, so that comic URLs are discovered
-            r = Resource(project, home_url)
-            rr_future = r.download()
-            await step_scheduler_until_done(project)
-            
-            # Start downloading a group and an individual resource
-            g = ResourceGroup(project, 'Comic', comic_pattern)
-            g.download()
-            await step_scheduler(project)
-            root_r = RootResource(project, '', Resource(project, comic50_url))
-            root_r.download()
-            await step_scheduler(project)
-            (old_drg_task, old_dr_task) = project.root_task.children
-            assert isinstance(old_drg_task, DownloadResourceGroupTask)
-            assert isinstance(old_dr_task, DownloadResourceTask)
-            
-            # Save untitled project to somewhere else
-            new_project_dirpath = os.path.join(
-                new_container_dirpath,
-                os.path.basename(old_project_dirpath))
-            await save_as_without_ui(project, new_project_dirpath)
-            append_deferred_top_level_tasks(project)
-            
-            # Ensure tasks are restored
-            (new_drg_task, new_dr_task) = project.root_task.children
-            assert isinstance(new_drg_task, DownloadResourceGroupTask)
-            assert isinstance(new_dr_task, DownloadResourceTask)
-            assert new_drg_task is not old_drg_task
-            assert new_dr_task is not old_dr_task
-            
-            # Ensure tasks can still be stepped without error
-            await step_scheduler(project)
+            # Create untitled project with UI
+            async with (await OpenOrCreateDialog.wait_for()).create() as (mw, project):
+                rmw = RealMainWindow._last_created
+                assert rmw is not None
+                
+                # Download a resource revision, so that comic URLs are discovered
+                r = Resource(project, home_url)
+                rr_future = r.download()
+                await step_scheduler_until_done(project)
+                
+                # Start downloading a group and an individual resource
+                g = ResourceGroup(project, 'Comic', comic_pattern)
+                g.download()
+                await step_scheduler(project)
+                root_r = RootResource(project, '', Resource(project, comic50_url))
+                root_r.download()
+                await step_scheduler(project)
+                (old_drg_task, old_dr_task) = project.root_task.children
+                assert isinstance(old_drg_task, DownloadResourceGroupTask)
+                assert isinstance(old_dr_task, DownloadResourceTask)
+                
+                if DEBUG_TASK_TREE:
+                    print(f'Task tree before save:')
+                    project.root_task.print_tree()
+                
+                # Save untitled project to somewhere else
+                new_project_dirpath = os.path.join(
+                    new_container_dirpath,
+                    os.path.basename(old_project_dirpath))
+                await save_as_with_ui(rmw, new_project_dirpath)
+                append_deferred_top_level_tasks(project)
+                
+                # Ensure tasks are restored (in Task Tree)
+                (new_drg_task, new_dr_task) = project.root_task.children
+                assert isinstance(new_drg_task, DownloadResourceGroupTask)
+                assert isinstance(new_dr_task, DownloadResourceTask)
+                assert new_drg_task is not old_drg_task
+                assert new_dr_task is not old_dr_task
+                
+                # Ensure tasks are restored (in Task Tree UI)
+                top_level_ttns = [
+                    not_none(TaskTreeNode.for_node_view(nv))
+                    for nv in rmw.task_tree.root.tree_node.children
+                ]
+                (new_drg_ttn, new_dr_ttn) = top_level_ttns
+                assert isinstance(new_drg_ttn.task, DownloadResourceGroupTask)
+                assert isinstance(new_dr_ttn.task, DownloadResourceTask)
+                assert new_drg_ttn.task is new_drg_task
+                assert new_dr_ttn.task is new_dr_task
+                
+                # Ensure tasks are restored (in low level Task Tree UI)
+                assert 2 == rmw.task_tree.peer.GetChildrenCount(rmw.task_tree.peer.GetRootItem(), recursively=False)
+                
+                # 1. Ensure restored tasks can still be stepped without error
+                # 2. Ensure restored tasks will complete
+                done = False
+                step_count = 0
+                if DEBUG_TASK_TREE:
+                    print(f'Task tree after save + 0 step(s):')
+                    project.root_task.print_tree()
+                while not done:
+                    done = await step_scheduler(project, expect_done=None)
+                    step_count += 1
+                    if DEBUG_TASK_TREE:
+                        print(f'Task tree after save + {step_count} step(s):')
+                        project.root_task.print_tree()
+                
+                # Ensure effects of restored tasks are correct
+                assert root_r.resource.has_any_revisions()
+                for member in g.members:
+                    assert member.has_any_revisions(), f'Member {member.url!r} did not finish downloading'
 
 
 # === Untitled Project: Create Tests ===

src/crystal/tests/util/tasks.py

@@ -234,7 +234,7 @@ def scheduler_thread_context(enabled: bool=True) -> Iterator[None]:
 @scheduler_affinity  # manual control of scheduler thread is assumed
 async def step_scheduler(
         project: Project,
-        *, expect_done: bool=False,
+        *, expect_done: bool | None=False,
         after_get: Callable[[], None] | None=None,
         ) -> bool:
     """
@@ -256,12 +256,12 @@ async def step_scheduler(
     if after_get is not None:
         after_get()
     if unit is None:
-        if not expect_done:
+        if expect_done == False:
             raise AssertionError('Expected there to be no more tasks')
         _set_scheduler_is_waiting(project.root_task, True)
         return True
     else:
-        if expect_done:
+        if expect_done == True:
             raise AssertionError('Expected there to be at least one task remaining')
         await bg_call_and_wait(scheduler_thread_context()(unit))
         return False

src/crystal/ui/tree2.py

@@ -6,6 +6,7 @@
 # Clients should be able to import TreeView from this package
 from crystal.ui.tree import NodeView as NodeView1
 from crystal.ui.tree import TreeView
+from typing import List
 
 
 class NodeView(NodeView1):
@@ -34,6 +35,9 @@ def _set_subtitle(self, value: str) -> None:
         self._update_base_title()
     subtitle = property(_get_subtitle, _set_subtitle)
 
+    # NOTE: Assumes that tree of NodeViews will all be of the same type
+    children: 'List[NodeView]'  # type: ignore[assignment]
+    
     def _update_base_title(self) -> None:
         subtitle = self.__subtitle  # cache
         combined_title = (