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

Migrate to Jetpack Tile previews #1162

Merged
merged 4 commits into from
Aug 2, 2024

Conversation

ithinkihaveacat
Copy link
Contributor

@ithinkihaveacat ithinkihaveacat commented Jul 31, 2024

This is a mostly mechanical change however there is one meaningful difference: previously the ImageResource passed to onTileResourceRequest() was passed through bitmapToImageResource(), where the Bitmap was extracted from the context via BitmapFactory.decodeResource(). I think this was mostly to demonstrate how to process a raw Bitmap.

However, with the new preview library, it's not possible to access the context (I think?), so we bypass this transformation step and instead generate the ImageResource via drawableResToImageResource() (which takes a resource id).

I think this is fine since the main point is to generate a preview rather than transform images from one format to another (the previews themselves are identical) but others may disagree!

Before ("compose"):

Screenshot 2024-07-31 at 15 09 10

Before ("Wear tile services"):

Screenshot 2024-07-31 at 15 09 19

After:

Screenshot 2024-07-31 at 15 10 45

@ithinkihaveacat ithinkihaveacat marked this pull request as ready for review July 31, 2024 14:11
@yschimke
Copy link
Contributor

The description doesn't match the PR

However, with the new preview library, it's not possible to access the context

When it clearly is.

Copy link
Contributor

@yschimke yschimke left a comment

Choose a reason for hiding this comment

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

Nice

addIdToImageMapping(
state.contacts[1].imageResourceId(),
bitmapToImageResource(
BitmapFactory.decodeResource(context.resources, R.drawable.ali)
Copy link
Contributor

Choose a reason for hiding this comment

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

Using drawableResToImageResource seems like an improvement.

You could consider inlining it if you want to minimise horologist usage here.

public fun drawableResToImageResource(
    @DrawableRes id: Int,
): ImageResource =
    ImageResource.Builder()
        .setAndroidResourceByResId(
            AndroidImageResourceByResId.Builder()
                .setResourceId(id)
                .build(),
        )
        .build()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was considering an extension function to enable addIdToImageMapping() to take a DrawableRes or ImageResource:

fun ResourceBuilders.Resources.Builder.addIdToImageMapping(
    id: String,
    @DrawableRes resId: Int
): ResourceBuilders.Resources.Builder = addIdToImageMapping(
    id, ResourceBuilders.ImageResource.Builder()
        .setAndroidResourceByResId(
            ResourceBuilders.AndroidImageResourceByResId.Builder()
                .setResourceId(resId)
                .build()
        )
        .build()
)

Not sure whether this is idiomatic (versus the function approach) or whether it should be called addIdToResourceMapping(), etc. so was leaving it for a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added enhanced addIdToImageMapping() in #1164.

Copy link
Contributor

@garanj garanj left a comment

Choose a reason for hiding this comment

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

Great!

@ithinkihaveacat
Copy link
Contributor Author

The description doesn't match the PR

However, with the new preview library, it's not possible to access the context

When it clearly is.

I don't follow this! This is the issue I was getting with a more mechanical transformation:

Screenshot 2024-07-31 at 13 36 11

@ithinkihaveacat ithinkihaveacat merged commit 3ec82cc into android:main Aug 2, 2024
1 check passed
@ithinkihaveacat
Copy link
Contributor Author

The description doesn't match the PR

However, with the new preview library, it's not possible to access the context

When it clearly is.

I don't follow this! This is the issue I was getting with a more mechanical transformation:

Screenshot 2024-07-31 at 13 36 11

Oh, there's a bug in the lint check. It's been reported.

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.

3 participants