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

Bypass font path prefix check in ResourcesCompat::loadFont #1798

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

DSteve595
Copy link
Contributor

@DSteve595 DSteve595 commented Jan 15, 2025

This changes the way in which ResourcesCompatVisitorFactory works around the annoying font prefix check in ResourcesCompat::loadFont, which checks that font paths begin with "res/". The workaround introduced in #1586 works when the font's file path in the project contains "res/", but that's not guaranteed (e.g. alternate source directories, generated sources).
Given that I don't think this resource path check gives any value to Paparazzi tests, this fixes those edge cases by bypassing the check completely, assuming startsWith returns true. Still hacky, but it already was.

Fixes #1794

Comment on lines +21 to +22
// For testing loading font resources from directories other than 'res/'
sourceSets.main.res.srcDirs += 'src/main/other_resources_dir'
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this looks good but might be better in a separate test project. Not a blocker for this PR.

Copy link
Collaborator

@geoff-powell geoff-powell left a comment

Choose a reason for hiding this comment

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

this change makes sense instead of an arbitrary contains method, skipping the whole startsWith method eval.
Thanks for the contribution!!

@geoff-powell geoff-powell merged commit 3608cef into cashapp:master Jan 21, 2025
10 checks passed
@DSteve595 DSteve595 deleted the font-res-startswith branch January 21, 2025 19:56
@jrodbx
Copy link
Collaborator

jrodbx commented Jan 22, 2025

The workaround introduced in #1586

FWIW, that workaround was adopted from Android Studio's own implementation here:
https://cs.android.com/android-studio/platform/tools/adt/idea/+/mirror-goog-studio-main:rendering/src/com/android/tools/rendering/classloading/ResourcesCompatTransform.kt;l=64?q=ResourcesCompat

, but that's not guaranteed (e.g. alternate source directories, generated sources).

generated sources are handled in a different manner. I'm curious: what's the use case for having alternate resource directories @DSteve595? And more importantly, does that mean that Android Studio's Compose Previews are broken for you given its reliance on the above linked implementation?

@geoff-powell
Copy link
Collaborator

generated sources are handled in a different manner. I'm curious: what's the use case for having alternate resource directories @DSteve595? And more importantly, does that mean that Android Studio's Compose Previews are broken for you given its reliance on the above linked implementation?

yes I had the same thoughts. As this unblocks your issue, I have the same hesitancy to mimic AS vs our own differing solution.

@DSteve595
Copy link
Contributor Author

Our use case is basically just multiplatform, where we're using Android's directory structure but happened to called the directory "resources" rather than "res". It's something that's easily changed on our part (or easily worked around by copying the resources into a generated dir named "res"). But my thinking with this was: if it's a valid resource that an Android app can read, then Paparazzi should be able to read it too.
Previews are broken in Android Studio too, and I didn't realize it was due to this. I consider it an Android Studio bug (well, really a ResourcesCompat bug, but one that affects Studio), for the same reason.
Feel free to revert if matching Studio's behavior seems preferable.

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.

Font resources fail to load if in a directory that doesn't contain "res/"
3 participants