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.

Sign up for GitHub

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

Jump to bottom

Tests fails when using AndroidX font-family #488

Open
magnusvs opened this issue Jul 6, 2022 · 7 comments · May be fixed by #679
Open

Tests fails when using AndroidX font-family #488

magnusvs opened this issue Jul 6, 2022 · 7 comments · May be fixed by #679
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Milestone
1.4

Comments

@magnusvs
Copy link

magnusvs commented Jul 6, 2022

Description
It seems like ResourcesInterceptor overrides to use the platform Resources.getFont instead of the AndroidX ResourcesCompat. This causes a problem when using a font-family that only specifies app: attributes, instead of android: attributes.

Steps to Reproduce
Use an AppTheme that sets textAppearance with a font-family like:

<?xml version="1.0" encoding="utf-8"?>
<font-family xmlns:app="http://schemas.android.com/apk/res-auto">
    <font
        app:font="@font/fk_grotesk_neue_regular"
        app:fontStyle="normal"
        app:fontWeight="400" />

    <font
        app:font="@font/fk_grotesk_neue_medium"
        app:fontStyle="normal"
        app:fontWeight="500" />

    <font
        app:font="@font/fk_grotesk_neue_bold"
        app:fontStyle="normal"
        app:fontWeight="700" />

</font-family>

By adding android: prefix the tests can inflate everything again.

<font-family xmlns:app="http://schemas.android.com/apk/res-auto"
xmlns:android="http://schemas.android.com/apk/res/android">
<font
    android:font="@font/fk_grotesk_neue_regular"
    android:fontStyle="normal"
    android:fontWeight="400"
    app:font="@font/fk_grotesk_neue_regular"
    app:fontStyle="normal"
    app:fontWeight="400" />

<font
    android:font="@font/fk_grotesk_neue_medium"
    android:fontStyle="normal"
    android:fontWeight="500"
    app:font="@font/fk_grotesk_neue_medium"
    app:fontStyle="normal"
    app:fontWeight="500" />

<font
    android:font="@font/fk_grotesk_neue_bold"
    android:fontStyle="normal"
    android:fontWeight="700"
    app:font="@font/fk_grotesk_neue_bold"
    app:fontStyle="normal"
    app:fontWeight="700" />

</font-family>

Expected behavior
It seems like the behaviour was added because of this bug https://issuetracker.google.com/issues/156065472 which now is supposed to be fixed. Could the Interception be removed?

Additional information:

  • Paparazzi Version: 1.0.0
  • OS: MacOS 12.3.1
  • Compile SDK: 32
  • Gradle Version: 7.4.1
  • Android Gradle Plugin Version: 7.2.1

Screenshots
@fcduarte
Copy link
Contributor

@magnusvs thanks for reporting! I tried to reproduce your issue with the current codebase and wasn't able to. Here's what I did:

  1. On file paparazzi-gradle-plugin/src/test/projects/custom-fonts-xml/src/main/res/font/cashmarket_medium.xml, I removed all references to android:font
  2. On file paparazzi-gradle-plugin/src/test/projects/custom-fonts-xml/src/main/res/layout/textviews.xml, I replaced android:fontFamily with app:fontFamily and changed all references@font/cashmarket_medium to @font/cashmarket_medium_normal
  3. Then ran ./gradlew clean :paparazzi-gradle-plugin:check and the test in question -- customFontsInXml worked (just had a small change on the image diff due to the font change I believe)

Could you try the same on our local and post your results? Also, if that is not the right way to simulate your issue could you provide some test cases?

Thanks!

@magnusvs
Copy link
Author

Hi,

Sorry for the late response @fcduarte . I'm trying it out locally today but I'm just hitting errors compiling and can't manage to run tests at the moment. First couldn't sync project with VERSION_NAME=1.2.0-SNAPSHOT, after downgrading to VERSION_NAME=1.1.0 I could sync but then fails with

Cannot perform signing task ':paparazzi-agent:signMavenPublication' because it has no configured signatory

Not too familiar with this maven publishing plugin so haven't gotten around it yet.

Back to the issue, if I remember everything correctly, I think on your step 2 is where it's different from my own project setup.

"and changed all references@font/cashmarket_medium to @font/cashmarket_medium_normal" I believe shouldn't be done. We adjust cashmarket_medium in step 1, to not have android:font prefixes but should still be able to use it, I believe that's why you didn't hit any errors.

Just to expand on the initial report:
Paparazzi does registerFontLookupInterceptionIfResourceCompatDetected
which seems to intercept requests to androidx.core.content.res.ResourcesCompat.getFont (which should handle app:fontFamily I guess)
and replaces it with android.content.res.Resources.getFont (which only handles android:fontFamily).
Now this registerFontLookupInterceptionIfResourceCompatDetected was added as a workaround to a bug that should be fixed now according to it's comment.

@jrodbx
Copy link
Collaborator

jrodbx commented Dec 4, 2022

Tried removing the method interceptor and the test still breaks due to the original issue. See comment here: https://issuetracker.google.com/issues/156065472#comment3

@jrodbx
Copy link
Collaborator

jrodbx commented Dec 15, 2022

Solution is to use ASM to apply a transform similar to the one used by Android Studio: https://issuetracker.google.com/issues/156065472#comment4. Any volunteers?

@jrodbx jrodbx added this to the 1.3 milestone Dec 15, 2022
@jrodbx jrodbx added help wanted Extra attention is needed good first issue Good for newcomers labels Dec 15, 2022
@aaalaniz
Copy link

Hey @jrodbx 👋🏼

I would like to give this issue a try.

Solution is to use ASM to apply a transform similar to the one used by Android Studio

Can you elaborate on this approach?

@tevjef tevjef linked a pull request Jan 10, 2023 that will close this issue
Open
@TWiStErRob
Copy link
Contributor

@aaalaniz, @tevjef created a PR: #679

@jrodbx jrodbx modified the milestones: 1.3, 1.4 Apr 24, 2023
@nileshteji
Copy link

Picking this up

@jrodbx jrodbx modified the milestones: 1.3.2, 1.4 Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Milestone
1.4
Development

Successfully merging a pull request may close this issue.

Fix AndroidX font-family loading with ASM transform on ResourcesCompat tevjef/paparazzi
6 participants
@fcduarte @aaalaniz @jrodbx @TWiStErRob @magnusvs @nileshteji