Skip to content
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

setNext changes #4

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

setNext changes #4

wants to merge 2 commits into from

Conversation

stoyicker
Copy link
Member

As discussed on Slack, this adds a new event to signify successful completion of setNext calls.

@asendra
Copy link

asendra commented Jul 15, 2024

Two small questions that I hope are not dumb.

1. What does it mean successful completion of setNext exactly?

If it means after loading the PlaybackInfo, same as when we do a load, then this new event on iOS might take a while, because we don't start to load the next item it until the current item is fully loaded. (Either fully downloaded because it is an offlined file, or fully cached by the player while streaming it)

If it means as soon as we internally set the self.nextItem property of PlayerEngine, then maybe we could clarify here what can cause the event to not be sent?

On iOS I see 3 possible conditions:

  • Player state is IDLE
  • setNext is called for the same MediaProduct we already have (I guess we could still send the event here..)
  • setNext is called with a nil MediaProduct

2. I consider this event an optional event for the consumers of our SDK. Do we need to clarify that in the spec or is that up to each platform?

@stoyicker
Copy link
Member Author

  1. My intent was that the meaning of successfully setting next refers to setting the property only. Also, regarding the conditions in which the event fails to be sent, I would think that for the last 2 bullet points you mention it is sent, leaving the IDLE case as the only condition where it does not get sent. I'd rather this not be specified in the documentation as it does not look to me like the value of the information it adds is worth having to update it if we change the internal implementation, but I can live with it.
  2. What does this mean? Are there any events that are not optional for consumers to listen to?

@asendra
Copy link

asendra commented Jul 15, 2024

  1. I did not mean to specify it in the documentation, just that we synced on it. But that can be done through other channels.
  2. Probably specific to iOS, but we have some events that are optional and others that are not. For those that are not optional, the Compiler will complain if the consumer does not define the listener to those events. They can of course be no-ops... But this can also be something up to each platform, or not defined at the spec level at least.

The ones that are not optional on iOS are:

  • stateChanged(to state: State)
  • ended(_ mediaProduct: MediaProduct)
  • mediaTransitioned(to mediaProduct: MediaProduct, with playbackContext: PlaybackContext)
  • failed(with error: PlayerError)

@stoyicker
Copy link
Member Author

I see. All events on Android are optional currently, and changing things to force consumers to treat some specifically would require changing the API from an event stream that the consumer hooks up to to more of a listener which the consumer implements and "installs" into Player, which I'm not sure respects the spec. Let's take this internally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants