-
Notifications
You must be signed in to change notification settings - Fork 7
feat: Update Media Content Time Spent Calculations with Ad Breaks #74
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
Changes from all 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 |
|---|---|---|
| @@ -1,6 +1,5 @@ | ||
| package com.mparticle.media | ||
|
|
||
| import android.annotation.SuppressLint | ||
| import com.mparticle.MPEvent | ||
| import com.mparticle.MParticle | ||
| import com.mparticle.media.events.* | ||
|
|
@@ -75,6 +74,7 @@ class MediaSession protected constructor(builder: Builder) { | |
| private var mparticleInstance: MParticle? | ||
| private var logMPEvents: Boolean | ||
| private var logMediaEvents: Boolean | ||
| private var excludeAdBreaksFromContentTime: Boolean = false | ||
|
|
||
| private var adContent: MediaAd? = null | ||
| private var segment: MediaSegment? = null | ||
|
|
@@ -140,6 +140,7 @@ class MediaSession protected constructor(builder: Builder) { | |
| streamType = builder.streamType.require("streamType") | ||
| logMPEvents = builder.logMPEvents | ||
| logMediaEvents = builder.logMediaEvents | ||
| this.excludeAdBreaksFromContentTime = builder.excludeAdBreaksFromContentTime | ||
| if ( 100 >= builder.mediaContentCompleteLimit && builder.mediaContentCompleteLimit > 0) { | ||
| mediaContentCompleteLimit = builder.mediaContentCompleteLimit | ||
| } | ||
|
|
@@ -281,6 +282,7 @@ class MediaSession protected constructor(builder: Builder) { | |
| * @param adBreak the {@link MediaAdBreak} instance | ||
| */ | ||
| fun logAdBreakStart(adBreak: MediaAdBreak, options: Options? = null) { | ||
| pauseContentTimeIfAdBreakExclusionEnabled() | ||
| val adBreakEvent = MediaEvent(this, MediaEventName.AD_BREAK_START, options = options).apply { | ||
| this.adBreak = adBreak | ||
| } | ||
|
|
@@ -292,6 +294,7 @@ class MediaSession protected constructor(builder: Builder) { | |
| * Log a MediaEvent of type {@link MediaEventName.AD_BREAK_END} | ||
| */ | ||
| fun logAdBreakEnd(options: Options? = null) { | ||
| resumeContentTimeIfAdBreakExclusionEnabled() | ||
| val adBreakEvent = MediaEvent(this, MediaEventName.AD_BREAK_END, options = options) | ||
| logEvent(adBreakEvent) | ||
| } | ||
|
|
@@ -571,6 +574,23 @@ class MediaSession protected constructor(builder: Builder) { | |
| } | ||
| } | ||
|
|
||
| private fun pauseContentTimeIfAdBreakExclusionEnabled() { | ||
| if (!excludeAdBreaksFromContentTime) { | ||
|
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. isn't this logic inverted? |
||
| currentPlaybackStartTimestamp?.let { | ||
| storedPlaybackTime += ((System.currentTimeMillis() - it) / 1000) | ||
| currentPlaybackStartTimestamp = null | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private fun resumeContentTimeIfAdBreakExclusionEnabled() { | ||
| if (!excludeAdBreaksFromContentTime) { | ||
| if (currentPlaybackStartTimestamp != null) { | ||
| currentPlaybackStartTimestamp = System.currentTimeMillis() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private fun logAdSummary(content: MediaAd?) { | ||
| content?.let { ad -> | ||
| ad.adStartTimestamp?.let { startTime -> | ||
|
|
@@ -679,6 +699,9 @@ class MediaSession protected constructor(builder: Builder) { | |
| var mediaSessionAttributes: MutableMap<String, Any?> = mutableMapOf() | ||
| @JvmSynthetic | ||
| set | ||
| var excludeAdBreaksFromContentTime: Boolean = false | ||
| @JvmSynthetic | ||
| set | ||
| /** | ||
| * Set the Title of the {@link MediaContent} for this {@link MediaSession} | ||
| */ | ||
|
|
@@ -753,6 +776,15 @@ class MediaSession protected constructor(builder: Builder) { | |
| return this | ||
| } | ||
|
|
||
| /** | ||
| * When enabled, automatically pauses content time tracking during ad breaks and resumes after. | ||
| * When disabled (default), ad break time is included in content time spent. | ||
| */ | ||
| fun pauseContentDuringAdBreaks(shouldPause: Boolean): Builder { | ||
| this.excludeAdBreaksFromContentTime = shouldPause | ||
| return this | ||
| } | ||
|
|
||
| /** | ||
| * Build to a {@link MediaSession} | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,9 @@ | ||
| package com.mparticle | ||
|
|
||
| import com.mparticle.media.MediaSegmentSummary | ||
| import com.mparticle.media.MediaSession | ||
| import com.mparticle.media.events.* | ||
| import com.mparticle.testutils.RandomUtils | ||
| import junit.framework.Assert.* | ||
| import org.junit.Assert | ||
| import org.junit.Assert.assertNotEquals | ||
| import org.junit.Test | ||
| import java.lang.reflect.Method | ||
|
|
@@ -679,6 +677,60 @@ class MediaSessionTest { | |
| assertNotNull(mparticle.loggedEvents[3].customAttributes) | ||
| assertNull(mparticle.loggedEvents[3].customAttributes?.get("ad_break_id")) | ||
| } | ||
|
|
||
| @Test | ||
| fun testContentTimeExcludesAdBreak_When_Flag_Disable() { | ||
| val mparticle = MockMParticle() | ||
| val events = mutableListOf<MediaEvent>() | ||
| val mediaSession = MediaSession.builder(mparticle) { | ||
| title = "hello" | ||
| mediaContentId ="123" | ||
| duration =1000 | ||
| excludeAdBreaksFromContentTime = false | ||
| } | ||
| mediaSession.mediaEventListener = { events.add(it) } | ||
|
|
||
| mediaSession.logPlay() | ||
| Thread.sleep(1000) | ||
| mediaSession.logAdBreakStart { | ||
| id = "break-1" | ||
| } | ||
| Thread.sleep(1000) // should NOT count toward content time | ||
| mediaSession.logPause() | ||
| mediaSession.logAdBreakEnd() | ||
|
|
||
|
|
||
| val contentTime = mediaSession.mediaContentTimeSpent | ||
| assertEquals(1.0, contentTime) | ||
|
|
||
| val playCount = events.count { it.eventName == MediaEventName.PLAY } | ||
| val pauseCount = events.count { it.eventName == MediaEventName.PAUSE } | ||
| assertEquals(1, playCount) | ||
| assertEquals(1, pauseCount) | ||
| } | ||
|
|
||
| @Test | ||
| fun testDefaultBehavior_Unchanged_When_Flag_Enable() { | ||
| val mparticle = MockMParticle() | ||
| val mediaSession = MediaSession.builder(mparticle) { | ||
| title = "hello" | ||
| mediaContentId ="123" | ||
| duration =1000 | ||
| excludeAdBreaksFromContentTime = true | ||
| } | ||
|
|
||
| mediaSession.logPlay() | ||
| Thread.sleep(1000) | ||
| mediaSession.logAdBreakStart { | ||
| id = "break-2" | ||
| } | ||
| Thread.sleep(1000) // should count toward content time when flag disabled | ||
|
Member
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. this should be enabled since it's true 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. why didn't this comment stop the PR? this test is indeed asserting the opposite of what it should be.
Member
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 actually added this comment after it was merged. |
||
| mediaSession.logAdBreakEnd() | ||
| mediaSession.logPause() | ||
|
|
||
| val contentTime = mediaSession.mediaContentTimeSpent | ||
| assertEquals(2.0, contentTime) | ||
|
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. this and any other asserts are going to be flakey. you can't guarantee the code is going to run for exactly this long. |
||
| } | ||
| } | ||
|
|
||
| fun <T: Any> T.assertTrue(assertion: (T) -> Boolean) { | ||
|
|
||
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.
why do you have a different public and private way of describing the same thing? you use the word "exclude" here and "pause" in the public. that leads to confusion and perhaps even led to the bug noted below?