Fix loading race conditions.

borgmatic/actions/browse/archives_list.py

@@ -19,8 +19,9 @@ class Archives_list(textual.widgets.OptionList):
 
     def __init__(self, config, repository):
         '''
-        Given a configuration dict and a repository dict, start loading the archives from the
-        repository for eventual display in this widget.
+        Given a configuration dict and a repository dict, prepare to load the archives from the
+        repository for eventual display in this widget. Actual loading kicks off in on_mount()
+        below.
         '''
         self.config = config
         self.repository = repository
@@ -34,6 +35,16 @@ class Archives_list(textual.widgets.OptionList):
 
         self.loading_timer = borgmatic.actions.browse.loading.add_inline_loading_indicator(self)
 
+    def on_mount(self):
+        '''
+        When this widget gets mounted in the DOM, subscribe to archive loaded events so that we can
+        find out about archives as they load. Also start loading archives from the repository.
+
+        Loading is started *after* subscribing to archive loaded signals so that there's not a gap
+        where we might miss out on signal publishes.
+        '''
+        self.archive_loaded.subscribe(self, self.on_archive_loaded)
+
         borgmatic.actions.browse.workers.add_repository_archives(
             self.app,
             archive_loaded=self.archive_loaded,
@@ -42,31 +53,26 @@ class Archives_list(textual.widgets.OptionList):
             loading_timer=self.loading_timer,
         )
 
-    def on_mount(self):
-        '''
-        When this widget gets mounted in the DOM, subscribe to archive loaded events so that we can
-        find out about archives as they load.
-        '''
-        self.archive_loaded.subscribe(self, self.on_archive_loaded)
-
-    def on_archive_loaded(self, data):
+    def on_archive_loaded(self, archive_name):
         '''
         When an archive loads, add it as an option to this archives list. But if we get a
         signal that all path loading is complete, stop and remove our loading indicator.
         '''
-        if data is borgmatic.actions.browse.workers.LOADING_DONE:
+        if archive_name is borgmatic.actions.browse.workers.LOADING_DONE:
             self.loading_timer.stop()
             self.remove_option('loading-indicator')
             return
 
-        label_pieces = (data, '[dim](latest)[/dim]') if len(self.options) == 1 else (data,)
+        label_pieces = (
+            (archive_name, '[dim](latest)[/dim]') if len(self.options) == 1 else (archive_name,)
+        )
         highlighted_option = self.highlighted_option
 
         loading_indicator = self.get_option('loading-indicator')
         self.remove_option('loading-indicator')
         self.add_options(
             (
-                textual.widgets.option_list.Option(' '.join(label_pieces), id=data),
+                textual.widgets.option_list.Option(' '.join(label_pieces), id=archive_name),
                 loading_indicator,
             ),
         )

borgmatic/actions/browse/directory_list.py

@@ -119,10 +119,9 @@ class Directory_list(textual.widgets.OptionList):
         '''
         Given a configuration dict, a repository dict, an archive name, an optional
         Archive_path_loaded instance for signalling new paths as they load, and an optional tuple of
-        path components indicating this directory's position in the backed up filesystem, start
-        loading paths from the archive for eventual display in this widget. Or, if paths have
-        already started loading (by the root directory list), just listen for new paths as they come
-        in.
+        path components indicating this directory's position in the backed up filesystem, prepare to
+        load paths from the archive for eventual display in this widget. Actual loading kicks off in
+        on_mount() below.
         '''
         self.config = config
         self.repository = repository
@@ -156,22 +155,15 @@ class Directory_list(textual.widgets.OptionList):
         if not self.path_loaded.complete:
             self.timer = borgmatic.actions.browse.loading.add_inline_loading_indicator(self)
 
-        if not self.path_components:
-            borgmatic.actions.browse.workers.load_archive_paths(
-                self.app,
-                path_loaded=self.path_loaded,
-                config=self.config,
-                repository=self.repository,
-                archive_name=self.archive_name,
-            )
-
     def on_mount(self):
         '''
-        When this widget gets mounted in the DOM, subcribe to path loaded events so that we can
-        find out about relevant archive paths as they load. And if this is a non-root directory
-        list, add any already loaded archive paths to this widget as options. This is done *after*
-        subscribing to path loaded signals so that there's not a gap where we might miss out on any
-        paths.
+        When this widget gets mounted in the DOM, subcribe to path loaded events so that we can find
+        out about relevant archive paths as they load. And if this is a root directory list, start
+        loading paths from the archive. If this is a non-root directory list, add any already loaded
+        archive paths to this widget as options.
+
+        Loading is started *after* subscribing to path loaded signals so that there's not a gap
+        where we might miss out on any paths.
         '''
         self.path_loaded.subscribe(self, self.on_archive_path_loaded)
 
@@ -185,6 +177,14 @@ class Directory_list(textual.widgets.OptionList):
                     self.path_loaded.path_hierarchy, self.path_components
                 ),
             )
+        else:
+            borgmatic.actions.browse.workers.load_archive_paths(
+                self.app,
+                path_loaded=self.path_loaded,
+                config=self.config,
+                repository=self.repository,
+                archive_name=self.archive_name,
+            )
 
     def on_archive_path_loaded(self, data):
         '''

borgmatic/actions/browse/file_preview.py

@@ -1,3 +1,4 @@
+import logging
 import contextlib
 import os
 
@@ -9,6 +10,9 @@ import borgmatic.actions.browse.loading
 import borgmatic.actions.browse.workers
 
 
+logger = logging.getLogger('__name__')
+
+
 class File_preview(textual.widgets.RichLog):
     '''
     A widget for extracting and previewing the contents of a file stored in a Borg archive.
@@ -33,7 +37,8 @@ class File_preview(textual.widgets.RichLog):
     def __init__(self, config, repository, archive_name, file_path):
         '''
         Given a configuration dict, a repository dict, an archive name, and the path of a file in
-        the archive, start loading the file's contents for eventual display in this widget.
+        the archive, prepare to load the file's contents for eventual display in this widget. Actual
+        loading kicks off in on_mount() below.
         '''
         self.config = config
         self.repository = repository
@@ -49,6 +54,16 @@ class File_preview(textual.widgets.RichLog):
 
         self.loading_timer = borgmatic.actions.browse.loading.add_inline_loading_indicator(self)
 
+    def on_mount(self):
+        '''
+        When this widget gets mounted in the DOM, subscribe to archive loaded events so that we can
+        find out about archives as they load. Also start loading file contents from the archive.
+
+        Loading is started *after* subscribing to file preview loaded signals so that there's not a
+        gap where we might miss out on signal publishes.
+        '''
+        self.file_preview_loaded.subscribe(self, self.on_file_preview_loaded)
+
         borgmatic.actions.browse.workers.load_file_preview(
             self.app,
             file_preview_loaded=self.file_preview_loaded,
@@ -59,20 +74,20 @@ class File_preview(textual.widgets.RichLog):
             loading_timer=self.loading_timer,
         )
 
-    def on_mount(self):
-        '''
-        When this widget gets mounted in the DOM, subscribe to archive loaded events so that we can
-        find out about archives as they load.
-        '''
-        self.file_preview_loaded.subscribe(self, self.on_file_preview_loaded)
+    def on_file_preview_loaded(self, file_contents):
+        import time
 
-    def on_file_preview_loaded(self, data):
+        logger.debug(('on_file_preview_loaded', time.time()))
         self.loading_timer.stop()
         self.clear()
 
-        if data is None:
+        if file_contents is None:
             self.write('Cannot display a preview for this file')
         else:
+            logger.debug(('before write', time.time()))
             self.write(
-                rich.syntax.Syntax(data, rich.syntax.Syntax.guess_lexer(self.file_path, data))
+                rich.syntax.Syntax(
+                    file_contents, rich.syntax.Syntax.guess_lexer(self.file_path, file_contents)
+                )
             )
+            logger.debug(('after write', time.time()))

borgmatic/actions/browse/workers.py

@@ -171,6 +171,9 @@ def load_file_preview(
     repository dict, an archive name, the path of a file in that archive, and a loading indicator
     timer, load the contents of the file and write it into the given file preview widget.
     '''
+    import time
+
+    logger.debug(('before get_archive_file_content', time.time()))
     file_contents = borgmatic.actions.browse.archive.get_archive_file_content(
         config, repository, archive_name, file_path
     )

tests/integration/actions/browse/test_archives_list.py

@@ -6,10 +6,6 @@ import textual.widgets.option_list
 
 def test_archives_list_on_option_list_option_highlighted_with_highlighted_none_marks_it_unchanged():
     flexmock(module.borgmatic.actions.browse.loading).should_receive('add_inline_loading_indicator')
-    flexmock(module.borgmatic.actions.browse.workers).should_receive('add_repository_archives')
-    flexmock(module.borgmatic.actions.browse.archives_list.Archives_list).should_receive(
-        'app'
-    ).and_return(flexmock())
 
     archives_list = module.Archives_list(config=flexmock(), repository=flexmock())
     archives_list.highlighted = None
@@ -20,10 +16,6 @@ def test_archives_list_on_option_list_option_highlighted_with_highlighted_none_m
 
 def test_archives_list_on_option_list_option_highlighted_with_highlighted_zero_marks_it_unchanged():
     flexmock(module.borgmatic.actions.browse.loading).should_receive('add_inline_loading_indicator')
-    flexmock(module.borgmatic.actions.browse.workers).should_receive('add_repository_archives')
-    flexmock(module.borgmatic.actions.browse.archives_list.Archives_list).should_receive(
-        'app'
-    ).and_return(flexmock())
 
     archives_list = module.Archives_list(config=flexmock(), repository=flexmock())
     archives_list.highlighted = 0
@@ -34,10 +26,6 @@ def test_archives_list_on_option_list_option_highlighted_with_highlighted_zero_m
 
 def test_archives_list_on_option_list_option_highlighted_with_highlighted_zero_marks_it_unchanged():
     flexmock(module.borgmatic.actions.browse.loading).should_receive('add_inline_loading_indicator')
-    flexmock(module.borgmatic.actions.browse.workers).should_receive('add_repository_archives')
-    flexmock(module.borgmatic.actions.browse.archives_list.Archives_list).should_receive(
-        'app'
-    ).and_return(flexmock())
 
     archives_list = module.Archives_list(config=flexmock(), repository=flexmock())
     archives_list.add_option(textual.widgets.option_list.Option('zero', id='zero'))
@@ -49,10 +37,6 @@ def test_archives_list_on_option_list_option_highlighted_with_highlighted_zero_m
 
 def test_archives_list_on_option_list_option_highlighted_with_highlighted_non_zero_marks_it_changed():
     flexmock(module.borgmatic.actions.browse.loading).should_receive('add_inline_loading_indicator')
-    flexmock(module.borgmatic.actions.browse.workers).should_receive('add_repository_archives')
-    flexmock(module.borgmatic.actions.browse.archives_list.Archives_list).should_receive(
-        'app'
-    ).and_return(flexmock())
 
     archives_list = module.Archives_list(config=flexmock(), repository=flexmock())
     archives_list.add_option(textual.widgets.option_list.Option('zero', id='zero'))

tests/integration/actions/browse/test_directory_list.py

@@ -121,12 +121,8 @@ def test_add_archive_paths_retains_loading_indicator_at_bottom():
     assert directory_list.highlighted == 2
 
 
-def test_directory_list_with_root_directory_starts_loading_archive_paths():
-    flexmock(module.borgmatic.actions.browse.loading).should_receive('add_inline_loading_indicator')
-    flexmock(module.borgmatic.actions.browse.workers).should_receive('load_archive_paths').once()
-    flexmock(module.borgmatic.actions.browse.directory_list.Directory_list).should_receive(
-        'app'
-    ).and_return(flexmock())
+def test_directory_list_with_root_directory_adds_loading_indicator():
+    flexmock(module.borgmatic.actions.browse.loading).should_receive('add_inline_loading_indicator').once()
 
     directory_list = module.Directory_list(
         config=flexmock(), repository=flexmock(), archive_name='archive'
@@ -136,26 +132,6 @@ def test_directory_list_with_root_directory_starts_loading_archive_paths():
     assert not directory_list.path_loaded.complete
 
 
-def test_directory_list_with_non_root_directory_relies_on_existing_path_loading_worker():
-    flexmock(module.borgmatic.actions.browse.loading).should_receive('add_inline_loading_indicator')
-    flexmock(module.borgmatic.actions.browse.workers).should_receive('load_archive_paths').never()
-    flexmock(module.borgmatic.actions.browse.directory_list.Directory_list).should_receive(
-        'app'
-    ).and_return(flexmock())
-
-    directory_list = module.Directory_list(
-        config=flexmock(),
-        repository=flexmock(),
-        archive_name='archive',
-        path_loaded=flexmock(complete=False),
-        path_components=('etc',),
-    )
-    assert directory_list.border_title == '📁 etc'
-    assert len(directory_list.options) == 1
-    assert directory_list.options[0].prompt == '📁 ..'
-    assert directory_list.options[0].id == '..'
-
-
 def test_directory_list_with_already_complete_loading_skips_loading_indicator():
     flexmock(module.borgmatic.actions.browse.loading).should_receive(
         'add_inline_loading_indicator'
@@ -178,12 +154,12 @@ def test_directory_list_with_already_complete_loading_skips_loading_indicator():
     assert directory_list.options[0].id == '..'
 
 
-def test_directory_list_on_mount_with_root_directory_skips_adding_archives_paths():
+def test_directory_list_on_mount_with_root_directory_loads_archive_paths():
     flexmock(module.borgmatic.actions.browse.loading).should_receive('add_inline_loading_indicator')
-    flexmock(module.borgmatic.actions.browse.workers).should_receive('load_archive_paths')
     flexmock(module.borgmatic.actions.browse.directory_list.Directory_list).should_receive(
         'app'
     ).and_return(flexmock())
+    flexmock(module.borgmatic.actions.browse.workers).should_receive('load_archive_paths').once()
     flexmock(module).should_receive('add_archive_paths').never()
     directory_list = module.Directory_list(
         config=flexmock(), repository=flexmock(), archive_name='archive'
@@ -198,6 +174,7 @@ def test_directory_list_on_mount_with_non_root_directory_adds_archive_paths():
     flexmock(module.borgmatic.actions.browse.directory_list.Directory_list).should_receive(
         'app'
     ).and_return(flexmock())
+    flexmock(module.borgmatic.actions.browse.workers).should_receive('load_archive_paths').never()
     flexmock(module).should_receive('add_archive_paths').once()
     directory_list = module.Directory_list(
         config=flexmock(),

tests/integration/actions/browse/test_file_preview.py

@@ -6,10 +6,6 @@ import textual.widgets.option_list
 
 def test_file_preview_does_not_raise():
     flexmock(module.borgmatic.actions.browse.loading).should_receive('add_inline_loading_indicator')
-    flexmock(module.borgmatic.actions.browse.workers).should_receive('load_file_preview')
-    flexmock(module.borgmatic.actions.browse.file_preview.File_preview).should_receive(
-        'app'
-    ).and_return(flexmock())
 
     module.File_preview(
         config=flexmock(), repository=flexmock(), archive_name='archive', file_path='foo/bar.txt'