Bugfixes: Issue 320, 338, and 349 (biggest feature: constructors copy values) (#359)

* Ensure that upgrade functions are sorted.
* Remove unneccesary check from deserialize_from_string
* Make a copy of the environment in the rv adapter.
* Enforce copying arguments to items.
* Ensure that end_time_inclusive returns a copy.
* Copy arguments to TimeRange and TimeTransform.
* Use the "super" syntax for calling the parent constructor.
* cleaned up extended_by.
* Gate deepcopy with if rather than 'or'
* Add unit tests for coverage.

Author: ssteinbach

opentimelineio/core/composable.py

@@ -30,6 +30,8 @@
 from . import serializable_object
 from . import type_registry
 
+import copy
+
 
 @type_registry.register_type
 class Composable(serializable_object.SerializableObject):
@@ -58,7 +60,7 @@ def __init__(self, name=None, metadata=None):
 
         # initialize the serializable fields
         self.name = name
-        self.metadata = metadata or {}
+        self.metadata = copy.deepcopy(metadata) if metadata else {}
 
     @staticmethod
     def visible():

opentimelineio/core/composition.py

@@ -57,12 +57,16 @@ def __init__(
         name=None,
         children=None,
         source_range=None,
+        markers=None,
+        effects=None,
         metadata=None
     ):
         item.Item.__init__(
             self,
             name=name,
             source_range=source_range,
+            markers=markers,
+            effects=effects,
             metadata=metadata
         )
         collections.MutableSequence.__init__(self)

opentimelineio/core/item.py

@@ -61,14 +61,11 @@ def __init__(
         markers=None,
         metadata=None,
     ):
-        serializable_object.SerializableObject.__init__(self)
-
-        self.name = name
-        self.source_range = source_range
-        self.effects = effects or []
-        self.markers = markers or []
-        self.metadata = metadata or {}
-        self._parent = None
+        super(Item, self).__init__(name=name, metadata=metadata)
+
+        self.source_range = copy.deepcopy(source_range)
+        self.effects = copy.deepcopy(effects) if effects else []
+        self.markers = copy.deepcopy(markers) if markers else []
 
     name = serializable_object.serializable_field("name", doc="Item name.")
     source_range = serializable_object.serializable_field(

opentimelineio/core/json_serializer.py

@@ -206,12 +206,7 @@ def _as_otio(dct):
 def deserialize_json_from_string(otio_string):
     """ Deserialize a string containing JSON to OTIO objects. """
 
-    json_data = json.loads(otio_string, object_hook=_as_otio)
-
-    if json_data is {}:
-        raise exceptions.CouldNotReadFileError
-
-    return json_data
+    return json.loads(otio_string, object_hook=_as_otio)
 
 
 def deserialize_json_from_file(otio_filepath):

opentimelineio/core/media_reference.py

@@ -32,6 +32,8 @@
     serializable_object,
 )
 
+import copy
+
 
 @type_registry.register_type
 class MediaReference(serializable_object.SerializableObject):
@@ -52,11 +54,11 @@ def __init__(
         available_range=None,
         metadata=None
     ):
-        serializable_object.SerializableObject.__init__(self)
+        super(MediaReference, self).__init__()
 
         self.name = name
-        self.available_range = available_range
-        self.metadata = metadata or {}
+        self.available_range = copy.deepcopy(available_range)
+        self.metadata = copy.deepcopy(metadata) or {}
 
     name = serializable_object.serializable_field(
         "name",

opentimelineio/core/type_registry.py

@@ -135,7 +135,10 @@ def instance_from_schema(schema_name, schema_version, data_dict):
         )
 
     if cls.schema_version() != schema_version:
-        for version, upgrade_func in (
+        # since the keys are the versions to upgrade to, sorting the keys
+        # before iterating through them should ensure that upgrade functions
+        # are called in order.
+        for version, upgrade_func in sorted(
             _UPGRADE_FUNCTIONS[cls].items()
         ):
             if version < schema_version:

opentimelineio/hooks.py

@@ -97,7 +97,7 @@ def __init__(
     ):
         """HookScript plugin constructor."""
 
-        plugins.PythonPlugin.__init__(self, name, execution_scope, filepath)
+        super(HookScript, self).__init__(name, execution_scope, filepath)
 
     def run(self, in_timeline, argument_map={}):
         """Run the hook_function associated with this plugin."""

opentimelineio/media_linker.py

@@ -137,7 +137,7 @@ def __init__(
         execution_scope=None,
         filepath=None,
     ):
-        plugins.PythonPlugin.__init__(self, name, execution_scope, filepath)
+        super(MediaLinker, self).__init__(name, execution_scope, filepath)
 
     def link_media_reference(self, in_clip, media_linker_argument_map=None):
         media_linker_argument_map = media_linker_argument_map or {}

opentimelineio/opentime.py

@@ -136,6 +136,7 @@ def __iadd__(self, other):
         self.value = value
         self.rate = scale
 
+        # @TODO: make this construct and return a new object
         return self
 
     def __add__(self, other):
@@ -233,7 +234,7 @@ class TimeTransform(object):
     """1D Transform for RationalTime.  Has offset and scale."""
 
     def __init__(self, offset=RationalTime(), scale=1.0, rate=None):
-        self.offset = offset
+        self.offset = copy.copy(offset)
         self.scale = scale
         self.rate = rate
 
@@ -315,8 +316,8 @@ class TimeRange(object):
     """
 
     def __init__(self, start_time=RationalTime(), duration=RationalTime()):
-        self.start_time = start_time
-        self.duration = duration
+        self.start_time = copy.copy(start_time)
+        self.duration = copy.copy(duration)
 
     def __copy__(self, memodict=None):
         # Construct a new one directly to avoid the overhead of deepcopy
@@ -368,7 +369,7 @@ def end_time_inclusive(self):
 
             return result
         else:
-            return self.start_time
+            return copy.deepcopy(self.start_time)
 
     def end_time_exclusive(self):
         """"Time of the first sample outside the time range.
@@ -385,24 +386,20 @@ def end_time_exclusive(self):
     def extended_by(self, other):
         """Construct a new TimeRange that is this one extended by another."""
 
-        result = TimeRange(self.start_time, self.duration)
-        if isinstance(other, TimeRange):
-            result.start_time = min(self.start_time, other.start_time)
-            new_end_time = max(
-                self.end_time_exclusive(),
-                other.end_time_exclusive()
-            )
-            result.duration = duration_from_start_end_time(
-                result.start_time,
-                new_end_time
-            )
-        else:
+        if not isinstance(other, TimeRange):
             raise TypeError(
                 "extended_by requires rtime be a TimeRange, not a '{}'".format(
                     type(other)
                 )
             )
-        return result
+
+        start_time = min(self.start_time, other.start_time)
+        new_end_time = max(
+            self.end_time_exclusive(),
+            other.end_time_exclusive()
+        )
+        duration = duration_from_start_end_time(start_time, new_end_time)
+        return TimeRange(start_time, duration)
 
     # @TODO: remove?
     def clamped(
@@ -790,8 +787,12 @@ def duration_from_start_end_time(start_time, end_time_exclusive):
         )
     else:
         return RationalTime(
-            end_time_exclusive.value_rescaled_to(start_time) - start_time.value,
-            start_time.rate)
+            (
+                end_time_exclusive.value_rescaled_to(start_time)
+                - start_time.value
+            ),
+            start_time.rate
+        )
 
 
 # @TODO: create range from start/end [in,ex]clusive

opentimelineio/plugins/manifest.py

@@ -82,7 +82,7 @@ class Manifest(core.SerializableObject):
     _serializable_label = "PluginManifest.1"
 
     def __init__(self):
-        core.SerializableObject.__init__(self)
+        super(Manifest, self).__init__()
         self.adapters = []
         self.schemadefs = []
         self.media_linkers = []