task(SDK-5584) - Adds support for resuming the video on rotation and …#980
task(SDK-5584) - Adds support for resuming the video on rotation and …#980Anush-Shand wants to merge 4 commits intotask/release/8.1.0from
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR implements a caching system for video player handles during configuration changes. The system preserves player state and fullscreen UI across rotations by storing the player handle with associated URL and fullscreen status in a cache, then restoring it when the activity/fragment is recreated. New interface methods enable surface detachment and soft pause operations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StreamHandler as InAppStreamMediaHandler
participant Cache as InAppVideoPlayerCache
participant Handle as Player Handle
participant View as PlayerView
User->>StreamHandler: Configuration change (rotation)
StreamHandler->>Handle: detachSurface()
Handle->>View: Detach player & clear view
Handle->>Handle: Capture playback position
StreamHandler->>Cache: store(handle, url, fullscreen=true)
Cache->>Cache: Save handle + state
Note over StreamHandler: Activity/Fragment recreated
StreamHandler->>StreamHandler: setup()
StreamHandler->>Cache: consume(url)
Cache->>Cache: Verify URL match
Cache-->>StreamHandler: Return cached handle
StreamHandler->>Cache: consumeFullscreen()
Cache-->>StreamHandler: Return fullscreen state
StreamHandler->>Handle: Reattach to new view
StreamHandler->>View: Open fullscreen dialog
Handle->>View: Resume playback from saved position
User->>User: Video continues seamlessly
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/media/InAppVideoPlayerCache.kt (1)
23-49: Consider consuming one cached entry instead of splitting handle and fullscreen state.
store()produces{handle, url, isFullscreen}together, but the restore path consumes them in two different methods and two different lifecycle phases. Returning one immutable entry fromconsume(url)would remove the half-consumed state and make the cache easier to reason about.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/media/InAppVideoPlayerCache.kt` around lines 23 - 49, The cache currently stores handle, cachedUrl and cachedIsFullscreen separately and exposes consume(url): InAppVideoPlayerHandle? and consumeFullscreen(): Boolean, which allows half-consumed state; instead create and return a single immutable cache entry (e.g., VideoCacheEntry { handle: InAppVideoPlayerHandle, url: String, isFullscreen: Boolean }) from consume(url) and remove consumeFullscreen; update store(handle, url, isFullscreen) to populate the three fields as before but change consume(url) to atomically check url, clear all cached fields (handle, cachedUrl, cachedIsFullscreen) and return the immutable entry or null, and update callers to use the returned entry for both handle and fullscreen state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/media/InAppVideoPlayerCache.kt`:
- Around line 23-49: The cache currently stores handle, cachedUrl and
cachedIsFullscreen separately and exposes consume(url): InAppVideoPlayerHandle?
and consumeFullscreen(): Boolean, which allows half-consumed state; instead
create and return a single immutable cache entry (e.g., VideoCacheEntry {
handle: InAppVideoPlayerHandle, url: String, isFullscreen: Boolean }) from
consume(url) and remove consumeFullscreen; update store(handle, url,
isFullscreen) to populate the three fields as before but change consume(url) to
atomically check url, clear all cached fields (handle, cachedUrl,
cachedIsFullscreen) and return the immutable entry or null, and update callers
to use the returned entry for both handle and fullscreen state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 77315480-d1b5-4af1-bf85-d870c17bf159
📒 Files selected for processing (5)
clevertap-core/src/main/java/com/clevertap/android/sdk/inapp/media/InAppStreamMediaHandler.ktclevertap-core/src/main/java/com/clevertap/android/sdk/inapp/media/InAppVideoPlayerCache.ktclevertap-core/src/main/java/com/clevertap/android/sdk/video/InAppVideoPlayerHandle.ktclevertap-core/src/main/java/com/clevertap/android/sdk/video/inapps/ExoplayerHandle.ktclevertap-core/src/main/java/com/clevertap/android/sdk/video/inapps/Media3Handle.kt
| internal object InAppVideoPlayerCache { | ||
|
|
||
| @Volatile private var handle: InAppVideoPlayerHandle? = null | ||
| @Volatile private var cachedUrl: String? = null |
There was a problem hiding this comment.
Do we need Volatile? I assume callers of this class are from main thread and in that case , remove @volatile (which implies multi-thread awareness) and add @mainthread annotation instead
| player?.pause() | ||
| } | ||
|
|
||
| override fun detachSurface() { |
There was a problem hiding this comment.
Code Duplication between both handles
There was a problem hiding this comment.
Entire ExoplayerHandle is mostly duplicate of Media3Handle. Since Exoplayer is being deprecated, we can let it be
| } | ||
|
|
||
| override fun softPause() { | ||
| player?.pause() |
There was a problem hiding this comment.
Code Duplication between both handles
There was a problem hiding this comment.
Entire ExoplayerHandle is mostly duplicate of Media3Handle. Since Exoplayer is being deprecated, we can let it be
| // Sync icon with current mute state — important after rotation where a fresh | ||
| // PlayerView is created but isMuted may already be false from a prior user toggle. | ||
| val iconRes = if (isMuted) R.drawable.ct_ic_volume_off else R.drawable.ct_ic_volume_on | ||
| val descRes = if (isMuted) R.string.ct_unmute_button_content_description |
There was a problem hiding this comment.
Code Duplication between both handles
There was a problem hiding this comment.
Entire ExoplayerHandle is mostly duplicate of Media3Handle. Since Exoplayer is being deprecated, we can let it be
| // Background: leave fullscreen dialog intact — it will still be there on resume. | ||
| handle?.softPause() | ||
| } | ||
| handle?.savePosition() |
There was a problem hiding this comment.
savePosition() unused code
3 participants
…foregrounding
Summary by CodeRabbit
New Features
Bug Fixes