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

Style from CustomTabProvider is overwritten #68

Closed
Nutomic opened this issue Feb 18, 2015 · 21 comments
Closed

Style from CustomTabProvider is overwritten #68

Nutomic opened this issue Feb 18, 2015 · 21 comments
Assignees
Labels
Milestone

Comments

@Nutomic
Copy link

Nutomic commented Feb 18, 2015

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
Copy link

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

@jpardogo
Copy link
Owner

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
Copy link
Owner

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
Copy link

ataulm commented Feb 25, 2015

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.

@jpardogo jpardogo reopened this Feb 26, 2015
@ataulm
Copy link

ataulm commented Feb 28, 2015

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
Copy link

ataulm commented Feb 28, 2015

#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
Copy link

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

@Nutomic
Copy link
Author

Nutomic commented Mar 2, 2015

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
Copy link

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
Copy link
Author

Nutomic commented Mar 10, 2015

@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
Copy link

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

@jpardogo jpardogo added this to the 1.1 milestone Mar 12, 2015
@jpardogo jpardogo added the bug label Mar 12, 2015
@jpardogo
Copy link
Owner

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
Copy link
Owner

@Nutomic
@ouchadam
@ataulm
@camposbrunocampos

What do you think?

@ataulm
Copy link

ataulm commented May 15, 2015

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
Copy link
Author

Nutomic commented May 18, 2015

@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
Copy link
Owner

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
Copy link

ataulm commented May 18, 2015

@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! 💃 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 added a commit that referenced this issue May 20, 2015
- Change order attr. Now the order is the same in attr.xml
and java class constructor.
- Better attr names, every attr related with tabs start with pstsTab
- Android attr only use for container
- No flipping the styles/appearance on selection/deselection tabs
- Remove custom attire `pstsTextColorSelected ` and
`pstsTextSelectedStyle `
- Callbacks for selection/deselection of tabs passing the tab view as
parameter when custom tabs are use.
@jpardogo
Copy link
Owner

@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 (d8f1a1e) 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. 👍

@Nutomic
Copy link
Author

Nutomic commented May 21, 2015

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
Copy link
Owner

@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
Copy link
Owner

jpardogo commented Jun 1, 2015

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

@jpardogo jpardogo closed this as completed Jun 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants