feat: use engage-sdk for continue watching #150
Conversation
|
Hello @vighnesh153, Once this new SDK is integrated, can you clarify which is the compatibility with older TVs with 'Play next' row? Thanks! |
|
2 reasons why the integration isn't working:
|
miguelmontemayor
requested review from
miguelmontemayor,
pflammertsma and
chikoski
|
I will review this PR, but I am not familiar with the API. @miguelmontemayor Would you please review this PR if it uses API properly or not? |
vighnesh153
force-pushed
the
reference-app
branch
from
1daa528
to
bc19b51
Compare
| import com.google.android.engage.service.Intents.ACTION_PUBLISH_CONTINUATION | ||
|
|
||
| /** Broadcast Receiver to trigger a one time publish of clusters. */ | ||
| class EngageServiceBroadcastReceiver : BroadcastReceiver() { |
There was a problem hiding this comment.
I am wondering this Broadcast Receiver is not registered. Would you please double check it is registered dynamically or statically in the Manifest?
|
|
||
| /** | ||
| * [EngageServiceWorker] is a [Worker] class that is tasked with publishing cluster | ||
| * requests to Engage Service |
There was a problem hiding this comment.
Would you please add a URL of the document describing context of using ServiceWorker to publish cluster requests?
| import com.google.android.gms.tasks.Task | ||
| import com.google.android.gms.tasks.Tasks | ||
| import com.google.common.annotations.VisibleForTesting | ||
| import timber.log.Timber |
There was a problem hiding this comment.
Suggesting to replace Timber with the standard Log.
| return Result.failure() | ||
| } | ||
|
|
||
| Timber.i("Checking for Engage Service availability") |
There was a problem hiding this comment.
Suggesting to replace with Log.i
| return Result.failure() | ||
| } | ||
|
|
||
| Timber.i("Engage Service is available. Proceeding to publish clusters") |
There was a problem hiding this comment.
Suggesting to replace with Log.i
| import com.android.tv.reference.watchnext.Publisher.publishContinuationClusters | ||
| import com.google.android.engage.service.Intents.ACTION_PUBLISH_CONTINUATION | ||
|
|
||
| /** Broadcast Receiver to trigger a one time publish of clusters. */ |
There was a problem hiding this comment.
Suggesting to add a link to the document describing why BroadcastReceiver is necessary.
| if ((playerState != WatchNextHelper.PLAY_STATE_PAUSED) and | ||
| (playerState != WatchNextHelper.PLAY_STATE_ENDED) | ||
| ) { | ||
| Timber.e("Error.Invalid entry for Watch Next. Player state: $playerState") |
| val workRequest = | ||
| PeriodicWorkRequest.Builder( | ||
| EngageServiceWorker::class.java, | ||
| /* repeatInterval= */ 24, |
There was a problem hiding this comment.
We should use named parameter here if we could.
| PeriodicWorkRequest.Builder( | ||
| EngageServiceWorker::class.java, | ||
| /* repeatInterval= */ 24, | ||
| /* repeatIntervalTimeUnit= */ TimeUnit.HOURS |
There was a problem hiding this comment.
We should use named parameter here if we could.
| import com.google.android.engage.video.datamodel.VideoEntity | ||
| import com.google.android.engage.video.datamodel.WatchNextType | ||
|
|
||
| object VideoToEngageEntityConverter { |
There was a problem hiding this comment.
This object is useful as it shows how we can convert the data models for Client Side Play Next to the ones for Video Discovery At the same time, I am wondering if we should remove all Client Side Play Next code and replace them with the ones for Video Discovery API.
There was a problem hiding this comment.
Let's keep the current implementation as is.
This Pull request integrates Engage SDK with TV samples app. In the current implementation, we are making use of
androidx.tvproviderto update the Watch Next program row on a TV device. Sinceandroidx.tvprovideris no longer maintained and we are now moving towards cross-device continue watching, we are migrating to making use of Engage SDK to publish to the watch next program row on a TV device.