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

Support custom image with context in Squoosh #1803

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rylin8
Copy link
Collaborator

@rylin8 rylin8 commented Nov 19, 2024

Fixes #1785. Change the image with context customization to be a normal function instead of a composable function, and update its usages everywhere.

Copy link

github-actions bot commented Nov 19, 2024

Snapshot diff report vs base branch: main
Last updated: Mon Nov 18 17:02:46 PST 2024, Sha: 2520e63
No differences detected

Fixes #1785. Change the image with context customization to be a normal function instead of a composable function, and update its usages everywhere.
@rylin8 rylin8 marked this pull request as ready for review November 19, 2024 06:17
@@ -184,7 +184,7 @@ internal fun stringTypeToCustomizationType(strType: String): CustomizationType {
"@Composable (ComponentReplacementContext) -> Unit" ->
CustomizationType.ComponentReplacement
"com.android.designcompose.ListContent" -> CustomizationType.ListContent
"@Composable (ImageReplacementContext) -> Bitmap?" -> CustomizationType.ImageWithContext
"(ImageReplacementContext) -> Bitmap?" -> CustomizationType.ImageWithContext
Copy link
Collaborator

Choose a reason for hiding this comment

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

This must need a docs update, too, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interestingly we never had a section for this customization, but I'll add one.

@@ -415,6 +420,10 @@ class MediaAdapter(

val currentlyPlaying =
if (isMetadataSame(item, currentMetadata)) CurrentlyPlaying.On else CurrentlyPlaying.Off

// Unsubscribe this setter function whenever this leaves the composition
DisposableEffect(setIcon) { onDispose { artRequestManager.unsubscribe(setIcon) } }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think ideally the effect would also perform the subscription (or the subscribe-unsubscribe thing would be encapsulated in a composable function) just so they can't get out of sync. I never remember if DisposableEffect executes eagerly, or if it runs after MediaBrowseItem finished...).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was a bit messy due to the removal of @Composable. I subscribe in the image replacement context function because that's when I know the size and color to make the image that I want. I don't have that info here. However to ensure we don't leak memory and have extra subscriptions when this composable leaves composition, I added this DisposableEffect.

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.

Squoosh ImageWithContext customization support
2 participants