-
Notifications
You must be signed in to change notification settings - Fork 0
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
Implement Uplynk ads api #29
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,9 @@ import com.theoplayer.android.connector.uplynk.internal.UplynkAdIntegration | |
import com.theoplayer.android.connector.uplynk.internal.UplynkSsaiDescriptionConverter | ||
import com.theoplayer.android.connector.uplynk.internal.UplynkEventDispatcher | ||
import com.theoplayer.android.connector.uplynk.internal.network.UplynkApi | ||
import com.theoplayer.android.connector.uplynk.network.UplynkAd | ||
import com.theoplayer.android.connector.uplynk.network.UplynkAdBreak | ||
import com.theoplayer.android.connector.uplynk.network.UplynkAds | ||
|
||
internal const val TAG = "UplynkConnector" | ||
|
||
|
@@ -23,7 +26,52 @@ class UplynkConnector( | |
private val theoplayerView: THEOplayerView, | ||
) { | ||
private lateinit var integration: UplynkAdIntegration | ||
private val eventDispatcher = UplynkEventDispatcher() | ||
|
||
/** | ||
* Scheduled ad breaks | ||
*/ | ||
var adBreaks: List<UplynkAdBreak>? = null | ||
private set | ||
|
||
/** | ||
* The ad break that is currently played or `null` otherwise | ||
*/ | ||
var currentAdBreak: UplynkAdBreak? = null | ||
private set | ||
|
||
/** | ||
* The ad that is currently played or `null` otherwise | ||
*/ | ||
var currentAd: UplynkAd? = null | ||
private set | ||
|
||
private val eventDispatcher = UplynkEventDispatcher().also { | ||
it.addListener(object : UplynkListener { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about it, but didn't do it because Do you think it would be better to make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, it would be confusing.. |
||
override fun onAdBreaksUpdated(ads: UplynkAds) { | ||
[email protected] = ads.breaks | ||
MattiasBuelens marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
override fun onAdBegin(ad: UplynkAd) { | ||
check([email protected] == null) { "Begin ad that before ending previous currentAd = ${[email protected]} beginAd = $ad" } | ||
[email protected] = ad | ||
} | ||
|
||
override fun onAdEnd(ad: UplynkAd) { | ||
check([email protected] == ad) { "Trying to end ad that is not current. currentAd = ${[email protected]} endedAd = $ad" } | ||
[email protected] = null | ||
} | ||
|
||
override fun onAdBreakBegin(adBreak: UplynkAdBreak) { | ||
check([email protected] == null) { "Begin adbreak before ending previous currentAdBreak = ${[email protected]} beginAdBreak = $adBreak" } | ||
[email protected] = adBreak | ||
} | ||
|
||
override fun onAdBreakEnd(adBreak: UplynkAdBreak) { | ||
check([email protected] == adBreak) { "Trying to end adbreak that is not current. currentAdBreak = ${[email protected]} endedAdBreak = $adBreak" } | ||
[email protected] = null | ||
} | ||
}) | ||
} | ||
|
||
init { | ||
theoplayerView.player.ads.registerServerSideIntegration(INTEGRATION_ID, this::setupIntegration) | ||
|
@@ -41,8 +89,14 @@ class UplynkConnector( | |
return integration | ||
} | ||
|
||
/** | ||
* Add a listener for events | ||
*/ | ||
fun addListener(listener: UplynkListener) = eventDispatcher.addListener(listener) | ||
|
||
/** | ||
* Remove a listener for events | ||
*/ | ||
fun removeListener(listener: UplynkListener) = eventDispatcher.removeListener(listener) | ||
|
||
companion object { | ||
|
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.
Maybe more of a philosophical question: why do we have both
UplynkConnector.currentAds
andplayer.ads.currentAds
? Wasn't the whole point of the custom SSAI integrations that we can put everything behind the THEOplayer ads API?Same thing with the new
UplynkListener
methods: how are those different from the existing events onplayer.ads
?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 only difference between them is the type of Ads object.
player.ads.currentAds
- has common type of an ad, that is part of THEOplayer project.UplynkConnector.currentAds
has specific type for Uplynk ads. The same as forUplynkListener
I think we have to discuss if we even need this PR or if we could find some other solution
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.
Okay, I think this is a limitation of our custom SSAI API then. We should provide a way to add some integration-specific data to an custom
Ad
orAdBreak
.I suggest we park this PR for now until we have that new API.