-
Notifications
You must be signed in to change notification settings - Fork 6.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove GlidePainter in favor of Modifier nodes / Flows #5230
Conversation
8f5321f
to
4765d04
Compare
private var requestBuilder: RequestBuilder<Drawable>? = null | ||
private lateinit var resolvableGlideSize: ResolvableGlideSize | ||
private lateinit var contentScale: ContentScale | ||
private lateinit var alignment: Alignment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why make these lateinit? the only place you are constructing this you already have all of these values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just saves some duplicate code vs:
val glideNode = GlideNode(size, scale, alignment)
glideNode.onNewRequest(size, scale, alignment, ...)
You could remove the onNewRequest part entirely, but then you're starting a request in a constructor and refactoring that code to be less duplicative. Lesser evil really.
private val size: ResolvableGlideSize, | ||
private val contentScale: ContentScale, | ||
private val alignment: Alignment, | ||
private val colorFilter: ColorFilter?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should make sure that all of the classes you are using here have sensible equals functions. The GlideNodeElement will be reallocated on recompositions and if nothing for this image has changed you want this element to compare equals with the one created in the previous composition. It is only when these elements are not equal that update
will get called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks for pointing this out!
this.resolvableGlideSize = resolvableGlideSize | ||
this.contentScale = contentScale | ||
this.alignment = alignment | ||
this.colorFilter = colorFilter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider handling the case where alignment/colorfilter/etc change but the requestBuilder does not. In this case we can just invalidate draw or layout but not initiate an entirely new request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
is ImmediateGlideSize -> {} | ||
} | ||
val placeable = measurable.measure(constraints) | ||
return layout(placeable.width, placeable.height) { placeable.place(0, 0) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently you are not really influencing layout at all here, as you are just assuming the size of your children and placing at 0,0.
Ideally you would look at the drawable's intrinsic size or the size specified by resolvableGlideSize and take up that size here. However consumers are supposed to specify the size of the image they want, here is where you should use it. Then inside of draw you can just draw the image to take up the full amount of space allocated to it.
Additionally, I think if the user did not tell you what size they wanted, a sensible default would be to occupy the space given to you (constraints.maxWidth), and maintain aspect ratio.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is interesting. We really want to occupy the size specified by the Modifier, and ideally nothing else.
If we base the layout on the size of the image you load, then your layout is likely to shift when the image load completes, which is ugly. You could fix that by providing a specific size to Glide, but if you can do that you might as well just do it in your layout instead. Moreover you may very well want to load an image that's smaller than the layout size to save memory or reduce jank.
With all that said, can I simply rely on other modifier attributes to specify an explicit size? If I use maxWidth, will that override any sizing information provided by Modifiers earlier in the chain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably deserves a full conversation along with some examples, but.... i think there's more that you should do here than you are currently probably regardless.
I think a reasonable behavior would be:
- if fixed constraints come in you should honor that size
- if width is constrained and height is not, or vice versa, you should fill to that constrained width/height and utilize the aspect ratio of the image to further constrain the other
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've switched to using the constraint's max dimensions. I filed #5234 to follow up. I think (hope) this is pretty similar to what we had before, so ideally we can tackle more complicated layout issues in a subsequent change.
e9e2fa6
to
a5c0028
Compare
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.
a5c0028
to
85f157b
Compare
Bitmap resource, | ||
Object model, | ||
@NonNull Bitmap resource, | ||
@NonNull Object model, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sjudd Are you aware that this change could break compilation of Kotlin code?
Example fail:
https://github.com/TWiStErRob/net.twisterrob.colorfilters/actions/runs/5942409261/job/16115335917?pr=221#step:5:286
And corresponding code:
https://github.com/TWiStErRob/net.twisterrob.colorfilters/blob/0c85d9c/feature/image/src/main/kotlin/net/twisterrob/colorfilters/android/image/BitmapKeeper.kt#L111
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [com.github.bumptech.glide:okhttp3-integration](https://github.com/bumptech/glide) | `4.15.1` -> `4.16.0` | [![age](https://developer.mend.io/api/mc/badges/age/maven/com.github.bumptech.glide:okhttp3-integration/4.16.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/maven/com.github.bumptech.glide:okhttp3-integration/4.16.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/maven/com.github.bumptech.glide:okhttp3-integration/4.15.1/4.16.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/maven/com.github.bumptech.glide:okhttp3-integration/4.15.1/4.16.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | | [com.github.bumptech.glide:glide](https://github.com/bumptech/glide) | `4.15.1` -> `4.16.0` | [![age](https://developer.mend.io/api/mc/badges/age/maven/com.github.bumptech.glide:glide/4.16.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/maven/com.github.bumptech.glide:glide/4.16.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/maven/com.github.bumptech.glide:glide/4.15.1/4.16.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/maven/com.github.bumptech.glide:glide/4.15.1/4.16.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | | [com.github.bumptech.glide:ksp](https://github.com/bumptech/glide) | `4.15.1` -> `4.16.0` | [![age](https://developer.mend.io/api/mc/badges/age/maven/com.github.bumptech.glide:ksp/4.16.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/maven/com.github.bumptech.glide:ksp/4.16.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/maven/com.github.bumptech.glide:ksp/4.15.1/4.16.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/maven/com.github.bumptech.glide:ksp/4.15.1/4.16.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>bumptech/glide (com.github.bumptech.glide:okhttp3-integration)</summary> ### [`v4.16.0`](https://github.com/bumptech/glide/releases/tag/v4.16.0): Glide 4.16.0 This release focuses on some build improvements and Compose. The two major Compose improvements are adding support for Compose specific transitions (e.g. cross fade) and supporting recomposition based on request state using `GlideSubcomposition`. There's also been a bunch of internal refactoring to move away from Painters to Modifier nodes based on feedback from the Compose team. This is still an alpha release of Compose, but barring unexpectedly negative feedback, the next release should be beta. This should be the last release of Glide that targets Java 7. That probably means our next release will be a major version change. ##### Features - Allow passing an executor into ChromiumRequestSerializer in [bumptech/glide#5077 - Allow host app to provide a way to clear all resources onStop() by [@​osamaaftab](https://github.com/osamaaftab) in [bumptech/glide#5145 ##### Compose - Add a Transition API and a CrossFade Transition for Compose by [@​sjudd](https://github.com/sjudd) in [bumptech/glide#5235 - Influence layout using intrinsics in GlideNode by [@​sjudd](https://github.com/sjudd) in [bumptech/glide#5240 \* Log instead of throwing parsing manifests to fix compose previews by [@​sjudd](https://github.com/sjudd) in [bumptech/glide#5167 - Launch no more than one request per onRemembered by [@​sjudd](https://github.com/sjudd) in [bumptech/glide#5062 - Remove GlidePainter in favor of Modifier nodes / Flows by [@​sjudd](https://github.com/sjudd) in [bumptech/glide#5230 - Replace flows in GlideSubcomposition with a listener on GlideNode by [@​sjudd](https://github.com/sjudd) in [bumptech/glide#5238 ##### Bugs - Read library glide module names from Java indexes by [@​sjudd](https://github.com/sjudd) in [bumptech/glide#5052 - Fix typo. anay -> any in GlideSymbolProcessor.kt. by [@​trevorhackman](https://github.com/trevorhackman) in [bumptech/glide#5029 - Include URL in error log by [@​paulsowden](https://github.com/paulsowden) in [bumptech/glide#5164 - Add `isInitialized` visible for testing method by [@​paulsowden](https://github.com/paulsowden) in [bumptech/glide#5163 - Use onIdle to avoid a race in FlowTests by [@​sjudd](https://github.com/sjudd) in [bumptech/glide#5202 - Add a isEquivalentTo method to correctly check equality by [@​mori-atsushi](https://github.com/mori-atsushi) in [bumptech/glide#5232 - Add [@​RequiresPermission](https://github.com/RequiresPermission) to NotificationTarget by [@​TWiStErRob](https://github.com/TWiStErRob) in [bumptech/glide#5220 ##### Deprecations - `placeholderOf(@​Composable)` in `GlideImage` is deprecated, use `GlideSubcomposition` instead. Keep in mind that using either forces a recomposition each time the state of the image load changes. Recomposition will have a significant performance penalty in scrolling lists and should be avoided. ##### Behavior Changes - Hard code disabling hardware bitmaps on O/OMR1. by [@​sjudd](https://github.com/sjudd) in [bumptech/glide#5115 - Do not set requireOriginal on Android photo picker uris. by [@​phoenixli](https://github.com/phoenixli) in [bumptech/glide#5162 ##### Breaking Changes ##### Build Changes - Add integration tests for ksp library modules. by [@​sjudd](https://github.com/sjudd) in [bumptech/glide#5054 - Update README.md to use https by [@​simoarpe](https://github.com/simoarpe) in [bumptech/glide#5058 - Use dokka to build scripts/update_javadocs.sh by [@​sjudd](https://github.com/sjudd) in [bumptech/glide#5104 - avif integration: Update libavif dependency by [@​vigneshvg](https://github.com/vigneshvg) in [bumptech/glide#5128 - Disable java 7 source obsolete warning. by [@​sjudd](https://github.com/sjudd) in [bumptech/glide#5168 - Update mockito version to fix j16 compilation. by [@​sjudd](https://github.com/sjudd) in [bumptech/glide#5169 - Switch Glide's dependencies to a version catalog. by [@​sjudd](https://github.com/sjudd) in [bumptech/glide#5183 - Remove jetifier by [@​sjudd](https://github.com/sjudd) in [bumptech/glide#5184 - Add an updated proguard plugin to compile on Java 17. by [@​sjudd](https://github.com/sjudd) in [bumptech/glide#5185 - Configure Renovate in [bumptech/glide#5186 - Increment ROBOLECTRIC_SDK to 19 from 18. by [@​brettchabot](https://github.com/brettchabot) in [bumptech/glide#5208 and [bumptech/glide#5207 - AGP: Upgrade AndroidManifest.xml's package to build.gradle's namespace. by [@​TWiStErRob](https://github.com/TWiStErRob) in [bumptech/glide#5221 ##### New Contributors - [@​trevorhackman](https://github.com/trevorhackman) made their first contribution in [bumptech/glide#5029 - [@​simoarpe](https://github.com/simoarpe) made their first contribution in [bumptech/glide#5058 - [@​paulsowden](https://github.com/paulsowden) made their first contribution in [bumptech/glide#5164 - [@​phoenixli](https://github.com/phoenixli) made their first contribution in [bumptech/glide#5162 - [@​osamaaftab](https://github.com/osamaaftab) made their first contribution in [bumptech/glide#5145 - [@​brettchabot](https://github.com/brettchabot) made their first contribution in [bumptech/glide#5207 - [@​mori-atsushi](https://github.com/mori-atsushi) made their first contribution in [bumptech/glide#5232 **Full Changelog**: bumptech/glide@v4.15.0...v4.16.0 Note - there's been a change in the gpg key used to sign these releases. The new public key is attached </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/pachli/pachli-android). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4xMDcuMiIsInVwZGF0ZWRJblZlciI6IjM3LjguMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Nik Clayton <nik@ngo.org.uk>
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.