Skip to content

Commit

Permalink
Remove GlidePainter in favor of Modifier nodes / Flows
Browse files Browse the repository at this point in the history
Using a custom Modifier node instead of a Painter follows a
recommendation from the Compose team. It will allow us or
library users to compose Glide's modifier in the future for animations
of other effects.  For now we do not actually expose the Modifier
directly.

This change adds a GlideSubcomposition that uses' Glide's flows
integration to allow subcomposition based on image load state.
Subcomposition allows us to deprecate the expensive Composable
placeholder variants and also allows users to implement complex layouts
or animations.

In a subsequent change we'll explore adding a default crossfade and/or
exposing the Glide modifier node so that users of the library can
compose modifiers themselves.
  • Loading branch information
sjudd committed Aug 7, 2023
1 parent fd96de2 commit 8f5321f
Show file tree
Hide file tree
Showing 27 changed files with 851 additions and 302 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import android.app.Application;
import android.graphics.Bitmap;
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import androidx.annotation.RawRes;
import androidx.benchmark.BenchmarkState;
Expand Down Expand Up @@ -111,17 +112,17 @@ private void loadImageWithExpectedDataSource(
public boolean onLoadFailed(
@Nullable GlideException e,
Object model,
Target<Bitmap> target,
@NonNull Target<Bitmap> target,
boolean isFirstResource) {
return false;
}

@Override
public boolean onResourceReady(
Bitmap resource,
Object model,
@NonNull Bitmap resource,
@NonNull Object model,
Target<Bitmap> target,
DataSource dataSource,
@NonNull DataSource dataSource,
boolean isFirstResource) {
dataSourceRef.set(dataSource);
return false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,17 +68,17 @@ public void thumbnail_onResourceReady_forPrimary_isComplete_whenRequestListenerI
public boolean onLoadFailed(
@Nullable GlideException e,
Object model,
Target<Drawable> target,
@NonNull Target<Drawable> target,
boolean isFirstResource) {
return false;
}

@Override
public boolean onResourceReady(
Drawable resource,
Object model,
@NonNull Drawable resource,
@NonNull Object model,
Target<Drawable> target,
DataSource dataSource,
@NonNull DataSource dataSource,
boolean isFirstResource) {
isPrimaryRequestComplete.set(target.getRequest().isComplete());
countDownLatch.countDown();
Expand Down Expand Up @@ -115,7 +115,7 @@ public void thumbnail_onLoadFailed_forPrimary_isNotRunningOrComplete_whenRequest
public boolean onLoadFailed(
@Nullable GlideException e,
Object model,
Target<Drawable> target,
@NonNull Target<Drawable> target,
boolean isFirstResource) {
Request request = target.getRequest();
isNeitherRunningNorComplete.set(!request.isComplete() && !request.isRunning());
Expand All @@ -125,10 +125,10 @@ public boolean onLoadFailed(

@Override
public boolean onResourceReady(
Drawable resource,
Object model,
@NonNull Drawable resource,
@NonNull Object model,
Target<Drawable> target,
DataSource dataSource,
@NonNull DataSource dataSource,
boolean isFirstResource) {
return false;
}
Expand Down
32 changes: 32 additions & 0 deletions integration/compose/api/compose.api
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ public abstract interface annotation class com/bumptech/glide/integration/compos

public final class com/bumptech/glide/integration/compose/GlideImageKt {
public static final fun GlideImage (Ljava/lang/Object;Ljava/lang/String;Landroidx/compose/ui/Modifier;Landroidx/compose/ui/Alignment;Landroidx/compose/ui/layout/ContentScale;FLandroidx/compose/ui/graphics/ColorFilter;Lcom/bumptech/glide/integration/compose/Placeholder;Lcom/bumptech/glide/integration/compose/Placeholder;Lkotlin/jvm/functions/Function1;Landroidx/compose/runtime/Composer;II)V
public static final fun GlideSubcomposition (Ljava/lang/Object;Landroidx/compose/ui/Modifier;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function3;Landroidx/compose/runtime/Composer;II)V
public static final fun placeholder (I)Lcom/bumptech/glide/integration/compose/Placeholder;
public static final fun placeholder (Landroid/graphics/drawable/Drawable;)Lcom/bumptech/glide/integration/compose/Placeholder;
public static final fun placeholder (Lkotlin/jvm/functions/Function2;)Lcom/bumptech/glide/integration/compose/Placeholder;
Expand All @@ -13,6 +14,11 @@ public abstract interface class com/bumptech/glide/integration/compose/GlidePrel
public abstract fun getSize ()I
}

public abstract interface class com/bumptech/glide/integration/compose/GlideSubcompositionScope {
public abstract fun getPainter ()Landroidx/compose/ui/graphics/painter/Painter;
public abstract fun getState ()Lcom/bumptech/glide/integration/compose/RequestState;
}

public abstract class com/bumptech/glide/integration/compose/Placeholder {
public static final field $stable I
}
Expand All @@ -22,3 +28,29 @@ public final class com/bumptech/glide/integration/compose/PreloadKt {
public static final fun rememberGlidePreloadingData-u6VnWhU (ILkotlin/jvm/functions/Function1;JILjava/lang/Integer;Lkotlin/jvm/functions/Function2;Landroidx/compose/runtime/Composer;II)Lcom/bumptech/glide/integration/compose/GlidePreloadingData;
}

public abstract class com/bumptech/glide/integration/compose/RequestState {
public static final field $stable I
}

public final class com/bumptech/glide/integration/compose/RequestState$Failure : com/bumptech/glide/integration/compose/RequestState {
public static final field $stable I
public static final field INSTANCE Lcom/bumptech/glide/integration/compose/RequestState$Failure;
}

public final class com/bumptech/glide/integration/compose/RequestState$Loading : com/bumptech/glide/integration/compose/RequestState {
public static final field $stable I
public static final field INSTANCE Lcom/bumptech/glide/integration/compose/RequestState$Loading;
}

public final class com/bumptech/glide/integration/compose/RequestState$Success : com/bumptech/glide/integration/compose/RequestState {
public static final field $stable I
public fun <init> (Lcom/bumptech/glide/load/DataSource;)V
public final fun component1 ()Lcom/bumptech/glide/load/DataSource;
public final fun copy (Lcom/bumptech/glide/load/DataSource;)Lcom/bumptech/glide/integration/compose/RequestState$Success;
public static synthetic fun copy$default (Lcom/bumptech/glide/integration/compose/RequestState$Success;Lcom/bumptech/glide/load/DataSource;ILjava/lang/Object;)Lcom/bumptech/glide/integration/compose/RequestState$Success;
public fun equals (Ljava/lang/Object;)Z
public final fun getDataSource ()Lcom/bumptech/glide/load/DataSource;
public fun hashCode ()I
public fun toString ()Ljava/lang/String;
}

4 changes: 2 additions & 2 deletions integration/compose/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ plugins {
}

android {
compileSdk 33
compileSdk 34

defaultConfig {
minSdk 21
targetSdk 33
targetSdk 34

testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner"
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
@file:OptIn(ExperimentalGlideComposeApi::class, ExperimentalCoroutinesApi::class)

package com.bumptech.glide.integration.compose

import android.graphics.Canvas
Expand All @@ -16,7 +14,6 @@ import com.bumptech.glide.integration.compose.test.Constants
import com.bumptech.glide.integration.compose.test.GlideComposeRule
import com.bumptech.glide.integration.compose.test.assertDisplaysInstance
import com.bumptech.glide.integration.compose.test.onNodeWithDefaultContentDescription
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.test.runTest
import org.junit.Rule
import org.junit.Test
Expand All @@ -28,6 +25,7 @@ import org.junit.runners.Parameterized
*
* Transformable types are tested in [GlideImageDefaultTransformationTest].
*/
@OptIn(ExperimentalGlideComposeApi::class)
@RunWith(Parameterized::class)
class GlideImageCustomDrawableTransformationTest(
private val contentScale: ContentScale,
Expand Down Expand Up @@ -98,8 +96,8 @@ class GlideImageCustomDrawableTransformationTest(
@Suppress("DeprecatedCallableAddReplaceWith")
private open class FakeDrawable : Drawable() {
override fun draw(p0: Canvas) {}
override fun setAlpha(p0: Int) = throw UnsupportedOperationException()
override fun setColorFilter(p0: ColorFilter?) = throw UnsupportedOperationException()
override fun setAlpha(p0: Int) {}
override fun setColorFilter(p0: ColorFilter?) {}
@Deprecated("Deprecated in Java")
override fun getOpacity(): Int = throw UnsupportedOperationException()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@
package com.bumptech.glide.integration.compose

import android.content.Context
import android.content.res.Resources
import android.graphics.drawable.Drawable
import android.util.TypedValue
import androidx.annotation.DrawableRes
import androidx.compose.foundation.layout.size
import androidx.compose.runtime.Composable
Expand All @@ -28,7 +26,6 @@ import com.bumptech.glide.integration.ktx.ExperimentGlideFlows
import com.bumptech.glide.integration.ktx.Resource
import com.bumptech.glide.integration.ktx.Status
import com.bumptech.glide.integration.ktx.flow
import kotlin.math.roundToInt
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.test.runTest
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -295,17 +295,17 @@ class GlideImageTest {
override fun onLoadFailed(
e: GlideException?,
model: Any?,
target: Target<Drawable>?,
target: Target<Drawable>,
isFirstResource: Boolean,
): Boolean {
throw UnsupportedOperationException()
}

override fun onResourceReady(
resource: Drawable?,
model: Any?,
target: Target<Drawable>?,
dataSource: DataSource?,
resource: Drawable,
model: Any,
target: Target<Drawable>,
dataSource: DataSource,
isFirstResource: Boolean,
): Boolean {
onResourceReadyCounter.incrementAndGet()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
package com.bumptech.glide.integration.compose

import android.content.Context
import androidx.compose.foundation.Image
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.size
import androidx.compose.ui.Modifier
import androidx.compose.ui.unit.dp
import androidx.test.core.app.ApplicationProvider
import com.bumptech.glide.Glide
import com.bumptech.glide.integration.compose.test.GlideComposeRule
import com.bumptech.glide.load.DataSource
import com.google.common.truth.Truth.assertThat
import org.junit.Rule
import org.junit.Test

@OptIn(ExperimentalGlideComposeApi::class)
class GlideSubcompositionTest {
val context: Context = ApplicationProvider.getApplicationContext()

@get:Rule
val glideComposeRule = GlideComposeRule()

@Test
fun glideSubcomposition_withoutSize_startsWithStateLoading() {
var currentState: RequestState? = null
glideComposeRule.setContent {
GlideSubcomposition(model = android.R.drawable.star_big_on) {
if (currentState == null) {
currentState = state
}
}
}
assertThat(currentState).isEqualTo(RequestState.Loading)
}

@Test
fun glideSubcomposition_withOverrideSize_loadsImage() {
var currentState: RequestState? = null
glideComposeRule.setContent {
GlideSubcomposition(
android.R.drawable.star_big_on,
requestBuilderTransform = { it.override(50) }
) {
currentState = state
}
}
glideComposeRule.waitForIdle()
assertThat(currentState).isInstanceOf(RequestState.Success::class.java)
}

@Test
fun glideSubcomposition_whenDrawnWithSize_loadsImage() {
var currentState: RequestState? = null
glideComposeRule.setContent {
GlideSubcomposition(model = android.R.drawable.star_big_on) {
currentState = state
Image(
painter = painter,
contentDescription = "",
)
}
}
glideComposeRule.waitForIdle()
assertThat(currentState).isInstanceOf(RequestState.Success::class.java)
}

@Test
fun glideSubcomposition_withLayoutSize_startsWithStateLoading() {
var currentState: RequestState? = null
glideComposeRule.setContent {
GlideSubcomposition(model = android.R.drawable.star_big_on, Modifier.size(10.dp)) {
if (currentState == null) {
currentState = state
}
Image(
painter = painter,
contentDescription = "",
)
}
}
assertThat(currentState).isEqualTo(RequestState.Loading)
}

@Test
fun glideSubcomposition_withLayoutSize_appliedToBox_loadsImage() {
var currentState: RequestState? = null
glideComposeRule.setContent {
GlideSubcomposition(model = android.R.drawable.star_big_on, Modifier.size(10.dp)) {
currentState = state
Box(Modifier.size(10.dp))
}
}
glideComposeRule.waitForIdle()
assertThat(currentState).isInstanceOf(RequestState.Success::class.java)
}

@Test
fun glideSubcomposition_withOverrideSize_andInvalidImage_setsStateToFailed() {
var currentState: RequestState? = null
glideComposeRule.setContent {
GlideSubcomposition(model = 1234, requestBuilderTransform = { it.override(50) }) {
currentState = state
}
}
glideComposeRule.waitForIdle()
assertThat(currentState).isEqualTo(RequestState.Failure)
}

@Test
fun glideSubcomposition_withLayoutSize_andInvalidImage_setsStateToFailed() {
var currentState: RequestState? = null
glideComposeRule.setContent {
GlideSubcomposition(model = 1234, Modifier.size(10.dp)) {
currentState = state
}
}
glideComposeRule.waitForIdle()
assertThat(currentState).isEqualTo(RequestState.Failure)
}

@Test
fun glideSubcomposition_onLoadFromSource_setsDataSourceToSource() {
var dataSource: DataSource? = null
glideComposeRule.setContent {
GlideSubcomposition(
model = android.R.drawable.star_big_on,
requestBuilderTransform = { it.override(50) }
) {
val currentState = state
if (currentState is RequestState.Success) {
dataSource = currentState.dataSource
}
}
}
glideComposeRule.waitForIdle()
assertThat(dataSource).isEqualTo(DataSource.LOCAL)
}

@Test
fun glideSubcomposition_onLoadFromMemory_setsDataSourceToMemory() {
var dataSource: DataSource? = null
val resourceId = android.R.drawable.star_big_on
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()
glideComposeRule.waitForIdle()
future.get()
glideComposeRule.setContent {
GlideSubcomposition(
model = resourceId,
requestBuilderTransform = { it.override(overrideSize) }
) {
val currentState = state
if (currentState is RequestState.Success) {
dataSource = currentState.dataSource
}
}
}

glideComposeRule.waitForIdle()
assertThat(dataSource).isEqualTo(DataSource.MEMORY_CACHE)
}

// private fun assertThatDataSource(painter: GlidePainter?): ComparableSubject<DataSource> =
// assertThat((painter!!.state as GlidePainter.State.Success).dataSource)
}

Loading

0 comments on commit 8f5321f

Please sign in to comment.