-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Generate functions to find resources by a string ID. #5068
Conversation
As this is a new API, it needs 2 approves. @m-sasha, could you look at the new API? I will look at the API and the implementation |
import org.jetbrains.compose.resources.StringArrayResource | ||
import org.jetbrains.compose.resources.StringResource | ||
|
||
@ExperimentalResourceApi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's create a task of removing @ExperimentalResourceApi
in 1.8 or 1.9 (what is more appropriate in your opinion).
import org.jetbrains.compose.resources.StringResource | ||
|
||
@ExperimentalResourceApi | ||
public expect val Res.allDrawableResources: Map<String, DrawableResource> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would drop the all
prefix, but it's really a minor nit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just something to consider: providing a Map
is a large API surface. It's good for the user, but could be hard for us to provide/maintain. If we just provide a getter function instead, it would limit the API surface to the bare minimum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a getter is not enough in case I want to iterate through all resources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would drop the all prefix, but it's really a minor nit.
then IDE will suggest you
Res.drawab_ (Ctrl+Space)
drawable //object for accessors
drawableResources //map with all drawables
I find it confusing
Thanks. I appreciate the work that has been done so please don't take this as a complaint, rather some observations in solving this problem myself. Building a map of the resources may lead to a large map where the references to all resources are generated when
I hope this makes sense.This will lead to lazy loading of resources. I store a |
The map is instantiated lazy when a user calls it |
@terrakok ahh, yes. Good. I was more meaning a map of all For my app, most strings will be referenced using the I'm not trying to push this code. The method implemented with a To be honest, I'm not sure how much improvement this code would make. I tend to focus on code that might slow down app start up times. |
Could you explain: instead of the map the method will find ... |
Forgive me if I misinterpret this code:
Which I believe will generate something like:
But instead of generating this
Then the My app has over a hundred string resources that are used in the UI and dialogs but the app will only request about 6 string resources by name. In this scenario, the map will only have 6 items in it, rather that a map with over a hundred items where over 90% will never be requested by name. I hope that makes sense?? |
I posted about this approach for |
Yes, your solution will work but only for a case to find a resource by ID. What if I want to iterate through all of drawables in my app? And, for the mark, the map with resources is not too big because resources themself are being cached after first access already |
Good point regarding the list of ids. Maybe one day we can get the best of all worlds. In the meantime, I'll stick to my But will use this Thanks. |
If you nervous about a speed - take a look on my demo with 16 000 icons: |
Cool. I wasn't really nervous but if I was, I would be less so now :-) Thanks. |
The PR adds a generation special properties with maps a string ID to the resource for each type of resources:
Fixes #4880
Fixes https://youtrack.jetbrains.com/issue/CMP-1607
Testing
I checked it in the sample project but this should be tested by QA (KMP and JVM only projects)
Release Notes
Features - Resources