Move loading logic into GlideModifier

Breaking change - this will add the default transformation to GlideSubcomposition where previously there wasn't one. Moving more logic into GlideNode simplifies GlideImage a bit and it moves us closer to exposing a Modifier based API.
bumptech · Oct 2, 2023 · 582077d · 582077d
1 parent 9eb82a3
commit 582077d
Show file tree

Hide file tree

Showing 3 changed files with 105 additions and 84 deletions.
diff --git a/...se/src/androidTest/java/com/bumptech/glide/integration/compose/GlideSubcompositionTest.kt b/...se/src/androidTest/java/com/bumptech/glide/integration/compose/GlideSubcompositionTest.kt
@@ -165,7 +165,14 @@ class GlideSubcompositionTest {
     val overrideSize = 50
     // TODO: Compose always uses the generic paths to load models, so it skips options that are
     // set by default by Glide's various class specific .load() method overrides.
-    val future = Glide.with(context).load(resourceId as Any).override(overrideSize).submit()
+    val future =
+      Glide
+        .with(context)
+        .load(resourceId as Any)
+        .override(overrideSize)
+        // Match the default transformation in Compose
+        .centerInside()
+        .submit()
     glideComposeRule.waitForIdle()
     future.get()
     glideComposeRule.setContent {

diff --git a/integration/compose/src/main/java/com/bumptech/glide/integration/compose/GlideImage.kt b/integration/compose/src/main/java/com/bumptech/glide/integration/compose/GlideImage.kt
@@ -20,9 +20,7 @@ import androidx.compose.ui.layout.Layout
 import androidx.compose.ui.platform.LocalContext
 import androidx.compose.ui.platform.LocalInspectionMode
 import androidx.compose.ui.res.painterResource
-import com.bumptech.glide.Glide
 import com.bumptech.glide.RequestBuilder
-import com.bumptech.glide.RequestManager
 import com.bumptech.glide.load.DataSource
 
 /** Mutates and returns the given [RequestBuilder] to apply relevant options. */
@@ -76,8 +74,6 @@ public typealias RequestBuilderTransform<T> = (RequestBuilder<T>) -> RequestBuil
  * multiple are made. The transition will not be called if you use [placeholder] or [failure] with
  * the deprecated [Composable] API. See [CrossFade]
  */
-// TODO(judds): the API here is not particularly composeesque, we should consider alternatives
-// to RequestBuilder (though thumbnail() may make that a challenge).
 @ExperimentalGlideComposeApi
 @Composable
 public fun GlideImage(
@@ -88,19 +84,11 @@ public fun GlideImage(
   contentScale: ContentScale = ContentScale.Fit,
   alpha: Float = DefaultAlpha,
   colorFilter: ColorFilter? = null,
-  // TODO(judds): Consider using separate GlideImage* methods instead of sealed classes.
-  // See http://shortn/_x79pjkMZIH for an internal discussion.
   loading: Placeholder? = null,
   failure: Placeholder? = null,
   transition: Transition.Factory? = null,
-  // TODO(judds): Consider defaulting to load the model here instead of always doing so below.
   requestBuilderTransform: RequestBuilderTransform<Drawable> = { it },
 ) {
-  val context = LocalContext.current
-  val requestManager: RequestManager = remember(context) { Glide.with(context) }
-  val requestBuilder =
-    rememberRequestBuilderWithDefaults(model, requestManager, requestBuilderTransform, contentScale)
-
   // TODO(judds): It seems like we should be able to use the production paths for
   // resource / drawables as well as Composables. It's not totally clear what part of the prod code
   // isn't supported.
@@ -112,15 +100,16 @@ public fun GlideImage(
   SimpleLayout(
     modifier
       .glideNode(
-        requestBuilder,
-        contentDescription,
-        alignment,
-        contentScale,
-        alpha,
-        colorFilter,
-        transition,
-        loadingPlaceholder = loading?.painter(),
-        errorPlaceholder = failure?.painter(),
+        model = model,
+        contentDescription = contentDescription,
+        alignment = alignment,
+        contentScale = contentScale,
+        alpha = alpha,
+        colorFilter = colorFilter,
+        loading = loading,
+        failure = failure,
+        transition = transition,
+        requestBuilderTransform = requestBuilderTransform,
       )
   )
 }
@@ -228,34 +217,21 @@ public fun GlideSubcomposition(
   requestBuilderTransform: RequestBuilderTransform<Drawable> = { it },
   content: @Composable GlideSubcompositionScope.() -> Unit
 ) {
-  val requestManager: RequestManager = LocalContext.current.let { remember(it) { Glide.with(it) } }
-  val requestBuilder =
-    remember(model, requestManager, requestBuilderTransform) {
-      requestBuilderTransform(requestManager.load(model))
-    }
-
-  val requestState: MutableState<RequestState> =
-    remember(model, requestManager, requestBuilderTransform) {
-      mutableStateOf(RequestState.Loading)
-    }
-  val painter: MutableState<Painter?> = remember(model, requestManager, requestBuilderTransform) {
-    mutableStateOf(null)
-  }
-
   val requestListener: StateTrackingListener =
-    remember(model, requestManager, requestBuilderTransform) {
-      StateTrackingListener(
-        requestState,
-        painter
-      )
+    remember(LocalContext.current, model, requestBuilderTransform) {
+      StateTrackingListener()
     }
 
-  val scope = GlideSubcompositionScopeImpl(painter.value, requestState.value)
+  val scope = GlideSubcompositionScopeImpl(
+    requestListener.painter.value,
+    requestListener.state.value
+  )
 
   Box(
     modifier
       .glideNode(
-        requestBuilder,
+        model = model,
+        requestBuilderTransform = requestBuilderTransform,
         draw = false,
         requestListener = requestListener,
       )
@@ -265,10 +241,9 @@ public fun GlideSubcomposition(
 }
 
 @ExperimentalGlideComposeApi
-private class StateTrackingListener(
-  val state: MutableState<RequestState>,
-  val painter: MutableState<Painter?>
-) : RequestListener {
+private class StateTrackingListener : RequestListener {
+  val state: MutableState<RequestState> = mutableStateOf(RequestState.Loading)
+  val painter: MutableState<Painter?> = mutableStateOf(null)
 
   override fun onStateChanged(model: Any?, painter: Painter?, requestState: RequestState) {
     state.value = requestState
@@ -367,43 +342,6 @@ public sealed class Placeholder {
   }
 }
 
-
-@Composable
-private fun rememberRequestBuilderWithDefaults(
-  model: Any?,
-  requestManager: RequestManager,
-  requestBuilderTransform: RequestBuilderTransform<Drawable>,
-  contentScale: ContentScale
-) =
-  remember(model, requestManager, requestBuilderTransform, contentScale) {
-    requestBuilderTransform(requestManager.load(model).contentScaleTransform(contentScale))
-  }
-
-private fun RequestBuilder<Drawable>.contentScaleTransform(
-  contentScale: ContentScale
-): RequestBuilder<Drawable> {
-  return when (contentScale) {
-    ContentScale.Crop -> {
-      optionalCenterCrop()
-    }
-
-    ContentScale.Inside,
-    ContentScale.Fit -> {
-      // Outside compose, glide would use fitCenter() for FIT. But that's probably not a good
-      // decision given how unimportant Bitmap re-use is relative to minimizing texture sizes now.
-      // So instead we'll do something different and prefer not to upscale, which means using
-      // centerInside(). The UI can still scale the view even if the Bitmap is smaller.
-      optionalCenterInside()
-    }
-
-    else -> {
-      this
-    }
-  }
-  // TODO(judds): Think about how to handle the various fills
-}
-
-
 @Composable
 private fun SimpleLayout(
   modifier: Modifier,

diff --git a/integration/compose/src/main/java/com/bumptech/glide/integration/compose/GlideModifier.kt b/integration/compose/src/main/java/com/bumptech/glide/integration/compose/GlideModifier.kt
@@ -5,9 +5,12 @@ import android.graphics.drawable.Animatable
 import android.graphics.drawable.Drawable
 import android.os.Handler
 import android.os.Looper
+import androidx.compose.runtime.Composable
+import androidx.compose.runtime.remember
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.ExperimentalComposeUiApi
 import androidx.compose.ui.Modifier
+import androidx.compose.ui.composed
 import androidx.compose.ui.draw.clipToBounds
 import androidx.compose.ui.geometry.Size
 import androidx.compose.ui.geometry.isSpecified
@@ -32,6 +35,7 @@ import androidx.compose.ui.node.SemanticsModifierNode
 import androidx.compose.ui.node.invalidateDraw
 import androidx.compose.ui.node.invalidateMeasurement
 import androidx.compose.ui.platform.InspectorInfo
+import androidx.compose.ui.platform.LocalContext
 import androidx.compose.ui.semantics.Role
 import androidx.compose.ui.semantics.SemanticsPropertyKey
 import androidx.compose.ui.semantics.SemanticsPropertyReceiver
@@ -43,7 +47,9 @@ import androidx.compose.ui.unit.IntOffset
 import androidx.compose.ui.unit.IntSize
 import androidx.compose.ui.unit.constrainHeight
 import androidx.compose.ui.unit.constrainWidth
+import com.bumptech.glide.Glide
 import com.bumptech.glide.RequestBuilder
+import com.bumptech.glide.RequestManager
 import com.bumptech.glide.integration.ktx.AsyncGlideSize
 import com.bumptech.glide.integration.ktx.ExperimentGlideFlows
 import com.bumptech.glide.integration.ktx.ImmediateGlideSize
@@ -68,6 +74,76 @@ internal interface RequestListener {
   fun onStateChanged(model: Any?, painter: Painter?, requestState: RequestState)
 }
 
+@ExperimentalGlideComposeApi
+internal fun Modifier.glideNode(
+  model: Any?,
+  contentDescription: String? = null,
+  alignment: Alignment = Alignment.Center,
+  contentScale: ContentScale = ContentScale.Fit,
+  alpha: Float = DefaultAlpha,
+  colorFilter: ColorFilter? = null,
+  draw: Boolean? = true,
+  loading: com.bumptech.glide.integration.compose.Placeholder? = null,
+  failure: com.bumptech.glide.integration.compose.Placeholder? = null,
+  transition: Transition.Factory? = null,
+  requestListener: RequestListener? = null,
+  requestBuilderTransform: RequestBuilderTransform<Drawable> = { it },
+) = composed {
+  val context = LocalContext.current
+  val requestManager: RequestManager = remember(context) { Glide.with(context) }
+  val requestBuilder =
+    rememberRequestBuilderWithDefaults(model, requestManager, requestBuilderTransform, contentScale)
+
+  glideNode(
+    requestBuilder,
+    contentDescription,
+    alignment,
+    contentScale,
+    alpha,
+    colorFilter,
+    transition,
+    requestListener,
+    loadingPlaceholder = loading?.painter(),
+    errorPlaceholder = failure?.painter(),
+    draw = draw,
+  )
+}
+
+@Composable
+internal fun rememberRequestBuilderWithDefaults(
+  model: Any?,
+  requestManager: RequestManager,
+  requestBuilderTransform: RequestBuilderTransform<Drawable>,
+  contentScale: ContentScale
+) =
+  remember(model, requestManager, requestBuilderTransform, contentScale) {
+    requestBuilderTransform(requestManager.load(model).contentScaleTransform(contentScale))
+  }
+
+private fun RequestBuilder<Drawable>.contentScaleTransform(
+  contentScale: ContentScale
+): RequestBuilder<Drawable> {
+  return when (contentScale) {
+    ContentScale.Crop -> {
+      optionalCenterCrop()
+    }
+
+    ContentScale.Inside,
+    ContentScale.Fit -> {
+      // Outside compose, glide would use fitCenter() for FIT. But that's probably not a good
+      // decision given how unimportant Bitmap re-use is relative to minimizing texture sizes now.
+      // So instead we'll do something different and prefer not to upscale, which means using
+      // centerInside(). The UI can still scale the view even if the Bitmap is smaller.
+      optionalCenterInside()
+    }
+
+    else -> {
+      this
+    }
+  }
+  // TODO(judds): Think about how to handle the various fills
+}
+
 @ExperimentalGlideComposeApi
 internal fun Modifier.glideNode(
   requestBuilder: RequestBuilder<Drawable>,