Nutomic commented 9 years ago

I am trying to have a single tab with a different text color. For this, I implement CustomTabProvider and call TextView.setTextColor().

However, this change is overwritten by updateTabStyles().

I suggest to change the code so, that the tab style is only applied by the library if CustomTabProvider is not implemented. (I will probably make a pull request for this myself).

ouchadam commented 9 years ago

:+1:
for now I'm using a different id to tab_title so that updateTabStyles() skips

jpardogo commented 9 years ago

Do you think we really need to update the library for that? Don´t you think it is fair enough just not using the id R.id.psts_tab_title in your custom tabs.

NOTE: I have just prefix the xml files and id´s of the library for v1.0.9

jpardogo commented 9 years ago

I leave it like that. If we change it someone will create another issue asking why the text and the style are not apply to the TextView title of the custom tabs. If you don't want the library manage your TextView title for the tab, use a different id than R.id.psts_tab_title in your tab layout.

ataulm commented 9 years ago

Just had a chat with @ouchadam. The problem he was facing was that he uses a custom tab layout so that a custom font can be used.

If using a different ID as you suggest, it won't change the alpha of the text of tabs when one is selected/not-selected, because the class cannot find the text view. But if you do use the same ID, then many of the textview attributes are updated.

So the problem comes back to the fact that there's no selected/not selected state for the tab (#70) else this differentiation between selected/not-selected tabs can be done by us.

ataulm commented 9 years ago

oh seems that PR I submitted was covered by #51. If you supply a custom tab layout that doesn't use the R.id.psts_tab_title ID, your TextView should be untouched.

The problem remains here I think:

private void updateTabStyles() {
    for (int i = 0; i < tabCount; i++) {
        View v = tabsContainer.getChildAt(i);
        v.setBackgroundResource(tabBackgroundResId);
        v.setPadding(tabPadding, v.getPaddingTop(), tabPadding, v.getPaddingBottom());
        TextView tab_title = (TextView) v.findViewById(R.id.psts_tab_title);

        if (tab_title != null) {
        ...

or more specifically:

View v = tabsContainer.getChildAt(i);
v.setBackgroundResource(tabBackgroundResId);
v.setPadding(tabPadding, v.getPaddingTop(), tabPadding, v.getPaddingBottom());

where it updates your tab regardless of ID. If a custom tab was supplied, the library should do no styling on the tab itself.

ataulm commented 9 years ago

51 seems to use `updateSelection(int position)` (which sets the selected state) in only one place, but there are other places where `selected(int position)` and `unselected(int position)` are used directly, meaning the selected state won't be set correctly (I think, haven't looked too long at it).

ouchadam commented 9 years ago

I like the idea of the CustomTabProvider being given all of the major callbacks for tab selection, style reseting etc

Nutomic commented 9 years ago

Sorry for the late reply.

I tried to use an ID different than tab_title. The problem with that is that no font style is applied at all. Imo it would be nice to have the default style applied, and then apply my own costumizations afterwards.

One possibility for this would be changing updateTabStyles() so that it's public and can be called from getCustomTabView(), or call it here if CustomTabProvider is not implemented.

I hope you undestand what I mean, otherwise I could just make a PR.

camposbrunocampos commented 9 years ago

Hello Guys, turns out that there is a better solution. You could just create a selector resource inside res/colors/ and use it in your PagerSlidingStrip android:textColor attribute. I`ll print the code below. The text will change its color like a selector button, simple like that. There is no need of changing the lib.

res/color/btn

<?xml version="1.0" encoding="utf-8"?>
<selector xmlns:android="http://schemas.android.com/apk/res/android">
    <item android:state_pressed="true"
        android:color="#000000" /> <!-- pressed -->
    <item android:state_focused="true"
        android:color="@color/active_text" /> <!-- focused -->
    <item android:color="#FFFFFF" /> <!-- default -->
</selector>

and here is where I use this resource in andorid:textColor attribute

slideFragment.xml

<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:app="http://schemas.android.com/apk/res-auto"
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    android:orientation="vertical">
    <com.astuetz.PagerSlidingTabStrip
        android:id="@+id/pager_title"
        android:layout_width="match_parent"
        style="@style/PagerTitleStrip"
        android:layout_gravity="top"
        android:textColor="@color/btn"
        android:textSize="13sp"
        android:paddingBottom="@dimen/padding_logo"
        android:paddingTop="@dimen/padding_logo"
        app:pstsIndicatorColor="@color/border_title_strip"
        android:layout_height="@dimen/height_pager_title_strip"/>
    <android.support.v4.view.ViewPager
        android:id="@+id/pager"
        android:layout_width="match_parent"
        android:layout_height="match_parent"
        android:layout_weight="1">

    </android.support.v4.view.ViewPager>
</LinearLayout>

Nutomic commented 9 years ago

@camposbrunocampos If I understand this correct, it changes the color for all tabs. I only want to change a single tab's color (so there are different colors).

camposbrunocampos commented 9 years ago

@Nutomic Yes, you are right, only use this solution if you want to change the textColor for all tabs.

jpardogo commented 9 years ago

What about this:

    private void notSelected(View tab) {
        if (tab != null) {
            TextView title = (TextView) tab.findViewById(R.id.psts_tab_title);
            if (title != null) {
                if(usingCustomTabs){
                    if(!((CustomTabProvider)pager.getAdapter()).setNotSelectedTabTypeface(title)){
                        setTitleTypeface(title,tabTypefaceStyle);
                    }
                    if(!((CustomTabProvider)pager.getAdapter()).setNotSelectedTextColor(title)){
                        setTitleTextColor(title,tabTextColor);
                    }
                }else{
                    setTitleTypeface(title, tabTypefaceStyle);
                    setTitleTextColor(title,tabTextColor);
                }
            }
        }
    }

    private void selected(View tab) {
        if (tab != null) {
            TextView title = (TextView) tab.findViewById(R.id.psts_tab_title);
            if (title != null) {
                if(usingCustomTabs){
                    if(!((CustomTabProvider)pager.getAdapter()).setSelectedTabTypeface(title)){
                        setTitleTypeface(title,tabTypefaceSelectedStyle);
                    }
                    if(!((CustomTabProvider)pager.getAdapter()).setSelectedTextColor(title)){
                        setTitleTextColor(title,tabTextColorSelected);
                    }
                }else{
                    setTitleTypeface(title, tabTypefaceSelectedStyle);
                    setTitleTextColor(title,tabTextColorSelected);
                }
            }
        }
    }

    private void setTitleTextColor(TextView title, ColorStateList tabTextColorSelected) {
        title.setTextColor(tabTextColorSelected);
    }

    private void setTitleTypeface(TextView title, int tabTypefaceSelectedStyle) {
        title.setTypeface(tabTypeface, tabTypefaceSelectedStyle);
    }

So if you are using custom tabs you will have the callbacks to apply your own style to the typeface or the color of the text. If you return false on your callback the default implementation will be apply. The interface would be like that:

   public interface CustomTabProvider {
        View getCustomTabView(ViewGroup parent, int position);

        boolean setSelectedTabTypeface(TextView textView);
        boolean setSelectedTextColor(TextView textView);

        boolean setNotSelectedTextColor(TextView textView);
        boolean setNotSelectedTabTypeface(TextView textView);
    }

This should solve the problem.

jpardogo commented 9 years ago

@Nutomic @ouchadam @ataulm @camposbrunocampos

What do you think?

ataulm commented 9 years ago

Hi @jpardogo

For me, this feels like patching over an issue with the library - as you're starting to see, the base approach is making it difficult to satisfy the constraints from everyone using it, while still keeping it easy enough to maintain.

IMHO, I'd take a look at the base approach to styling. The main function of the library is to provide the sliding tabs, which - when clicked - change the viewpager's current item and when the viewpager's current item is changed, the correct tab is selected.

If the library doesn't apply any styles whatsoever to the tabs, then most of the custom requests for the library/bugs also disappear. Just ask for the layout for a tab (which you do), and treat it as the tab. You can either assume that the root is a TextView, and thus still keep setTitle(), or you can also ask for the ID of the view to direct setTitle() to (like the old SimpleCursorListAdapter from back in the day).

This is the approach @ouchadam and I have taken for our projects (we couldn't wait for this fix on this library, sorry) and the result is a much leaner class, with vastly fewer attributes, easier to maintain - though at the tradeoff that it's less tested (only has 2 projects using it) and doesn't match the feature set.

Nutomic commented 9 years ago

@jpardogo I agree with autaim that your approach seems overly complicated (consider if someone wants to change text size, or any of the dozen other attributes).

@ataulm I found your code here, but am not so familiar with styles. Do you have any example code?

jpardogo commented 9 years ago

The main function of the library is to provide the sliding tabs, which - when clicked - change the viewpager's current item and when the viewpager's current item is changed, the correct tab is selected.

@ataulm In general, I completely agree with your approach. I think I wrote about it before in another issue, but maybe not for this library. The reason is that this library have a 2 strong points to achieve:

The one you mention (No difference from the original one, both try that)
The main reason I did this fork for. A way to style in an easy way without thinking much about it. Just place your layout and the tabs will be styled as your theme.

There are many people that won't bother going through the hassle of changing alphas, colours and styles and even less to control the selection or deselection of tabs. Basically I want to provide an easy way for people that don't know as much as a professional android developer know. It is true that I don't want to forget simplicity, cleanliness and good practices but I want to find a middle way, which is exactly the challenge.

It is not that I want to add more attributes to the library but I think I would like to maintain the ones we have:

pstsTextAlpha Set the text alpha transparency for non selected tabs. Range 0..255. 150 is it's default value. It WON'T be used if textColor is defined in the layout. If textColor is NOT defined, It will be applied to the non selected tabs.
pstsTextColorSelected Set selected tab text color. textPrimaryColor will be it's default color value.
pstsTextStyle Set the text style, default normal on API 21, bold on older APIs.
pstsTextSelectedStyle Set the text style of the selected tab, default normal on API 21, bold on older APIs.
pstsTextFontFamily Set the font family name. Default sans-serif-medium on API 21, sans-serif on older APIs.
pstsPaddingMiddle If true, the tabs start at the middle of the view (Like Newsstand google app).

    public void setTypeface(Typeface typeface, int style, int selectedStyle) {
        this.tabTypeface = typeface;
        this.tabTypefaceSelectedStyle = selectedStyle;
        this.tabTypefaceStyle = style;
        updateTabStyles();
    }

This last function actually solve the issue for custom fonts, although we can keep just callbacks as selected and unselected return true or false.

A part of this attributes and the way we use the already existing ones, the library is the same as the original.

Anyway I need to go trough the whole class and think about several stuff and clean it up... I didn't have much time lately but at some point I will try to find this middle point between simple clean class and auto styled tabs.

ataulm commented 9 years ago

@Nutomic the library we're using is not publicly released - the link you found is a fork of this library.

@jpardogo a default style is perfect for those that don't want to provide their own, and I agree with you on this! :dancer: I would ensure that the default style isn't given any unnecessary advantages just because it's part of this library - i.e. keep the sliding tabs functionality entirely separate from the styles you supply, and force it to go through the same mechanism that other users of the library have to go through to style their tabs.

This would mean no modifying the tabs after they are inflated - limiting yourself to changing the tabs' states (selected/focused/checked etc.). For example, the default implementation could inflate a tab as you do, but all the styles for that should be isolated in the layout.xml of the tab you inflate.

This last function actually solve the issue for custom fonts, although we can keep just callbacks as selected and unselected return true or false.

If you saw code that was flipping the styles/appearance of a View in a callback based on the view's state, would that appear fishy to you? I wouldn't (and thus didn't) use a library that forced me to programmatically set styles where elsewhere in my app I'm using widgets that react correctly to view states (ListView, RecyclerView, Buttons, EditText, etc.).

jpardogo commented 9 years ago

@Nutomic @ataulm After reviewing our old conversation on this issue, and after thinking of the pros and cons, I think we have arrived to a good middle point. So I have just pushed (d8f1a1efc4c573f16e684c49f03105d627226f5a) a new branch. This is a massive API change but I think It is for good (Even if the people complain too much :P ). We still can clean and refactor several things but first thing would be to agree in the functionality and merge with dev.

Cheers for your help! Really appreciate it. :+1:

Nutomic commented 9 years ago

Actually, I misunderstood code @jpardogo. What we need is not a seperate style for the active tab, but for the first tab (and only under some conditions).

So I think the only option for that would be an optional callback where I can set the style myself.

jpardogo commented 9 years ago

@Nutomic What do you mean separate style for active tab or first tab?

Now we are working with selectors and view states, no modifying the tabs after they are inflated and adding callbacks for custom tabs (selected/deselected).

Did you read the code for the branch iss68_iss70? and this commit?

Clone the branch and try the code.

jpardogo commented 9 years ago

iss68_iss70 merged on dev. It will be in for the next release 679a74e5d0cf45674efe0985fc706f0015ec22d1

jpardogo / PagerSlidingTabStrip

Style from `CustomTabProvider` is overwritten #68

51 seems to use `updateSelection(int position)` (which sets the selected state) in only one place, but there are other places where `selected(int position)` and `unselected(int position)` are used directly, meaning the selected state won't be set correctly (I think, haven't looked too long at it).

res/color/btn

slideFragment.xml

jpardogo / PagerSlidingTabStrip

Style from `CustomTabProvider` is overwritten #68

51 seems to use updateSelection(int position) (which sets the selected state) in only one place, but there are other places where selected(int position) and unselected(int position) are used directly, meaning the selected state won't be set correctly (I think, haven't looked too long at it).

res/color/btn

slideFragment.xml

51 seems to use `updateSelection(int position)` (which sets the selected state) in only one place, but there are other places where `selected(int position)` and `unselected(int position)` are used directly, meaning the selected state won't be set correctly (I think, haven't looked too long at it).