Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12521025:
Added fadeout effect for textview in navigation popup. Created widget for fade-out effect. (Closed)
|
Created:
7 years, 9 months ago by apiccion Modified:
7 years, 7 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdded fadeout effect for textview in navigation popup. Created widget for fade-out effect.
BUG=181222
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199280
Patch Set 1 #Patch Set 2 : Forgot to remove logs. #Patch Set 3 : Fixed spelling. Added setSingleLine assert guard to FadeOutTextView #
Total comments: 9
Patch Set 4 : Removed complicated fader implementation. Applied Ted's simpler fix :) #
Total comments: 4
Patch Set 5 : Refactored creation of list item TextView and computation of dimensions into its own privat class. … #Patch Set 6 : Removed Ellipses #
Total comments: 1
Patch Set 7 : #Patch Set 8 : #Messages
Total messages: 16 (0 generated)
lgtm but let Ted take a look before landing.
I hope my comment in NavigationPopup is correct and this can be a simple 3 line change. https://codereview.chromium.org/12521025/diff/4001/chrome/android/java/src/or... File chrome/android/java/src/org/chromium/chrome/browser/FadeOutTextView.java (right): https://codereview.chromium.org/12521025/diff/4001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/FadeOutTextView.java:26: * Only support single line textview. Multi-line is possible but would require I think the /* */ style comments are overkill here. Use // instead since it's only two lines. https://codereview.chromium.org/12521025/diff/4001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/FadeOutTextView.java:47: * We can move this to another class/file if we need more TextViews with multiple styles. the second line seems like a comment to go in a review not in the code. https://codereview.chromium.org/12521025/diff/4001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/FadeOutTextView.java:58: public static Shader createShader(TextView textView) { Won't this fade text even if it's exactly (or slightly less) the same width as the Textview. Shouldn't it only be applying the shader if the text is longer? https://codereview.chromium.org/12521025/diff/4001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/FadeOutTextView.java:59: assert textView.getWidth() > 0 if getWidth() == 0, should you return? https://codereview.chromium.org/12521025/diff/4001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/FadeOutTextView.java:60: : "textView layout should be complete before this invocation."; +4 indent https://codereview.chromium.org/12521025/diff/4001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/FadeOutTextView.java:63: - textView.getTotalPaddingRight() - textView.getTotalPaddingLeft(); +4 indent https://codereview.chromium.org/12521025/diff/4001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/FadeOutTextView.java:64: int gradientStart = gradientStop - (int)(TEXT_FADEOUT_WIDTH_DP * density); space after the cast (int) https://codereview.chromium.org/12521025/diff/4001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/FadeOutTextView.java:67: new int[]{textView.getCurrentTextColor(), 0xff101010, 0x00101010, 0x00101010}, space here and the line below after the [] https://codereview.chromium.org/12521025/diff/4001/chrome/android/java/src/or... File chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java (right): https://codereview.chromium.org/12521025/diff/4001/chrome/android/java/src/or... chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:186: view.setMinimumHeight(mListItemHeight); can you confirm that adding these (or something like them) doesn't achieve this for you already: view.setEllipsize(android.text.TextUtils.TruncateAt.MARQUEE); view.setFadingEdgeLength(25); // should be compensated for density view.setHorizontalFadingEdgeEnabled(true);
Make sure you press "Publish+Mail Comments" again otherwise nobody knows when you upload another patch to this issue! https://codereview.chromium.org/12521025/diff/11001/chrome/android/java/src/o... File chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java (right): https://codereview.chromium.org/12521025/diff/11001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:152: int fadePadding = (int)(fadeLength * (1 - fadeStop)); We should only have to calculate this once. I don't know where the best place to put it is, but it would be nice if we didn't recalculate the same values for every entry/menu. https://codereview.chromium.org/12521025/diff/11001/chrome/android/java/src/o... chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:163: view.setPadding(mPadding, 0, mPadding + fadePadding , 0); We should make sure we support RTL here. You could check for RTL via LocalizationUtils.isSystemLayoutDirectionRtl() and set the padding on the other side if necessary.
https://chromiumcodereview.appspot.com/12521025/diff/11001/chrome/android/jav... File chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java (right): https://chromiumcodereview.appspot.com/12521025/diff/11001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:152: int fadePadding = (int)(fadeLength * (1 - fadeStop)); On 2013/04/18 17:44:02, David Trainor wrote: > We should only have to calculate this once. I don't know where the best place > to put it is, but it would be nice if we didn't recalculate the same values for > every entry/menu. Done. https://chromiumcodereview.appspot.com/12521025/diff/11001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:163: view.setPadding(mPadding, 0, mPadding + fadePadding , 0); On 2013/04/18 17:44:02, David Trainor wrote: > We should make sure we support RTL here. You could check for RTL via > LocalizationUtils.isSystemLayoutDirectionRtl() and set the padding on the other > side if necessary. Done.
Removed ellipses truncation.
On 2013/05/08 21:40:45, apiccion wrote: > Removed ellipses truncation. I'm almost okay with this. The only issue I have is I want to be sure we won't fade things that fit in the space. Ie: If the text is 190dp and the area is 200dp, I don't want to fade starting at 175dp -> 200dp, since there would be no point. Does that make sense? Just want to confirm this works as expected.
On 2013/05/08 22:18:40, David Trainor wrote: > On 2013/05/08 21:40:45, apiccion wrote: > > Removed ellipses truncation. > > I'm almost okay with this. The only issue I have is I want to be sure we won't > fade things that fit in the space. Ie: If the text is 190dp and the area is > 200dp, I don't want to fade starting at 175dp -> 200dp, since there would be no > point. Does that make sense? Just want to confirm this works as expected. Yes I brought this exact issue up with design.
nit. but otherwise lgtm. https://chromiumcodereview.appspot.com/12521025/diff/19001/chrome/android/jav... File chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java (right): https://chromiumcodereview.appspot.com/12521025/diff/19001/chrome/android/jav... chrome/android/java/src/org/chromium/chrome/browser/NavigationPopup.java:176: float fadeLength = (25.0f * density); pull 25.0f to a constant.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/12521025/26001
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/12521025/26001
Failed to trigger a try job on android_dbg HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/12521025/45001
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/apiccion@chromium.org/12521025/45001
Message was sent while issue was closed.
Change committed as 199280 |
