Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
465493002:
Font Size UI for Distilled Pages (Closed)
|
Created:
6 years, 4 months ago by smaslo Modified:
6 years, 4 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionFont Size UI for Distilled Pages
Adds the ability for users to change the font size of distilled pages.
Adds SeekBar to layout and supporting observers.
BUG=383630
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289481
Patch Set 1 #
Total comments: 27
Patch Set 2 : Changes from comments #
Total comments: 2
Patch Set 3 : Changing comment for string #
Total comments: 2
Patch Set 4 : Nit from comment #
Total comments: 6
Patch Set 5 : Changes to Comments and equations #Patch Set 6 : More changes to comments #Patch Set 7 : Auto Bots #
Messages
Total messages: 18 (0 generated)
Nothing big. Just lots of little comments. https://codereview.chromium.org/465493002/diff/1/chrome/android/java/res/layo... File chrome/android/java/res/layout/distilled_page_prefs_view.xml (right): https://codereview.chromium.org/465493002/diff/1/chrome/android/java/res/layo... chrome/android/java/res/layout/distilled_page_prefs_view.xml:14: android:splitMotionEvents="false" Just curious: why does this need to be false? https://codereview.chromium.org/465493002/diff/1/chrome/android/java/res/layo... chrome/android/java/res/layout/distilled_page_prefs_view.xml:19: android:id="@+id/radio_button_group" put id before layout_width https://codereview.chromium.org/465493002/diff/1/chrome/android/java/res/layo... chrome/android/java/res/layout/distilled_page_prefs_view.xml:45: android:id="@+id/font_size_percentage" Setting "sp" units in the width is a bit unusual. We'd typically use a minWidth and some padding/margin in a case like this. Does that work for you? https://codereview.chromium.org/465493002/diff/1/chrome/android/java/res/layo... chrome/android/java/res/layout/distilled_page_prefs_view.xml:48: android:orientation="horizontal" "orientation" doesn't apply to TextView, or any of the views below. These should all be removed. https://codereview.chromium.org/465493002/diff/1/chrome/android/java/res/layo... chrome/android/java/res/layout/distilled_page_prefs_view.xml:56: android:gravity="center" Use center_vertical to be more explicit with your intentions. Same below. https://codereview.chromium.org/465493002/diff/1/chrome/android/java/res/layo... chrome/android/java/res/layout/distilled_page_prefs_view.xml:59: android:text="A" /> I suspect that different letters would be appropriate for users in other locales. You should add a string to chrome_android_strings.grd and use that here (and below), so that it can be localized. https://codereview.chromium.org/465493002/diff/1/chrome/android/java/res/layo... chrome/android/java/res/layout/distilled_page_prefs_view.xml:65: android:layout_weight="4" Just use layout_weight="1". It's equivalent and doesn't leave the reader wondering, why the number "4"? https://codereview.chromium.org/465493002/diff/1/chrome/android/java/src/org/... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java (right): https://codereview.chromium.org/465493002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:47: // Text field showing font zoom percentage. s/zoom/size/g I'd say "font size" or "font scale" instead of "font zoom" throughout this file to be consistent with terminology elsewhere. https://codereview.chromium.org/465493002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:48: private TextView mFontZoomPercentage; These two views (mFontZoomPercentage and mFontZoom) have confusingly similar names. How about mFontSizeTextView and mFontSizeSlider or something along those lines? https://codereview.chromium.org/465493002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:88: float initialValue = mFontSizePrefs.getFontScaleFactor(); You could replace lines 88-90 with: onChangeFontSize(mFontSizePrefs.getFontScaleFactor()); https://codereview.chromium.org/465493002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:156: public void onStartTrackingTouch(SeekBar seekBar) {}; remove ";". same below https://codereview.chromium.org/465493002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:166: setFontZoomProgress(newFontSize); Changing the seekbar value will cause the textview to be updated automatically. So you don't technically need to call setFontZoomPercentage() here, though you might want to add a comment since that's not obvious. Alternatively (and I like this better), you could return early in onProgressChanged() if fromUser is false. https://codereview.chromium.org/465493002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:204: mFontZoomPercentage.setText(df.format(percentage) + "%"); Be careful: in Turkish, the percent sign comes before the number instead of after. What? Srsly. Luckily, Java has built-in support for formatting percentages: NumberFormat percentageFormatter = NumberFormat.getPercentInstance(Locale.getDefault()); mFontZoomPercentage.setText(percentageFormatter.format(percentage)); Note: You don't need to multiply the percentage by 100 beforehand. Also, I think NumberFormat is a pretty heavy object, so you should probably store a single instance as a member variable instead of calling NumberFormat.getPercentInstance() each time this method is called. (Code borrowed from https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... )
https://codereview.chromium.org/465493002/diff/1/chrome/android/java/src/org/... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java (right): https://codereview.chromium.org/465493002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:166: setFontZoomProgress(newFontSize); On 2014/08/11 17:15:30, newt wrote: > Changing the seekbar value will cause the textview to be updated automatically. > So you don't technically need to call setFontZoomPercentage() here, though you > might want to add a comment since that's not obvious. Alternatively (and I like > this better), you could return early in onProgressChanged() if fromUser is > false. Yeah, that sounds reasonable.
https://codereview.chromium.org/465493002/diff/1/chrome/android/java/res/layo... File chrome/android/java/res/layout/distilled_page_prefs_view.xml (right): https://codereview.chromium.org/465493002/diff/1/chrome/android/java/res/layo... chrome/android/java/res/layout/distilled_page_prefs_view.xml:14: android:splitMotionEvents="false" Done. On 2014/08/11 17:15:30, newt wrote: > Just curious: why does this need to be false? https://codereview.chromium.org/465493002/diff/1/chrome/android/java/res/layo... chrome/android/java/res/layout/distilled_page_prefs_view.xml:19: android:id="@+id/radio_button_group" On 2014/08/11 17:15:30, newt wrote: > put id before layout_width Done. https://codereview.chromium.org/465493002/diff/1/chrome/android/java/res/layo... chrome/android/java/res/layout/distilled_page_prefs_view.xml:45: android:id="@+id/font_size_percentage" On 2014/08/11 17:15:30, newt wrote: > Setting "sp" units in the width is a bit unusual. We'd typically use a minWidth > and some padding/margin in a case like this. Does that work for you? Done. https://codereview.chromium.org/465493002/diff/1/chrome/android/java/res/layo... chrome/android/java/res/layout/distilled_page_prefs_view.xml:48: android:orientation="horizontal" On 2014/08/11 17:15:30, newt wrote: > "orientation" doesn't apply to TextView, or any of the views below. These should > all be removed. Done. https://codereview.chromium.org/465493002/diff/1/chrome/android/java/res/layo... chrome/android/java/res/layout/distilled_page_prefs_view.xml:56: android:gravity="center" On 2014/08/11 17:15:30, newt wrote: > Use center_vertical to be more explicit with your intentions. Same below. Done. https://codereview.chromium.org/465493002/diff/1/chrome/android/java/res/layo... chrome/android/java/res/layout/distilled_page_prefs_view.xml:59: android:text="A" /> On 2014/08/11 17:15:30, newt wrote: > I suspect that different letters would be appropriate for users in other > locales. You should add a string to chrome_android_strings.grd and use that here > (and below), so that it can be localized. Done. https://codereview.chromium.org/465493002/diff/1/chrome/android/java/res/layo... chrome/android/java/res/layout/distilled_page_prefs_view.xml:65: android:layout_weight="4" On 2014/08/11 17:15:30, newt wrote: > Just use layout_weight="1". It's equivalent and doesn't leave the reader > wondering, why the number "4"? Done. https://codereview.chromium.org/465493002/diff/1/chrome/android/java/src/org/... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java (right): https://codereview.chromium.org/465493002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:47: // Text field showing font zoom percentage. On 2014/08/11 17:15:30, newt wrote: > s/zoom/size/g > > I'd say "font size" or "font scale" instead of "font zoom" throughout this file > to be consistent with terminology elsewhere. Done. https://codereview.chromium.org/465493002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:48: private TextView mFontZoomPercentage; On 2014/08/11 17:15:30, newt wrote: > These two views (mFontZoomPercentage and mFontZoom) have confusingly similar > names. How about mFontSizeTextView and mFontSizeSlider or something along those > lines? Done. https://codereview.chromium.org/465493002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:88: float initialValue = mFontSizePrefs.getFontScaleFactor(); On 2014/08/11 17:15:30, newt wrote: > You could replace lines 88-90 with: > > onChangeFontSize(mFontSizePrefs.getFontScaleFactor()); Done. https://codereview.chromium.org/465493002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:156: public void onStartTrackingTouch(SeekBar seekBar) {}; On 2014/08/11 17:15:30, newt wrote: > remove ";". same below Done. https://codereview.chromium.org/465493002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:166: setFontZoomProgress(newFontSize); I tried removing the line with setFontZoomPercentage entirely, but then the textview did not update. On 2014/08/11 20:53:24, nyquist wrote: > On 2014/08/11 17:15:30, newt wrote: > > Changing the seekbar value will cause the textview to be updated > automatically. > > So you don't technically need to call setFontZoomPercentage() here, though you > > might want to add a comment since that's not obvious. Alternatively (and I > like > > this better), you could return early in onProgressChanged() if fromUser is > > false. > > Yeah, that sounds reasonable. https://codereview.chromium.org/465493002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:204: mFontZoomPercentage.setText(df.format(percentage) + "%"); On 2014/08/11 17:15:30, newt wrote: > Be careful: in Turkish, the percent sign comes before the number instead of > after. What? Srsly. Luckily, Java has built-in support for formatting > percentages: > > NumberFormat percentageFormatter = > NumberFormat.getPercentInstance(Locale.getDefault()); > mFontZoomPercentage.setText(percentageFormatter.format(percentage)); > > Note: You don't need to multiply the percentage by 100 beforehand. Also, I think > NumberFormat is a pretty heavy object, so you should probably store a single > instance as a member variable instead of calling > NumberFormat.getPercentInstance() each time this method is called. > > (Code borrowed from > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... > ) Done.
lgtm after the string description update. Thanks! https://codereview.chromium.org/465493002/diff/40001/chrome/android/java/stri... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/465493002/diff/40001/chrome/android/java/stri... chrome/android/java/strings/android_chrome_strings.grd:366: <message name="IDS_TEXT_SIZE_SIGNIFIER" desc="Used to signify that a SeekBar is for font size. A small letter is placed on one end and a big letter is placed on the other end"> I think this description can be a little clearer. Translators will have no clue what a "SeekBar" is, so I'd avoid that terminology. Maybe: "A typical letter from the alphabet. This is drawn on either side of a slider bar that allows the user to change the font size. On one side, it's drawn smaller and on the other larger."
https://codereview.chromium.org/465493002/diff/40001/chrome/android/java/stri... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/465493002/diff/40001/chrome/android/java/stri... chrome/android/java/strings/android_chrome_strings.grd:366: <message name="IDS_TEXT_SIZE_SIGNIFIER" desc="Used to signify that a SeekBar is for font size. A small letter is placed on one end and a big letter is placed on the other end"> On 2014/08/12 02:38:49, newt wrote: > I think this description can be a little clearer. Translators will have no clue > what a "SeekBar" is, so I'd avoid that terminology. Maybe: > > "A typical letter from the alphabet. This is drawn on either side of a slider > bar that allows the user to change the font size. On one side, it's drawn > smaller and on the other larger." Done.
https://codereview.chromium.org/465493002/diff/60001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java (right): https://codereview.chromium.org/465493002/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:151: SeekBar seekBar, int progress, boolean fromUser) { Nit: Does this fit within 100 chars on the previous line?
https://codereview.chromium.org/465493002/diff/60001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java (right): https://codereview.chromium.org/465493002/diff/60001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:151: SeekBar seekBar, int progress, boolean fromUser) { On 2014/08/12 21:59:46, nyquist wrote: > Nit: Does this fit within 100 chars on the previous line? Done.
lgtm
lgtm with nits. https://codereview.chromium.org/465493002/diff/80001/chrome/android/java/res/... File chrome/android/java/res/layout/distilled_page_prefs_view.xml (right): https://codereview.chromium.org/465493002/diff/80001/chrome/android/java/res/... chrome/android/java/res/layout/distilled_page_prefs_view.xml:27: <RadioButton Fix spacing. https://codereview.chromium.org/465493002/diff/80001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java (right): https://codereview.chromium.org/465493002/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:151: float newValue = (progress * 5 + 50) / 100f; Quick comment on this calculation would be nice. https://codereview.chromium.org/465493002/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:195: int progress = (int) (newValue * 20 - 10); Add a comment on why we multiply by 20 and subtract 10.
https://codereview.chromium.org/465493002/diff/80001/chrome/android/java/res/... File chrome/android/java/res/layout/distilled_page_prefs_view.xml (right): https://codereview.chromium.org/465493002/diff/80001/chrome/android/java/res/... chrome/android/java/res/layout/distilled_page_prefs_view.xml:27: <RadioButton On 2014/08/13 01:14:54, robliao wrote: > Fix spacing. Done. https://codereview.chromium.org/465493002/diff/80001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java (right): https://codereview.chromium.org/465493002/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:151: float newValue = (progress * 5 + 50) / 100f; On 2014/08/13 01:14:54, robliao wrote: > Quick comment on this calculation would be nice. Done. https://codereview.chromium.org/465493002/diff/80001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/DistilledPagePrefsView.java:195: int progress = (int) (newValue * 20 - 10); On 2014/08/13 01:14:54, robliao wrote: > Add a comment on why we multiply by 20 and subtract 10. Done.
thanks for the comments. they were very helpful!
The CQ bit was checked by smaslo@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/smaslo@chromium.org/465493002/180001
The CQ bit was checked by smaslo@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/smaslo@chromium.org/465493002/200001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
Message was sent while issue was closed.
Committed patchset #7 (200001) as 289481 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
