Add wrapper for clip_if() and children_if()

[jradha11]

No description provided.

[KarthikRIyer]

@jradha11 This looks good. You might have missed a few Composable types like Stack. Could you please check if you've covered all of them?

Also, I would add a check in the tests for the size equality of the reference and result Lists.

[jradha11]

@KarthikRIyer
Nice point about the test. I will check for length too in the equality cases.
Also, I will be adding the remaining composable types - Item, Composition, Transition, and Stack.

[KarthikRIyer]

Looks good!

[reinecke]

Looking good! I had a few notes to help code readability and maintainability, but nothing major. Glad to have someone keeping the API up to date!

[KarthikRIyer]

Could you also remove this plugin from the build.gradle file? We do not publish to bintray.

dependencies {
        classpath 'com.jfrog.bintray.gradle:gradle-bintray-plugin:1.+'
    }

[KarthikRIyer]

Looks good.
Tiny suggestion:

JNIEXPORT jobjectArray JNICALL Java_io_opentimeline_opentimelineio_SerializableCollection_clipIfNative
        (JNIEnv *env, jobject thisObj, jobject searchRangeTimeRange, jboolean shallowSearch)

I would rename searchRangeTimeRange to searchRangeTimeRangeOptional everywhere, including the utility function. It'll be very clear that the value is an optional and cannot be used directly as a timerange.

[jradha11]

will do the change

[reinecke]

I think we're pretty much there! One question, I saw tests for:

// Make sure we find expected clips within range
track.childrenIf(Composable.class, Optional.of(search_range), false);

and:

// Make sure null throws NullPointerException
track.childrenIf(Composable.class, null, false);

But I think we were missing:

// Make sure full time range is traversed
track.childrenIf(Composable.class, Optional.empty(), false);

[jradha11]

oh yes! I must have missed this edge case. Will add them too!

[jradha11]

Hey @reinecke @KarthikRIyer ,
I have updated tests with the trimmed and full-time range. But, I was not able to do the same for SerialisableCollectionTest. I am not getting how to filter it. Could you please review and help me out here?

[reinecke]

Oh wow, good catch. I just checked the python implementation and it doesn't seem to have a test for this case.
Looking at the implementation, I think it is designed to:

1. ignore search range for direct children
2. apply search range for child compositions if shallow_search is false

So, with shallow_search as true, I think search_range has no impact on the results of children_if for SerializableCollection. With shallow_search set false, I think it returns all the children of the SerializableCollection and uses type-based behavior for the children it recurses into.

Does that make sense?

Since this test is for language bindings, I don't think it needs to be too rigorous around the behavior of c++ implementation, we just want our testing to verify all the parameters are being passed into the underlying implementation correctly.

[jradha11]

@reinecke
Does that mean we cannot filter using time range in this case?

[KarthikRIyer]

@jradha11 I think what @reinecke means is that if shallow_search = true, search_range is ignored.

If shallow_search = false, it will take search_range into consideration only for Compositions (like Track) inside the SerializableCollection. Track organises objects temporally (one after another). So I think a search_range for it makes sense. Since a Stack organise objects spatially (one over another), but it can contain tracks, search_range makes sense for it too. But a SerializableCollection is just a collection with no specific rules. So unless it has other compositions, a search_range doesn't mean anything.

So you could create a Track with 4 clips, add it to your SerializableCollection and check if clipIf works.

@reinecke please correct me if I'm wrong.

[jradha11]

@KarthikRIyer @reinecke I have made the changes. I'd love if you could review! :)

[reinecke]

Looks Good! Thanks for seeing this through!