Bug slow import 934 (#942)

* Working on slow import, #934

* Tests working for #934

* Removed unnecessary report record updates

examples/post_function_import.py

@@ -3,9 +3,15 @@
 import typing as t
 import photoscript
 import pathlib
+from osxphotos.cli.import_cli import ReportRecord
+
 
 def post_function(
-    photo: photoscript.Photo, filepath: pathlib.Path, verbose: t.Callable, **kwargs
+    photo: photoscript.Photo,
+    filepath: pathlib.Path,
+    verbose: t.Callable,
+    report_record: ReportRecord,
+    **kwargs,
 ):
     """Call this with osxphotos import /file/to/import --post-function post_function.py::post_function
         This will get called immediately after the photo has been imported into Photos
@@ -15,6 +21,7 @@ def post_function(
         photo: photoscript.Photo instance for the photo that's just been imported
         filepath: pathlib.Path to the file that was imported (this is the path to the source file, not the path inside the Photos library)
         verbose: A function to print verbose output if --verbose is set; if --verbose is not set, acts as a no-op (nothing gets printed)
+        report_record: ReportRecord instance for the photo that's just been imported; update this if you want to change the report output
         **kwargs: reserved for future use; recommend you include **kwargs so your function still works if additional arguments are added in future versions
 
     Notes:
@@ -25,4 +32,16 @@ def post_function(
 
     # add a note to the photo's description
     verbose("Adding note to description")
-    photo.description = f"{photo.description} (imported with osxphotos)"
+    description = photo.description
+    description = (
+        f"{description} (imported with osxphotos)"
+        if description
+        else "(imported with osxphotos)"
+    )
+
+    # update report_record if you modify the photo and want the report to reflect the change
+    # the report_record object passed to the function is mutable so you can update it directly
+    report_record.description = description
+
+    # update the photo's description via the Photo object
+    photo.description = description

osxphotos/cli/import_cli.py

@@ -1,9 +1,6 @@
 """import command for osxphotos CLI to import photos into Photos"""
 
-# Note: the style in this module is a bit different than much of the other osxphotos code
+from __future__ import annotations
-# As an experiment, I've used mostly functions instead of classes (e.g. the report writer
-# functions vs ReportWriter class used by export) and I've kept everything for import
-# self-contained in this one file
 
 import csv
 import datetime
@@ -59,6 +56,12 @@ from .common import THEME_OPTION
 from .rich_progress import rich_progress
 from .verbose import get_verbose_console, verbose_print
 
+# Note: the style in this module is a bit different than much of the other osxphotos code
+# As an experiment, I've used mostly functions instead of classes (e.g. the report writer
+# functions vs ReportWriter class used by export) and I've kept everything for import
+# self-contained in this one file
+
+
 MetaData = namedtuple("MetaData", ["title", "description", "keywords", "location"])
 
 OSXPHOTOS_ABOUT_STRING = f"Created by osxphotos version {__version__} (https://github.com/RhetTbull/osxphotos) on {datetime.datetime.now()}"
@@ -165,7 +168,8 @@ def import_photo(
     Args:
         filepath: path to the file to import
         dup_check: enable or disable Photo's duplicate check on import
-        verbose: Callable"""
+        verbose: Callable
+    """
     if imported := PhotosLibrary().import_photos(
         [filepath], skip_duplicate_check=not dup_check
     ):
@@ -207,7 +211,7 @@ def add_photo_to_albums(
     split_folder: str,
     exiftool_path: Path,
     verbose: Callable[..., None],
-):
+) -> list[str]:
     """Add photo to one or more albums"""
     albums = []
     for a in album:
@@ -225,6 +229,7 @@ def add_photo_to_albums(
             a, verbose=verbose, split_folder=split_folder, rich=True
         )
         photos_album.add(photo)
+    return albums
 
 
 def clear_photo_metadata(photo: Photo, filepath: Path, verbose: Callable[..., None]):
@@ -348,7 +353,11 @@ def location_from_file(
     return latitude, longitude
 
 
-def set_photo_metadata(photo: Photo, metadata: MetaData, merge_keywords: bool):
+def set_photo_metadata(
+    photo: Photo,
+    metadata: MetaData,
+    merge_keywords: bool,
+) -> MetaData:
     """Set metadata (title, description, keywords) for a Photo object"""
     photo.title = metadata.title
     photo.description = metadata.description
@@ -358,6 +367,7 @@ def set_photo_metadata(photo: Photo, metadata: MetaData, merge_keywords: bool):
             keywords.extend(old_keywords)
             keywords = list(set(keywords))
     photo.keywords = keywords
+    return MetaData(metadata.title, metadata.description, keywords, metadata.location)
 
 
 def set_photo_metadata_from_exiftool(
@@ -366,12 +376,12 @@ def set_photo_metadata_from_exiftool(
     exiftool_path: str,
     merge_keywords: bool,
     verbose: Callable[..., None],
-):
+) -> MetaData:
     """Set photo's metadata by reading metadata form file with exiftool"""
     verbose(f"Setting metadata and location from EXIF for [filename]{filepath.name}[/]")
     metadata = metadata_from_file(filepath, exiftool_path)
     if any([metadata.title, metadata.description, metadata.keywords]):
-        set_photo_metadata(photo, metadata, merge_keywords)
+        metadata = set_photo_metadata(photo, metadata, merge_keywords)
         verbose(f"Set metadata for [filename]{filepath.name}[/]:")
         verbose(
             f"title='{metadata.title}', description='{metadata.description}', keywords={metadata.keywords}"
@@ -387,6 +397,7 @@ def set_photo_metadata_from_exiftool(
         )
     else:
         verbose(f"No location to set for [filename]{filepath.name}[/]")
+    return metadata
 
 
 def set_photo_title(
@@ -396,7 +407,7 @@ def set_photo_title(
     title_template: str,
     exiftool_path: str,
     verbose: Callable[..., None],
-):
+) -> str:
     """Set title of photo"""
     title_text = render_photo_template(
         filepath, relative_filepath, title_template, exiftool_path
@@ -412,6 +423,9 @@ def set_photo_title(
             f"Setting title of photo [filename]{filepath.name}[/] to '{title_text[0]}'"
         )
         photo.title = title_text[0]
+        return title_text[0]
+    else:
+        return ""
 
 
 def set_photo_description(
@@ -421,7 +435,7 @@ def set_photo_description(
     description_template: str,
     exiftool_path: str,
     verbose: Callable[..., None],
-):
+) -> str:
     """Set description of photo"""
     description_text = render_photo_template(
         filepath, relative_filepath, description_template, exiftool_path
@@ -437,6 +451,9 @@ def set_photo_description(
             f"Setting description of photo [filename]{filepath.name}[/] to '{description_text[0]}'"
         )
         photo.description = description_text[0]
+        return description_text[0]
+    else:
+        return ""
 
 
 def set_photo_keywords(
@@ -447,7 +464,7 @@ def set_photo_keywords(
     exiftool_path: str,
     merge: bool,
     verbose: Callable[..., None],
-):
+) -> list[str]:
     """Set keywords of photo"""
     keywords = []
     for keyword in keyword_template:
@@ -460,6 +477,7 @@ def set_photo_keywords(
                 keywords = list(set(keywords))
         verbose(f"Setting keywords of photo [filename]{filepath.name}[/] to {keywords}")
         photo.keywords = keywords
+    return keywords
 
 
 def set_photo_location(
@@ -467,17 +485,18 @@ def set_photo_location(
     filepath: Path,
     location: Tuple[float, float],
     verbose: Callable[..., None],
-):
+) -> tuple[float, float]:
     """Set location of photo"""
     verbose(
         f"Setting location of photo [filename]{filepath.name}[/] to {location[0]}, {location[1]}"
     )
     photo.location = location
+    return location
 
 
 def set_photo_date_from_filename(
     photo: Photo, filepath: Path, parse_date: str, verbose: Callable[..., None]
-):
+) -> datetime.datetime | None:
     """Set date of photo from filename"""
     # TODO: handle timezone (use code from timewarp), for now convert timezone to local timezone
     try:
@@ -495,11 +514,12 @@ def set_photo_date_from_filename(
         verbose(
             f"[warning]Could not parse date from filename [filename]{filepath.name}[/][/]"
         )
-        return
+        return None
     verbose(
         f"Setting date of photo [filename]{filepath.name}[/] to [time]{date.strftime('%Y-%m-%d %H:%M:%S')}[/]"
     )
     photo.date = date
+    return date
 
 
 def get_relative_filepath(filepath: Path, relative_to: Optional[str]) -> Path:
@@ -592,6 +612,8 @@ def check_templates_and_exit(
 
 @dataclass
 class ReportRecord:
+    """Dataclass that records metadata on each file imported for writing to report"""
+
     albums: List[str] = field(default_factory=list)
     description: str = ""
     error: bool = False
@@ -619,6 +641,13 @@ class ReportRecord:
         )
         return cls(**dict_data)
 
+    def update_from_metadata(self, metadata: MetaData):
+        """Update a ReportRecord with data from a MetaData"""
+        self.title = metadata.title
+        self.description = metadata.description
+        self.keywords = metadata.keywords
+        self.location = metadata.location
+
     def asdict(self):
         return asdict(self)
 
@@ -632,15 +661,15 @@ class ReportRecord:
 
 def update_report_record(report_record: ReportRecord, photo: Photo, filepath: Path):
     """Update a ReportRecord with data from a Photo"""
-    report_record.albums = [a.title for a in photo.albums]
+
-    report_record.description = photo.description
+    # do not update albums as they are added to the report record as they are imported (#934)
     report_record.filename = filepath.name
     report_record.filepath = filepath
-    report_record.imported = True
+    report_record.uuid = photo.uuid
+    report_record.title = photo.title
+    report_record.description = photo.description
     report_record.keywords = photo.keywords
     report_record.location = photo.location
-    report_record.title = photo.title
-    report_record.uuid = photo.uuid
 
     return report_record
 
@@ -1477,11 +1506,13 @@ def import_cli(
             report_data[filepath] = ReportRecord(
                 filepath=filepath, filename=filepath.name
             )
+            report_record = report_data[filepath]
             photo, error = import_photo(filepath, dup_check, verbose)
             if error:
                 error_count += 1
-                report_data[filepath].error = True
+                report_record.error = True
                 continue
+            report_record.imported = True
             imported_count += 1
 
             if clear_metadata:
@@ -1492,12 +1523,21 @@ def import_cli(
 
             if exiftool:
                 set_photo_metadata_from_exiftool(
-                    photo, filepath, exiftool_path, merge_keywords, verbose
+                    photo,
+                    filepath,
+                    exiftool_path,
+                    merge_keywords,
+                    verbose,
                 )
 
             if title:
                 set_photo_title(
-                    photo, filepath, relative_filepath, title, exiftool_path, verbose
+                    photo,
+                    filepath,
+                    relative_filepath,
+                    title,
+                    exiftool_path,
+                    verbose,
                 )
 
             if description:
@@ -1526,9 +1566,10 @@ def import_cli(
 
             if parse_date:
                 set_photo_date_from_filename(photo, filepath, parse_date, verbose)
+                # TODO: ReportRecord doesn't currently record date
 
             if album:
-                add_photo_to_albums(
+                report_record.albums = add_photo_to_albums(
                     photo,
                     filepath,
                     relative_filepath,
@@ -1543,14 +1584,15 @@ def import_cli(
                     # post function is tuple of (function, filename.py::function_name)
                     verbose(f"Calling post-function [bold]{function[1]}")
                     try:
-                        function[0](photo, filepath, verbose)
+                        function[0](photo, filepath, verbose, report_record)
                     except Exception as e:
                         rich_echo_error(
                             f"[error]Error running post-function [italic]{function[1]}[/italic]: {e}"
                         )
 
-            update_report_record(report_data[filepath], photo, filepath)
+            # update report data
-            import_db.set(str(filepath), report_data[filepath])
+            update_report_record(report_record, photo, filepath)
+            import_db.set(str(filepath), report_record)
 
             progress.advance(task)
 

tests/conftest.py

@@ -50,13 +50,21 @@ def get_os_version():
     return (ver, major, minor)
 
 
-OS_VER = get_os_version()[1]
+OS_VER = get_os_version()
-if OS_VER == "15":
+if OS_VER[0] == "10" and OS_VER[1] == "15":
+    # Catalina
     TEST_LIBRARY = "tests/Test-10.15.7.photoslibrary"
     TEST_LIBRARY_IMPORT = TEST_LIBRARY
     TEST_LIBRARY_SYNC = TEST_LIBRARY
     from tests.config_timewarp_catalina import TEST_LIBRARY_TIMEWARP
 
+    TEST_LIBRARY_ADD_LOCATIONS = None
+elif OS_VER[0] == "13":
+    # Ventura
+    TEST_LIBRARY = "tests/Test-13.0.0.photoslibrary"
+    TEST_LIBRARY_IMPORT = TEST_LIBRARY
+    TEST_LIBRARY_SYNC = TEST_LIBRARY
+    TEST_LIBRARY_TIMEWARP = None
     TEST_LIBRARY_ADD_LOCATIONS = None
 else:
     TEST_LIBRARY = None
@@ -210,7 +218,7 @@ def pytest_collection_modifyitems(config, items):
 
     if not (config.getoption("--test-import") and TEST_LIBRARY_IMPORT is not None):
         skip_test_import = pytest.mark.skip(
-            reason="need --test-import option and MacOS Catalina to run"
+            reason="need --test-import option and MacOS Catalina or Ventura to run"
         )
         for item in items:
             if "test_import" in item.keywords:

tests/test_cli_import.py

@@ -81,10 +81,6 @@ try:
 except FileNotFoundError:
     exiftool_path = None
 
-OS_VER = get_os_version()[1]
-if OS_VER != "15":
-    pytest.skip(allow_module_level=True)
-
 
 def prompt(message):
     """Helper function for tests that require user input"""
@@ -1010,7 +1006,7 @@ def test_import_post_function():
         with open("foo1.py", "w") as f:
             f.writelines(
                 [
-                    "def foo(photo, filepath, verbose, **kwargs):\n",
+                    "def foo(photo, filepath, verbose, report_record, **kwargs):\n",
                     "    verbose('FOO BAR')\n",
                 ]
             )