Fix CrossfadePainter intrinsic size logic#3175
Conversation
| if (isStartSpecified && isEndSpecified) { | ||
| return Size( | ||
| width = maxOf(startSize.width, endSize.width), | ||
| height = maxOf(startSize.height, endSize.height), | ||
| ) |
There was a problem hiding this comment.
When you initially wrote the logic, may I ask why you chose to use maxOf for startSize and endSize?
There was a problem hiding this comment.
It was to ensure that the placeholder isn't cut off. It's also to have consistency with CrossfadeDrawable.
|
|
||
| if (isEndSpecified) { | ||
| return endSize | ||
| } |
There was a problem hiding this comment.
In my opinion, we could simply set the intrinsicSize based on the endSize. What do you think?
There was a problem hiding this comment.
That would be a significant behaviour change and I'm not sure about the ramifications. How would crossfading from a painter with an intrinsic size to a painter with no intrinsic size (e.g. ColorPainter) inside an AsyncImage with no constraints look? Would it have 0 size?
|
@tak8997 Thanks for looking into that bug. Unfortunately I don't think we can make this change without flagging as this is a fairly sizeable behaviour change for existing users. Could you:
You'll likely need to add a secondary |
…te scaling * This change ensures that the ContentScale is applied correctly to the final image, regardless of the placeholder's dimensions.
…nsic size calculation
tak8997
force-pushed
the
placeholder_image_messes_when_crossfading
branch
from
e5c0719 to
683e131
Compare
…ward it to CrossfadePainter
|
colinrtwhite
left a comment
colinrtwhite
left a comment
There was a problem hiding this comment.
Thanks for implementing this! Left two remaining comments otherwise LGTM
| * @param preferExactIntrinsicSize If true, this drawable's intrinsic width/height will only be -1 | ||
| * if [start] **and** [end] return -1 for that dimension. If false, the intrinsic width/height will | ||
| * be -1 if [start] **or** [end] return -1 for that dimension. This is useful for views that | ||
| * require an exact intrinsic size to scale the drawable. |
| val timeSource: TimeSource = TimeSource.Monotonic, | ||
| val fadeStart: Boolean = true, | ||
| val preferExactIntrinsicSize: Boolean = false, | ||
| val preferEndFirstIntrinsicSize: Boolean = false |
There was a problem hiding this comment.
We'll need to add a @Deprecated("Kept for binary compatibility.", level = DeprecationLevel.HIDDEN) version of this constructor without the preferEndFirstIntrinsicSize argument to preserve binary compatibility. Check out NetworkClient for an example .
colinrtwhite
left a comment
colinrtwhite
left a comment
There was a problem hiding this comment.
Thanks for implementing this. Looks good!
3 participants
Fix CrossfadePainter intrinsic size logic for proper ContentScale behavior
Solution
Refactored the intrinsic size computation logic to:
Changes
Motivation