Skip to content
This repository has been archived by the owner on Apr 19, 2018. It is now read-only.

New feature: TitleBarIndicator can now split titles around their center #104

Closed
wants to merge 3 commits into from

Conversation

mttkay
Copy link

@mttkay mttkay commented Jun 25, 2012

New attribute clipToCenter[BOOL] makes titles in TitlePageIndicator split around their center point.

This is useful if you don't know in advance how long your page labels are going to be, or if you have long titles that constantly bump into one another.

First commit contains all the feature changes. The second commit contains a code refactoring, since I found onDraw was barely legible anymore.

Tested on Nexus One and Galaxy Nexus.

I couldn't add a sample Activity, since the sample app doesn't compile for me. I think I opened an issue for that already.

…split around their center point.

This is useful if you don't know in advance how long your page labels are going to be, or if you have long titles that constantly bump into one another.
That method was barely readable anymore, so I broke some key code paths down into smaller methods.
@buildhive
Copy link

Jake Wharton » Android-ViewPagerIndicator #4 FAILURE
Looks like there's a problem with this pull request
(what's this?)

@mttkay
Copy link
Author

mttkay commented Jun 25, 2012

I get that same build failure on my dev machine, but I think it's unrelated to my commits. It's missing an artifact which is flagged as deprecated on the project page:

Failed to execute goal on project sample: Could not resolve dependencies for project com.viewpagerindicator:sample:apk:2.3.1: Could not find artifact com.directionalviewpager:library:jar:1.2.0

@JakeWharton
Copy link
Owner

It's fixed on the dev branch which is the one development (and patches) occur on. You can create a new pull against dev if you'd like but it's certainly not necessary since I can just review and merge there manually. I'll try to get to this tonight. Looks good from a quick glance.

@mttkay
Copy link
Author

mttkay commented Jun 25, 2012

Thanks Jake--if you're interested, here's a screendump of how it looks like: https://skitch.com/kaeppler/en6cc/title-bar-indicator

I'm still trying to figure out how to make fading edges work (I think the custom onDraw breaks that), but that's unrelated.

@mttkay
Copy link
Author

mttkay commented Jun 26, 2012

Hold on with merging this back actually, I have a very small patch upcoming that will make fading edges work. This goes very well visually with the TitleBarIndicator and the centerClip so I guess it's fine to merge this as part of centerClip.

You can enable fading edges using standard Android attributes:
android:fadingEdge="horizontal"
android:fadingEdgeLength="30dp"

There is one additional attribute that is usually computed by Android by inspecting scroll state, but since there is no scrolling in TitlePageIndicator, we need to inject this manually via a custom attribute:

fadingEdgeStrength="0.5"

This is a value between 0 and 1, and defaults to 1. It affects how long/strong the fading effect is.
@buildhive
Copy link

Jake Wharton » Android-ViewPagerIndicator #5 FAILURE
Looks like there's a problem with this pull request
(what's this?)

@mttkay
Copy link
Author

mttkay commented Jul 27, 2012

Hey Jake, have you had a chance to review this? We're about to release it with our app next week.

@mttkay
Copy link
Author

mttkay commented Jul 27, 2012

One thing to note: to enable fading edges, one must use a different Android attribute on ICS and beyond. On Gingerbread, one must set fadingEdge, while on ICS one must use requiresFadingEdge.

In our app I simply set both in the style sheet to make sure it works on all platform versions (or alternatively, provide a custom v14 style sheet.)

@scompt
Copy link
Contributor

scompt commented Jul 30, 2012

Cherry-picking the b0d029d commit worked great for me to add faded edges to my app. It might be good to add something to the sample app demonstrating this behavior since it's not immediately obvious that it's possible.

@JakeWharton
Copy link
Owner

I'm trying to bring in your fading-edge stuff now but it has a few problems:

  1. Fading edge needs to be enabled when mFadingEdgeStrength is greater than 0.
  2. There is no getter nor setter for the property.
  3. The fading affects the underline too which we don't want.

private void clipToCenter(Rect curViewBound, int edge) {
int width = curViewBound.width();
curViewBound.left = (int) (edge - (width / 2));
curViewBound.right = (int) (edge + (width / 2));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't calculate width / 2 twice.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seriously? I think this makes it easier to read than using a halfWidth or widthByTwo variable, and I don't think the CPU will sweat over dividing a number by 2...

If you feel strong about it, I can change it though. I usually code with the mindset to strive for easier readability of code rather than micro improving expressions that the JVM will optimize away anyway.

@JakeWharton
Copy link
Owner

I would've preferred these three commit as separate pull requests. Much easier to review and get merged.

@mttkay
Copy link
Author

mttkay commented Sep 2, 2012

Yeah sorry for that, will keep that in mind. To answer your other remarks:

ad 1) Yes of course it does, to get fading edges, you enable it first... that's how it works in all Android widgets that support fading edges. The strength attribute is merely a modifier when fading edges are enabled and it defaults to a standard value so setting it is not mandatory. See http://developer.android.com/reference/android/R.attr.html#fadingEdge and fadingEdgeLength for an equivalent example.

ad 2) Good point, I'll add them.

ad 3) Why not? It would be odd to fade the text, but not the underline. The fading edge effect in Android always fades everything. We should stick to that behavior.

@SimonVT
Copy link
Contributor

SimonVT commented Sep 2, 2012

The underline serves as a static divider between the titlepager and the content. It doesn't make sense to fade it, plus it'll look horrible.

@Shusshu
Copy link
Contributor

Shusshu commented Apr 9, 2013

bump

@mttkay mttkay closed this Feb 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants