refactor: simplify HDR request tracking per code review

- HdrRequests tracks plain headroom values instead of identity tokens (equal requests are interchangeable) and carries the window's original colorMode, replacing the separate HdrWindowState wrapper. - RequestHdrFor takes `fullscreen` and picks the headroom itself, so the two image views no longer repeat the UNCAPPED/FEED_HEADROOM choice. - ZoomableImageDialog passes imageBounds and the zoom state to DialogContent as lambdas, read only in graphicsLayer. The dialog shell no longer recomposes on every pager swipe frame (was ~60 per swipe, now 0), which was the root cause behind the attribute copy clobbering the dialog's HDR mode.
2026-10-05 19:28:25 +00:00 · 2026-09-30 12:15:32 +02:00
parent bf8e11ae1a
commit ac5936c149
5 changed files with 55 additions and 68 deletions
@@ -65,8 +65,8 @@ class HdrWindowModeTest {

        rule.setContent {
            window = getActivityWindow()
-            if (showFirst) RequestHdrFor(image, HdrRequests.FEED_HEADROOM)
-            if (showSecond) RequestHdrFor(image, HdrRequests.FEED_HEADROOM)
+            if (showFirst) RequestHdrFor(image, fullscreen = false)
+            if (showSecond) RequestHdrFor(image, fullscreen = false)
        }
        rule.waitForIdle()
        assertEquals(ActivityInfo.COLOR_MODE_HDR, window!!.colorMode)
@@ -85,7 +85,7 @@ class HdrWindowModeTest {
        var window: Window? = null
        rule.setContent {
            window = getActivityWindow()
-            RequestHdrFor(sdrImage(), HdrRequests.FEED_HEADROOM)
+            RequestHdrFor(sdrImage(), fullscreen = false)
        }
        rule.waitForIdle()
        assertEquals(ActivityInfo.COLOR_MODE_DEFAULT, window!!.colorMode)
@@ -101,7 +101,7 @@ class HdrWindowModeTest {
            activityWindow = getActivityWindow()
            Dialog(onDismissRequest = {}) {
                dialogWindow = getDialogWindow()
-                RequestHdrFor(image, HdrRequests.UNCAPPED)
+                RequestHdrFor(image, fullscreen = true)
            }
        }
        rule.waitForIdle()
@@ -118,7 +118,7 @@ class HdrWindowModeTest {

        rule.setContent {
            window = getActivityWindow()
-            repeat(count) { RequestHdrFor(image, HdrRequests.FEED_HEADROOM) }
+            repeat(count) { RequestHdrFor(image, fullscreen = false) }
        }
        rule.waitForIdle()
        rule.runOnUiThread {
@@ -38,25 +38,25 @@ import java.util.WeakHashMap
 * An Ultra HDR photo (a JPEG with a gain map) only renders brighter than SDR white while its
 * window is in [ActivityInfo.COLOR_MODE_HDR]; otherwise the platform silently draws the SDR base
 * image. The window is shared by every card in a feed, so HDR stays on until the last image
- * asking for it leaves composition.
+ * asking for it leaves composition, and then the window gets its [originalColorMode] back.
 */
-class HdrRequests {
-    private val tokens = mutableListOf<Request>()
+class HdrRequests(
+    val originalColorMode: Int,
+) {
+    private val headrooms = mutableListOf<Float>()

-    private class Request(
-        val headroom: Float,
-    )
+    val wantsHdr: Boolean get() = headrooms.isNotEmpty()

-    val wantsHdr: Boolean get() = tokens.isNotEmpty()
+    /** [UNCAPPED] if any request is uncapped or there are none, else the largest cap asked for. */
+    val headroom: Float get() = if (UNCAPPED in headrooms) UNCAPPED else headrooms.maxOrNull() ?: UNCAPPED

-    /** [UNCAPPED] if any request is uncapped, else the largest cap asked for. */
-    val headroom: Float
-        get() = if (tokens.any { it.headroom == UNCAPPED }) UNCAPPED else tokens.maxOfOrNull { it.headroom } ?: UNCAPPED
+    fun add(headroom: Float) {
+        headrooms.add(headroom)
+    }

-    fun add(headroom: Float): Any = Request(headroom).also { tokens.add(it) }
-
-    fun remove(token: Any) {
-        tokens.removeAll { it === token }
+    /** Drops one request for [headroom]; equal requests are interchangeable. */
+    fun remove(headroom: Float) {
+        headrooms.remove(headroom)
    }

    companion object {
@@ -71,23 +71,16 @@ class HdrRequests {
    }
 }

-private class HdrWindowState(
-    val originalColorMode: Int,
-) {
-    val requests = HdrRequests()
-}
-
 // Main-thread only: composition and disposal both run there.
-private val windowStates = WeakHashMap<Window, HdrWindowState>()
+private val windowRequests = WeakHashMap<Window, HdrRequests>()

 // Each setter dispatches the window attributes to the window manager even when the value is
 // unchanged, and cards scroll in and out of a feed constantly: only write what actually changed.
-private fun Window.applyHdr(state: HdrWindowState) {
-    val mode = if (state.requests.wantsHdr) ActivityInfo.COLOR_MODE_HDR else state.originalColorMode
+private fun Window.applyHdr(requests: HdrRequests) {
+    val mode = if (requests.wantsHdr) ActivityInfo.COLOR_MODE_HDR else requests.originalColorMode
    if (colorMode != mode) colorMode = mode
    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.VANILLA_ICE_CREAM) {
-        val headroom = if (state.requests.wantsHdr) state.requests.headroom else HdrRequests.UNCAPPED
-        if (desiredHdrHeadroom != headroom) desiredHdrHeadroom = headroom
+        if (desiredHdrHeadroom != requests.headroom) desiredHdrHeadroom = requests.headroom
    }
 }

@@ -96,24 +89,26 @@ fun Image.hasGainmap(): Boolean = Build.VERSION.SDK_INT >= Build.VERSION_CODES.U
 /**
 * Puts the hosting window (the dialog's own window inside a `Dialog`) into HDR mode while this
 * is in composition and [image] carries a gain map, so an Ultra HDR photo shows its highlights.
+ * A [fullscreen] image gets the display's full HDR range; a feed card is capped.
 */
@Composable
 fun RequestHdrFor(
    image: Image,
-    headroom: Float,
+    fullscreen: Boolean,
 ) {
    if (!remember(image) { image.hasGainmap() }) return
    val window = getDialogWindow() ?: getActivityWindow() ?: return
+    val headroom = if (fullscreen) HdrRequests.UNCAPPED else HdrRequests.FEED_HEADROOM

    DisposableEffect(window, headroom) {
-        val state = windowStates.getOrPut(window) { HdrWindowState(window.colorMode) }
-        val token = state.requests.add(headroom)
-        window.applyHdr(state)
+        val requests = windowRequests.getOrPut(window) { HdrRequests(window.colorMode) }
+        requests.add(headroom)
+        window.applyHdr(requests)

        onDispose {
-            state.requests.remove(token)
-            window.applyHdr(state)
-            if (!state.requests.wantsHdr) windowStates.remove(window)
+            requests.remove(headroom)
+            window.applyHdr(requests)
+            if (!requests.wantsHdr) windowRequests.remove(window)
        }
    }
 }
@@ -184,8 +184,8 @@ fun ZoomableImageDialog(
        val activityWindow = getActivityWindow()
        val dialogWindow = getDialogWindow()

-        // Keyed, not inline: this content recomposes on every frame of a pager swipe (the image
-        // bounds it reads move), and each assignment below is a window-manager round trip.
+        // Keyed, not inline: each assignment below is a window-manager round trip, so it must run
+        // on orientation changes only, not on every recomposition of this content.
        DisposableEffect(orientation, activityWindow, dialogWindow) {
            if (activityWindow != null && dialogWindow != null) {
                // Preserve what the dialog window owns: the brightness override applied by the
@@ -226,9 +226,9 @@ fun ZoomableImageDialog(
                allImages = allImages,
                imageUrl = imageUrl,
                sourceBounds = sourceBounds,
-                imageBounds = imageBounds,
+                imageBounds = { imageBounds },
                onImageBoundsChanged = updateImageBounds,
-                currentZoomState = currentZoomState,
+                currentZoomState = { currentZoomState },
                onZoomStateChanged = { currentZoomState = it },
                progress = progressProvider,
                onDismiss = dismissWithAnimation,
@@ -243,9 +243,9 @@ private fun DialogContent(
    allImages: ImmutableList<BaseMediaContent>,
    imageUrl: BaseMediaContent,
    sourceBounds: Rect?,
-    imageBounds: Rect?,
+    imageBounds: () -> Rect?,
    onImageBoundsChanged: (Rect) -> Unit,
-    currentZoomState: ZoomState?,
+    currentZoomState: () -> ZoomState?,
    onZoomStateChanged: (ZoomState) -> Unit,
    progress: () -> Float,
    onDismiss: () -> Unit,
@@ -282,12 +282,12 @@ private fun DialogContent(
                    .fillMaxSize()
                    .graphicsLayer {
                        val src = sourceBounds
-                        val img = imageBounds
+                        val img = imageBounds()
                        if (src != null && img != null && src.hasArea() && img.hasArea()) {
                            // Account for user-applied zoom: the exit animation must start from
                            // the visible bounds, not the unzoomed layout bounds — otherwise
                            // dismissing a zoomed-in image jumps.
-                            val zoomed = img.zoomedBy(currentZoomState)
+                            val zoomed = img.zoomedBy(currentZoomState())
                            // Uniform scale so non-square images keep their aspect ratio during
                            // the grow animation. The image covers the source rect; the overflow
                            // is clipped below.
@@ -458,7 +458,7 @@ fun LocalImageView(
                            SubcomposeAsyncImageContent(loadedImageModifier)

                            val image = (state as AsyncImagePainter.State.Success).result.image
-                            RequestHdrFor(image, if (fullResolution) HdrRequests.UNCAPPED else HdrRequests.FEED_HEADROOM)
+                            RequestHdrFor(image, fullscreen = fullResolution)

                            SideEffect {
                                MediaAspectRatioCache.add(content.localJavaFile.toString(), image.width, image.height)
@@ -602,7 +602,7 @@ fun UrlImageView(
                        ShowHashAnimated(content, controllerVisible, Modifier.align(Alignment.TopEnd))

                        val image = (state as AsyncImagePainter.State.Success).result.image
-                        RequestHdrFor(image, if (fullResolution) HdrRequests.UNCAPPED else HdrRequests.FEED_HEADROOM)
+                        RequestHdrFor(image, fullscreen = fullResolution)

                        SideEffect {
                            MediaAspectRatioCache.add(content.url, image.width, image.height)
@@ -26,28 +26,31 @@ import org.junit.Assert.assertTrue
 import org.junit.Test

 class HdrRequestsTest {
+    private fun requests() = HdrRequests(originalColorMode = 0)
+
    @Test
    fun noRequestsMeansSdr() {
-        val requests = HdrRequests()
+        val requests = requests()
        assertFalse(requests.wantsHdr)
+        assertEquals(HdrRequests.UNCAPPED, requests.headroom)
    }

    @Test
    fun hdrStaysOnUntilTheLastRequestLeaves() {
-        val requests = HdrRequests()
-        val first = requests.add(2f)
-        val second = requests.add(2f)
+        val requests = requests()
+        requests.add(2f)
+        requests.add(2f)

-        requests.remove(first)
+        requests.remove(2f)
        assertTrue(requests.wantsHdr)

-        requests.remove(second)
+        requests.remove(2f)
        assertFalse(requests.wantsHdr)
    }

    @Test
    fun theLargestCappedHeadroomWins() {
-        val requests = HdrRequests()
+        val requests = requests()
        requests.add(2f)
        requests.add(3f)
        assertEquals(3f, requests.headroom)
@@ -55,23 +58,12 @@ class HdrRequestsTest {

    @Test
    fun anUncappedRequestLiftsTheCap() {
-        val requests = HdrRequests()
+        val requests = requests()
        requests.add(2f)
-        val fullscreen = requests.add(HdrRequests.UNCAPPED)
+        requests.add(HdrRequests.UNCAPPED)
        assertEquals(HdrRequests.UNCAPPED, requests.headroom)

-        requests.remove(fullscreen)
+        requests.remove(HdrRequests.UNCAPPED)
        assertEquals(2f, requests.headroom)
    }
-
-    @Test
-    fun equalHeadroomsAreStillSeparateRequests() {
-        val requests = HdrRequests()
-        val first = requests.add(2f)
-        requests.add(2f)
-
-        requests.remove(first)
-        requests.remove(first)
-        assertTrue("removing one token twice must not release the other", requests.wantsHdr)
-    }
 }