-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update: allow starting new views with videoChange
without changing AVPlayerItem
#258
base: master
Are you sure you want to change the base?
Conversation
@@ -1232,6 +1247,9 @@ - (BOOL) doubleValueIsEqual:(NSNumber *) x toOther:(NSNumber *) n { | |||
} | |||
|
|||
- (void)didTriggerManualVideoChange { | |||
// TODO: Should we deprecate? we no longer need this method. videoChange can happen whenever it's needed now, not just when AVPlayerItem changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strong +1 to deprecate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If keeping this functionality around makes sense we could also expose a better named API with the same functionality while deprecating this one. (And we'd drop the deprecated method in the next major version bump.)
…o items, this is more close to the real-world usage
} | ||
|
||
[self.playDispatchDelegate videoChangedForPlayer:_name]; | ||
// TODO: test that we have all the metadata we need on the subsequent view. On android we needed to catch up with the current state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this refer to external stuff like MUXSDKCustomerData
or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The customer data, and also more general things like the private stuff in the player binding
I saw some odd stuff with playhead time during the subsequent views after changing, so I think there might still be something to clear. It could be here or in the player binding (edit: oops, here or in the stores I mean), I'm not really sure
@@ -257,7 +257,7 @@ FOUNDATION_EXPORT | |||
|
|||
*/ | |||
+ (void)videoChangeForPlayer:(nonnull NSString *)name | |||
withCustomerData:(nullable MUXSDKCustomerData *)customerData; | |||
withCustomerData:(nonnull MUXSDKCustomerData *)customerData; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change and requires a major version bump. Particularly for Swift callers.
I'm not sure if it's absolutely required too implement the required functionality in this PR, though. Based on everything else here I don't feel like it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I don't need to make this nullable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest SDK changes look good overall!
If we absolutely need the API break for videoChangeForPlayer: withCustomerData:
we need to take a major version bump to release this.
No description provided.