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 ImageVector support to the preference datastore ui #2

Merged
merged 3 commits into from
Mar 28, 2024

Conversation

lm41
Copy link
Contributor

@lm41 lm41 commented Mar 26, 2024

This is required to fully switch to ImageVectors in the https://github.com/florisboard/florisboard project.

The old method of using Drawable Ids is still supported. Consideration may be given to deprecating these methods.

Copy link
Owner

@patrickgold patrickgold left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the changes.

I have reviewed them and am currently a bit torn apart about the way you've implemented the image vector support. What if we take a bit of convenience away from the composables and only expose the following attributes:

icon: @Composable (() -> Unit)? = null,
iconSpaceReserved: Boolean,

on each composable that has an icon? Basically we (almost) eliminate the need for maybeJetIcon, we only need the ({ }) composable if iconSpaceReserved=true. What we definitely save is duplication of all composables which reduces maintainability issues going on. In the FlorisBoard code base we can introduce a convenience function which makes an icon composable from a drawable id or an ImageVector. Would also solve the awkward need to specify imageVector=null on the composable if I see it correctly.

You do not need to mark any of the old functions as deprecated as jetpref has not had any stable release yet so breaking API changes are expected.

@lm41 lm41 force-pushed the add-image-vector branch 2 times, most recently from 41cee15 to ef7b65f Compare March 27, 2024 14:35
@lm41
Copy link
Contributor Author

lm41 commented Mar 27, 2024

I've thought about it a bit and I think I've found a good option via the JetIcon interface.
This also enables additional support for your own icon implementations (such as custom painter etc.)

Copy link
Owner

@patrickgold patrickgold left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the changes! I've had a look at the changes and like the JetIcon idea! There are some issues/questions though that need clarification or fixing. (See comments)

Copy link
Owner

@patrickgold patrickgold left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, I've re-reviewed your incoming changes. After this iteration I think the code should be ready to merge

Copy link
Owner

@patrickgold patrickgold left a comment

Choose a reason for hiding this comment

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

Reviewed again and tested out locally, everything checks out fine, merging into main. Thanks for the efforts!

@patrickgold
Copy link
Owner

patrickgold commented Mar 28, 2024

For test using you'd have to use snapshots, here temporarily enable the jitpack repo in the core repo and then use the version table from here to grab a snapshot version (easier than using maven snapshots)

@patrickgold patrickgold merged commit 80b175b into patrickgold:main Mar 28, 2024
@lm41 lm41 deleted the add-image-vector branch May 7, 2024 18:28
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.

None yet

2 participants