refactor(mediaviewer): replace android-gif-drawable with Coil - #6785
rapterjet2004 wants to merge 1 commit into
Conversation
GIFs in the media viewer are now decoded by the app's existing Coil ImageLoader (ImageDecoderDecoder on API 28+, GifDecoder below) and shown in the same zoomable view as other images. This drops the native pl.droidsonroids.gif dependency and its verification metadata. Assisted-by: Claude Code:claude-opus-5-5 Signed-off-by: rapterjet2004 <juliuslinus1@gmail.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe media viewer no longer uses a separate GIF rendering path. Non-video media uses ImagePage, which starts decoded drawables that implement Animatable and stops the displayed drawable when the PhotoView is released. The android-gif-drawable dependency and its trusted-key and checksum records are removed. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to Prefetched animated images can continue using device resources before they are visible. The impact is bounded, but playback should be limited to the current page. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to GIFs move to the app’s existing image loader, and animation now starts for other supported image formats too. Playback is stopped when its view is released, but it is not limited to the currently selected page. The plausible impact is extra work on the viewer’s device, not a demonstrated access-control bypass. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 8cff50d7-75b4-414e-b0ea-b4d8c010ca2b
📒 Files selected for processing (3)
app/build.gradle.ktsapp/src/main/java/com/nextcloud/talk/mediaviewer/activities/MediaViewerScreen.ktgradle/verification-metadata.xml
💤 Files with no reviewable changes (2)
- app/build.gradle.kts
- gradle/verification-metadata.xml
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
| update = { photoView -> | ||
| drawable?.let { | ||
| photoView.setImageDrawable(it) | ||
| // Animated images (GIF, animated WebP/HEIF) decoded by Coil are not started automatically | ||
| (it as? Animatable)?.start() | ||
| } | ||
| }, | ||
| onRelease = { photoView -> (photoView.drawable as? Animatable)?.stop() }, |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '270,390p' app/src/main/java/com/nextcloud/talk/mediaviewer/activities/MediaViewerScreen.kt
rg -n 'HorizontalPager|PagerState|currentPage|beyondViewportPageCount|ImagePage\\(' app/src/main/java/com/nextcloud/talk/mediaviewerRepository: nextcloud/talk-android
Length of output: 4472
🏁 Script executed:
#!/bin/bash
set -e
sed -n '1,280p' app/src/main/java/com/nextcloud/talk/mediaviewer/activities/MediaViewerScreen.kt
printf '\n--- relevant usages ---\n'
rg -n -e 'HorizontalPager' -e 'PagerState' -e 'currentPage' -e 'beyondViewportPageCount' -e 'ImagePage' app/src/main/java/com/nextcloud/talk/mediaviewer
printf '\n--- PR diff for target file ---\n'
git diff --unified=30 f704b95fba3c9ac6b3136df59863a011a760e7aa 4ee468d7ebe069c815b97c05d8bac24db96ded53 -- app/src/main/java/com/nextcloud/talk/mediaviewer/activities/MediaViewerScreen.ktRepository: nextcloud/talk-android
Length of output: 25442
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Compose dependency declarations ---'
rg -n -i -e 'compose' -e 'foundation' -e 'androidx.compose' --glob '*.gradle' --glob '*.gradle.kts' --glob 'libs.versions.toml' --glob 'gradle.properties' .
printf '%s\n' '--- local AndroidX or pager source candidates ---'
find . -path '*/build' -prune -o -type f \( -iname '*Pager*.kt' -o -iname '*Pager*.java' \) -print | head -80
printf '%s\n' '--- comparable pager lifecycle usage ---'
rg -n -C 3 -e 'onRelease\s*=' -e 'beyondViewportPageCount' -e 'HorizontalPager' --glob '*.kt' .Repository: nextcloud/talk-android
Length of output: 9187
🌐 Web query:
official AndroidX Compose Foundation HorizontalPager default beyondViewportPageCount composition offscreen pages onRelease
💡 Result:
For AndroidX Compose Foundation `HorizontalPager`, **the default `beyondViewportPageCount` is `0`** (`PagerDefaults.BeyondViewportPageCount`). That means it doesn’t deliberately compose and lay out extra pages before or after the visible pages. ([developer.android.com](https://developer.android.com/reference/kotlin/androidx/compose/foundation/pager/PagerDefaults?utm_source=openai))
It may still compose pages ahead during scrolling: the pager’s prefetcher can compose and lay out pages in the scroll direction independently of that setting. ([developer.android.com](https://developer.android.com/reference/kotlin/androidx/compose/foundation/pager/PagerDefaults?utm_source=openai))
If by “onRelease” you mean what happens to offscreen pages after a gesture ends: `beyondViewportPageCount` isn’t an offscreen-retention limit. Pager is lazy, so pages outside the visible/beyond-viewport range may leave composition; prefetching during scrolling is a separate behavior. ([developer.android.com](https://developer.android.com/reference/kotlin/androidx/compose/foundation/pager/HorizontalPager.composable?utm_source=openai))
Citations:
- 1: https://developer.android.com/reference/kotlin/androidx/compose/foundation/pager/PagerDefaults?utm_source=openai
- 2: https://developer.android.com/reference/kotlin/androidx/compose/foundation/pager/PagerDefaults?utm_source=openai
- 3: https://developer.android.com/reference/kotlin/androidx/compose/foundation/pager/HorizontalPager.composable?utm_source=openai
Stop animated drawables when the page is not current.
HorizontalPager can compose a page during prefetch before that page is visible. ImagePage starts every decoded Animatable regardless of isCurrentPage, so a prefetched animated image can consume animation resources until AndroidView.onRelease removes the page.
Pass the existing activity flag to ImagePage and stop the drawable when the page is not current.
Suggested fix
- else -> ImagePage(localPath = localPath, onToggleControls = onToggleControls)
+ else -> ImagePage(
+ localPath = localPath,
+ isCurrentPage = isCurrentPage,
+ onToggleControls = onToggleControls
+ )
...
-private fun ImagePage(localPath: String, onToggleControls: () -> Unit) {
+private fun ImagePage(
+ localPath: String,
+ isCurrentPage: Boolean,
+ onToggleControls: () -> Unit
+) {
...
- (it as? Animatable)?.start()
+ (it as? Animatable)?.let { animatable ->
+ if (isCurrentPage) animatable.start() else animatable.stop()
+ }
📱 QA build
The QA build installs alongside a released Nextcloud app, so you can keep Downloading the file requires a GitHub account, so open this link on the |
1 participant
GIFs in the media viewer are now decoded by the app's existing Coil ImageLoader (ImageDecoderDecoder on API 28+, GifDecoder below) and shown in the same zoomable view as other images. This drops the native pl.droidsonroids.gif dependency and its verification metadata.
Assisted-by: Claude Code:claude-opus-5-5
🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)