Skip to content
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

Add an experimental rememberGlidePainter API #5204

Closed
wants to merge 1 commit into from

Conversation

sjudd
Copy link
Collaborator

@sjudd sjudd commented Jul 4, 2023

A separate painter can be used outside of GlideImage with other composables. It can also be used for crossfades or custom animations.

This is very similar in concept to Coil's implementation but the details are a bit different.

@sjudd sjudd force-pushed the remember_glide_painter branch 9 times, most recently from f82523d to c37b7b0 Compare July 9, 2023 16:37
public fun rememberGlidePainter(
model: Any?,
requestBuilderTransform: RequestBuilderTransform<Drawable> = { it },
): Pair<Painter, GlidePainterState> {
Copy link

@arriolac arriolac Jul 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API is a bit unusual for Compose code. Typically a remember* method will return the direct mutable state object, rather than something wrapper in another type such as a Pair. For example, I'd expect that this would return a GlidePainter object instead. Might be good to create a class like that?

EDIT: Looks like lhs of the Pair is already a GlidePainter - might as well return that directly

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the disadvantage is that you expose a broader API surface, you get all of GlidePainter when all you need is a state.

I could extract an interface to avoid that:
[

GlidePainter extends Painter(public val state: GlidePainterState) { }

val painter = rememberGlidePainter(model)
match (painter.state) {
...
}

It feels slightly less nice to return a Painter interface that's just Painter + State? That is the way that Coil does this though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Composable
public fun rememberGlidePainter(
model: Any?,
modifier: Modifier,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically, modifiers are used for UI elements and not in remember functions. I don't have a good suggestion here though on how to accomplish what you are trying to do however as I understand that the modifier is the mechanism for determining the image size.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other option is extracting a method that deals with modifiers and then having people pass in ResolvableGlideSize to rememberGlidePainter instead:

  val (size, updatedModifier) = waitForSizeFromModifier(modifier);
  val painter = rememberGlidePainter(model, size);
  Image(painter, contentDescription, updatedModifier)

Maybe that's a bit nicer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@sjudd sjudd force-pushed the remember_glide_painter branch 2 times, most recently from 2908a65 to 56bbb3a Compare July 13, 2023 15:57
@sjudd
Copy link
Collaborator Author

sjudd commented Aug 12, 2023

Replaced with #5230

@sjudd sjudd closed this Aug 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants