Fix for exporting slow mo videos, issue #252

CHANGELOG.md

@@ -4,6 +4,25 @@ All notable changes to this project will be documented in this file. Dates are d
 
 Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog).
 
+#### [v0.36.8](https://github.com/RhetTbull/osxphotos/compare/v0.36.7...v0.36.8)
+
+> 5 November 2020
+
+- Refactored exiftool.py [`2202f1b`](https://github.com/RhetTbull/osxphotos/commit/2202f1b1e9c4f83558ef48e58cb94af6b3a38cdd)
+- README.md update [`a509ef1`](https://github.com/RhetTbull/osxphotos/commit/a509ef18d3db2ac15a661e763a7254974cf8d84a)
+
+#### [v0.36.7](https://github.com/RhetTbull/osxphotos/compare/v0.36.6...v0.36.7)
+
+> 4 November 2020
+
+- Implemented context manager for ExifTool, closes #250 [`#250`](https://github.com/RhetTbull/osxphotos/issues/250)
+
+#### [v0.36.6](https://github.com/RhetTbull/osxphotos/compare/v0.36.5...v0.36.6)
+
+> 2 November 2020
+
+- Fix for issue #39 [`c7c5320`](https://github.com/RhetTbull/osxphotos/commit/c7c5320587e31070b55cc8c7e74f30b0f9e61379)
+
 #### [v0.36.5](https://github.com/RhetTbull/osxphotos/compare/v0.36.4...v0.36.5)
 
 > 1 November 2020

README.md

@@ -1269,6 +1269,9 @@ Returns True if photo is a panorama, otherwise False.
 
 **Note**: The result of `PhotoInfo.panorama` will differ from the "Panoramas" Media Types smart album in that it will also identify panorama photos from older phones that Photos does not recognize as panoramas. 
 
+#### `slow_mo`
+Returns True if photo is a slow motion video, otherwise False
+
 #### `labels`
 Returns image categorization labels associated with the photo as list of str.
 
@@ -1859,6 +1862,7 @@ The following substitutions are availabe for use with `PhotoInfo.render_template
 |{modified.month}|Month name in user's locale of the file modification time|
 |{modified.mon}|Month abbreviation in the user's locale of the file modification time|
 |{modified.dd}|2-digit day of the month (zero padded) of the file modification time|
+|{modified.dow}|Day of week in user's locale of the photo modification time|
 |{modified.doy}|3-digit day of year (e.g Julian day) of file modification time, starting from 1 (zero padded)|
 |{modified.hour}|2-digit hour of the file modification time|
 |{modified.min}|2-digit minute of the file modification time|

osxphotos/__main__.py

@@ -208,7 +208,11 @@ class ExportCommand(click.Command):
             + "has no value, '_' (underscore) will be used as the default value. For example, in the "
             + "above example, this would result in '2020/_/photoname.jpg' if address was null."
         )
-
+        formatter.write("\n")
+        formatter.write_text(
+            'You may specify a null default (e.g. "" or empty string) by omitting the value after '
+            + 'the comma, e.g. {title,} which would render to "" if title had no value.'
+        )
         formatter.write("\n")
         templ_tuples = [("Substitution", "Description")]
         templ_tuples.extend((k, v) for k, v in TEMPLATE_SUBSTITUTIONS.items())
@@ -2273,6 +2277,8 @@ def export_photo(
     global VERBOSE
     VERBOSE = bool(verbose_)
 
+    # TODO: if --skip-original-if-edited, it's possible edited version is on disk but
+    # original is missing, in which case we should download the edited version
     if not download_missing:
         if photo.ismissing:
             space = " " if not verbose_ else ""
@@ -2299,6 +2305,16 @@ def export_photo(
     results_touched = []
 
     export_original = not (skip_original_if_edited and photo.hasadjustments)
+    # slow_mo photos will always have hasadjustments=True even if not edited
+    if photo.path_edited is None:
+        if photo.slow_mo:
+            export_original = True
+            export_edited = False
+        elif not download_missing:
+            # requested edited version but it's missing, download original
+            export_original = True
+            export_edited = False
+            verbose(f"Edited file for {photo.original_filename} is missing, downloading original")
 
     filenames = get_filenames_from_template(photo, filename_template, original_name)
     for filename in filenames:

osxphotos/_version.py

@@ -1,4 +1,4 @@
 """ version info """
 
-__version__ = "0.36.7"
+__version__ = "0.36.9"
 

osxphotos/exiftool.py

@@ -100,7 +100,7 @@ class _ExifToolProc:
             ],
             stdin=subprocess.PIPE,
             stdout=subprocess.PIPE,
-            stderr=subprocess.DEVNULL,
+            stderr=subprocess.STDOUT,
         )
         self._process_running = True
 
@@ -133,13 +133,19 @@ class ExifTool:
     """ Basic exiftool interface for reading and writing EXIF tags """
 
     def __init__(self, filepath, exiftool=None, overwrite=True):
-        """ Return ExifTool object
-            file: path to image file
-            exiftool: path to exiftool, if not specified will look in path
-            overwrite: if True, will overwrite image file without creating backup, default=False """
+        """ Create ExifTool object
+
+        Args:
+                file: path to image file
+                exiftool: path to exiftool, if not specified will look in path
+                overwrite: if True, will overwrite image file without creating backup, default=False
+        Returns:
+            ExifTool instance
+        """
         self.file = filepath
         self.overwrite = overwrite
         self.data = {}
+        self.error = None
         # if running as a context manager, self._context_mgr will be True
         self._context_mgr = False
         self._exiftoolproc = _ExifToolProc(exiftool=exiftool)
@@ -147,8 +153,18 @@ class ExifTool:
         self._read_exif()
 
     def setvalue(self, tag, value):
-        """ Set tag to value(s) 
-            if value is None, will delete tag """
+        """ Set tag to value(s); if value is None, will delete tag
+
+        Args:
+            tag: str; name of tag to set
+            value: str; value to set tag to
+        
+        Returns:
+            True if success otherwise False
+            
+            If error generated by exiftool, returns False and sets self.error to error string
+            If called in context manager, returns True (execution is delayed until exiting context manager)
+        """
 
         if value is None:
             value = ""
@@ -157,19 +173,32 @@ class ExifTool:
             command.append("-overwrite_original")
         if self._context_mgr:
             self._commands.extend(command)
+            return True
         else:
-            self.run_commands(*command)
+            _, self.error = self.run_commands(*command)
+            return self.error is None
 
     def addvalues(self, tag, *values):
         """ Add one or more value(s) to tag
             If more than one value is passed, each value will be added to the tag
-            Notes: exiftool may add duplicate values for some tags so the caller must ensure
-                   the values being added are not already in the EXIF data
-                   For some tags, such as IPTC:Keywords, this will add a new value to the list of keywords, 
-                   but for others, such as EXIF:ISO, this will literally add a value to the existing value.
-                   It's up to the caller to know what exiftool will do for each tag
-                   If setvalue called before addvalues, exiftool does not appear to add duplicates, 
-                   but if addvalues called without first calling setvalue, exiftool will add duplicate values
+
+        Args:
+            tag: str; tag to set
+            *values: str; one or more values to set
+        
+        Returns:
+            True if success otherwise False
+            
+            If error generated by exiftool, returns False and sets self.error to error string
+            If called in context manager, returns True (execution is delayed until exiting context manager)
+            
+        Notes: exiftool may add duplicate values for some tags so the caller must ensure
+               the values being added are not already in the EXIF data
+               For some tags, such as IPTC:Keywords, this will add a new value to the list of keywords, 
+               but for others, such as EXIF:ISO, this will literally add a value to the existing value.
+               It's up to the caller to know what exiftool will do for each tag
+               If setvalue called before addvalues, exiftool does not appear to add duplicates, 
+               but if addvalues called without first calling setvalue, exiftool will add duplicate values
         """
         if not values:
             raise ValueError("Must pass at least one value")
@@ -185,14 +214,26 @@ class ExifTool:
 
         if self._context_mgr:
             self._commands.extend(command)
+            return True
         else:
-            self.run_commands(*command)
+            _, self.error = self.run_commands(*command)
+            return self.error is None
 
     def run_commands(self, *commands, no_file=False):
-        """ run commands in the exiftool process and return result 
-            no_file: (bool) do not pass the filename to exiftool (default=False)
-                     by default, all commands will be run against self.file
-                     use no_file=True to run a command without passing the filename """
+        """ Run commands in the exiftool process and return result.
+
+        Args:
+                *commands: exiftool commands to run
+                no_file: (bool) do not pass the filename to exiftool (default=False)
+                        by default, all commands will be run against self.file
+                        use no_file=True to run a command without passing the filename
+        Returns:
+            (output, errror)
+            output: bytes is containing output of exiftool commands
+            error: if exiftool generated an error, bytes containing error string otherwise None
+
+        Note: Also sets self.error if error generated.
+        """
         if not (hasattr(self, "_process") and self._process):
             raise ValueError("exiftool process is not running")
 
@@ -218,9 +259,16 @@ class ExifTool:
 
         # read the output
         output = b""
+        error = b""
         while EXIFTOOL_STAYOPEN_EOF not in str(output):
-            output += self._process.stdout.readline().strip()
-        return output[:-EXIFTOOL_STAYOPEN_EOF_LEN]
+            line = self._process.stdout.readline()
+            if line.startswith(b"Warning"):
+                error += line
+            else:
+                output += line.strip()
+        error = None if error == b"" else error
+        self.error = error
+        return output[:-EXIFTOOL_STAYOPEN_EOF_LEN], error
 
     @property
     def pid(self):
@@ -230,14 +278,14 @@ class ExifTool:
     @property
     def version(self):
         """ returns exiftool version """
-        ver = self.run_commands("-ver", no_file=True)
+        ver, _ = self.run_commands("-ver", no_file=True)
         return ver.decode("utf-8")
 
     def asdict(self):
         """ return dictionary of all EXIF tags and values from exiftool 
             returns empty dict if no tags
         """
-        json_str = self.run_commands("-json")
+        json_str, _ = self.run_commands("-json")
         if json_str:
             exifdict = json.loads(json_str)
             return exifdict[0]
@@ -246,7 +294,8 @@ class ExifTool:
 
     def json(self):
         """ returns JSON string containing all EXIF tags and values from exiftool """
-        return self.run_commands("-json")
+        json, _ = self.run_commands("-json")
+        return json
 
     def _read_exif(self):
         """ read exif data from file """
@@ -265,4 +314,4 @@ class ExifTool:
         if exc_type:
             return False
         elif self._commands:
-            self.run_commands(*self._commands)
+            _, self.error = self.run_commands(*self._commands)

osxphotos/phototemplate.py

@@ -174,9 +174,9 @@ class PhotoTemplate:
         #          there would be 6 possible renderings (2 albums x 3 persons)
 
         # regex to find {template_field,optional_default} in strings
-        # for explanation of regex see https://regex101.com/r/4JJg42/1
+        # for explanation of regex see https://regex101.com/r/MbOlJV/4
         # pylint: disable=anomalous-backslash-in-string
-        regex = r"(?<!\{)\{([^\\,}]+)(,{0,1}(([\w\-\%. ]+))?)(?=\}(?!\}))\}"
+        regex = r"(?<!\{)\{([^}]*\+)?([^\\,}+]+)(,{0,1}([\w\-\%. ]+)?)(?=\}(?!\}))\}"
         if type(template) is not str:
             raise TypeError(f"template must be type str, not {type(template)}")
 
@@ -198,17 +198,25 @@ class PhotoTemplate:
             def subst(matchobj):
                 groups = len(matchobj.groups())
                 if groups == 4:
+                    delim = matchobj.group(1)
+                    field = matchobj.group(2)
+                    default = matchobj.group(3)
+                    default_val = matchobj.group(4)
                     try:
-                        val = get_func(matchobj.group(1), matchobj.group(3))
+                        val = get_func(field, default_val)
                     except ValueError:
                         return matchobj.group(0)
 
                     if val is None:
-                        val = (
-                            matchobj.group(3)
-                            if matchobj.group(3) is not None
-                            else none_str
-                        )
+                        # field valid but didn't match a value
+                        if default == ",":
+                            val = ""
+                        else:
+                            val = (
+                                default_val
+                                if default_val is not None
+                                else none_str
+                            )
 
                     return val
                 else:
@@ -249,7 +257,7 @@ class PhotoTemplate:
         rendered_strings = [rendered]
         for field in MULTI_VALUE_SUBSTITUTIONS:
             # Build a regex that matches only the field being processed
-            re_str = r"(?<!\\)\{(" + field + r")(,(([\w\-\%. ]{0,})))?\}"
+            re_str = r"(?<!\{)\{([^}]*\+)?(" + field + r")(,{0,1}([\w\-\%. ]+)?)(?=\}(?!\}))\}"
             regex_multi = re.compile(re_str)
 
             # holds each of the new rendered_strings, dict to avoid repeats (dict.keys())
@@ -319,9 +327,9 @@ class PhotoTemplate:
         for rendered_str in rendered_strings:
             unmatched.extend(
                 [
-                    no_match[0]
+                    no_match[1]
                     for no_match in re.findall(regex, rendered_str)
-                    if no_match[0] not in unmatched
+                    if no_match[1] not in unmatched
                 ]
             )
 

tests/test_exiftool.py

@@ -103,10 +103,28 @@ def test_setvalue_1():
 
     exif = osxphotos.exiftool.ExifTool(tempfile)
     exif.setvalue("IPTC:Keywords", "test")
+    assert not exif.error
+
     exif._read_exif()
     assert exif.data["IPTC:Keywords"] == "test"
 
 
+def test_setvalue_error():
+    # test setting illegal tag value generates error
+    import os.path
+    import tempfile
+    import osxphotos.exiftool
+    from osxphotos.fileutil import FileUtil
+
+    tempdir = tempfile.TemporaryDirectory(prefix="osxphotos_")
+    tempfile = os.path.join(tempdir.name, os.path.basename(TEST_FILE_ONE_KEYWORD))
+    FileUtil.copy(TEST_FILE_ONE_KEYWORD, tempfile)
+
+    exif = osxphotos.exiftool.ExifTool(tempfile)
+    exif.setvalue("IPTC:Foo", "test")
+    assert exif.error
+
+
 def test_setvalue_context_manager():
     # test setting a tag value as context manager
     import os.path
@@ -124,6 +142,8 @@ def test_setvalue_context_manager():
         exif.setvalue("XMP:Title", "title")
         exif.setvalue("XMP:Subject", "subject")
 
+    assert exif.error is None
+
     exif2 = osxphotos.exiftool.ExifTool(tempfile)
     exif2._read_exif()
     assert sorted(exif2.data["IPTC:Keywords"]) == ["test1", "test2"]
@@ -131,6 +151,22 @@ def test_setvalue_context_manager():
     assert exif2.data["XMP:Subject"] == "subject"
 
 
+def test_setvalue_context_manager_error():
+    # test setting a tag value as context manager when error generated
+    import os.path
+    import tempfile
+    import osxphotos.exiftool
+    from osxphotos.fileutil import FileUtil
+
+    tempdir = tempfile.TemporaryDirectory(prefix="osxphotos_")
+    tempfile = os.path.join(tempdir.name, os.path.basename(TEST_FILE_ONE_KEYWORD))
+    FileUtil.copy(TEST_FILE_ONE_KEYWORD, tempfile)
+
+    with osxphotos.exiftool.ExifTool(tempfile) as exif:
+        exif.setvalue("Foo:Bar", "test1")
+    assert exif.error
+
+
 def test_clear_value():
     # test clearing a tag value
     import os.path

tests/test_export_catalina_10_15_7.py

@@ -744,7 +744,7 @@ def test_xmp_sidecar_is_valid(tmp_path):
     xmp_file = tmp_path / XMP_FILENAME
     assert xmp_file.is_file()
     exiftool = ExifTool(str(xmp_file))
-    output = exiftool.run_commands("-validate", "-warning")
+    output, _ = exiftool.run_commands("-validate", "-warning")
     assert output == b"[ExifTool]      Validate                        : 0 0 0"
 
 

tests/test_template.py

@@ -267,7 +267,7 @@ def test_subst_default_val_2():
 
     template = "{place.name.area_of_interest,}"
     rendered, _ = photo.render_template(template)
-    assert rendered[0] == "_"
+    assert rendered[0] == ""
 
 
 def test_subst_unknown_val():
@@ -284,10 +284,6 @@ def test_subst_unknown_val():
     assert rendered[0] == "2020/{foo}"
     assert unknown == ["foo"]
 
-    template = "{place.name.area_of_interest,}"
-    rendered, _ = photo.render_template(template)
-    assert rendered[0] == "_"
-
 
 def test_subst_double_brace():
     """ Test substitution with double brace {{ which should be ignored """