jpardogo / PagerSlidingTabStrip

An interactive indicator to navigate between the different pages of a ViewPager
2.19k stars 353 forks source link

View does not use its own textColorPrimary value #48

Closed elliottsj closed 9 years ago

elliottsj commented 9 years ago

If I specify the textColorPrimary as an attribute of the view (or if I specify it in a theme via the style attribute), the value is ignored and textColorPrimary from the parent theme is used instead.

<com.astuetz.PagerSlidingTabStrip
        android:id="@+id/tabs"
        android:layout_width="match_parent"
        android:layout_height="?android:actionBarSize"
        android:textColorPrimary="@android:color/white"
        app:pstsShouldExpand="true" />

(i.e. the @android:color/white value is ignored)

I've tried breaking on this line (PagerSlidingTabStrip.java#L156) to see if obtainStyledAttributes is returning the right attributes:

While paused, running the expression

context.obtainStyledAttributes(attrs, new int[] {android.R.attr.textColorPrimary}).getString(0)

gives me the correct value of "#ffffffff". But running

context.obtainStyledAttributes(attrs, ATTRS).getString(TEXT_COLOR_PRIMARY)

gives me the incorrect value of "#ff212121" (from the parent theme).

Then, stepping past L#159, int textPrimaryColor has that same incorrect value (converted to an int): -14606047 (== 0xFF212121)


I'm wondering if this is an Android issue because

context.obtainStyledAttributes(attrs, new int[] {android.R.attr.textColorPrimary}).getString(0)

should really be the same value as

context.obtainStyledAttributes(attrs, ATTRS).getString(TEXT_COLOR_PRIMARY)
elliottsj commented 9 years ago

http://stackoverflow.com/questions/25402452/attempting-to-get-attribute-values-in-code-returns-incorrect-values It turns out that the ATTRS array needs to be sorted in ascending order, thanks to the way Android iterates over the attributes deep in its C++ source.

Sorting ATTRS in ascending order should fix the issue:

private static final int[] ATTRS = new int[]{
    android.R.attr.textColorPrimary,
    android.R.attr.textSize,
    android.R.attr.textColor,
    android.R.attr.paddingLeft,
    android.R.attr.paddingRight
};

I'll do some testing to make sure it works and make a PR

elliottsj commented 9 years ago

Submitted an issue to Android: https://code.google.com/p/android/issues/detail?id=91445

jpardogo commented 9 years ago

Why do you use android:textColorPrimary="@android:color/white" ? this attr doesn't exist. You should use android:textColor="@android:color/white"

elliottsj commented 9 years ago

android:textColorPrimary is defined in the app theme, so I thought you should be able to override theme attributes like this on a view-by-view basis.

<com.astuetz.PagerSlidingTabStrip
        android:id="@+id/tabs"
        android:layout_width="match_parent"
        android:layout_height="?android:actionBarSize"
        android:textColorPrimary="@android:color/white"
        app:pstsShouldExpand="true" />

Should be equivalent to

<com.astuetz.PagerSlidingTabStrip
        style="@style/TabStrip"
        android:id="@+id/tabs"
        android:layout_width="match_parent"
        android:layout_height="?android:actionBarSize"
        app:pstsShouldExpand="true" />
<!-- res/values/styles.xml -->
...
<style name="TabStrip">
    <item name="android:textColorPrimary">@android:color/white</item>
</style>
...
jpardogo commented 9 years ago

Sorry I got confuse, yes we need android.R.attr.textColor and android.R.attr.textColorPrimary https://github.com/jpardogo/PagerSlidingTabStrip/blob/master/library/src/com/astuetz/PagerSlidingTabStrip.java#L163