IssueTracker

Alan - what's the expectation of arbitrary "nesting" of color state lists?

...

But what if that other selector (primary_60) had multiple entries for different states? Would that reference resolve to the default color?

I say we keep to the simple case, which is what the framework implicitly does: squash down to the default color.

Agreed, I think adding nesting to CsL would be going a bit too far.

Chris - what do you think of the CL (https://android-review.googlesource.com/c/platform/frameworks/support/+/1320657) that moves the logic and the caching from appcompat-resources to core?

LGTM to me overall

I had a look into adding a callback so that CSLInflaterCompat could hook back into ResourcesCompat, if it needed to inflate another CSL, which has a cache of inflated CSLs.

/**
 * @hide
 */
@RestrictTo(LIBRARY_GROUP_PREFIX)
interface ColorStateListCache {
    @Nullable
    ColorStateList getCachedColorStateList(Resources resources, @ColorRes int resId);
}

Since ResourcesCompat is the only one with a CSL cache, we could pass a nullable cache to createFromXml:

@NonNull
public static ColorStateList createFromXml(
    @NonNull Resources r,
    @NonNull XmlPullParser parser,
    @Nullable Resources.Theme theme,
    @Nullable ColorStateListCache cache
) throws XmlPullParserException, IOException {
    final AttributeSet attrs = Xml.asAttributeSet(parser);
    // ...

and createFromXmlInner:

@NonNull
public static ColorStateList createFromXmlInner(
    @NonNull Resources r,
    @NonNull XmlPullParser parser,
    @NonNull AttributeSet attrs,
    @Nullable Resources.Theme theme,
    @Nullable ColorStateListCache cache
) throws XmlPullParserException, IOException {
    final String name = parser.getName();
    // ...

Although these functions are public, this class is restricted to the library group, so only a few changes are required (and no public API changes):

• ComplexColorCompat.inflate() uses createFromXmlInner. This would pass a null cache.
• TypedArrayUtils.getNamedColorStateList() uses createFromXml(). This would pass a null cache too.
• ResourcesCompat uses createFromXml(). This passes a non-null cache:

@Nullable
private static ColorStateList inflateColorStateList(Resources resources, int resId,
        @Nullable Theme theme) {
    if (isColorInt(resources, resId)) {
        // The resource is a color int, we can't handle it so return null
        return null;
    }

    final XmlPullParser xml = resources.getXml(resId);
    try {
        return ColorStateListInflaterCompat.createFromXml(resources, xml, theme, cache);
    } catch (Exception e) {
        Log.e(TAG, "Failed to inflate ColorStateList, leaving it to the framework", e);
    }
    return null;
}

private static final ColorStateListCache cache = new ColorStateListCache() {
    @Nullable
    @Override
    public ColorStateList getCachedColorStateList(Resources resources, int resId) {
        return ResourcesCompat.getCachedColorStateList(resources, resId);
    }
};

What do you think?

*The logic in the private inflate function gets pretty gnarly in Java though:

int resourceId = a.getResourceId(R.styleable.ColorStateListItem_android_color, -1);
int baseColor;
if (resourceId != -1 && !isColorInt(r, resourceId)) {
    try {
        if (cache != null) {
            ColorStateList cachedCsl = cache.getCachedColorStateList(r, resourceId);
            if (cachedCsl != null) {
                baseColor = cachedCsl.getDefaultColor();
            } else {
                baseColor = createFromXml(r, r.getXml(resourceId),
                        theme).getDefaultColor();
            }
        } else {
            baseColor = createFromXml(r, r.getXml(resourceId), theme).getDefaultColor();
        }
    } catch (Exception e) {
        baseColor = a.getColor(R.styleable.ColorStateListItem_android_color,
                Color.MAGENTA);
    }
} else {
    baseColor = a.getColor(R.styleable.ColorStateListItem_android_color, Color.MAGENTA);
}

Hmmm, I say we keep to the simple solution which we spoke about in #8.

The contributor CL has been merged and will be available in the next alphas of core and appcompat

Detected a regression that this CL introduced.

P1 to block stable.

Looks like the CSL cache is keying resolved CSLs on Resources without accounting for the Theme against which they was resolved. It was always broken, we just never noticed.

Global cache keyed on Resources:

private static final WeakHashMap<Resources, SparseArray<ColorStateListCacheEntry>>
            sColorStateCaches = new WeakHashMap<>(0);

Resolved ColorStateList and no mention of Theme:

    private static class ColorStateListCacheEntry {
        final ColorStateList mValue;
        final Configuration mConfiguration;

        ColorStateListCacheEntry(@NonNull ColorStateList value,
                @NonNull Configuration configuration) {
            mValue = value;
            mConfiguration = configuration;
        }
    }

From Alan:

https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/content/res/ResourcesImpl.java has resource caching (but doesn't look like https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/content/res/ResourcesImpl.java#1073 is doing any caching for CSLs).

The base cache class is https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/content/res/ThemedResourceCache.java and it's using the hidden Resources.ThemeKey. The old implementation in appcompat-resources never ran into this issue since it never supported nested CSLs. However, now you can run into a cached CSL that was resolved against a Theme, and then get the same CSL when you were asking for one that matches Resources with no Theme. So keying on Configuration alone is not enough for caching.

A potential quick fix is to disable caching altogether (same as in framework). Or alternatively add something to the ColorStateListCacheEntry that would respect the Theme set on Resources

Note that the platform's ResourcesImpl keys the cache on Resources, Theme, and asset cookie:

        final long key = (((long) value.assetCookie) << 32) | value.data;
        final ConfigurationBoundResourceCache<ComplexColor> cache = mComplexColorCache;
        ComplexColor complexColor = cache.getInstance(key, wrapper, theme);

Theme doesn't implement hashCode and its comparable ThemeKey isn't public API, but you'd still get some benefits from keying off the Object implementation of hashCode.

What's the preferred approach? I can pick this up.

I didn't understand when the Resources might be different - is it this?

ResourcesCompat.getColorStateList(
    context.resources,
    R.color.my_csl,
    ContextThemeWrapper(context, R.style.ThemeOverlay_EverythingDifferent).theme
)

If adding Theme to ColorStateListCacheEntry isn't good enough because the lack of a legit hashcode implementation means potential collisions, could we instead keep Resources as the key in the global cache, but use theme?.resources ?: res instead?

The old implementation in appcompat-resources never ran into this issue since it never supported nested CSLs. However, now you can run into a cached CSL that was resolved against a Theme, and then get the same CSL when you were asking for one that matches Resources with no Theme.

This is the part I didn't understand. The nested CSL isn't resolved from the cache, it's always inflated, so I didn't follow why this is a problem now, but wasn't before.

@Alan, is this still blocking?

could we instead keep Resources as the key in the global cache, but use theme?.resources ?: res instead?

No, because many Themes will reference the same Resources object. Theme is a non-static inner class of Resources and contains a reference to the Resources object from which it was created.

@Alan, is this still blocking?

Not blocking, per se. We had to revert the CL. Lowering to P2 and considering this a FR now.

Alan, can I get a +2 on https://android-review.googlesource.com/c/platform/frameworks/support/+/1350483 so that we're not blocked on internal drops?

I'm able to reproduce the bug and I understand what you mean now. It's a very common case, e.g. when using theme overlays:

<TextView
    android:layout_width="wrap_content"
    android:layout_height="wrap_content"
    android:text="Purple"
    android:textColor="@color/csl_pointing_to_color_primary_which_is_purple" />

<TextView
    android:layout_width="wrap_content"
    android:layout_height="wrap_content"
    android:text="Green"
    android:textColor="@color/csl_pointing_to_color_primary_which_is_purple"
    android:theme="@style/ThemeOverlay.GreenPrimary" />

We'd expect green text for the second one, but @color/csl_pointing_to_color_primary_which_is_purple is cached, so it'll be purple. :+1:

I added a key that takes Theme into account:

private static class ColorStateListCacheKey {
    final Resources mResources;
    @Nullable final Theme mTheme;

    ColorStateListCacheKey(@NonNull Resources resources, @Nullable Theme theme) {
        mResources = resources;
        mTheme = theme;
    }

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;
        ColorStateListCacheKey that = (ColorStateListCacheKey) o;
        return mResources.equals(that.mResources) &&
                Objects.equals(mTheme, that.mTheme);
    }

    @Override
    public int hashCode() {
        return Objects.hash(mResources, mTheme);
    }
}

I don't think there would be any false positive matches anymore (the cache bug), but there could still be some false negatives (unnecessary inflation, duplicate values in cache). As you said this is what ResourcesImpl does (*), is this a sufficient solution?

(*) I didn't understand the value of including the asset cookie in the key. Is that applicable for this case?

Bump, I've got time to work on this this month. If I submit a CL with the above, reckon someone could take a look?

Sure, if you have a CL with test covering the new paths, we'll be happy to get it in and see if it breaks any of the app tests that we have.

CL here https://android-review.googlesource.com/c/platform/frameworks/support/+/1372582

I didn't add any new tests - ResourcesCompatTest already covered the bug as I understood it, where R.color.complex_themed_selector is loaded twice with distinct themes. I extended the coverage down to the minSdkVersion of the library (but could only test it on API 15).