Fix for SerializableCollection::children_if (AcademySoftwareFoundation#1404)

* Fix for children_if
* Missing shallow_search parameter fixes
* Refactor test_children_if tests
* Lint fixes
* Add Python tests

Signed-off-by: Darby Johnston <[email protected]>
Signed-off-by: Michele Spina <[email protected]>

Author: darbyjohnston

src/opentimelineio/serializableCollection.h

@@ -5,6 +5,7 @@
 
 #include "opentimelineio/composition.h"
 #include "opentimelineio/serializableObjectWithMetadata.h"
+#include "opentimelineio/timeline.h"
 #include "opentimelineio/version.h"
 
 namespace opentimelineio { namespace OPENTIMELINEIO_VERSION {
@@ -98,8 +99,8 @@ SerializableCollection::children_if(
             out.push_back(valid_child);
         }
 
-        // if not a shallow_search, for children that are serialiable collections or compositions,
-        // recurse into their children
+        // if not a shallow_search, for children that are serializable collections,
+        // compositions, or timelines, recurse into their children
         if (!shallow_search)
         {
             if (auto collection =
@@ -129,6 +130,19 @@ SerializableCollection::children_if(
                     out.push_back(valid_child);
                 }
             }
+            else if (auto timeline = dynamic_cast<Timeline*>(child.value))
+            {
+                const auto valid_children =
+                    timeline->children_if<T>(error_status, search_range);
+                if (is_error(error_status))
+                {
+                    return out;
+                }
+                for (const auto& valid_child: valid_children)
+                {
+                    out.push_back(valid_child);
+                }
+            }
         }
     }
     return out;

src/py-opentimelineio/opentimelineio-bindings/otio_serializableObjects.cpp

@@ -289,12 +289,12 @@ A :class:`~SerializableCollection` is useful for serializing multiple timelines,
         .def("__iter__", [](SerializableCollection* c) {
                 return new SerializableCollectionIterator(c);
             })
-        .def("clip_if", [](SerializableCollection* t, optional<TimeRange> const& search_range) {
-                return clip_if(t, search_range);
-            }, "search_range"_a = nullopt)
-        .def("children_if", [](SerializableCollection* t, py::object descended_from_type, optional<TimeRange> const& search_range) {
-                return children_if(t, descended_from_type, search_range);
-            }, "descended_from_type"_a = py::none(), "search_range"_a = nullopt);
+        .def("clip_if", [](SerializableCollection* t, optional<TimeRange> const& search_range, bool shallow_search) {
+                return clip_if(t, search_range, shallow_search);
+            }, "search_range"_a = nullopt, "shallow_search"_a = false)
+        .def("children_if", [](SerializableCollection* t, py::object descended_from_type, optional<TimeRange> const& search_range, bool shallow_search) {
+                return children_if(t, descended_from_type, search_range, shallow_search);
+            }, "descended_from_type"_a = py::none(), "search_range"_a = nullopt, "shallow_search"_a = false);
 
 }
 
@@ -611,9 +611,9 @@ Should be subclassed (for example by :class:`.Track` and :class:`.Stack`), not u
              "markers"_a = py::none(),
              "effects"_a = py::none(),
              py::arg_v("metadata"_a = py::none()))
-        .def("clip_if", [](Stack* t, optional<TimeRange> const& search_range) {
-                return clip_if(t, search_range);
-            }, "search_range"_a = nullopt);
+        .def("clip_if", [](Stack* t, optional<TimeRange> const& search_range, bool shallow_search) {
+                return clip_if(t, search_range, shallow_search);
+            }, "search_range"_a = nullopt, "shallow_search"_a = false);
 
     py::class_<Timeline, SerializableObjectWithMetadata, managing_ptr<Timeline>>(m, "Timeline", py::dynamic_attr())
         .def(py::init([](std::string name,
@@ -641,12 +641,12 @@ Should be subclassed (for example by :class:`.Track` and :class:`.Stack`), not u
             })
         .def("video_tracks", &Timeline::video_tracks)
         .def("audio_tracks", &Timeline::audio_tracks)
-        .def("clip_if", [](Timeline* t, optional<TimeRange> const& search_range) {
-                return clip_if(t, search_range);
-            }, "search_range"_a = nullopt)
-        .def("children_if", [](Timeline* t, py::object descended_from_type, optional<TimeRange> const& search_range) {
-                return children_if(t, descended_from_type, search_range);
-            }, "descended_from_type"_a = py::none(), "search_range"_a = nullopt);
+        .def("clip_if", [](Timeline* t, optional<TimeRange> const& search_range, bool shallow_search) {
+                return clip_if(t, search_range, shallow_search);
+            }, "search_range"_a = nullopt, "shallow_search"_a = false)
+        .def("children_if", [](Timeline* t, py::object descended_from_type, optional<TimeRange> const& search_range, bool shallow_search) {
+                return children_if(t, descended_from_type, search_range, shallow_search);
+            }, "descended_from_type"_a = py::none(), "search_range"_a = nullopt, "shallow_search"_a = false);
 }
 
 static void define_effects(py::module m) {

src/py-opentimelineio/opentimelineio/schema/serializable_collection.py

@@ -31,7 +31,8 @@ def __repr__(self):
 
 
 @add_method(_otio.SerializableCollection)
-def each_child(self, search_range=None, descended_from_type=_otio.Composable):
+def each_child(self, search_range=None, descended_from_type=_otio.Composable,
+               shallow_search=False):
     """ Generator that returns each child contained in the serializable
     collection in the order in which it is found.
 
@@ -42,13 +43,15 @@ def each_child(self, search_range=None, descended_from_type=_otio.Composable):
                                    with the search range will be yielded.
     :param type descended_from_type: if specified, only children who are a descendent
                                      of the descended_from_type will be yielded.
+    :param bool shallow_search: if True, will only search children of self and not
+                                recurse into children of children.
     """
-    for child in self.children_if(descended_from_type, search_range):
+    for child in self.children_if(descended_from_type, search_range, shallow_search):
         yield child
 
 
 @add_method(_otio.SerializableCollection)
-def each_clip(self, search_range=None):
+def each_clip(self, search_range=None, shallow_search=False):
     """ Generator that returns each clip contained in the serializable
     collection in the order in which it is found.
 
@@ -57,6 +60,8 @@ def each_clip(self, search_range=None):
 
     :param TimeRange search_range: if specified, only children whose range overlaps
                                    with the search range will be yielded.
+    :param bool shallow_search: if True, will only search children of self and not
+                                recurse into children of children.
     """
-    for child in self.clip_if(search_range):
+    for child in self.clip_if(search_range, shallow_search):
         yield child

src/py-opentimelineio/opentimelineio/schema/stack.py

@@ -6,7 +6,7 @@
 
 
 @add_method(_otio.Stack)
-def each_clip(self, search_range=None):
+def each_clip(self, search_range=None, shallow_search=False):
     """Generator that returns each clip contained in the stack
     in the order in which it is found.
 
@@ -15,6 +15,8 @@ def each_clip(self, search_range=None):
 
     :param TimeRange search_range: if specified, only children whose range overlaps
                                    with the search range will be yielded.
+    :param bool shallow_search: if True, will only search children of self and not
+                                recurse into children of children.
     """
-    for child in self.clip_if(search_range):
+    for child in self.clip_if(search_range, shallow_search):
         yield child

src/py-opentimelineio/opentimelineio/schema/timeline.py

@@ -21,7 +21,8 @@ def __repr__(self):
 
 
 @add_method(_otio.Timeline)
-def each_child(self, search_range=None, descended_from_type=_otio.Composable):
+def each_child(self, search_range=None, descended_from_type=_otio.Composable,
+               shallow_search=False):
     """Generator that returns each child contained in the timeline
     in the order in which it is found.
 
@@ -32,13 +33,15 @@ def each_child(self, search_range=None, descended_from_type=_otio.Composable):
                                    with the search range will be yielded.
     :param type descended_from_type: if specified, only children who are a descendent
                                      of the descended_from_type will be yielded.
+    :param bool shallow_search: if True, will only search children of self and not
+                                recurse into children of children.
     """
-    for child in self.children_if(descended_from_type, search_range):
+    for child in self.children_if(descended_from_type, search_range, shallow_search):
         yield child
 
 
 @add_method(_otio.Timeline)
-def each_clip(self, search_range=None):
+def each_clip(self, search_range=None, shallow_search=False):
     """Generator that returns each clip contained in the timeline
     in the order in which it is found.
 
@@ -47,6 +50,8 @@ def each_clip(self, search_range=None):
 
     :param TimeRange search_range: if specified, only children whose range overlaps
                                    with the search range will be yielded.
+    :param bool shallow_search: if True, will only search children of self and not
+                                recurse into children of children.
     """
-    for child in self.clip_if(search_range):
+    for child in self.clip_if(search_range, shallow_search):
         yield child

src/py-opentimelineio/opentimelineio/schema/track.py

@@ -14,9 +14,9 @@ def each_clip(self, search_range=None, shallow_search=False):
         Use :meth:`clip_if` instead.
 
     :param TimeRange search_range: if specified, only children whose range overlaps with
-                      the search range will be yielded.
-    :param bool shallow_search: if True, will only search children of self, not
-                        and not recurse into children of children.
+                                   the search range will be yielded.
+    :param bool shallow_search: if True, will only search children of self and not
+                                recurse into children of children.
     """
-    for child in self.clip_if(search_range):
+    for child in self.clip_if(search_range, shallow_search):
         yield child

tests/CMakeLists.txt

@@ -15,7 +15,7 @@ foreach(test ${tests_opentime})
            WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR})
 endforeach()
 
-list(APPEND tests_opentimelineio test_clip)
+list(APPEND tests_opentimelineio test_clip test_serializableCollection test_timeline test_track)
 foreach(test ${tests_opentimelineio})
     add_executable(${test} utils.h utils.cpp ${test}.cpp)
 

tests/test_clip.cpp

@@ -7,6 +7,7 @@
 #include <opentimelineio/deserialization.h>
 #include <opentimelineio/externalReference.h>
 #include <opentimelineio/missingReference.h>
+#include <opentimelineio/serializableCollection.h>
 #include <opentimelineio/timeline.h>
 
 #include <iostream>

tests/test_serializableCollection.cpp

@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright Contributors to the OpenTimelineIO project
+
+#include "utils.h"
+
+#include <opentimelineio/clip.h>
+#include <opentimelineio/serializableCollection.h>
+#include <opentimelineio/timeline.h>
+#include <opentimelineio/track.h>
+
+#include <iostream>
+
+namespace otime = opentime::OPENTIME_VERSION;
+namespace otio  = opentimelineio::OPENTIMELINEIO_VERSION;
+
+int
+main(int argc, char** argv)
+{
+    Tests tests;
+
+    tests.add_test(
+        "test_children_if", [] {
+        using namespace otio;
+        otio::SerializableObject::Retainer<otio::Clip> cl =
+            new otio::Clip();
+        otio::SerializableObject::Retainer<otio::Track> tr =
+            new otio::Track();
+        tr->append_child(cl);
+        otio::SerializableObject::Retainer<otio::Timeline> tl =
+            new otio::Timeline();
+        tl->tracks()->append_child(tr);
+        otio::SerializableObject::Retainer<otio::SerializableCollection>
+            sc = new otio::SerializableCollection();
+        sc->insert_child(0, tl);
+        opentimelineio::v1_0::ErrorStatus err;
+        auto result = sc->children_if<otio::Clip>(&err, {}, false);
+        assertEqual(result.size(), 1);
+        assertEqual(result[0].value, cl.value);
+    });
+    tests.add_test(
+        "test_children_if_search_range", [] {
+        using namespace otio;
+        const TimeRange range(RationalTime(0.0, 24.0), RationalTime(24.0, 24.0));
+        otio::SerializableObject::Retainer<otio::Clip> cl0 =
+            new otio::Clip();
+        cl0->set_source_range(range);
+        otio::SerializableObject::Retainer<otio::Clip> cl1 =
+            new otio::Clip();
+        cl1->set_source_range(range);
+        otio::SerializableObject::Retainer<otio::Clip> cl2 =
+            new otio::Clip();
+        cl2->set_source_range(range);
+        otio::SerializableObject::Retainer<otio::Track> tr =
+            new otio::Track();
+        tr->append_child(cl0);
+        tr->append_child(cl1);
+        tr->append_child(cl2);
+        otio::SerializableObject::Retainer<otio::Timeline> tl =
+            new otio::Timeline();
+        tl->tracks()->append_child(tr);
+        otio::SerializableObject::Retainer<otio::SerializableCollection>
+            sc = new otio::SerializableCollection();
+        sc->insert_child(0, tl);
+        opentimelineio::v1_0::ErrorStatus err;
+        auto result = sc->children_if<otio::Clip>(&err, range);
+        assertEqual(result.size(), 1);
+        assertEqual(result[0].value, cl0.value);
+    });
+    tests.add_test(
+        "test_children_if_shallow_search", [] {
+        using namespace otio;
+        otio::SerializableObject::Retainer<otio::Clip> cl =
+            new otio::Clip();
+        otio::SerializableObject::Retainer<otio::Track> tr =
+            new otio::Track();
+        tr->append_child(cl);
+        otio::SerializableObject::Retainer<otio::Timeline> tl =
+            new otio::Timeline();
+        tl->tracks()->append_child(tr);
+        otio::SerializableObject::Retainer<otio::SerializableCollection>
+            sc = new otio::SerializableCollection();
+        sc->insert_child(0, tl);
+        opentimelineio::v1_0::ErrorStatus err;
+        auto result = sc->children_if<otio::Clip>(&err, nullopt, true);
+        assertEqual(result.size(), 0);
+        result = sc->children_if<otio::Clip>(&err, nullopt, false);
+        assertEqual(result.size(), 1);
+        assertEqual(result[0].value, cl.value);
+    });
+
+    tests.run(argc, argv);
+    return 0;
+}

tests/test_serializable_collection.py

@@ -71,6 +71,54 @@ def test_repr(self):
             ")"
         )
 
+    def test_children_if(self):
+        cl = otio.schema.Clip()
+        tr = otio.schema.Track()
+        tr.append(cl)
+        tl = otio.schema.Timeline()
+        tl.tracks.append(tr)
+        sc = otio.schema.SerializableCollection()
+        sc.append(tl)
+        result = sc.children_if(otio.schema.Clip)
+        self.assertEqual(len(result), 1)
+        self.assertEqual(result[0], cl)
+
+    def test_children_if_search_range(self):
+        range = otio.opentime.TimeRange(
+            otio.opentime.RationalTime(0.0, 24.0),
+            otio.opentime.RationalTime(24.0, 24.0))
+        cl0 = otio.schema.Clip()
+        cl0.source_range = range
+        cl1 = otio.schema.Clip()
+        cl1.source_range = range
+        cl2 = otio.schema.Clip()
+        cl2.source_range = range
+        tr = otio.schema.Track()
+        tr.append(cl0)
+        tr.append(cl1)
+        tr.append(cl2)
+        tl = otio.schema.Timeline()
+        tl.tracks.append(tr)
+        sc = otio.schema.SerializableCollection()
+        sc.append(tl)
+        result = sc.children_if(otio.schema.Clip, range)
+        self.assertEqual(len(result), 1)
+        self.assertEqual(result[0], cl0)
+
+    def test_children_if_shallow_search(self):
+        cl = otio.schema.Clip()
+        tr = otio.schema.Track()
+        tr.append(cl)
+        tl = otio.schema.Timeline()
+        tl.tracks.append(tr)
+        sc = otio.schema.SerializableCollection()
+        sc.append(tl)
+        result = sc.children_if(otio.schema.Clip, shallow_search=True)
+        self.assertEqual(len(result), 0)
+        result = sc.children_if(otio.schema.Clip, shallow_search=False)
+        self.assertEqual(len(result), 1)
+        self.assertEqual(result[0], cl)
+
 
 if __name__ == '__main__':
     unittest.main()