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

Generate functions to find resources by a string ID. #5068

Merged
merged 3 commits into from
Jul 10, 2024
Merged

Conversation

terrakok
Copy link
Member

@terrakok terrakok commented Jul 5, 2024

The PR adds a generation special properties with maps a string ID to the resource for each type of resources:

val Res.allDrawableResources: Map<String, DrawableResource>
val Res.allStringResources: Map<String, StringResource>
val Res.allStringArrayResources: Map<String, StringArrayResource>
val Res.allPluralStringResources: Map<String, PluralStringResource>
val Res.allFontResources: Map<String, FontResource>

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

  • Now the gradle plugin generates resources map to find a resource by a string ID

@terrakok terrakok requested a review from igordmn July 5, 2024 11:34
@igordmn igordmn requested a review from m-sasha July 10, 2024 10:53
@igordmn
Copy link
Collaborator

igordmn commented Jul 10, 2024

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
Copy link
Collaborator

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>
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member Author

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

@terrakok terrakok merged commit e4c7703 into master Jul 10, 2024
12 checks passed
@terrakok terrakok deleted the k.tskh/all-res branch July 10, 2024 12:55
@duanemalcolm
Copy link

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 Res is instantiated. My thinking was to use a MutableMap and a when statement instead. For example,

private val loadedStringResources = mutableMapOf<String, StringResource?>()

fun getStringResource(id: String): StringResource? {
   if (id in loadedStringResources.keys) {
      return loadedStringResources[id]
   }
   val resource = loadStringResource(id)
   loadedStringResources[id] = resource
   return resource
}

fun loadStringResource(id: String): StringResource? {
   return when(id) {
      "hello" -> Res.string.hello
      "world" -> Res.string.world
      else -> null
   }
}

I hope this makes sense.This will lead to lazy loading of resources. I store a null in the map to indicate the resource has already been searched for and it doesn't exist.

@terrakok
Copy link
Member Author

The map is instantiated lazy when a user calls it

@duanemalcolm
Copy link

@terrakok ahh, yes. Good.

I was more meaning a map of all StringResources would be instantiated when one string resource is requested by name, which may be unnecessary and inefficient.

For my app, most strings will be referenced using the Res variable (e.g., Res.string.hello) but some strings will be referenced by name (e.g., Res.stringResources["world"]). The issue here is that a Map of all of string resources would be created when the app would request less than 5% by name.

I'm not trying to push this code. The method implemented with a Map will work for me. I just thought I would throw this idea out there since there isn't much difference between the code to generate a Map of resources vs a when statement to find a resource.

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.

@terrakok
Copy link
Member Author

Could you explain: instead of the map the method will find ...
Find where?

@duanemalcolm
Copy link

Forgive me if I misinterpret this code:

        chunkFile.addFunction(
            FunSpec.builder("_collect${chunkClassName}Resources")
                .addAnnotation(internalAnnotation)
                .addModifiers(KModifier.INTERNAL)
                .addParameter(
                    "map",
                    MUTABLE_MAP.parameterizedBy(String::class.asClassName(), type.getClassName())
                )
                .also { collectFun ->
                    idToResources.keys.forEach { resName ->
                        collectFun.addStatement("map.put(%S, $chunkClassName.%N)", resName, resName)
                    }
                }
                .build()
        )

Which I believe will generate something like:

val map = mutableMapOf<String, StringResource>()
map.put("hello", Res.string.hello)
map.put("world", Res.string.world)
...

But instead of generating this Map code, we can generate a similar function with a when statement, for example:

fun loadStringResource(id: String): StringResource? {
   return = when(id) {
      "hello" -> Res.string.hello
      "world" -> Res.string.world
      ...
      else -> null
   }
}

Then the getStringResource(id: String) function will load each resources into the Map as each string resource is requested by name.

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??

@duanemalcolm
Copy link

I posted about this approach for drawable here:
#4880 (comment)

@terrakok
Copy link
Member Author

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

@duanemalcolm
Copy link

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 Iconic method for drawables here:
#4880 (comment)

But will use this Map implementation for strings. I will at some stage check the load at start up for instantiating the map of string resource references.

Thanks.

@terrakok
Copy link
Member Author

If you nervous about a speed - take a look on my demo with 16 000 icons:
https://github.com/terrakok/fluentui-system-icons

https://androiddev.social/@terrakok/112775705319577461

@duanemalcolm
Copy link

Cool. I wasn't really nervous but if I was, I would be less so now :-) Thanks.

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.

Support getting resources by key
4 participants