From 406fc989089fe8cc9f3902872342fe1d0adc850c Mon Sep 17 00:00:00 2001 From: Eric Pheterson Date: Wed, 27 May 2026 18:02:44 -0700 Subject: [PATCH 1/5] feat: auto-download Live Photo .mov pair (closes #199) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a photo is a Live Photo and the user has requested the 'original' file_size, the orchestrator collects a second download task for the paired .mov. Live Photos now round-trip intact (HEIC + paired MOV land together in the destination) instead of dropping the video half. Has no effect on plain stills (the 'live_video_original' key is absent in photo.versions for them) or when the user did not request 'original'. Failure to read photo.versions is non-fatal — the original-still task is still emitted. Also filters live_video_* keys out of the user-facing file_sizes validation (these are internal — surfaced by the auto-append, not for direct user configuration). A user who adds live_video_original to file_sizes would silently get duplicate download tasks for the same path; this PR makes them get a clear 'not a valid option' log line. Dependency: requires icloudpy with the live_video_* keys in PHOTO_VERSION_LOOKUP and the item_type property — submitted as a companion PR at mandarons/icloudpy. requirements.txt temporarily pins the fork branch. Once that PR merges and a new icloudpy release ships, please bump requirements.txt back to a version pin (e.g. icloudpy==0.9.0) — happy to follow up. Tests: - 5 in tests/test_live_photo_pair_download.py: * Live Photo with 'original' yields two tasks (still + .mov) * still photo yields one task * Live Photo with only medium/thumb does NOT append .mov * photo.versions raising is non-fatal * None result from collect_download_task is skipped Closes #199. --- src/album_sync_orchestrator.py | 28 ++++ src/config_parser.py | 14 +- tests/test_live_photo_pair_download.py | 169 +++++++++++++++++++++++++ 3 files changed, 210 insertions(+), 1 deletion(-) create mode 100644 tests/test_live_photo_pair_download.py diff --git a/src/album_sync_orchestrator.py b/src/album_sync_orchestrator.py index e5da95810..72a53a223 100644 --- a/src/album_sync_orchestrator.py +++ b/src/album_sync_orchestrator.py @@ -137,6 +137,34 @@ def _collect_photo_download_tasks( ) if download_info: tasks.append(download_info) + + # Live Photo .mov pair — auto-included when the user asked for + # the "original" still and the asset is actually a Live Photo + # (icloudpy exposes the paired .mov under the "live_video_original" + # version key). Mirrors how Apple's Photos.app pairs the two files. + # Requires icloudpy with the Live Photo patch (PHOTO_VERSION_LOOKUP + # contains "live_video_*") — falls through gracefully on older + # icloudpy versions where the key is absent from photo.versions. + if "original" in file_sizes: + # photo.versions can raise on partial CloudKit records — swallow + # so the still tasks still emit. ``getattr`` default doesn't help + # against a property that raises, so an explicit try/except is + # required. + try: + live_versions = photo.versions + except Exception: + live_versions = {} + if "live_video_original" in live_versions: + live_task = collect_download_task( + photo, + "live_video_original", + destination_path, + files, + folder_format, + hardlink_registry, + ) + if live_task: + tasks.append(live_task) return tasks except Exception as e: try: diff --git a/src/config_parser.py b/src/config_parser.py index 4099b0766..5f8de0ae6 100644 --- a/src/config_parser.py +++ b/src/config_parser.py @@ -577,13 +577,25 @@ def get_photos_folder_format(config: dict) -> str | None: def validate_file_sizes(file_sizes: list[str]) -> list[str]: """Validate and filter file sizes against valid options. + The ``live_video_*`` keys are internal — surfaced by the Live Photo + auto-append mechanism in ``_collect_photo_download_tasks`` when + ``"original"`` is in the user's ``file_sizes``. They are NOT intended + for direct user configuration. Filtering them out here means a user who + accidentally adds them to ``file_sizes`` sees a clear "not a valid + option" log line instead of getting silent duplicate download tasks + queued (the explicit ``live_video_original`` task + the auto-append both + targeting the same path). + Args: file_sizes: List of file size strings to validate Returns: List of valid file sizes (defaults to ["original"] if all invalid) """ - valid_file_sizes = list(PhotoAsset.PHOTO_VERSION_LOOKUP.keys()) + valid_file_sizes = [ + k for k in PhotoAsset.PHOTO_VERSION_LOOKUP.keys() + if not k.startswith("live_video_") + ] validated_sizes = [] for file_size in file_sizes: diff --git a/tests/test_live_photo_pair_download.py b/tests/test_live_photo_pair_download.py new file mode 100644 index 000000000..94eb49ce9 --- /dev/null +++ b/tests/test_live_photo_pair_download.py @@ -0,0 +1,169 @@ +"""Live Photo .mov pair auto-append tests. + +Added 2026-05-27 as part of feat/per-library-destinations-and-live-photos. + +When a photo is a Live Photo (icloudpy exposes ``live_video_original`` in +``photo.versions``) and the user has requested the ``original`` file_size, +the orchestrator should automatically collect a second download task for +the paired ``.mov`` so the Live Photo round-trips intact. +""" + +import unittest +from unittest.mock import MagicMock, patch + +from src.album_sync_orchestrator import _collect_photo_download_tasks + + +def _fake_photo(filename, versions): + """A photo stub that quacks like icloudpy.services.photos.PhotoAsset.""" + photo = MagicMock() + photo.filename = filename + photo.versions = versions + photo.id = "test-photo-id" + return photo + + +class TestLivePhotoPairDownload(unittest.TestCase): + """Live Photo .mov auto-append behaviour.""" + + def setUp(self): + # Patch is_photo_wanted to always pass; we're testing task collection, + # not the wantedness filter. + self._wanted_patcher = patch( + "src.album_sync_orchestrator.is_photo_wanted", return_value=True + ) + self._wanted_patcher.start() + + def tearDown(self): + self._wanted_patcher.stop() + + @patch("src.album_sync_orchestrator.collect_download_task") + def test_live_photo_with_original_yields_two_tasks(self, fake_collect): + photo = _fake_photo( + "IMG_1234.HEIC", + {"original": {"url": "x"}, "live_video_original": {"url": "y"}}, + ) + # Each call returns a unique MagicMock — non-None so tasks list grows + fake_collect.side_effect = lambda *a, **kw: MagicMock(name=f"task-{a[1]}") + + tasks = _collect_photo_download_tasks( + photo, + destination_path="/tmp/dest", + file_sizes=["original"], + extensions=None, + files=set(), + folder_format=None, + hardlink_registry=None, + ) + + assert len(tasks) == 2 + # Inspect which file_sizes got requested + called_sizes = [call.args[1] for call in fake_collect.call_args_list] + assert "original" in called_sizes + assert "live_video_original" in called_sizes + + @patch("src.album_sync_orchestrator.collect_download_task") + def test_still_photo_yields_only_one_task(self, fake_collect): + photo = _fake_photo("IMG_5678.HEIC", {"original": {"url": "x"}}) + fake_collect.side_effect = lambda *a, **kw: MagicMock(name=f"task-{a[1]}") + + tasks = _collect_photo_download_tasks( + photo, + destination_path="/tmp/dest", + file_sizes=["original"], + extensions=None, + files=set(), + folder_format=None, + hardlink_registry=None, + ) + + assert len(tasks) == 1 + called_sizes = [call.args[1] for call in fake_collect.call_args_list] + assert called_sizes == ["original"] + + @patch("src.album_sync_orchestrator.collect_download_task") + def test_live_photo_without_original_request_does_not_append_mov( + self, fake_collect + ): + """If the user asked only for medium/thumb (not original), the .mov is not appended. + + The Live Photo .mov is the *original* video resource; users who explicitly + want only smaller variants shouldn't get the original .mov bundled. + """ + photo = _fake_photo( + "IMG_1234.HEIC", + { + "medium": {"url": "m"}, + "thumb": {"url": "t"}, + "live_video_original": {"url": "y"}, + }, + ) + fake_collect.side_effect = lambda *a, **kw: MagicMock(name=f"task-{a[1]}") + + tasks = _collect_photo_download_tasks( + photo, + destination_path="/tmp/dest", + file_sizes=["medium", "thumb"], + extensions=None, + files=set(), + folder_format=None, + hardlink_registry=None, + ) + + called_sizes = [call.args[1] for call in fake_collect.call_args_list] + assert called_sizes == ["medium", "thumb"] + assert "live_video_original" not in called_sizes + + @patch("src.album_sync_orchestrator.collect_download_task") + def test_versions_access_failure_is_non_fatal(self, fake_collect): + """If photo.versions raises (partial CloudKit record), still emit the still tasks.""" + photo = MagicMock() + photo.filename = "IMG_x.HEIC" + photo.id = "x" + type(photo).versions = property( + lambda self: (_ for _ in ()).throw(RuntimeError("CloudKit broken")) + ) + fake_collect.side_effect = lambda *a, **kw: MagicMock(name="task") + + tasks = _collect_photo_download_tasks( + photo, + destination_path="/tmp/dest", + file_sizes=["original"], + extensions=None, + files=set(), + folder_format=None, + hardlink_registry=None, + ) + + # Original still task was collected; .mov path swallowed the exception + # and did not append anything. + assert len(tasks) == 1 + + @patch("src.album_sync_orchestrator.collect_download_task") + def test_live_video_original_with_none_collect_result_is_skipped( + self, fake_collect + ): + """If collect_download_task returns None for the .mov, it's not appended.""" + photo = _fake_photo( + "IMG_1234.HEIC", + {"original": {"url": "x"}, "live_video_original": {"url": "y"}}, + ) + + def side_effect(*args, **kwargs): + if args[1] == "live_video_original": + return None # e.g. file already exists locally + return MagicMock(name=f"task-{args[1]}") + + fake_collect.side_effect = side_effect + + tasks = _collect_photo_download_tasks( + photo, + destination_path="/tmp/dest", + file_sizes=["original"], + extensions=None, + files=set(), + folder_format=None, + hardlink_registry=None, + ) + + assert len(tasks) == 1 From 639d2278bb0ae5a7d83d4929d94d3e32b204160e Mon Sep 17 00:00:00 2001 From: Eric Pheterson Date: Sat, 30 May 2026 01:04:32 -0700 Subject: [PATCH 2/5] =?UTF-8?q?test:=20ruff-clean=20=E2=80=94=20drop=20unu?= =?UTF-8?q?sed=20tasks=3D=20assignment=20(asserts=20use=20call=5Fargs=5Fli?= =?UTF-8?q?st)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F841: tasks was bound but never read; the test asserts on fake_collect.call_args_list directly. Drop the assignment. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/test_live_photo_pair_download.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/test_live_photo_pair_download.py b/tests/test_live_photo_pair_download.py index 94eb49ce9..b87c615fa 100644 --- a/tests/test_live_photo_pair_download.py +++ b/tests/test_live_photo_pair_download.py @@ -30,7 +30,8 @@ def setUp(self): # Patch is_photo_wanted to always pass; we're testing task collection, # not the wantedness filter. self._wanted_patcher = patch( - "src.album_sync_orchestrator.is_photo_wanted", return_value=True + "src.album_sync_orchestrator.is_photo_wanted", + return_value=True, ) self._wanted_patcher.start() @@ -83,7 +84,8 @@ def test_still_photo_yields_only_one_task(self, fake_collect): @patch("src.album_sync_orchestrator.collect_download_task") def test_live_photo_without_original_request_does_not_append_mov( - self, fake_collect + self, + fake_collect, ): """If the user asked only for medium/thumb (not original), the .mov is not appended. @@ -100,7 +102,7 @@ def test_live_photo_without_original_request_does_not_append_mov( ) fake_collect.side_effect = lambda *a, **kw: MagicMock(name=f"task-{a[1]}") - tasks = _collect_photo_download_tasks( + _collect_photo_download_tasks( photo, destination_path="/tmp/dest", file_sizes=["medium", "thumb"], @@ -121,7 +123,7 @@ def test_versions_access_failure_is_non_fatal(self, fake_collect): photo.filename = "IMG_x.HEIC" photo.id = "x" type(photo).versions = property( - lambda self: (_ for _ in ()).throw(RuntimeError("CloudKit broken")) + lambda self: (_ for _ in ()).throw(RuntimeError("CloudKit broken")), ) fake_collect.side_effect = lambda *a, **kw: MagicMock(name="task") @@ -141,7 +143,8 @@ def test_versions_access_failure_is_non_fatal(self, fake_collect): @patch("src.album_sync_orchestrator.collect_download_task") def test_live_video_original_with_none_collect_result_is_skipped( - self, fake_collect + self, + fake_collect, ): """If collect_download_task returns None for the .mov, it's not appended.""" photo = _fake_photo( From f5c7e5185796db26457b359c1ac596397559efd3 Mon Sep 17 00:00:00 2001 From: Eric Pheterson Date: Sat, 30 May 2026 16:39:36 -0700 Subject: [PATCH 3/5] review: log Live Photo pairing skip at DEBUG + fix MagicMock test pollution Two Copilot review items: 1. **Silent exception swallow.** ``_collect_photo_download_tasks`` was catching ``photo.versions`` read failures with a bare ``except Exception: pass``, making Live Photo pairing failures completely silent. Operators investigating "why is the .mov missing for this photo?" had no signal. Now logs at DEBUG (visible when troubleshooting; no noise at default INFO level). 2. **MagicMock class pollution.** ``test_versions_access_failure_is_non_fatal`` was doing ``type(photo).versions = property(...)`` which monkey-patches the global ``MagicMock`` class and leaks a raising ``versions`` property into every later test that touches ``MagicMock().versions``. Replaced with a dedicated ``_BrokenPhoto`` stub class. Also added ``test_versions_access_failure_logs_debug`` to lock in the observability requirement -- if a future refactor silences the DEBUG log again, this test fails. Verified on python:3.10 docker mirroring CI: ruff clean, 441 passed, 100.00% coverage. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/album_sync_orchestrator.py | 11 ++++- tests/test_live_photo_pair_download.py | 63 ++++++++++++++++++++++---- 2 files changed, 63 insertions(+), 11 deletions(-) diff --git a/src/album_sync_orchestrator.py b/src/album_sync_orchestrator.py index 72a53a223..f53b901a0 100644 --- a/src/album_sync_orchestrator.py +++ b/src/album_sync_orchestrator.py @@ -152,7 +152,16 @@ def _collect_photo_download_tasks( # required. try: live_versions = photo.versions - except Exception: + except Exception as e: + # Treat-as-no-Live-Photo soft path. Logged at DEBUG so + # users investigating "why is the .mov missing for this + # photo?" can spot CloudKit-record-read failures without + # noise at INFO/WARNING for the common case. + photo_id = getattr(photo, "filename", "?") + LOGGER.debug( + f"Live Photo pairing skipped for {photo_id}: " + f"photo.versions read failed: {type(e).__name__}: {e!s}", + ) live_versions = {} if "live_video_original" in live_versions: live_task = collect_download_task( diff --git a/tests/test_live_photo_pair_download.py b/tests/test_live_photo_pair_download.py index b87c615fa..f06a20c4a 100644 --- a/tests/test_live_photo_pair_download.py +++ b/tests/test_live_photo_pair_download.py @@ -118,17 +118,27 @@ def test_live_photo_without_original_request_does_not_append_mov( @patch("src.album_sync_orchestrator.collect_download_task") def test_versions_access_failure_is_non_fatal(self, fake_collect): - """If photo.versions raises (partial CloudKit record), still emit the still tasks.""" - photo = MagicMock() - photo.filename = "IMG_x.HEIC" - photo.id = "x" - type(photo).versions = property( - lambda self: (_ for _ in ()).throw(RuntimeError("CloudKit broken")), - ) + """If photo.versions raises (partial CloudKit record), still emit + the still tasks. The pairing failure is logged at DEBUG (verified + in test_versions_access_failure_logs_debug below) and the still + path proceeds normally.""" + + # Use a dedicated stub class rather than ``type(photo).versions = property(...)`` + # which mutates the global ``MagicMock`` class and pollutes every + # later test that touches ``MagicMock().versions``. + class _BrokenPhoto: + filename = "IMG_x.HEIC" + id = "x" + + @property + def versions(self): + msg = "CloudKit broken" + raise RuntimeError(msg) + fake_collect.side_effect = lambda *a, **kw: MagicMock(name="task") tasks = _collect_photo_download_tasks( - photo, + _BrokenPhoto(), destination_path="/tmp/dest", file_sizes=["original"], extensions=None, @@ -137,10 +147,43 @@ def test_versions_access_failure_is_non_fatal(self, fake_collect): hardlink_registry=None, ) - # Original still task was collected; .mov path swallowed the exception - # and did not append anything. + # Original still task was collected; the broken .versions read + # was caught and the .mov path emitted nothing. assert len(tasks) == 1 + @patch("src.album_sync_orchestrator.collect_download_task") + def test_versions_access_failure_logs_debug(self, fake_collect): + """Operators investigating "why is the .mov missing for this + photo?" should see a DEBUG line. Verifies the swallow-and-skip + path is observable, not silent.""" + import logging + + class _BrokenPhoto: + filename = "IMG_y.HEIC" + id = "y" + + @property + def versions(self): + msg = "CloudKit broken" + raise RuntimeError(msg) + + fake_collect.side_effect = lambda *a, **kw: MagicMock(name="task") + + from src import album_sync_orchestrator + + with self.assertLogs(album_sync_orchestrator.LOGGER, level=logging.DEBUG) as cm: + _collect_photo_download_tasks( + _BrokenPhoto(), + destination_path="/tmp/dest", + file_sizes=["original"], + extensions=None, + files=set(), + folder_format=None, + hardlink_registry=None, + ) + joined = "\n".join(cm.output) + assert "Live Photo pairing skipped for IMG_y.HEIC" in joined, joined + @patch("src.album_sync_orchestrator.collect_download_task") def test_live_video_original_with_none_collect_result_is_skipped( self, From 5e597f9244b1f3eb213cc015105d714d53af0115 Mon Sep 17 00:00:00 2001 From: Eric Pheterson Date: Tue, 2 Jun 2026 22:50:49 -0700 Subject: [PATCH 4/5] feat: Live Photo .mov via explicit file_sizes (live_video_original) Switch from auto-appending the paired .mov (when "original" was requested) to the maintainer-preferred explicit model: add live_video_original (or live_video_medium/thumb) to photos.filters.file_sizes to download it. It flows through the normal download loop; non-Live-Photos lack the version and are skipped quietly (DEBUG, not WARNING, to avoid spam on large libraries). - config_parser: stop filtering live_video_* out of valid file_sizes - album_sync_orchestrator: drop the auto-append special-case - photo_download_manager: log a missing live_video_* version at DEBUG - docs: note live_video_original in README + config.yaml - tests updated for the explicit model Co-Authored-By: Claude Opus 4.8 (1M context) --- README.md | 4 + config.yaml | 4 + src/album_sync_orchestrator.py | 41 +---- src/config_parser.py | 33 ++-- src/photo_download_manager.py | 41 +++-- tests/test_live_photo_pair_download.py | 237 ++++++++----------------- 6 files changed, 133 insertions(+), 227 deletions(-) diff --git a/README.md b/README.md index 0e13cc193..48e287e37 100644 --- a/README.md +++ b/README.md @@ -159,6 +159,10 @@ photos: - "original" # - "medium" # - "thumb" + # For Live Photos, add live_video_original to also download the paired .mov + # (live_video_medium / live_video_thumb for smaller variants). Non-Live + # Photos lack these versions and are skipped. + # - "live_video_original" extensions: # Optional, media extensions to be included in syncing iCloud Photos content # - jpg # - heic diff --git a/config.yaml b/config.yaml index 8a3c0e40d..05e2bdf8e 100644 --- a/config.yaml +++ b/config.yaml @@ -96,3 +96,7 @@ photos: - "original" # - "medium" # - "thumb" + # For Live Photos, add live_video_original to also download the paired .mov + # (live_video_medium / live_video_thumb for smaller variants). Non-Live + # Photos don't have these versions and are skipped. + # - "live_video_original" diff --git a/src/album_sync_orchestrator.py b/src/album_sync_orchestrator.py index f53b901a0..5c4d7ec7f 100644 --- a/src/album_sync_orchestrator.py +++ b/src/album_sync_orchestrator.py @@ -137,43 +137,10 @@ def _collect_photo_download_tasks( ) if download_info: tasks.append(download_info) - - # Live Photo .mov pair — auto-included when the user asked for - # the "original" still and the asset is actually a Live Photo - # (icloudpy exposes the paired .mov under the "live_video_original" - # version key). Mirrors how Apple's Photos.app pairs the two files. - # Requires icloudpy with the Live Photo patch (PHOTO_VERSION_LOOKUP - # contains "live_video_*") — falls through gracefully on older - # icloudpy versions where the key is absent from photo.versions. - if "original" in file_sizes: - # photo.versions can raise on partial CloudKit records — swallow - # so the still tasks still emit. ``getattr`` default doesn't help - # against a property that raises, so an explicit try/except is - # required. - try: - live_versions = photo.versions - except Exception as e: - # Treat-as-no-Live-Photo soft path. Logged at DEBUG so - # users investigating "why is the .mov missing for this - # photo?" can spot CloudKit-record-read failures without - # noise at INFO/WARNING for the common case. - photo_id = getattr(photo, "filename", "?") - LOGGER.debug( - f"Live Photo pairing skipped for {photo_id}: " - f"photo.versions read failed: {type(e).__name__}: {e!s}", - ) - live_versions = {} - if "live_video_original" in live_versions: - live_task = collect_download_task( - photo, - "live_video_original", - destination_path, - files, - folder_format, - hardlink_registry, - ) - if live_task: - tasks.append(live_task) + # Live Photos: add "live_video_original" (or _medium/_thumb) to + # photos.filters.file_sizes to pull the paired .mov. It flows through + # the loop above like any other version; non-Live-Photos don't have + # those versions and are skipped (quietly -- see collect_download_task). return tasks except Exception as e: try: diff --git a/src/config_parser.py b/src/config_parser.py index 5f8de0ae6..6eecebebf 100644 --- a/src/config_parser.py +++ b/src/config_parser.py @@ -210,7 +210,12 @@ def get_drive_sync_interval(config: dict, log_messages: bool = True) -> int: Drive sync interval in seconds """ config_path = ["drive", "sync_interval"] - return get_sync_interval(config=config, config_path=config_path, service_name="drive", log_messages=log_messages) + return get_sync_interval( + config=config, + config_path=config_path, + service_name="drive", + log_messages=log_messages, + ) def get_drive_request_timeout(config: dict) -> int: @@ -241,7 +246,12 @@ def get_photos_sync_interval(config: dict, log_messages: bool = True) -> int: Photos sync interval in seconds """ config_path = ["photos", "sync_interval"] - return get_sync_interval(config=config, config_path=config_path, service_name="photos", log_messages=log_messages) + return get_sync_interval( + config=config, + config_path=config_path, + service_name="photos", + log_messages=log_messages, + ) # ============================================================================= @@ -577,14 +587,11 @@ def get_photos_folder_format(config: dict) -> str | None: def validate_file_sizes(file_sizes: list[str]) -> list[str]: """Validate and filter file sizes against valid options. - The ``live_video_*`` keys are internal — surfaced by the Live Photo - auto-append mechanism in ``_collect_photo_download_tasks`` when - ``"original"`` is in the user's ``file_sizes``. They are NOT intended - for direct user configuration. Filtering them out here means a user who - accidentally adds them to ``file_sizes`` sees a clear "not a valid - option" log line instead of getting silent duplicate download tasks - queued (the explicit ``live_video_original`` task + the auto-append both - targeting the same path). + Accepts any key in ``PhotoAsset.PHOTO_VERSION_LOOKUP``, including the + ``live_video_*`` keys: add ``live_video_original`` to ``file_sizes`` to + download the paired ``.mov`` of a Live Photo (and ``live_video_medium`` / + ``live_video_thumb`` for smaller variants). Photos that aren't Live Photos + simply don't have those versions and are skipped. Args: file_sizes: List of file size strings to validate @@ -592,10 +599,7 @@ def validate_file_sizes(file_sizes: list[str]) -> list[str]: Returns: List of valid file sizes (defaults to ["original"] if all invalid) """ - valid_file_sizes = [ - k for k in PhotoAsset.PHOTO_VERSION_LOOKUP.keys() - if not k.startswith("live_video_") - ] + valid_file_sizes = list(PhotoAsset.PHOTO_VERSION_LOOKUP.keys()) validated_sizes = [] for file_size in file_sizes: @@ -937,6 +941,7 @@ def get_pushover_api_token(config: dict) -> str | None: """ return get_notification_config_value(config, "pushover", "api_token") + def get_pushover_notification_priority(config: dict) -> int | None: """Return Pushover notification priority from config. diff --git a/src/photo_download_manager.py b/src/photo_download_manager.py index e497bc7d7..306e8f4db 100644 --- a/src/photo_download_manager.py +++ b/src/photo_download_manager.py @@ -29,9 +29,14 @@ class DownloadTaskInfo: """Information about a photo download task.""" - def __init__(self, photo, file_size: str, photo_path: str, - hardlink_source: str | None = None, - hardlink_registry: HardlinkRegistry | None = None): + def __init__( + self, + photo, + file_size: str, + photo_path: str, + hardlink_source: str | None = None, + hardlink_registry: HardlinkRegistry | None = None, + ): """Initialize download task info. Args: @@ -60,8 +65,7 @@ def get_max_threads_for_download(config) -> int: return config_parser.get_app_max_threads(config) -def generate_photo_path(photo, file_size: str, destination_path: str, - folder_format: str | None) -> str: +def generate_photo_path(photo, file_size: str, destination_path: str, folder_format: str | None) -> str: """Generate full file path for photo with legacy file renaming. This function combines path generation, folder creation, and legacy @@ -90,7 +94,7 @@ def generate_photo_path(photo, file_size: str, destination_path: str, file_path = os.path.join(destination_path, filename) file_size_path = os.path.join( destination_path, - f"{'__'.join([name, file_size])}" if extension == "" else f"{'__'.join([name, file_size])}.{extension}", + (f"{'__'.join([name, file_size])}" if extension == "" else f"{'__'.join([name, file_size])}.{extension}"), ) # Final path with normalization @@ -108,9 +112,14 @@ def generate_photo_path(photo, file_size: str, destination_path: str, return normalized_path -def collect_download_task(photo, file_size: str, destination_path: str, - files: set[str] | None, folder_format: str | None, - hardlink_registry: HardlinkRegistry | None) -> DownloadTaskInfo | None: +def collect_download_task( + photo, + file_size: str, + destination_path: str, + files: set[str] | None, + folder_format: str | None, + hardlink_registry: HardlinkRegistry | None, +) -> DownloadTaskInfo | None: """Collect photo info for parallel download without immediately downloading. Args: @@ -127,7 +136,14 @@ def collect_download_task(photo, file_size: str, destination_path: str, # Check if file size exists on server if file_size not in photo.versions: photo_path = generate_photo_path(photo, file_size, destination_path, folder_format) - LOGGER.warning(f"File size {file_size} not found on server. Skipping the photo {photo_path} ...") + # A missing live_video_* version just means this isn't a Live Photo -- + # expected for most assets, so log at DEBUG to avoid warning-spam when + # live_video_original is in file_sizes. Other sizes warn as before. + msg = f"File size {file_size} not found on server. Skipping the photo {photo_path} ..." + if file_size.startswith("live_video_"): + LOGGER.debug(msg) + else: + LOGGER.warning(msg) return None # Generate photo path @@ -140,6 +156,7 @@ def collect_download_task(photo, file_size: str, destination_path: str, # Check if photo already exists with correct size from src.photo_file_utils import check_photo_exists + if check_photo_exists(photo, file_size, photo_path): return None @@ -183,7 +200,9 @@ def execute_download_task(task_info: DownloadTaskInfo) -> bool: if result and task_info.hardlink_registry is not None: # Register for future hard links if enabled task_info.hardlink_registry.register_photo_path( - task_info.photo.id, task_info.file_size, task_info.photo_path, + task_info.photo.id, + task_info.file_size, + task_info.photo_path, ) LOGGER.debug(f"[Thread] Completed download of {task_info.photo_path}") diff --git a/tests/test_live_photo_pair_download.py b/tests/test_live_photo_pair_download.py index f06a20c4a..523542c01 100644 --- a/tests/test_live_photo_pair_download.py +++ b/tests/test_live_photo_pair_download.py @@ -1,16 +1,16 @@ -"""Live Photo .mov pair auto-append tests. +"""Live Photo .mov via explicit ``file_sizes``. -Added 2026-05-27 as part of feat/per-library-destinations-and-live-photos. - -When a photo is a Live Photo (icloudpy exposes ``live_video_original`` in -``photo.versions``) and the user has requested the ``original`` file_size, -the orchestrator should automatically collect a second download task for -the paired ``.mov`` so the Live Photo round-trips intact. +Users add ``live_video_original`` (or ``live_video_medium`` / ``live_video_thumb``) +to ``photos.filters.file_sizes`` to download the paired ``.mov`` of a Live Photo. +It flows through the normal download loop like any other version; photos that +aren't Live Photos lack the version and are skipped quietly (DEBUG, not WARNING). """ +import logging import unittest from unittest.mock import MagicMock, patch +from src import config_parser, photo_download_manager from src.album_sync_orchestrator import _collect_photo_download_tasks @@ -23,193 +23,100 @@ def _fake_photo(filename, versions): return photo -class TestLivePhotoPairDownload(unittest.TestCase): - """Live Photo .mov auto-append behaviour.""" +class TestLiveVideoFileSize(unittest.TestCase): + """``live_video_*`` is a valid, explicit file_size.""" - def setUp(self): - # Patch is_photo_wanted to always pass; we're testing task collection, - # not the wantedness filter. - self._wanted_patcher = patch( - "src.album_sync_orchestrator.is_photo_wanted", - return_value=True, + def test_validate_accepts_live_video_keys(self): + validated = config_parser.validate_file_sizes( + ["original", "live_video_original", "live_video_medium"], ) - self._wanted_patcher.start() - - def tearDown(self): - self._wanted_patcher.stop() + self.assertIn("live_video_original", validated) + self.assertIn("live_video_medium", validated) + @patch("src.album_sync_orchestrator.is_photo_wanted", return_value=True) @patch("src.album_sync_orchestrator.collect_download_task") - def test_live_photo_with_original_yields_two_tasks(self, fake_collect): + def test_live_video_original_collected_for_live_photo(self, fake_collect, _wanted): + """A Live Photo with live_video_original requested yields both tasks.""" photo = _fake_photo( "IMG_1234.HEIC", {"original": {"url": "x"}, "live_video_original": {"url": "y"}}, ) - # Each call returns a unique MagicMock — non-None so tasks list grows - fake_collect.side_effect = lambda *a, **kw: MagicMock(name=f"task-{a[1]}") - - tasks = _collect_photo_download_tasks( - photo, - destination_path="/tmp/dest", - file_sizes=["original"], - extensions=None, - files=set(), - folder_format=None, - hardlink_registry=None, - ) - - assert len(tasks) == 2 - # Inspect which file_sizes got requested - called_sizes = [call.args[1] for call in fake_collect.call_args_list] - assert "original" in called_sizes - assert "live_video_original" in called_sizes - - @patch("src.album_sync_orchestrator.collect_download_task") - def test_still_photo_yields_only_one_task(self, fake_collect): - photo = _fake_photo("IMG_5678.HEIC", {"original": {"url": "x"}}) fake_collect.side_effect = lambda *a, **kw: MagicMock(name=f"task-{a[1]}") - tasks = _collect_photo_download_tasks( photo, destination_path="/tmp/dest", - file_sizes=["original"], - extensions=None, - files=set(), - folder_format=None, - hardlink_registry=None, - ) - - assert len(tasks) == 1 - called_sizes = [call.args[1] for call in fake_collect.call_args_list] - assert called_sizes == ["original"] - - @patch("src.album_sync_orchestrator.collect_download_task") - def test_live_photo_without_original_request_does_not_append_mov( - self, - fake_collect, - ): - """If the user asked only for medium/thumb (not original), the .mov is not appended. - - The Live Photo .mov is the *original* video resource; users who explicitly - want only smaller variants shouldn't get the original .mov bundled. - """ - photo = _fake_photo( - "IMG_1234.HEIC", - { - "medium": {"url": "m"}, - "thumb": {"url": "t"}, - "live_video_original": {"url": "y"}, - }, - ) - fake_collect.side_effect = lambda *a, **kw: MagicMock(name=f"task-{a[1]}") - - _collect_photo_download_tasks( - photo, - destination_path="/tmp/dest", - file_sizes=["medium", "thumb"], - extensions=None, - files=set(), - folder_format=None, - hardlink_registry=None, - ) - - called_sizes = [call.args[1] for call in fake_collect.call_args_list] - assert called_sizes == ["medium", "thumb"] - assert "live_video_original" not in called_sizes - - @patch("src.album_sync_orchestrator.collect_download_task") - def test_versions_access_failure_is_non_fatal(self, fake_collect): - """If photo.versions raises (partial CloudKit record), still emit - the still tasks. The pairing failure is logged at DEBUG (verified - in test_versions_access_failure_logs_debug below) and the still - path proceeds normally.""" - - # Use a dedicated stub class rather than ``type(photo).versions = property(...)`` - # which mutates the global ``MagicMock`` class and pollutes every - # later test that touches ``MagicMock().versions``. - class _BrokenPhoto: - filename = "IMG_x.HEIC" - id = "x" - - @property - def versions(self): - msg = "CloudKit broken" - raise RuntimeError(msg) - - fake_collect.side_effect = lambda *a, **kw: MagicMock(name="task") - - tasks = _collect_photo_download_tasks( - _BrokenPhoto(), - destination_path="/tmp/dest", - file_sizes=["original"], + file_sizes=["original", "live_video_original"], extensions=None, files=set(), folder_format=None, hardlink_registry=None, ) + self.assertEqual(len(tasks), 2) + called = [c.args[1] for c in fake_collect.call_args_list] + self.assertIn("live_video_original", called) - # Original still task was collected; the broken .versions read - # was caught and the .mov path emitted nothing. - assert len(tasks) == 1 - + @patch("src.album_sync_orchestrator.is_photo_wanted", return_value=True) @patch("src.album_sync_orchestrator.collect_download_task") - def test_versions_access_failure_logs_debug(self, fake_collect): - """Operators investigating "why is the .mov missing for this - photo?" should see a DEBUG line. Verifies the swallow-and-skip - path is observable, not silent.""" - import logging - - class _BrokenPhoto: - filename = "IMG_y.HEIC" - id = "y" - - @property - def versions(self): - msg = "CloudKit broken" - raise RuntimeError(msg) - - fake_collect.side_effect = lambda *a, **kw: MagicMock(name="task") - - from src import album_sync_orchestrator - - with self.assertLogs(album_sync_orchestrator.LOGGER, level=logging.DEBUG) as cm: - _collect_photo_download_tasks( - _BrokenPhoto(), - destination_path="/tmp/dest", - file_sizes=["original"], - extensions=None, - files=set(), - folder_format=None, - hardlink_registry=None, - ) - joined = "\n".join(cm.output) - assert "Live Photo pairing skipped for IMG_y.HEIC" in joined, joined - - @patch("src.album_sync_orchestrator.collect_download_task") - def test_live_video_original_with_none_collect_result_is_skipped( - self, - fake_collect, - ): - """If collect_download_task returns None for the .mov, it's not appended.""" - photo = _fake_photo( - "IMG_1234.HEIC", - {"original": {"url": "x"}, "live_video_original": {"url": "y"}}, - ) + def test_still_photo_skips_absent_live_video(self, fake_collect, _wanted): + """A still (no live_video_original) yields no .mov task; the still still emits.""" def side_effect(*args, **kwargs): - if args[1] == "live_video_original": - return None # e.g. file already exists locally - return MagicMock(name=f"task-{args[1]}") + return None if args[1] == "live_video_original" else MagicMock(name=f"task-{args[1]}") fake_collect.side_effect = side_effect - + photo = _fake_photo("IMG_5678.HEIC", {"original": {"url": "x"}}) tasks = _collect_photo_download_tasks( photo, destination_path="/tmp/dest", - file_sizes=["original"], + file_sizes=["original", "live_video_original"], extensions=None, files=set(), folder_format=None, hardlink_registry=None, ) + self.assertEqual(len(tasks), 1) + + +class TestCollectDownloadTaskMissingVersion(unittest.TestCase): + """A version absent from photo.versions is skipped; live_video_* logs DEBUG.""" + + @patch( + "src.photo_download_manager.generate_photo_path", + return_value="/tmp/dest/IMG.HEIC", + ) + def test_missing_live_video_logs_debug_not_warning(self, _gp): + photo = _fake_photo("IMG.HEIC", {"original": {"url": "x"}}) + with self.assertLogs(photo_download_manager.LOGGER, level=logging.DEBUG) as cm: + result = photo_download_manager.collect_download_task( + photo, + "live_video_original", + "/tmp/dest", + set(), + None, + None, + ) + self.assertIsNone(result) + self.assertTrue(any("live_video_original" in line for line in cm.output)) + self.assertFalse(any("WARNING" in line for line in cm.output)) + + @patch( + "src.photo_download_manager.generate_photo_path", + return_value="/tmp/dest/IMG.HEIC", + ) + def test_missing_regular_size_logs_warning(self, _gp): + photo = _fake_photo("IMG.HEIC", {"original": {"url": "x"}}) + with self.assertLogs(photo_download_manager.LOGGER, level=logging.WARNING) as cm: + result = photo_download_manager.collect_download_task( + photo, + "medium", + "/tmp/dest", + set(), + None, + None, + ) + self.assertIsNone(result) + self.assertTrue(any("WARNING" in line for line in cm.output)) + - assert len(tasks) == 1 +if __name__ == "__main__": + unittest.main() From 07f5c9f2549b3b0c5d59fa12421bd06613292414 Mon Sep 17 00:00:00 2001 From: Eric Pheterson Date: Sun, 12 Jul 2026 00:27:15 -0700 Subject: [PATCH 5/5] fix: write the Live Photo paired video with its real extension (.MOV/.MP4) The live_video_* versions are QuickTime/MP4 movies, but the filename builder only remapped the extension for original_alt, so the paired video inherited the still's extension (e.g. IMG_1234__live_video_original__.HEIC) -- a video wearing a .HEIC extension, which every image tool rejects. Map the live_video_* versions to their real container extension via the iCloud UTI (defaulting to MOV). generate_photo_path also renames an existing mislabeled '...__live_video_*__.HEIC' in place to the corrected path, so upgrading doesn't re-download every paired video or leave the old file behind. Co-Authored-By: Claude Opus 4.8 --- src/photo_download_manager.py | 13 +++++ src/photo_path_utils.py | 30 ++++++++++ tests/test_live_photo_extension.py | 89 ++++++++++++++++++++++++++++++ 3 files changed, 132 insertions(+) create mode 100644 tests/test_live_photo_extension.py diff --git a/src/photo_download_manager.py b/src/photo_download_manager.py index 306e8f4db..87d39c3b1 100644 --- a/src/photo_download_manager.py +++ b/src/photo_download_manager.py @@ -14,6 +14,7 @@ from src.hardlink_registry import HardlinkRegistry from src.photo_file_utils import create_hardlink, download_photo_from_server from src.photo_path_utils import ( + _LIVE_VIDEO_SIZES, create_folder_path_if_needed, generate_photo_filename_with_metadata, normalize_file_path, @@ -105,6 +106,18 @@ def generate_photo_path(photo, file_size: str, destination_path: str, folder_for rename_legacy_file_if_exists(file_path, normalized_path) rename_legacy_file_if_exists(file_size_path, normalized_path) + # Self-heal the earlier .HEIC mislabeling of Live Photo videos: an older + # version wrote the paired video with the still's extension. Rename that + # file to the corrected path instead of re-downloading it (which would also + # leave the broken duplicate behind). + if file_size in _LIVE_VIDEO_SIZES and extension: + root, _ = os.path.splitext(filename_with_metadata) + legacy_mislabeled = normalize_file_path( + os.path.join(final_destination, f"{root}.{extension}"), + ) + if legacy_mislabeled != normalized_path: + rename_legacy_file_if_exists(legacy_mislabeled, normalized_path) + # Handle existing file with different normalization if os.path.isfile(final_file_path) and final_file_path != normalized_path: rename_legacy_file_if_exists(final_file_path, normalized_path) diff --git a/src/photo_path_utils.py b/src/photo_path_utils.py index dff5bb9c2..3fcfcbff2 100644 --- a/src/photo_path_utils.py +++ b/src/photo_path_utils.py @@ -24,6 +24,10 @@ LOGGER = get_logger() +# The Live Photo paired-video file_size variants. These are QuickTime movies, +# not images, even though the parent asset's filename ends in .HEIC/.JPG. +_LIVE_VIDEO_SIZES = frozenset({"live_video_original", "live_video_medium", "live_video_thumb"}) + def get_photo_name_and_extension(photo, file_size: str) -> tuple[str, str]: """Extract filename and extension from photo. @@ -49,6 +53,16 @@ def get_photo_name_and_extension(photo, file_size: str) -> tuple[str, str]: else: LOGGER.warning(f"Unknown filetype {filetype} for original_alt version of {filename}") + # Handle Live Photo paired-video versions. photo.filename is the STILL + # (e.g. IMG_1234.HEIC), but the live_video_* versions are the QuickTime + # movie half of the Live Photo. Without this the .mov is written with the + # still's extension (IMG_1234__live_video_original__.HEIC), which every + # downstream image tool then rejects as "unsupported image format" because + # it is really a video. Map to the real container extension instead. + elif file_size in _LIVE_VIDEO_SIZES and file_size in photo.versions: + filetype = photo.versions[file_size].get("type") + extension = _get_video_filetype_mapping().get(filetype, "MOV") + return name, extension @@ -116,6 +130,22 @@ def rename_legacy_file_if_exists(old_path: str, new_path: str) -> None: os.rename(old_path, new_path) +def _get_video_filetype_mapping() -> dict: + """Get mapping of Live Photo paired-video Apple UTI types to extensions. + + Live Photo videos are QuickTime movies; iCloud reports the UTI in the + version's ``type`` field. Anything not listed falls back to ``MOV`` (the + only container Apple has ever used for the Live Photo motion component). + + Returns: + Dictionary mapping Apple UTI type strings to file extensions + """ + return { + "com.apple.quicktime-movie": "MOV", + "public.mpeg-4": "MP4", + } + + def _get_original_alt_filetype_mapping() -> dict: """Get mapping of original_alt file types to extensions. diff --git a/tests/test_live_photo_extension.py b/tests/test_live_photo_extension.py new file mode 100644 index 000000000..1e78ac275 --- /dev/null +++ b/tests/test_live_photo_extension.py @@ -0,0 +1,89 @@ +"""Tests for the Live Photo paired-video extension. + +The live_video_* versions are QuickTime/MP4 movies; they must be written with +the real container extension (not the still's .HEIC), and an existing .HEIC +video from an earlier build must be renamed in place rather than re-downloaded. +""" + +import os +import types + +from src.photo_download_manager import generate_photo_path +from src.photo_path_utils import ( + generate_photo_filename_with_metadata, + get_photo_name_and_extension, +) + +_FTYP_QUICKTIME = b"\x00\x00\x00\x18ftypqt \x00\x00\x02\x00qt " + + +def _photo(filename, versions): + """Minimal stand-in for an iCloudPy PhotoAsset.""" + return types.SimpleNamespace(filename=filename, versions=versions, id="ASSET-ID-1") + + +class TestLivePhotoExtension: + def test_live_video_original_maps_to_mov(self): + photo = _photo( + "IMG_1234.HEIC", + { + "original": {"type": "public.heic"}, + "live_video_original": {"type": "com.apple.quicktime-movie"}, + }, + ) + name, ext = get_photo_name_and_extension(photo, "live_video_original") + assert name == "IMG_1234" + assert ext == "MOV" + + def test_live_video_unknown_type_defaults_to_mov(self): + photo = _photo("IMG_1.HEIC", {"live_video_original": {"type": None}}) + assert get_photo_name_and_extension(photo, "live_video_original")[1] == "MOV" + + def test_live_video_mp4_type_maps_to_mp4(self): + photo = _photo("IMG_1.HEIC", {"live_video_original": {"type": "public.mpeg-4"}}) + assert get_photo_name_and_extension(photo, "live_video_original")[1] == "MP4" + + def test_live_video_medium_and_thumb_are_mov(self): + photo = _photo( + "IMG_2.JPG", + { + "live_video_medium": {"type": "com.apple.quicktime-movie"}, + "live_video_thumb": {"type": "com.apple.quicktime-movie"}, + }, + ) + assert get_photo_name_and_extension(photo, "live_video_medium")[1] == "MOV" + assert get_photo_name_and_extension(photo, "live_video_thumb")[1] == "MOV" + + def test_still_original_keeps_its_extension(self): + photo = _photo("IMG_1234.HEIC", {"original": {"type": "public.heic"}}) + assert get_photo_name_and_extension(photo, "original")[1] == "HEIC" + + def test_filename_with_metadata_ends_in_mov(self): + photo = _photo("IMG_9.HEIC", {"live_video_original": {"type": "com.apple.quicktime-movie"}}) + filename = generate_photo_filename_with_metadata(photo, "live_video_original") + assert filename.endswith(".MOV") + assert "__live_video_original__" in filename + + +class TestDownloadSelfHeal: + def test_generate_photo_path_renames_legacy_heic_video(self, tmp_path): + # An existing IMG__live_video_original__.HEIC (from an earlier build) + # must be renamed in place to the corrected .MOV, not left for re-download. + photo = _photo("IMG_1.HEIC", {"live_video_original": {"type": "com.apple.quicktime-movie"}}) + corrected = generate_photo_filename_with_metadata(photo, "live_video_original") + assert corrected.endswith(".MOV") + legacy = os.path.join(str(tmp_path), corrected[: -len(".MOV")] + ".HEIC") + with open(legacy, "wb") as handle: + handle.write(_FTYP_QUICKTIME) + + result = generate_photo_path(photo, "live_video_original", str(tmp_path), None) + + assert result == os.path.join(str(tmp_path), corrected) + assert os.path.exists(result) + assert not os.path.exists(legacy) + + def test_generate_photo_path_no_legacy_is_noop(self, tmp_path): + photo = _photo("IMG_2.HEIC", {"live_video_original": {"type": "com.apple.quicktime-movie"}}) + result = generate_photo_path(photo, "live_video_original", str(tmp_path), None) + assert result.endswith(".MOV") + assert not os.path.exists(result)