Fix for #516

osxphotos/_version.py

@@ -1,3 +1,3 @@
 """ version info """
 
-__version__ = "0.42.83"
+__version__ = "0.42.84"

osxphotos/photoinfo/_photoinfo_export.py

@@ -56,7 +56,7 @@ from ..photokit import (
 )
 from ..phototemplate import RenderOptions
 from ..uti import get_preferred_uti_extension
-from ..utils import findfiles, lineno, noop, normalize_fs_path
+from ..utils import increment_filename, increment_filename_with_count, lineno
 
 # retry if use_photos_export fails the first time (which sometimes it does)
 MAX_PHOTOSCRIPT_RETRIES = 3
@@ -683,18 +683,12 @@ def export2(
     # e.g. exporting sidecar for file1.png and file1.jpeg
     # if file1.png exists and exporting file1.jpeg,
     # dest will be file1 (1).jpeg even though file1.jpeg doesn't exist to prevent sidecar collision
-    count = 0
+    increment_file_count = 0
-    if not update and increment and not overwrite:
+    if increment and not update and not overwrite:
-        dest_files = findfiles(f"{dest_original.stem}*", str(dest_original.parent))
+        dest_original, increment_file_count = increment_filename_with_count(
-        # paths need to be normalized for unicode as filesystem returns unicode in NFD form
+            dest_original
-        dest_files = [
+        )
-            normalize_fs_path(pathlib.Path(f).stem.lower()) for f in dest_files
+        dest_original = pathlib.Path(dest_original)
-        ]
-        dest_new = dest_original.stem
-        while normalize_fs_path(dest_new.lower()) in dest_files:
-            count += 1
-            dest_new = f"{dest_original.stem} ({count})"
-        dest_original = dest_original.parent / f"{dest_new}{dest_original.suffix}"
 
     # if overwrite==False and #increment==False, export should fail if file exists
     if (
@@ -709,20 +703,11 @@ def export2(
         )
 
     if export_edited:
-        if not update and increment and not overwrite:
+        if increment and not update and not overwrite:
-            dest_files = findfiles(f"{dest_edited.stem}*", str(dest_edited.parent))
+            dest_edited, increment_file_count = increment_filename_with_count(
-            # paths need to be normalized for unicode as filesystem returns unicode in NFD form
+                dest_edited, increment_file_count
-            dest_files = [
+            )
-                normalize_fs_path(pathlib.Path(f).stem.lower()) for f in dest_files
+            dest_edited = pathlib.Path(dest_edited)
-            ]
-            dest_new = dest_edited.stem
-            if count:
-                # incremented above when checking original destination
-                dest_new = f"{dest_new} ({count})"
-            while normalize_fs_path(dest_new.lower()) in dest_files:
-                count += 1
-                dest_new = f"{dest.stem} ({count})"
-            dest_edited = dest_edited.parent / f"{dest_new}{dest_edited.suffix}"
 
         # if overwrite==False and #increment==False, export should fail if file exists
         if dest_edited.exists() and not update and not overwrite and not increment:
@@ -806,20 +791,16 @@ def export2(
                     )
                 if dest_uuid != self.uuid:
                     # not the right file, find the right one
-                    count = 1
                     glob_str = str(dest.parent / f"{dest.stem} (*{dest.suffix}")
                     dest_files = glob.glob(glob_str)
-                    found_match = False
                     for file_ in dest_files:
                         dest_uuid = export_db.get_uuid_for_file(file_)
                         if dest_uuid == self.uuid:
                             dest = pathlib.Path(file_)
-                            found_match = True
                             break
                         elif dest_uuid is None and fileutil.cmp(src, file_):
                             # files match, update the UUID
                             dest = pathlib.Path(file_)
-                            found_match = True
                             export_db.set_data(
                                 filename=dest,
                                 uuid=self.uuid,
@@ -831,18 +812,9 @@ def export2(
                                 exif_json=None,
                             )
                             break
-
+                    else:
-                    if not found_match:
                         # increment the destination file
-                        count = 1
+                        dest = pathlib.Path(increment_filename(dest))
-                        glob_str = str(dest.parent / f"{dest.stem}*")
-                        dest_files = glob.glob(glob_str)
-                        dest_files = [normalize_fs_path(pathlib.Path(f).stem) for f in dest_files]
-                        dest_new = dest.stem
-                        while normalize_fs_path(dest_new) in dest_files:
-                            dest_new = f"{dest.stem} ({count})"
-                            count += 1
-                        dest = dest.parent / f"{dest_new}{dest.suffix}"
 
             if export_original:
                 dest_original = dest
@@ -940,6 +912,7 @@ def export2(
             preview_path = pathlib.Path(self.path_derivatives[0])
             preview_ext = preview_path.suffix
             preview_name = dest.parent / f"{dest.stem}{preview_suffix}{preview_ext}"
+            preview_name = pathlib.Path(increment_filename(preview_name))
             if preview_path is not None:
                 results = self._export_photo(
                     preview_path,

osxphotos/utils.py

@@ -16,7 +16,7 @@ import sys
 import unicodedata
 import urllib.parse
 from plistlib import load as plistload
-from typing import Callable
+from typing import Callable, Union
 
 import CoreFoundation
 import objc
@@ -269,7 +269,7 @@ def normalize_fs_path(path: str) -> str:
     """Normalize filesystem paths with unicode in them"""
     with objc.autorelease_pool():
         normalized_path = NSString.fileSystemRepresentation(path)
-        return normalized_path.decode('utf8')
+        return normalized_path.decode("utf8")
 
 
 def findfiles(pattern, path_):
@@ -365,30 +365,50 @@ def normalize_unicode(value):
         return None
 
 
-def increment_filename(filepath):
+def increment_filename_with_count(filepath: Union[str,pathlib.Path], count: int = 0) -> str:
     """Return filename (1).ext, etc if filename.ext exists
 
         If file exists in filename's parent folder with same stem as filename,
         add (1), (2), etc. until a non-existing filename is found.
 
     Args:
-        filepath: str; full path, including file name
+        filepath: str or pathlib.Path; full path, including file name
+        count: int; starting increment value
+
+    Returns:
+        tuple of new filepath (or same if not incremented), count
+
+    Note: This obviously is subject to race condition so using with caution.
+    """
+    dest = filepath if isinstance(filepath, pathlib.Path) else pathlib.Path(filepath)
+    dest_files = findfiles(f"{dest.stem}*", str(dest.parent))
+    dest_files = [normalize_fs_path(pathlib.Path(f).stem.lower()) for f in dest_files]
+    dest_new = dest.stem
+    if count:
+        dest_new = f"{dest.stem} ({count})"
+    while normalize_fs_path(dest_new.lower()) in dest_files:
+        count += 1
+        dest_new = f"{dest.stem} ({count})"
+    dest = dest.parent / f"{dest_new}{dest.suffix}"
+    return str(dest), count
+
+
+def increment_filename(filepath: Union[str, pathlib.Path]) -> str:
+    """Return filename (1).ext, etc if filename.ext exists
+
+        If file exists in filename's parent folder with same stem as filename,
+        add (1), (2), etc. until a non-existing filename is found.
+
+    Args:
+        filepath: str or pathlib.Path; full path, including file name
 
     Returns:
         new filepath (or same if not incremented)
 
     Note: This obviously is subject to race condition so using with caution.
     """
-    dest = pathlib.Path(str(filepath))
+    new_filepath, _ = increment_filename_with_count(filepath)
-    count = 1
+    return new_filepath
-    dest_files = findfiles(f"{dest.stem}*", str(dest.parent))
-    dest_files = [pathlib.Path(f).stem.lower() for f in dest_files]
-    dest_new = dest.stem
-    while dest_new.lower() in dest_files:
-        dest_new = f"{dest.stem} ({count})"
-        count += 1
-    dest = dest.parent / f"{dest_new}{dest.suffix}"
-    return str(dest)
 
 
 def expand_and_validate_filepath(path: str) -> str:

tests/test_cli.py

@@ -479,6 +479,7 @@ CLI_EXPORT_UUID = "D79B8D77-BFFC-460B-9312-034F2877D35B"
 CLI_EXPORT_UUID_STATUE = "3DD2C897-F19E-4CA6-8C22-B027D5A71907"
 CLI_EXPORT_UUID_KEYWORD_PATHSEP = "7783E8E6-9CAC-40F3-BE22-81FB7051C266"
 CLI_EXPORT_UUID_LONG_DESCRIPTION = "8846E3E6-8AC8-4857-8448-E3D025784410"
+CLI_EXPORT_UUID_MISSING = "8E1D7BC9-9321-44F9-8CFB-4083F6B9232A"  # IMG_2000.JPG
 
 CLI_EXPORT_UUID_FILENAME = "Pumkins2.jpg"
 CLI_EXPORT_UUID_FILENAME_PREVIEW = "Pumkins2_preview.jpeg"
@@ -1375,6 +1376,48 @@ def test_export_preview():
         assert CLI_EXPORT_UUID_FILENAME_PREVIEW in files
 
 
+def test_export_preview_file_exists():
+    """test export with --preview when preview images already exist, issue #516"""
+    import glob
+    import os
+    import os.path
+
+    import osxphotos
+    from osxphotos.cli import export
+
+    runner = CliRunner()
+    cwd = os.getcwd()
+    # pylint: disable=not-context-manager
+    with runner.isolated_filesystem():
+        result = runner.invoke(
+            export,
+            [
+                os.path.join(cwd, CLI_PHOTOS_DB),
+                ".",
+                "-V",
+                "--preview",
+                "--uuid",
+                CLI_EXPORT_UUID_MISSING,
+            ],
+        )
+        assert result.exit_code == 0
+
+        # export again
+        result = runner.invoke(
+            export,
+            [
+                os.path.join(cwd, CLI_PHOTOS_DB),
+                ".",
+                "-V",
+                "--preview",
+                "--uuid",
+                CLI_EXPORT_UUID_MISSING,
+            ],
+        )
+        assert result.exit_code == 0
+        assert "Error exporting photo" not in result.output
+
+
 def test_export_preview_suffix():
     """test export with --preview and --preview-suffix"""
     import glob
@@ -4993,7 +5036,10 @@ def test_export_touch_files_update():
             ],
         )
         assert result.exit_code == 0
-        assert f"updated: 1, skipped: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7-1}" in result.output
+        assert (
+            f"updated: 1, skipped: {PHOTOS_NOT_IN_TRASH_LEN_15_7+PHOTOS_EDITED_15_7-1}"
+            in result.output
+        )
         assert "touched date: 1" in result.output
 
         for fname, mtime in zip(CLI_EXPORT_BY_DATE, CLI_EXPORT_BY_DATE_TOUCH_TIMES):

tests/test_utils.py

@@ -8,18 +8,20 @@ UTI_DICT = {"public.jpeg": "jpeg", "com.canon.cr2-raw-image": "cr2"}
 
 
 def test_debug_enable():
-    import osxphotos
     import logging
 
+    import osxphotos
+
     osxphotos._set_debug(True)
     logger = osxphotos._get_logger()
     assert logger.isEnabledFor(logging.DEBUG)
 
 
 def test_debug_disable():
-    import osxphotos
     import logging
 
+    import osxphotos
+
     osxphotos._set_debug(False)
     logger = osxphotos._get_logger()
     assert not logger.isEnabledFor(logging.DEBUG)
@@ -31,6 +33,7 @@ def test_dd_to_dms():
 
     assert _dd_to_dms(-0.001) == (0, 0, -3.6)
 
+
 @pytest.mark.skip(reason="Fails on some machines")
 def test_get_system_library_path():
     import osxphotos
@@ -54,9 +57,11 @@ def test_db_is_locked_unlocked():
 
     assert not osxphotos.utils._db_is_locked(DB_UNLOCKED_10_15)
 
+
 def test_findfiles():
-    import tempfile
     import os.path
+    import tempfile
+
     from osxphotos.utils import findfiles
 
     temp_dir = tempfile.TemporaryDirectory(prefix="osxphotos_")
@@ -72,9 +77,48 @@ def test_findfiles():
 
 def test_findfiles_invalid_dir():
     import tempfile
-    import os.path
+
     from osxphotos.utils import findfiles
 
     temp_dir = tempfile.TemporaryDirectory(prefix="osxphotos_")
-    files = findfiles("*.jpg", f"{temp_dir.name}/no_such_dir" )
+    files = findfiles("*.jpg", f"{temp_dir.name}/no_such_dir")
     assert len(files) == 0
+
+
+def test_increment_filename():
+    # test that increment_filename works
+    import pathlib
+    import tempfile
+
+    from osxphotos.utils import increment_filename, increment_filename_with_count
+
+    with tempfile.TemporaryDirectory(prefix="osxphotos_") as temp_dir:
+        temp_dir = pathlib.Path(temp_dir)
+        filename = str(temp_dir / "file.jpg")
+        assert increment_filename(filename) == str(temp_dir / "file.jpg")
+
+        new_file = temp_dir / "file.jpg"
+        new_file.touch()
+        assert increment_filename(filename) == str(temp_dir / "file (1).jpg")
+
+        # test pathlib.Path as argument
+        assert increment_filename(pathlib.Path(filename)) == str(
+            temp_dir / "file (1).jpg"
+        )
+
+        new_file = temp_dir / "file (1).jpg"
+        new_file.touch()
+        assert increment_filename(filename) == str(temp_dir / "file (2).jpg")
+
+        # test increment_filename_with_count
+        filename = str(temp_dir / "file2.jpg")
+        assert increment_filename_with_count(filename, count=2) == (
+            str(temp_dir / "file2 (2).jpg"),
+            2,
+        )
+        new_file = temp_dir / "file2 (2).jpg"
+        new_file.touch()
+        assert increment_filename_with_count(filename, count=2) == (
+            str(temp_dir / "file2 (3).jpg"),
+            3,
+        )