Improving lifecycle callbacks in order to reduce conviva errors: Invalid : Did you report playback ended?
#78
Conversation
…andled in the conviva sdk internally already (via ActivityLifecycleObserver)
...ivaExampleApp/src/main/java/com/bitmovin/analytics/convivaanalyticsexample/MainActivity.java
Outdated
Show resolved
Hide resolved
conviva/src/main/java/com/bitmovin/analytics/conviva/ConvivaAnalyticsIntegration.java
Show resolved
Hide resolved
conviva/src/main/java/com/bitmovin/analytics/conviva/ConvivaAnalyticsIntegration.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| /** | ||
| * This should be called when the app is paused | ||
| * <p> | ||
| * NOTE: This method must not be called, when the analytics is created via `ConvivaAnalytics.buildVideoAnalytics` or `ConvivaAnalytics.buildAdAnalytics` as this is handled internally already! |
There was a problem hiding this comment.
The constructors for inserting the Conviva video & ad analytics are meant for testing purposes only. Hence I am not sure if this comment makes sense.
There was a problem hiding this comment.
so the constructors that should be used are only following 2?
public ConvivaAnalyticsIntegration(Player player, String customerKey, Context context)public ConvivaAnalyticsIntegration(Player player, String customerKey, Context context, ConvivaConfig config)
what about the constructor withssai? also for testing only?
in the case the other constructors are for testing only, I would mark them as @VisibleForTesting, to disallow customers the usage of them. (which would allow us to remove the functions again 🤔 )
There was a problem hiding this comment.
The are removed in #81, still I am not super happy with just removing the functions given that this might be in use by customers.
I would suggest to keep them for now and maybe deprecate them to indicate that they should not be used any more.
Maybe there is still a scenario out there that we are missing where they could be useful. @dweinber or @stonko1994 might know more.
There was a problem hiding this comment.
I didn't notice this PR 🙈 -> will update and resolve the merge conflicts tomorrow first thing
ok, I am fine to keep the functions and mark them as deprecated 👍
There was a problem hiding this comment.
I don't think there is any value in having those constructors. I looked up the commit where they were introduced (at least the first one): ce87848#diff-4c62961b542a5646de9a27ab30562e6b508b8f3438a8c950a893960b81ee5733.
The commit messages clearly indicates that this was just for testing (mocking/stubbing). I'd take the risk of someone using it and keeping them removed.
There was a problem hiding this comment.
@stonko1994 thanks for your input about the constructors.
What do you think about the functions reportAppForegrounded and ...Backgrounded? (these functions shouldn't be called anymore, as this is handled inside the conviva-core sdk and will produce error messages in touchstone when being called by the customer)
@strangesource I updated the comment and marked the functions as deprecated here: 19ad7c3
There was a problem hiding this comment.
They were definitelly needed in the past and we also mention the need to call them in the readme AFAIK. If they should no longer used I'd deprecate them but I wouldn't throw errors (only log warnings).
Revert convivaAnalyticsIntegration = null
Conviva showed multiple errors while reporting metrics (mainly during startup / teardown)
PlayerViewlivecycle callbacksreportAppForegroundedandreportAppBackgroundedas it is handled internally alreadyendSessioncall before releasing the conviva sdkNote:
Conviva still shows an error, when ads are disabled.
This is because we are initializing the ad analytics always. (which will in turn automatically subscribe for
activityLifecycleObservercallbacks and tries to report foreground/background although no ad session is running)