Closed Bug 715179 Opened 8 years ago Closed 8 years ago

UI for changing default font size for font inflation

Categories

(Firefox for Android :: General, defect, P3)

All
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 17
Tracking Status
firefox13 --- wontfix
blocking-fennec1.0 --- -
fennec 16+ ---

People

(Reporter: padamczyk, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

(Whiteboard: [readability])

Attachments

(8 files, 62 obsolete files)

4.85 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
5.32 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
5.67 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
7.74 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
4.04 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
8.08 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
16.94 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
1.53 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
The basic font size setting has been implemented but should be expanded to included specified interaction / features. See screenshot:
http://www.flickr.com/photos/patrykdesign/6427428933/in/photostream
tracking-fennec: --- → 11+
Priority: -- → P3
Summary: Settings > Font Size (Feature Expansion) → UI for changing default font size for font inflation
Matt - I thought you already had a bug for this
Assignee: nobody → mbrubeck
(In reply to Mark Finkle (:mfinkle) from comment #1)
> Matt - I thought you already had a bug for this

Bug 703029 added a bare-bones UI with a standard menu list.  This bug is about replacing that basic list with the custom dialog from Patryk's mockup in comment 0.
OS: Mac OS X → Android
Hardware: x86 → All
Target Milestone: Firefox 11 → ---
Depends on: 703029, font-inflation
Whiteboard: [readability]
This just adds the new string from bug 703029 comment 7.  I hope to have the rest of the work done within a few days, but we could land this part early to make sure localization is able to start.
Attachment #588547 - Flags: review?(mark.finkle)
Comment on attachment 588547 [details] [diff] [review]
1/2: new string for preview dialog

This is fine, but I don't know if it needs to be landed before the rest of the code. L10N has told us the pre-landing strings without a way to see the strings in context is not the useful.
Attachment #588547 - Flags: review?(mark.finkle) → review+
Keywords: uiwanted
Whiteboard: [readability] → [readability][strings]
Keywords: late-l10n
Is this still 11 material?
(In reply to Axel Hecht [:Pike] from comment #5)
> Is this still 11 material?

IMO we can't really say until we see what is landed on m-c and evaluate the risk in taking it for fx11.
Mark, Erin, are we clearer on this now?
Not for 1.0 so won't land on aurora in april 2012
blocking-fennec1.0: --- → -
Soft blocker nom?

Seems like a bit of a stretch to get in, based on what I'm reading here, but it would be a nice and thoughtful piece of UI to have beyond our current selector menu.
blocking-fennec1.0: - → ?
We really can't add any UI for beta or for release. We should be back on the train schedule after we ship 1.0 of the rewrite so people can get this 6 weeks later.
blocking-fennec1.0: ? → -
Assignee: mbrubeck → nobody
Keywords: late-l10n, uiwanted
Whiteboard: [readability][strings] → [readability]
Version: Firefox 11 → Trunk
Assignee: nobody → mbrubeck
tracking-fennec: 11+ → 16+
Assignee: mbrubeck → michael.l.comella
I'm ready for the graphics to fill the text change buttons. Please attach them when they are ready.
Status: NEW → ASSIGNED
This change has been broken into many patches. Please wait for my comment when I am finished.
Attachment #588547 - Attachment is obsolete: true
This is the last patch. Note that while each patch should compile on its own, they are not completely functioning – a button could cause a NullPointerException and such.

Note that this changeset is not complete – the final images for the buttons are not finished yet so placeholder buttons with text are used instead. See the previously linked mockup to see an example of the images.
Also note that this only works on top of the patch from bug 770345, where a change of orientation does not restart the preferences activity.

I believe GeckoPreferences will throw a NullPointerException if the change font size buttons are pressed after an orientation change if this is used without this patch (due to onCreate and onWindowFocusChanged).
Attachment #640367 - Flags: feedback?(mbrubeck)
Attachment #640367 - Flags: feedback?(mbrubeck) → review?(bnicholson)
Attachment #640368 - Flags: review?(bnicholson)
Attachment #640369 - Flags: review?(bnicholson)
Attachment #640370 - Flags: review?(bnicholson)
Attachment #640371 - Flags: review?(bnicholson)
Attachment #640372 - Flags: review?(bnicholson)
Attachment #640373 - Flags: review?(bnicholson)
Attachment #640377 - Flags: review?(bnicholson)
Attachment #640367 - Flags: review?(bnicholson) → review+
Comment on attachment 640368 [details] [diff] [review]
02: Created FontSizePreference.java file.

You shouldn't need this - fold patch 4 into this one.
Attachment #640368 - Flags: review?(bnicholson) → review-
Comment on attachment 640369 [details] [diff] [review]
03: Fixed indentation and modified a few strings.

Review of attachment 640369 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/resources/values/arrays.xml
@@ +27,1 @@
>          <item>80</item>

Why did you change 0 to 40?

::: mobile/android/base/resources/xml/preferences.xml.in
@@ +35,5 @@
>          <ListPreference     android:key="font.size.inflation.minTwips"
>                              android:title="@string/pref_text_size"
>                              android:entries="@array/pref_font_size_entries"
>                              android:entryValues="@array/pref_font_size_values"
>                              android:persistent="false" />

This one should also be changed
Attachment #640369 - Flags: review?(bnicholson) → review-
(In reply to Brian Nicholson (:bnicholson) from comment #22)
> This one should also be changed

Wrong line...I was referring to this change:

-        <item>0</item>
+        <item>40</item>
Comment on attachment 640370 [details] [diff] [review]
04: Created a simple dialog with (non-functioning) Cancel and Set buttons.

Review of attachment 640370 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, except it should be folded into patch 2.

::: mobile/android/base/resources/xml/preferences.xml.in
@@ -35,5 @@
> -        <ListPreference     android:key="font.size.inflation.minTwips"
> -                            android:title="@string/pref_text_size"
> -                            android:entries="@array/pref_font_size_entries"
> -                            android:entryValues="@array/pref_font_size_values"
> -                            android:persistent="false" />

I guess you can ignore the last comment about indentation if you're removing this
Attachment #640370 - Flags: review?(bnicholson) → review+
Comment on attachment 640371 [details] [diff] [review]
05: Fleshed out the UI of the box - new buttons and preview TextView are present.

Review of attachment 640371 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/FontSizePreference.java
@@ +43,5 @@
> +            mIncreaseFontButton = (Button) dialogView.findViewById(R.id.increase_preview_font_button);
> +
> +            mPreviewFontView = (TextView) dialogView.findViewById(R.id.preview);
> +            mPreviewFontView.setMovementMethod(new ScrollingMovementMethod());
> +        } catch (InflateException ex) {

No need for this try/catch

::: mobile/android/base/resources/layout/font_size_preference.xml
@@ +12,5 @@
> +              android:layout_width="match_parent"
> +              android:layout_height="wrap_content"
> +              android:layout_weight="1"
> +              android:background="#ffffffff"
> +              android:padding="2pt"

Use dip rather than pt.

@@ +27,5 @@
> +        <Button android:id="@+id/decrease_preview_font_button"
> +                android:layout_width="0dp"
> +                android:layout_height="match_parent"
> +                android:layout_weight="1"
> +                android:text="A"

"A" should be localized as in patch 1.
Attachment #640371 - Flags: review?(bnicholson) → review-
Comment on attachment 640371 [details] [diff] [review]
05: Fleshed out the UI of the box - new buttons and preview TextView are present.

Review of attachment 640371 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/resources/layout/font_size_preference.xml
@@ +37,5 @@
> +                android:layout_height="match_parent"
> +                android:layout_weight="1"
> +                android:text="A"
> +                android:textSize="16sp"
> +                android:onClick="increasePreviewFontSize"/>

Also, I think it would be cleaner if you dropped the "increasePreviewFontSize" from here and GeckoPreferences. Just add the listeners in FontSizePreference so you don't have to forward them from GeckoPreferences.
(In reply to Brian Nicholson (:bnicholson) from comment #25)
> "A" should be localized as in patch 1.

I'm not sure we want to localize the "A" here, especially since it's just a placeholder until we have an image (which will also be a letter "A").
Comment on attachment 640372 [details] [diff] [review]
06: Dialog buttons work – they increase and decrease the size of the preview text.

Review of attachment 640372 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good overall if you remove your changes to GeckoPreferences.

::: mobile/android/base/GeckoPreferences.java
@@ +44,5 @@
>      private PreferenceScreen mPreferenceScreen;
>      private static boolean sIsCharEncodingEnabled = false;
>      private static final String NON_PREF_PREFIX = "android.not_a_preference.";
> +    private static final String FONT_SIZE_PREF_KEY = "font.size.inflation.minTwips";
> +    private FontSizePreference mFontSizePreference;

You shouldn't need mFontSizePreference if you remove these callbacks from GeckoPreference as described above.
Attachment #640372 - Flags: review?(bnicholson) → review+
(In reply to Matt Brubeck (:mbrubeck) from comment #27)
> I'm not sure we want to localize the "A" here, especially since it's just a
> placeholder until we have an image (which will also be a letter "A").

I take this back; keeping this as a localizable string seems like a good idea.  Just be sure to include an L10n note in the string file to explain to translators how this string is used.
(In reply to Brian Nicholson (:bnicholson) from comment #22)
> Comment on attachment 640369 [details] [diff] [review]
> 03: Fixed indentation and modified a few strings.
> 
> Review of attachment 640369 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/resources/values/arrays.xml
> @@ +27,1 @@
> >          <item>80</item>
> 
> Why did you change 0 to 40?

While I'm not sure what happens in Gecko, in the Android TextView I use to create the dialog, giving a font size of 0 pt causes no change – the font size will remain the previous value.

The choice to 40 was fairly arbitrary (I just wanted a number divisible by 20 so that it converts well from twips), though the text is considerably small on both my tablet and my phone.
Comment on attachment 640373 [details] [diff] [review]
07: Preferences are saved to Gecko.

Review of attachment 640373 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits fixed

::: mobile/android/base/FontSizePreference.java
@@ +38,5 @@
>      /** Index into the above arrays for the saved preference value (from Gecko). */
>      private int mSavedFontIndex = DEFAULT_FONT_INDEX;
>      /** Index into the above arrays for the currently displayed font size (the preview). */
>      private int mPreviewFontIndex = mSavedFontIndex;
> +

nit: remove blank line

@@ +89,5 @@
> +        if (prefChangeListener == null) {
> +            Log.e(LOGTAG, "PreferenceChangeListener is null. FontSizePreference will not be saved to Gecko.");
> +            return;
> +        }
> +        prefChangeListener.onPreferenceChange(this, (Object) twipVal);

You don't need this (Object) cast.

::: mobile/android/base/GeckoPreferences.java
@@ +171,5 @@
>              ((ListPreference)preference).setSummary(newEntry);
>          }
>          if (preference instanceof LinkPreference)
>              finish();
> +        if (preference instanceof FontSizePreference) {

Change these 'if's to be 'else if's.
Attachment #640373 - Flags: review?(bnicholson) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #22)
> Comment on attachment 640369 [details] [diff] [review]
> 03: Fixed indentation and modified a few strings.
> 
> Review of attachment 640369 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/resources/xml/preferences.xml.in
> @@ +35,5 @@
> >          <ListPreference     android:key="font.size.inflation.minTwips"
> >                              android:title="@string/pref_text_size"
> >                              android:entries="@array/pref_font_size_entries"
> >                              android:entryValues="@array/pref_font_size_values"
> >                              android:persistent="false" />
> 
> This one should also be changed
Attachment #640369 - Attachment is obsolete: true
Attachment #640456 - Flags: review?(bnicholson)
Uploaded correct patch.
Attachment #640377 - Attachment is obsolete: true
Attachment #640377 - Flags: review?(bnicholson)
Attachment #640460 - Flags: review?(bnicholson)
Folded patch 02 into patch 04. There should be no other changes. 04 was previously okayed after review.
Attachment #640368 - Attachment is obsolete: true
Attachment #640370 - Attachment is obsolete: true
Attachment #640467 - Flags: review?(bnicholson)
(In reply to Brian Nicholson (:bnicholson) from comment #25)
> Comment on attachment 640371 [details] [diff] [review]
> 05: Fleshed out the UI of the box - new buttons and preview TextView are
> present.
> 
> Review of attachment 640371 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/FontSizePreference.java
> @@ +43,5 @@
> > +            mIncreaseFontButton = (Button) dialogView.findViewById(R.id.increase_preview_font_button);
> > +
> > +            mPreviewFontView = (TextView) dialogView.findViewById(R.id.preview);
> > +            mPreviewFontView.setMovementMethod(new ScrollingMovementMethod());
> > +        } catch (InflateException ex) {
> 
> No need for this try/catch
Removed.

> ::: mobile/android/base/resources/layout/font_size_preference.xml
> @@ +12,5 @@
> > +              android:layout_width="match_parent"
> > +              android:layout_height="wrap_content"
> > +              android:layout_weight="1"
> > +              android:background="#ffffffff"
> > +              android:padding="2pt"
> 
> Use dip rather than pt.
Done.

> @@ +27,5 @@
> > +        <Button android:id="@+id/decrease_preview_font_button"
> > +                android:layout_width="0dp"
> > +                android:layout_height="match_parent"
> > +                android:layout_weight="1"
> > +                android:text="A"
> 
> "A" should be localized as in patch 1.
Done.

(In reply to Brian Nicholson (:bnicholson) from comment #26)
> Comment on attachment 640371 [details] [diff] [review]
> 05: Fleshed out the UI of the box - new buttons and preview TextView are
> present.
> 
> Review of attachment 640371 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/resources/layout/font_size_preference.xml
> @@ +37,5 @@
> > +                android:layout_height="match_parent"
> > +                android:layout_weight="1"
> > +                android:text="A"
> > +                android:textSize="16sp"
> > +                android:onClick="increasePreviewFontSize"/>
> 
> Also, I think it would be cleaner if you dropped the
> "increasePreviewFontSize" from here and GeckoPreferences. Just add the
> listeners in FontSizePreference so you don't have to forward them from
> GeckoPreferences.
I will apply this in a new patch on top of everything – I think it would be the cleanest approach.
Attachment #640477 - Flags: review?(bnicholson)
Attachment #640371 - Attachment is obsolete: true
Rebased patch 06. Moved r+ to this patch.
Attachment #640372 - Attachment is obsolete: true
Attachment #640481 - Flags: review+
(In reply to Brian Nicholson (:bnicholson) from comment #31)
> Comment on attachment 640373 [details] [diff] [review]
> 07: Preferences are saved to Gecko.
> 
> Review of attachment 640373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with nits fixed
> 

Fixed. Moved r+.
Attachment #640373 - Attachment is obsolete: true
Attachment #640482 - Flags: review+
Rebase. Previous review was "?".
Attachment #640460 - Attachment is obsolete: true
Attachment #640460 - Flags: review?(bnicholson)
Attachment #640485 - Flags: review?(bnicholson)
(In reply to Brian Nicholson (:bnicholson) from comment #26)
> Comment on attachment 640371 [details] [diff] [review]
> 05: Fleshed out the UI of the box - new buttons and preview TextView are
> present.
> 
> Review of attachment 640371 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/resources/layout/font_size_preference.xml
> @@ +37,5 @@
> > +                android:layout_height="match_parent"
> > +                android:layout_weight="1"
> > +                android:text="A"
> > +                android:textSize="16sp"
> > +                android:onClick="increasePreviewFontSize"/>
> 
> Also, I think it would be cleaner if you dropped the
> "increasePreviewFontSize" from here and GeckoPreferences. Just add the
> listeners in FontSizePreference so you don't have to forward them from
> GeckoPreferences.

Moved with this patch.
Attachment #640487 - Flags: review?(bnicholson)
As per mbrubeck's comment on IRC, I special cased a twip size of 0 that represents no font inflation. This patch will render text at a size of 1pt for a twip size of 0.

However, I just realized that this preference doesn't explain that it's inflating font, but rather implies that it's setting a universal text size (since the title is "Text Size" and the summary text is "Tiny", "Small", etc.). While I think a user would end up at the same outcome if it were explained as inflation (if they're trying to make the text larger, they're going to select a size that looks good for them without caring that it's inflation rather than a universal text size), it's a little misleading. For example, the idea that text inflation is turned off when "Text Size" is set to "Tiny" doesn't really feel right. I think the stock browser has a more intuitive representation of this.

So I think this dialog is better than it was before and we should push it, but we may want to reconsider it in the future.

Also, I have an idea. We should reload the current tab if the text size setting is changed (otherwise a user might change the text size, go to the page and see no difference, change it again, still get no results, etc.). I'm not sure how to implement this (if it's even possible - though I assume so), but do you want me to take a look into it?
Attachment #640496 - Flags: review?(bnicholson)
(In reply to Michael Comella (:mcomella) from comment #40)
> However, I just realized that this preference doesn't explain that it's
> inflating font, but rather implies that it's setting a universal text size
> (since the title is "Text Size" and the summary text is "Tiny", "Small",
> etc.)

I agree that a better name would be good, though I'm not sure of a name that is both more accurate and easily understandable.  Maybe "Text Zoom"?

We should also change the "Tiny" string to something "Original size" or "No zoom".

(Let's file a follow-up bug for these changes.)

> Also, I have an idea. We should reload the current tab if the text size
> setting is changed (otherwise a user might change the text size, go to the
> page and see no difference, change it again, still get no results, etc.).

Yes, and ideally we should re-lay-out the page without actually reloading it, to avoid losing any state.  I thought this already worked, though in my testing I see it doesn't.  (So either it stopped working, or I was just imagining things.)  Please file another follow-up bug for this.
(In reply to Matt Brubeck (:mbrubeck) from comment #41)
> I agree that a better name would be good, though I'm not sure of a name that
> is both more accurate and easily understandable.  Maybe "Text Zoom"?
> 
> We should also change the "Tiny" string to something "Original size" or "No
> zoom".
> 
> (Let's file a follow-up bug for these changes.)

Bug 752985.

> > Also, I have an idea. We should reload the current tab if the text size
> > setting is changed (otherwise a user might change the text size, go to the
> > page and see no difference, change it again, still get no results, etc.).
> 
> Yes, and ideally we should re-lay-out the page without actually reloading
> it, to avoid losing any state.  I thought this already worked, though in my
> testing I see it doesn't.  (So either it stopped working, or I was just
> imagining things.)  Please file another follow-up bug for this.

Bug 772541
See Also: → 772541
Comment on attachment 640456 [details] [diff] [review]
03a: Fixed indentation and modified a few strings.

Review of attachment 640456 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/resources/values/arrays.xml
@@ +22,5 @@
>          <item>@string/pref_font_size_large</item>
>          <item>@string/pref_font_size_xlarge</item>
>      </string-array>
>      <string-array name="pref_font_size_values">
> +        <item>40</item>

Since you're reverting this in patch 10, it'd be better to just not have this change at all.
Attachment #640456 - Flags: review?(bnicholson) → review+
Attachment #640467 - Flags: review?(bnicholson) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #43)
> Comment on attachment 640456 [details] [diff] [review]
> 03a: Fixed indentation and modified a few strings.
> 
> Review of attachment 640456 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/resources/values/arrays.xml
> @@ +22,5 @@
> >          <item>@string/pref_font_size_large</item>
> >          <item>@string/pref_font_size_xlarge</item>
> >      </string-array>
> >      <string-array name="pref_font_size_values">
> > +        <item>40</item>
> 
> Since you're reverting this in patch 10, it'd be better to just not have
> this change at all.

Removed. Moved r+ to new file. Will upload updated patch 10.
Attachment #640456 - Attachment is obsolete: true
Attachment #640676 - Flags: review+
Rebased.
Attachment #640679 - Flags: review?(bnicholson)
Attachment #640496 - Attachment is obsolete: true
Attachment #640496 - Flags: review?(bnicholson)
Comment on attachment 640477 [details] [diff] [review]
05a: Fleshed out the UI of the box - new buttons and preview TextView are present.

I noticed you're using tabs here:

+    @Override
+    protected void onPrepareDialogBuilder(AlertDialog.Builder builder) {
+        View dialogView = null;
+
+        final LayoutInflater inflater =
+            (LayoutInflater) mContext.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
+	dialogView = inflater.inflate(R.layout.font_size_preference, null);
+	mDecreaseFontButton = (Button) dialogView.findViewById(R.id.decrease_preview_font_button);
+	mIncreaseFontButton = (Button) dialogView.findViewById(R.id.increase_preview_font_button);
+
+	mPreviewFontView = (TextView) dialogView.findViewById(R.id.preview);
+	mPreviewFontView.setMovementMethod(new ScrollingMovementMethod());
+        
+        builder.setView(dialogView);

Change this to use spaces (along with any other patches where you're using tabs).

Also, I think it would be cleaner if you folded patch 10 into this one so you can drop the font size callbacks in GeckoPreferences. You should generally try to avoid adding code that will simply be reverted in later patches.
Attachment #640477 - Flags: review?(bnicholson) → review-
(In reply to Brian Nicholson (:bnicholson) from comment #46)
> Also, I think it would be cleaner if you folded patch 10 into this one

Rather, patch 9
Depends on bug 770345, otherwise the dialog may cause a NullPointerException on rotation.
Depends on: 770345
Comment on attachment 640485 [details] [diff] [review]
08b: (Last) Dialog dynamically resizes itself per device.

+                // Line height does not take into account spacing between lines. Since this info
+                // is not available until API 16, we add an arbitrary constant per line.

The method getLineSpacingExtra() was introduced in API 16, but it looks like an attribute has existed since API 1:
http://developer.android.com/reference/android/R.attr.html#lineSpacingExtra

+    private void setButtonState(int index) {
+        if (index == 0) {
+            mDecreaseFontButton.setEnabled(false);
+            mIncreaseFontButton.setEnabled(true);
+        } else if (index == mFontTwipValues.length - 1) {
+            mDecreaseFontButton.setEnabled(true);
+            mIncreaseFontButton.setEnabled(false);
+        } else {
+            mDecreaseFontButton.setEnabled(true);
+            mIncreaseFontButton.setEnabled(true);
+        }
+    }

Have these buttons enabled in XML and remove the setEnabled(true) calls.

+		if (pref instanceof FontSizePreference) {
+		    // Note, for safety, this instantiation is better put in onCreate. However, it is in
+		    // onWindowFocusChanged for speed purposes. If it is null, you may need to move it.
+		    mFontSizePreference = (FontSizePreference) pref;
+		}

+    private static final String NULL_FONT_SIZE_PREF_ERR_MSG = "mFontSizePreference is null. Buttons " +
+	"will have no effect.";

I don't think we want this change; if you're seeing a problem with onCreate()/onWindowFocusChanged() in your testing, we should just go ahead and make the fix.

decreasePreviewFontSize/increasePreviewFontSize/mFontSizePreference should be gone once patch 5 is updated.
Attachment #640485 - Flags: review?(bnicholson) → review-
Attachment #640679 - Flags: review?(bnicholson) → review+
Removed tabs. More updates to follow.
Attachment #640477 - Attachment is obsolete: true
Tabs -> spaces. Moved r+.
Attachment #640482 - Attachment is obsolete: true
Attachment #640740 - Flags: review+
Tabs -> spaces. Moved r-.
Attachment #640485 - Attachment is obsolete: true
Attachment #640742 - Flags: review-
Tabs -> spaces. Need to fold into patch 5 so turned off review.
Attachment #640487 - Attachment is obsolete: true
Attachment #640487 - Flags: review?(bnicholson)
Tabs -> spaces. Moved r+.
Attachment #640679 - Attachment is obsolete: true
Attachment #640747 - Flags: review+
(In reply to Brian Nicholson (:bnicholson) from comment #46)
> Comment on attachment 640477 [details] [diff] [review]
> 05a: Fleshed out the UI of the box - new buttons and preview TextView are
> present.
>
> Change this to use spaces (along with any other patches where you're using
> tabs).

Done.
(In reply to Michael Comella (:mcomella) from comment #56)
> (In reply to Brian Nicholson (:bnicholson) from comment #46)
> > Comment on attachment 640477 [details] [diff] [review]
> > 05a: Fleshed out the UI of the box - new buttons and preview TextView are
> > present.
> >
> > Change this to use spaces (along with any other patches where you're using
> > tabs).
> 
> Done.

Sorry, I may have done this incorrectly. I will update as soon as I fix it.
Correct indentation. Moved r+.
Attachment #640467 - Attachment is obsolete: true
Attachment #640785 - Flags: review+
'a(In reply to Brian Nicholson (:bnicholson) from comment #46)
> Comment on attachment 640477 [details] [diff] [review]
> 05a: Fleshed out the UI of the box - new buttons and preview TextView are
> present.
> 
> Also, I think it would be cleaner if you folded patch 10 into this one so
> you can drop the font size callbacks in GeckoPreferences.

(In reply to Brian Nicholson (:bnicholson) from comment #47)
> (In reply to Brian Nicholson (:bnicholson) from comment #46)
> > Also, I think it would be cleaner if you folded patch 10 into this one
> 
> Rather, patch 9

Folded patch 9 into 6 as I felt it was more appropriate. I will re-requset a review on 6 as a result. Patches 7 and 8 will get rebasing patches.
Attachment #640738 - Attachment is obsolete: true
Attachment #640790 - Flags: review?(bnicholson)
Uploaded the wrong file. I accidentally nuked my patch directory but I think I recovered properly so please double check this review.
Attachment #640790 - Attachment is obsolete: true
Attachment #640790 - Flags: review?(bnicholson)
Attachment #640796 - Flags: review?(bnicholson)
Attachment #640745 - Attachment is obsolete: true
Rebase. Moving r+.
Attachment #640740 - Attachment is obsolete: true
Attachment #640800 - Flags: review+
(In reply to Brian Nicholson (:bnicholson) from comment #49)
> Comment on attachment 640485 [details] [diff] [review]
> 08b: (Last) Dialog dynamically resizes itself per device.
> 
> I don't think we want this change; if you're seeing a problem with
> onCreate()/onWindowFocusChanged() in your testing, we should just go ahead
> and make the fix.
> 
> decreasePreviewFontSize/increasePreviewFontSize/mFontSizePreference should
> be gone once patch 5 is updated.

Done and rebased.
Attachment #640742 - Attachment is obsolete: true
Rebasing error.
Attachment #640796 - Attachment is obsolete: true
Attachment #640796 - Flags: review?(bnicholson)
Attachment #640821 - Flags: review?(bnicholson)
My apologies. Wrong file upload.
Attachment #640821 - Attachment is obsolete: true
Attachment #640821 - Flags: review?(bnicholson)
Attachment #640824 - Flags: review?(bnicholson)
Rebase.
Attachment #640799 - Attachment is obsolete: true
Attachment #640799 - Flags: review?(bnicholson)
Attachment #640826 - Flags: review?(bnicholson)
Rebase. Move r+.
Attachment #640800 - Attachment is obsolete: true
Attachment #640829 - Flags: review+
Wrong file upload - forgot to qref. Moved r+.
Attachment #640829 - Attachment is obsolete: true
Attachment #640830 - Flags: review+
Comment on attachment 640747 [details] [diff] [review]
10b: Special cased twip size of 0.

private void updatePreviewFontSize(String twip) {
-        mPreviewFontView.setTextSize(PREVIEW_FONT_SIZE_UNIT, convertTwipStrToPT(twip));
+    float pt = convertTwipStrToPT(twip);
+    // Android will not render a font size of 0 pt but for Gecko, 0 twip turns off font inflation.
+    // Thus we special case 0 twip to display a renderable font size.
+    if (pt == 0) {
+        mPreviewFontView.setTextSize(PREVIEW_FONT_SIZE_UNIT, 1);
+    } else {
+        mPreviewFontView.setTextSize(PREVIEW_FONT_SIZE_UNIT, pt);
+    }

Make sure you fix this spacing
Attachment #640826 - Flags: review?(bnicholson) → review+
Comment on attachment 640826 [details] [diff] [review]
06d: Dialog buttons work – they increase and decrease the size of the preview text.

+    private static final String FONT_SIZE_PREF_KEY = "font.size.inflation.minTwips";

I think you can remove this.
Attachment #640824 - Flags: review?(bnicholson) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #70)
> Comment on attachment 640826 [details] [diff] [review]
> 06d: Dialog buttons work – they increase and decrease the size of the
> preview text.
> 
> +    private static final String FONT_SIZE_PREF_KEY =
> "font.size.inflation.minTwips";
> 
> I think you can remove this.

Done. Note it will be re-added in patch 8. Moving r+.
Attachment #640826 - Attachment is obsolete: true
Attachment #641322 - Flags: review+
(In reply to Brian Nicholson (:bnicholson) from comment #49)
> Comment on attachment 640485 [details] [diff] [review]
> 08b: (Last) Dialog dynamically resizes itself per device.
> 
> +                // Line height does not take into account spacing between
> lines. Since this info
> +                // is not available until API 16, we add an arbitrary
> constant per line.
> 
> The method getLineSpacingExtra() was introduced in API 16, but it looks like
> an attribute has existed since API 1:
> http://developer.android.com/reference/android/R.attr.html#lineSpacingExtra

Took this into account in this patch, though not using that attribute (I was unable to get the value so I overrode it instead).

> 
> +    private void setButtonState(int index) {
> +        if (index == 0) {
> +            mDecreaseFontButton.setEnabled(false);
> +            mIncreaseFontButton.setEnabled(true);
> +        } else if (index == mFontTwipValues.length - 1) {
> +            mDecreaseFontButton.setEnabled(true);
> +            mIncreaseFontButton.setEnabled(false);
> +        } else {
> +            mDecreaseFontButton.setEnabled(true);
> +            mIncreaseFontButton.setEnabled(true);
> +        }
> +    }
> 
> Have these buttons enabled in XML and remove the setEnabled(true) calls.

onPrepareDialogBuilder() gets called each time so the buttons are initialized each time and the extra .setEnabled(true) calls are unnecessary (as is the extra xml).

There were a lot of fundamental changes so please look over this change carefully.
Attachment #640803 - Attachment is obsolete: true
Attachment #641326 - Flags: review?(bnicholson)
(In reply to Brian Nicholson (:bnicholson) from comment #69)
> Comment on attachment 640747 [details] [diff] [review]
> 10b: Special cased twip size of 0.
> 
> private void updatePreviewFontSize(String twip) {
> -        mPreviewFontView.setTextSize(PREVIEW_FONT_SIZE_UNIT,
> convertTwipStrToPT(twip));
> +    float pt = convertTwipStrToPT(twip);
> +    // Android will not render a font size of 0 pt but for Gecko, 0 twip
> turns off font inflation.
> +    // Thus we special case 0 twip to display a renderable font size.
> +    if (pt == 0) {
> +        mPreviewFontView.setTextSize(PREVIEW_FONT_SIZE_UNIT, 1);
> +    } else {
> +        mPreviewFontView.setTextSize(PREVIEW_FONT_SIZE_UNIT, pt);
> +    }
> 
> Make sure you fix this spacing

Fixed and rebased. Moving r+.
Attachment #640747 - Attachment is obsolete: true
Attachment #641328 - Flags: review+
As discussed with bnicholson on IRC, I replaced the arbitrary line spacing constant with a line to remove the padding whose value we could determine – TextView.setIncludeFontPadding(...) (which is why we needed the constant in the first place).
Attachment #641326 - Attachment is obsolete: true
Attachment #641326 - Flags: review?(bnicholson)
Attachment #641538 - Flags: review?(bnicholson)
Adjusted the padding from the previous change.
Attachment #641538 - Attachment is obsolete: true
Attachment #641538 - Flags: review?(bnicholson)
Attachment #641651 - Flags: review?(bnicholson)
As per discussion with bnicholson in real life/IRC, moved the onGlobalLayoutListener so it set only once per dialog. While I feel the change made the code less linear, I feel it is more intuitive.

This change required a lot of renaming so you may need to be extra careful with the review.
Attachment #641651 - Attachment is obsolete: true
Attachment #641651 - Flags: review?(bnicholson)
Attachment #641997 - Flags: review?(bnicholson)
Rebase. Move r+.
Attachment #641328 - Attachment is obsolete: true
Attachment #642004 - Flags: review+
Fixed Galaxy Note rotation bug. Only changed one line in resources/layout/font_size_preference.xml.

bnicholson: Removed r+ – while nothing but the above line should have changed, please check this is okay.
Attachment #640785 - Attachment is obsolete: true
Attachment #642755 - Flags: review?(bnicholson)
Rebase from Galaxy Note fix.
Attachment #641997 - Attachment is obsolete: true
Attachment #641997 - Flags: review?(bnicholson)
Attachment #642757 - Flags: review?(bnicholson)
Comment on attachment 642755 [details] [diff] [review]
04c: Created a simple dialog with (non-functioning) Cancel and Set buttons.

+        View dialogView = null;
+        dialogView = inflater.inflate(R.layout.font_size_preference, null);

nit: combine these lines
Attachment #642755 - Flags: review?(bnicholson) → review+
Comment on attachment 642757 [details] [diff] [review]
08i: Dialog dynamically resizes itself per device.

+        // Android does well with default width.

nit: remove or combine this comment with the "set dialog height" comment
Attachment #642757 - Flags: review?(bnicholson) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #80)
> Comment on attachment 642755 [details] [diff] [review]
> 04c: Created a simple dialog with (non-functioning) Cancel and Set buttons.
> 
> +        View dialogView = null;
> +        dialogView = inflater.inflate(R.layout.font_size_preference, null);
> 
> nit: combine these lines

Done. Moved r+.
Attachment #642755 - Attachment is obsolete: true
Attachment #642820 - Flags: review+
I accidentally uploaded 05 into 04 and requested a new review on it. This is uploading the correct 04. Moved r+.
Attachment #642820 - Attachment is obsolete: true
Attachment #642824 - Flags: review+
Updated 05 (including bnicholson's comment). Moved r+.
Attachment #642821 - Attachment is obsolete: true
Attachment #642825 - Flags: review+
Rebase. Moved r+.
Attachment #640830 - Attachment is obsolete: true
Attachment #642827 - Flags: review+
(In reply to Brian Nicholson (:bnicholson) from comment #81)
> Comment on attachment 642757 [details] [diff] [review]
> 08i: Dialog dynamically resizes itself per device.
> 
> +        // Android does well with default width.
> 
> nit: remove or combine this comment with the "set dialog height" comment

Done. Also, rebase. Moved r+.
Attachment #642757 - Attachment is obsolete: true
Attachment #642829 - Flags: review+
Rebase to trunk. Moved r+.
Attachment #640367 - Attachment is obsolete: true
Attachment #642830 - Flags: review+
Rebase to trunk. Moved r+.
Attachment #642824 - Attachment is obsolete: true
Attachment #642831 - Flags: review+
Wrong file - forgot to qref.
Attachment #642831 - Attachment is obsolete: true
Attachment #642832 - Flags: review+
Attachment #642830 - Attachment filename: mbrubeck.patch → 715179-01.patch
Attachment #640676 - Attachment filename: simple_dialog.patch → 715179-03.patch
Attachment #642832 - Attachment filename: new_file.patch → 715179-04.patch
Attachment #642825 - Attachment filename: dialog_box.patch → 715179-05.patch
Attachment #642826 - Attachment filename: working_dialog2.patch → 715179-06.patch
Attachment #642827 - Attachment filename: saving_dialog.patch → 715179-07.patch
Attachment #642829 - Attachment filename: dynamic_height.patch → 715179-08.patch
Comment on attachment 642004 [details] [diff] [review]
10d: Special cased twip size of 0.

Changed patch names to make them easier to check in.

Note, I found a bug when moving to the smallest font size on Galaxy Nexus so I will need to fix that and request an additional review (it never ends!).
Attachment #642004 - Attachment filename: special_case.patch → 715179-10.patch
Undid previous change to Galaxy Note fix.
Attachment #642825 - Attachment is obsolete: true
Attachment #643051 - Flags: review+
Now set textView width as well. Also changed some comments.

I am concerned because the dialog size does not seem to change in accordance with the textView size in ICS but does in Gingerbread and I can't seem to figure out why. bnicholson, do you know?
Attachment #642829 - Attachment is obsolete: true
Attachment #643190 - Flags: feedback?(bnicholson)
I should mention, ICS does a fine job setting width by itself so the code works. I'd like to know why I can't change the TextView width in ICS though.
Updated one more comment from the last patch.

I was unable to figure out why the dialog does not change size based on the content. In ICS, I found changing the size of the internal view to be larger or smaller than the dialog size will not actually change the dialog size (meaning the dialog is not wrapping the content). This is in opposition to Gingerbread, where the dialog wraps the content and we can set it to an arbitrary size based on the size of the internal views.
Attachment #643190 - Attachment is obsolete: true
Attachment #643190 - Flags: feedback?(bnicholson)
Attachment #643228 - Flags: review?(bnicholson)
As a final note, this does not affect the usability of the dialog. ICS will set the dialog size for us whereas Gingerbread (at least on a Galaxy Note) will use the dialog width code and set the width dynamically.

This patch adds this information to the comments in the code.
Attachment #643228 - Attachment is obsolete: true
Attachment #643228 - Flags: review?(bnicholson)
Attachment #643229 - Flags: review?(bnicholson)
Put the comments into the wrong patch. This is the correct patch.
Attachment #643229 - Attachment is obsolete: true
Attachment #643229 - Flags: review?(bnicholson)
Attachment #643230 - Flags: review?(bnicholson)
Comment on attachment 643230 [details] [diff] [review]
08n: Dialog dynamically resizes itself per device.

+        // onGlobalLayout(), and only then can changes can be made.

OCD nit: remove a can


+     * invalidates the view. Does not update the font indicies.

OCD nit: s/indicies/indices/
Attachment #643230 - Flags: review?(bnicholson) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #99)
> Comment on attachment 643230 [details] [diff] [review]
> 08n: Dialog dynamically resizes itself per device.
> 
> +        // onGlobalLayout(), and only then can changes can be made.
> 
> OCD nit: remove a can
> 
> 
> +     * invalidates the view. Does not update the font indicies.
> 
> OCD nit: s/indicies/indices/

Done and done. Moved r+.
Attachment #643230 - Attachment is obsolete: true
Attachment #643451 - Flags: review+
Updated patch titles and rebased to trunk. Moved r+'s.
Attachment #642004 - Attachment is obsolete: true
Attachment #643472 - Flags: review+
To the checker-in:
There are 8 patches total which skip numbers (namely 2 and 9). The patch file names are the same as the numbers on Bugzilla (715179-##.patch). The patches should be applied in ascending order. The patch titles (inside the file) list which patch is which without skipping numbers (ex: "Bug 715179: (# of 8)").
Keywords: checkin-needed
Depends on: 776369
The new UI has been added and the user can change the font size.

Verified on:
Aurora 17.0a2 2012-09-04 and Nightly 18.0a1 2012-09-04 
Samsung Galaxy R (Android 2.3.4)
Samsung Galaxy Tab 2 7.0 Android (4.0.4)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.