Skip to content

Commit b9b27fb

Browse files
gamepro65gamepro65
authored
Make PicassoPainter's request aware of state read changes (square#2432)
1 parent 9e87a42 commit b9b27fb

File tree

6 files changed

+313
-7
lines changed

6 files changed

+313
-7
lines changed

‎gradle/libs.versions.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ composeRuntime = { module = 'androidx.compose.runtime:runtime', version.ref = 'c
3030
composeUi-foundation = { module = 'androidx.compose.foundation:foundation', version.ref = 'composeUi' }
3131
composeUi-material = { module = 'androidx.compose.material:material', version.ref = 'composeUi' }
3232
composeUi-uiTooling = { module = 'androidx.compose.ui:ui-tooling', version.ref = 'composeUi' }
33+
composeUi-test = { module = 'androidx.compose.ui:ui-test-junit4', version.ref = 'composeUi' }
34+
composeUi-testManifest = { module = 'androidx.compose.ui:ui-test-manifest', version.ref = 'composeUi' }
3335

3436
drawablePainter = { module = 'com.google.accompanist:accompanist-drawablepainter', version = '0.30.1' }
3537

‎picasso-compose/build.gradle

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ android {
99

1010
defaultConfig {
1111
minSdkVersion libs.versions.minSdk.get() as int
12+
13+
testInstrumentationRunner 'androidx.test.runner.AndroidJUnitRunner'
1214
}
1315

1416
buildFeatures {
@@ -41,6 +43,12 @@ dependencies {
4143
implementation libs.drawablePainter
4244
implementation libs.composeUi
4345
implementation libs.composeUi.foundation
46+
implementation libs.composeRuntime
47+
48+
debugImplementation libs.composeUi.testManifest
49+
50+
androidTestImplementation libs.composeUi.test
51+
androidTestImplementation libs.truth
4452

4553
compileOnly libs.androidx.annotations
4654
}

‎picasso-compose/src/androidTest/java/com/squareup/picasso3/compose/PicassoPainterTest.kt

Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,197 @@
1+
/*
2+
* Copyright (C) 2013 Square, Inc.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package com.squareup.picasso3.compose
17+
18+
import android.graphics.Bitmap
19+
import android.graphics.Bitmap.Config.ARGB_8888
20+
import androidx.compose.foundation.Canvas
21+
import androidx.compose.foundation.layout.fillMaxSize
22+
import androidx.compose.foundation.layout.requiredSize
23+
import androidx.compose.runtime.CompositionLocalProvider
24+
import androidx.compose.runtime.getValue
25+
import androidx.compose.runtime.mutableStateOf
26+
import androidx.compose.runtime.setValue
27+
import androidx.compose.ui.Modifier
28+
import androidx.compose.ui.layout.onSizeChanged
29+
import androidx.compose.ui.platform.LocalDensity
30+
import androidx.compose.ui.test.junit4.createComposeRule
31+
import androidx.compose.ui.unit.Density
32+
import androidx.compose.ui.unit.IntSize
33+
import androidx.compose.ui.unit.dp
34+
import androidx.test.ext.junit.runners.AndroidJUnit4
35+
import androidx.test.platform.app.InstrumentationRegistry
36+
import com.google.common.truth.Truth.assertThat
37+
import com.squareup.picasso3.Picasso
38+
import com.squareup.picasso3.Picasso.LoadedFrom
39+
import com.squareup.picasso3.Request
40+
import com.squareup.picasso3.RequestHandler
41+
import org.junit.Rule
42+
import org.junit.Test
43+
import org.junit.runner.RunWith
44+
import kotlinx.coroutines.Dispatchers
45+
46+
@RunWith(AndroidJUnit4::class)
47+
class PicassoPainterTest {
48+
49+
@get:Rule
50+
val rule = createComposeRule()
51+
52+
@Test
53+
fun firstFrameConsumesStateFromLayout() {
54+
lateinit var lastRequest: Request
55+
val context = InstrumentationRegistry.getInstrumentation().targetContext
56+
57+
val picasso = Picasso.Builder(context)
58+
.callFactory { throw RuntimeException() }
59+
.dispatchers(Dispatchers.Unconfined, Dispatchers.Unconfined)
60+
.addRequestHandler(object : RequestHandler() {
61+
override fun canHandleRequest(data: Request): Boolean = true
62+
override fun load(picasso: Picasso, request: Request, callback: Callback) {
63+
lastRequest = request
64+
callback.onSuccess(Result.Bitmap(Bitmap.createBitmap(1, 1, ARGB_8888), LoadedFrom.MEMORY))
65+
}
66+
})
67+
.build()
68+
var size: IntSize by mutableStateOf(IntSize.Zero)
69+
var drawn = false
70+
71+
rule.setContent {
72+
CompositionLocalProvider(LocalDensity provides Density(1f)) {
73+
val painter = picasso.rememberPainter {
74+
it.load("http://example.com/")
75+
// Headers are not part of a cache key, using a stable key to break cache
76+
.stableKey("http://example.com/$size")
77+
.addHeader("width", size.width.toString())
78+
.addHeader("height", size.height.toString())
79+
}
80+
Canvas(
81+
Modifier
82+
.requiredSize(9.dp)
83+
.onSizeChanged { size = it }
84+
) {
85+
val canvasSize = this.size
86+
87+
with(painter) {
88+
draw(canvasSize)
89+
}
90+
drawn = true
91+
}
92+
}
93+
}
94+
95+
rule.waitUntil { drawn }
96+
97+
// Draw triggers request was made with size.
98+
assertThat(lastRequest.headers?.toMultimap()).containsAtLeastEntriesIn(
99+
mapOf(
100+
"width" to listOf("9"),
101+
"height" to listOf("9")
102+
)
103+
)
104+
}
105+
106+
@Test
107+
fun redrawDoesNotReexecuteUnchangedRequest() {
108+
var requestCount = 0
109+
val context = InstrumentationRegistry.getInstrumentation().targetContext
110+
val picasso = Picasso.Builder(context)
111+
.callFactory { throw RuntimeException() }
112+
.dispatchers(Dispatchers.Unconfined, Dispatchers.Unconfined)
113+
.addRequestHandler(object : RequestHandler() {
114+
override fun canHandleRequest(data: Request): Boolean = true
115+
override fun load(picasso: Picasso, request: Request, callback: Callback) {
116+
requestCount++
117+
callback.onSuccess(Result.Bitmap(Bitmap.createBitmap(1, 1, ARGB_8888), LoadedFrom.MEMORY))
118+
}
119+
})
120+
.build()
121+
122+
var drawInvalidator by mutableStateOf(0)
123+
var drawCount = 0
124+
rule.setContent {
125+
CompositionLocalProvider(LocalDensity provides Density(1f)) {
126+
val painter = picasso.rememberPainter {
127+
it.load("http://example.com/")
128+
}
129+
Canvas(Modifier.fillMaxSize()) {
130+
drawCount++
131+
drawInvalidator = 1
132+
133+
val canvasSize = this.size
134+
with(painter) {
135+
draw(canvasSize)
136+
}
137+
}
138+
}
139+
}
140+
141+
rule.waitUntil { drawCount == 2 }
142+
assertThat(requestCount).isEqualTo(1)
143+
}
144+
145+
@Test
146+
fun newRequestLoaded_whenRequestDependenciesChangedAfterFirstFrame() {
147+
var lastRequest: Request? = null
148+
val context = InstrumentationRegistry.getInstrumentation().targetContext
149+
val picasso = Picasso.Builder(context)
150+
.callFactory { throw RuntimeException() }
151+
.dispatchers(Dispatchers.Unconfined, Dispatchers.Unconfined)
152+
.addRequestHandler(object : RequestHandler() {
153+
override fun canHandleRequest(data: Request): Boolean = true
154+
override fun load(picasso: Picasso, request: Request, callback: Callback) {
155+
lastRequest = request
156+
callback.onSuccess(Result.Bitmap(Bitmap.createBitmap(1, 1, ARGB_8888), LoadedFrom.MEMORY))
157+
}
158+
})
159+
.build()
160+
var testHeader by mutableStateOf("one")
161+
162+
rule.setContent {
163+
CompositionLocalProvider(LocalDensity provides Density(1f)) {
164+
val painter = picasso.rememberPainter {
165+
it.load("http://example.com/")
166+
// Headers are not part of a cache key, using a stable key to break cache
167+
.stableKey("http://example.com/$testHeader")
168+
.addHeader("testHeader", testHeader)
169+
}
170+
Canvas(Modifier.fillMaxSize()) {
171+
val canvasSize = this.size
172+
173+
with(painter) {
174+
draw(canvasSize)
175+
}
176+
}
177+
}
178+
}
179+
180+
rule.waitUntil { lastRequest != null }
181+
assertThat(lastRequest!!.headers?.get("testHeader")).isEqualTo("one")
182+
183+
var currentRequest = lastRequest
184+
testHeader = "two"
185+
186+
// On API 21 runOnIdle runs before the composition recomposes :-(
187+
// Waiting until the request updates, then asserting
188+
rule.waitUntil { currentRequest != lastRequest }
189+
assertThat(lastRequest!!.headers?.get("testHeader")).isEqualTo("two")
190+
191+
currentRequest = lastRequest
192+
testHeader = "three"
193+
194+
rule.waitUntil { currentRequest != lastRequest }
195+
assertThat(lastRequest!!.headers?.get("testHeader")).isEqualTo("three")
196+
}
197+
}

‎picasso-compose/src/main/java/com/squareup/picasso3/compose/PicassoPainter.kt

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,12 @@ package com.squareup.picasso3.compose
1818
import android.graphics.drawable.Drawable
1919
import androidx.compose.runtime.Composable
2020
import androidx.compose.runtime.RememberObserver
21+
import androidx.compose.runtime.derivedStateOf
2122
import androidx.compose.runtime.getValue
2223
import androidx.compose.runtime.mutableStateOf
2324
import androidx.compose.runtime.remember
2425
import androidx.compose.runtime.setValue
26+
import androidx.compose.runtime.snapshots.Snapshot
2527
import androidx.compose.ui.geometry.Size
2628
import androidx.compose.ui.graphics.ColorFilter
2729
import androidx.compose.ui.graphics.DefaultAlpha
@@ -48,12 +50,19 @@ internal class PicassoPainter(
4850
private val onError: ((Exception) -> Unit)? = null
4951
) : Painter(), RememberObserver, DrawableTarget {
5052

53+
private var lastRequestCreator: RequestCreator? by mutableStateOf(null)
54+
private val requestCreator: RequestCreator by derivedStateOf { request(picasso) }
5155
private var painter: Painter by mutableStateOf(EmptyPainter)
5256
private var alpha: Float by mutableStateOf(DefaultAlpha)
5357
private var colorFilter: ColorFilter? by mutableStateOf(null)
5458

5559
override val intrinsicSize: Size
56-
get() = painter.intrinsicSize
60+
get() {
61+
// Make sure we're using the latest request. If the request function reads any state, it will
62+
// invalidate whatever scope this property is being read from.
63+
load()
64+
return painter.intrinsicSize
65+
}
5766

5867
override fun applyAlpha(alpha: Float): Boolean {
5968
this.alpha = alpha
@@ -66,13 +75,18 @@ internal class PicassoPainter(
6675
}
6776

6877
override fun DrawScope.onDraw() {
78+
// Make sure we're using the latest request. If the request function reads any state, it will
79+
// invalidate this draw scope when it changes.
80+
load()
6981
with(painter) {
7082
draw(size, alpha, colorFilter)
7183
}
7284
}
7385

7486
override fun onRemembered() {
75-
request.invoke(picasso).into(this)
87+
// This is called from composition, but if the request provider function reads any state we
88+
// don't want that to invalidate composition. It will invalidate draw, later.
89+
Snapshot.withoutReadObservation { load() }
7690
}
7791

7892
override fun onAbandoned() {
@@ -100,6 +114,22 @@ internal class PicassoPainter(
100114
errorDrawable?.let(::setPainter)
101115
}
102116

117+
private fun load() {
118+
// This derived state read will return the same instance of RequestCreator if one has been
119+
// cached and none of the state dependencies have since changed.
120+
val requestCreator = requestCreator
121+
// lastRequestCreator is just used for diffing, we don't want it to invalidate anything.
122+
val lastRequestCreator = Snapshot.withoutReadObservation { lastRequestCreator }
123+
124+
// Only launch a new request if anything has actually changed. RequestCreator does not
125+
// currently implement an equals method, relying here on reference equality, future improvement
126+
// will be to implement equals which can prevent further re-requests.
127+
if (requestCreator != lastRequestCreator) {
128+
this.lastRequestCreator = requestCreator
129+
requestCreator.into(this)
130+
}
131+
}
132+
103133
private fun setPainter(drawable: Drawable) {
104134
(painter as? RememberObserver)?.onForgotten()
105135
painter = DrawablePainter(drawable).apply(DrawablePainter::onRemembered)

‎picasso/src/main/java/com/squareup/picasso3/InternalCoroutineDispatcher.kt

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,12 @@ import android.content.Context
1919
import android.net.NetworkInfo
2020
import android.os.Handler
2121
import com.squareup.picasso3.Dispatcher.Companion.RETRY_DELAY
22+
import com.squareup.picasso3.Picasso.Priority.HIGH
23+
import com.squareup.picasso3.Utils.OWNER_DISPATCHER
24+
import com.squareup.picasso3.Utils.VERB_CANCELED
25+
import com.squareup.picasso3.Utils.log
2226
import kotlin.coroutines.CoroutineContext
27+
import kotlin.coroutines.EmptyCoroutineContext
2328
import kotlinx.coroutines.CoroutineName
2429
import kotlinx.coroutines.CoroutineScope
2530
import kotlinx.coroutines.ExperimentalCoroutinesApi
@@ -127,8 +132,22 @@ internal class InternalCoroutineDispatcher internal constructor(
127132
}
128133

129134
override fun dispatchSubmit(hunter: BitmapHunter) {
130-
hunter.job = scope.launch(CoroutineName(hunter.getName())) {
131-
hunter.run()
135+
val highPriority = hunter.action?.request?.priority == HIGH
136+
val context = if (highPriority) EmptyCoroutineContext else mainContext
137+
138+
scope.launch(context) {
139+
channel.trySend {
140+
if (hunter.action != null) {
141+
hunter.job = scope.launch(CoroutineName(hunter.getName())) {
142+
hunter.run()
143+
}
144+
} else {
145+
hunterMap.remove(hunter.key)
146+
if (hunter.picasso.isLoggingEnabled) {
147+
log(OWNER_DISPATCHER, VERB_CANCELED, hunter.key)
148+
}
149+
}
150+
}
132151
}
133152
}
134153

0 commit comments

Comments
 (0)