921557 - Banner text on small-screen devices is cut off

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: Banner (Nexus One) - Screenshot

Akin to announcement notifications we don't really have a current solution for small-screen devices. Should text that exceeds the defined maxLines amount in the TextView marquee? Is this something we even care about?

See screenshot from a Nexus One

Comment 1

[Lucas Rocha (:lucasr)]

(In reply to Aaron Train [:aaronmt] from comment #0)
> Created attachment 811249 [details]
> Banner (Nexus One) - Screenshot
> 
> Akin to announcement notifications we don't really have a current solution
> for small-screen devices. Should text that exceeds the defined maxLines
> amount in the TextView marquee? Is this something we even care about?
> 
> See screenshot from a Nexus One

maxLines is actually 3 right now but we're setting a fixed height on the banner. Maybe we should change it to wrap_content instead and set min height to be the current home_banner_height?

Comment 2

[Sriram Ramasubramanian [:sriram]]

No no no no. The height is fixed -- like say 48dp. But the text inside can be 1-3 lines. So that banner looks the same for any kind of text :(

Comment 3

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #2)
> No no no no. The height is fixed -- like say 48dp. But the text inside can
> be 1-3 lines. So that banner looks the same for any kind of text :(

I can see your point and agree with the aim for consistency. But maybe we should allow more vertical space to fit more content on devices with smaller screens. I mean, gosh, the amount of text shown in the screenshot is not even half a tweet (~50 chars). Another option could be to dynamically reduce the text size just a bit to fit some more content.

Or maybe we don't really care about smaller screens that much. Ian?

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

I think dropping the font size slightly is an option, but interested in Ian's thoughts too. We have a "must be readable" situation, not a "must be tappable" situation.

Comment 5

[Ian Barlow (:ibarlow)]

Heh, don't put words in my mouth Lucas, of course we care about small screens ;)

Perhaps we can try a couple of things:
1. Support text wrapping to three lines
2. Dynamically shrink text a *little* as Lucas suggested. Perhaps in cases where we do this we also shrink the icon a little to offer more space.

Comment 6

[:Margaret Leibovic]

We should fix this before we ship a dynamic snippets add-on.

Comment 7

[:Margaret Leibovic]

Testing on my Nexus S, I think I'm running into this Android issue:
http://code.google.com/p/android/issues/detail?id=2254

However, even on a Nexus 4, where we show 3 lines of text, a 140 character message gets cut off. Working with the snippets team, we talked about supporting a 140 character message, but should we make that shorter? Or would be be okay with showing 4 lines of text?

Comment 8

[Sriram Ramasubramanian [:sriram]]

Thoughts:
To drop the font-size, check for the text's layout (getLayout()) and see how many lines have been set based on the text, or may be even it is ellipsized, and then decrease the size until everything fits.

http://developer.android.com/reference/android/text/Layout.html

FadedTextView uses a similar logic.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea722ce55e26

Comment 10

[:Margaret Leibovic]

(In reply to Carsten Book [:Tomcat] from comment #9)
> https://hg.mozilla.org/mozilla-central/rev/ea722ce55e26

Ugh, I landed that with the wrong bug number :( That was part of bug 917970.

Comment 11

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #10)
> (In reply to Carsten Book [:Tomcat] from comment #9)
> > https://hg.mozilla.org/mozilla-central/rev/ea722ce55e26
> 
> Ugh, I landed that with the wrong bug number :( That was part of bug 917970.

And I even did that wrong! I meant bug 921668.

Comment 12

[:Margaret Leibovic]

Josh has been working on this :)

Comment 13

[Josh Dover [:jdover]]

Attachment: Use AutoResizeTextView for home banner

Found a pretty decent AutoResizeTextView on SO that I used to resize the text to the space. Also will resize on rotation to increase text size for larger space.

Comment 14

[:Margaret Leibovic]

Comment on attachment 8367499 [details] [diff] [review]
Use AutoResizeTextView for home banner

Clearing review until we hear back about licensing. I also think that if we can modify this code, we should slim it down to only include what we actually need.

Comment 15

[Josh Dover [:jdover]]

Attachment: 01 - Use AutoResizeTextView for home banner

Stripped out unneeded code and simplified AutoResizeTextView.

Comment 16

[:Margaret Leibovic]

Comment on attachment 8377888 [details] [diff] [review]
01 - Use AutoResizeTextView for home banner

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

I know using third-party code is convenient, but I'm a bit dubious as to the quality of this code... at the very least, we need to make this l10n-friendly, but I would like to see if we can let Android take care of more for us if possible.

::: mobile/android/base/resources/layout/home_banner.xml
@@ +16,5 @@
>                 android:layout_height="fill_parent"
>                 android:layout_weight="1"
>                 android:layout_marginLeft="10dp"
> +               android:paddingTop="4dp"
> +               android:paddingBottom="4dp"

You should share some screenshots for ibarlow to see how he feels about this change.

::: mobile/android/base/widget/AutoResizeTextView.java
@@ +1,4 @@
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

What was the final result about the licensing here? Can we just stick our license on it like this?

@@ +24,5 @@
> +    // Minimum text size for this text view
> +    public static final float MIN_TEXT_SIZE = 10;
> +
> +    // Our ellipse string
> +    private static final String ELLIPSES = "...";

I don't like the look of this... we should be getting this out of a string resource, and we should use the real ellipsis character, not 3 periods.

However, why can't we just let Android's ellipsis logic take care of this for us?

@@ +191,5 @@
> +            // Trim characters off until we have enough room to draw the ellipsis
> +            while (width < lineWidth + ellipseWidth) {
> +                lineWidth = textPaint.measureText(text.subSequence(start, --end + 1).toString());
> +            }
> +            setText(text.subSequence(0, end) + ELLIPSES);

Instead of all this manual futzing, couldn't we just set maxLines to 3 and ellipsize to end?

Comment 17

[Josh Dover [:jdover]]

(In reply to :Margaret Leibovic from comment #16)
> Comment on attachment 8377888 [details] [diff] [review]
> Use AutoResizeTextView for home banner
> 
> Review of attachment 8377888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You should share some screenshots for ibarlow to see how he feels about this
> change.

For the life of me, I cannot figure out how to take screenshots on Gingerbread. Here is a lovely picture taken from my camera: https://drive.google.com/a/mozilla.com/file/d/0BwqqgfwYmRg4Rk13SHQ4TlFQVU0/edit?usp=sharing

> 
> ::: mobile/android/base/widget/AutoResizeTextView.java
> @@ +1,4 @@
> > +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
> > + * This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> What was the final result about the licensing here? Can we just stick our
> license on it like this?

Gerv from licensing in bug 965552: "feel free to take this code and change and integrate it however you like, without any attribution or license text requirement"

> @@ +24,5 @@
> > +    // Minimum text size for this text view
> > +    public static final float MIN_TEXT_SIZE = 10;
> > +
> > +    // Our ellipse string
> > +    private static final String ELLIPSES = "...";
> 
> I don't like the look of this... we should be getting this out of a string
> resource, and we should use the real ellipsis character, not 3 periods.
> 
> However, why can't we just let Android's ellipsis logic take care of this
> for us?

I tried to get it working with Android's ellipsis handling, but I can't get anything to wrap past 2 lines. I think it may have to do with the Android bug mentioned above: http://code.google.com/p/android/issues/detail?id=2254. That or the custom drawing isn't getting called

Where exactly should the ellipsis string resource (unicode character …) go?

> 
> @@ +191,5 @@
> > +            // Trim characters off until we have enough room to draw the ellipsis
> > +            while (width < lineWidth + ellipseWidth) {
> > +                lineWidth = textPaint.measureText(text.subSequence(start, --end + 1).toString());
> > +            }
> > +            setText(text.subSequence(0, end) + ELLIPSES);
> 
> Instead of all this manual futzing, couldn't we just set maxLines to 3 and
> ellipsize to end?

See above.

Comment 18

[:Margaret Leibovic]

Okay, I believe you that there's an annoying Android bug here, since I myself tried to figure this out at one point and failed to do so.

Let's go ahead with the ellipsis approach. You can just put the resource in our regular Android string resources:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/locales/en-US/android_strings.dtd
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/strings.xml.in

And FYI, for screenshots on Gingerbread, you can capture the screen using DDMS (or now the Android Device Monitor), tools included in the SDK. It also took me a while to figure that one out :)

Comment 19

[Kevin Brosnan [Ex-Mozilla]]

Note that the Nexus S is not a small screen device. We support as low as 320 pixels high and 240 pixels wide. http://en.wikipedia.org/wiki/Graphics_display_resolution#QVGA_.28320x240.29

Comment 20

[Josh Dover [:jdover]]

Attachment: 02 - Add ellipsis character to strings resources

Comment 21

[:Margaret Leibovic]

Comment on attachment 8379971 [details] [diff] [review]
02 - Add ellipsis character to strings resources

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

Looking good, but I'd like to see a new version before landing.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +403,5 @@
>  <!ENTITY exit_guest_session_title "&brandShortName; will now restart">
>  <!ENTITY exit_guest_session_text "The browsing data from this session will be deleted.">
>  
> +<!-- Miscellaneous -->
> +<!ENTITY ellipsis "&#8230;">

I think you should be able to include the unicode character itself in the .dtd file.

You should also add a localization note here, something like this:
http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd#30

::: mobile/android/base/widget/AutoResizeTextView.java
@@ +186,5 @@
>              int lastLine = layout.getLineForVertical(height) - 1;
>              int start = layout.getLineStart(lastLine);
>              int end = layout.getLineEnd(lastLine);
>              float lineWidth = layout.getLineWidth(lastLine);
> +            float ellipseWidth = textPaint.measureText(ellipsis());

Instead of creating this helper method, I think it would be better to just set a local variable here, since we're only using this within this method.

Comment 22

[:Margaret Leibovic]

Attachment: screenshot - Nexus S

It looks like we're going to need to tweak this widget a bit... this text looks way too small. I also think we only want to wrap to 3 lines at most? We should talk to ibarlow about more details for when we should ellipsize.

Comment 23

[:Margaret Leibovic]

Also, I'm not actually seeing the ellipsis appear, even when building without your second patch... I feel a bit skeptical about this widget :/

Comment 24

[Josh Dover [:jdover]]

I agree that is a bit small. The default size is 14 and that was 10. I tested 12 and it seems reasonable.

But, if we want to keep this to 3 lines, we are going to only gain about 4-5 characters by shrinking the text to 12. I'm not sure what we want to do here, but if the gain is that minimal, I would argue the added complexity (and performance hit?) is not worth it. I think we should still fix the issue of text only wrapping to 2 lines.

Comment 25

[:Margaret Leibovic]

Changing the text size wasn't part of the original scope of this bug, so I'd say it's fine to keep the text size constant, but yes, we need to make sure we wrap to 3 lines.

Comment 26

[Josh Dover [:jdover]]

Attachment: Use custom EllipsisTextView to handle bug on older devices

This uses a very simple custom TextView that properly handles max lines and ellipsizing, which older versions of Android (I presume pre ICS) do not properly do. Allows for 3 lines on smaller screen devices without any text size changes.

Comment 27

[:Margaret Leibovic]

Comment on attachment 8383346 [details] [diff] [review]
Use custom EllipsisTextView to handle bug on older devices

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

I really like this approach of a super simple widget that addresses our use case, but I still want to think about this a bit more.

::: mobile/android/base/home/HomeBanner.java
@@ +111,5 @@
>              setTag(message.getString("id"));
>  
>              // Display styled text from an HTML string.
>              final Spanned text = Html.fromHtml(message.getString("text"));
> +            final EllipsisTextView textView = (EllipsisTextView) findViewById(R.id.text);

This must be built on top of an older build, we store mTextView now that bug 975173 landed.

::: mobile/android/base/widget/EllipsisTextView.java
@@ +32,5 @@
> +    public void ellipsizeAtLine(final int maxLines) {
> +        getViewTreeObserver().addOnGlobalLayoutListener(new OnGlobalLayoutListener() {
> +            @Override
> +            public void onGlobalLayout() {
> +            getViewTreeObserver().removeGlobalOnLayoutListener(this);

Instead of using this OnGlobalLayoutListener, could we just override the view's onLayout method? Also, since you're removing the observer after its called once, how will the work if the layout changes, such as when the screen orientation changes?

@@ +36,5 @@
> +            getViewTreeObserver().removeGlobalOnLayoutListener(this);
> +                if (getLineCount() > maxLines) {
> +                    final int lineEndIndex = getLayout().getLineEnd(maxLines - 1);
> +                    final String ellipsis = getResources().getString(R.string.ellipsis);
> +                    final String text = getText().subSequence(0, lineEndIndex - 3) + ellipsis;

This makes an assumption how about many characters are in the ellipsis. I'm also still a bit nervous because this won't work properly in RTL locales. I feel like the proper solution involves using some sort of pattern matching string where we sub in the text. Maybe we can get some feedback from the l10n team (or the desktop team) to see if we do anything like this in desktop Firefox.

Comment 28

[:Margaret Leibovic]

Comment on attachment 8383346 [details] [diff] [review]
Use custom EllipsisTextView to handle bug on older devices

Let's also see what bnicholson thinks of this.

Comment 29

[Josh Dover [:jdover]]

(In reply to :Margaret Leibovic from comment #27)
> Comment on attachment 8383346 [details] [diff] [review]
> 
> This makes an assumption how about many characters are in the ellipsis. I'm
> also still a bit nervous because this won't work properly in RTL locales. I
> feel like the proper solution involves using some sort of pattern matching
> string where we sub in the text. Maybe we can get some feedback from the
> l10n team (or the desktop team) to see if we do anything like this in
> desktop Firefox.

Flagging Francesco for input on how we should handle ellipsizing text for RTL languages.

Comment 30

[Josh Dover [:jdover]]

Attachment: Use custom EllipsisTextView to handle bug on older devices

Fixed to use onLayout(), need input from Francesco on RTL.

Comment 31

[Brian Nicholson (:bnicholson)]

Comment on attachment 8383974 [details] [diff] [review]
Use custom EllipsisTextView to handle bug on older devices

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

::: mobile/android/base/resources/layout/home_banner.xml
@@ +10,5 @@
>                  android:layout_height="48dip"
>                  android:layout_marginLeft="10dp"
>                  android:scaleType="centerInside"/>
>  
> +     <org.mozilla.gecko.widget.EllipsisTextView android:id="@+id/text"

Nit: either align the lines below with android:id, or put android:id on its own line. I'd opt for the latter since this is a long View name.

::: mobile/android/base/widget/EllipsisTextView.java
@@ +15,5 @@
> +/**
> + * Text view that correctly handles maxLines and ellipsizing for Android < 2.3.
> + */
> +public class EllipsisTextView extends TextView {
> +

Nit: drop this line

@@ +20,5 @@
> +    private final String ellipsis = getResources().getString(R.string.ellipsis);
> +    private int mMaxLines = -1;
> +
> +    public EllipsisTextView(Context context) {
> +      this(context, null);

Nit: 4 space indentation. Also, I would just do 'super(context)'.

@@ +24,5 @@
> +      this(context, null);
> +    }
> +
> +    public EllipsisTextView(Context context, AttributeSet attrs) {
> +      this(context, attrs, 0);

Nit: 4 space indentation. Also, I would just do 'super(context, attrs)'.

@@ +40,5 @@
> +    public void onLayout(boolean changed, int left, int top, int right, int bottom) {
> +        super.onLayout(changed, left, top, right, bottom);
> +        if (mMaxLines > 0 && getLineCount() > mMaxLines) {
> +            final int lineEndIndex = getLayout().getLineEnd(mMaxLines - 1);
> +            final String text = getText().subSequence(0, lineEndIndex - 3) + ellipsis;

The 3 here seems arbitrary. It kind of made sense when you were using "..." instead of "…" (but even then, this isn't a fixed-width font, so 3 wouldn't guarantee anything).

I'm wondering if 2 would be a better choice, and here's why: you immediately do setText() after truncating the text and adding the ellipsis. Let's assume the text is still too long to fit. Because you're doing setText, I think we'll do *another* layout pass, this time with the truncated version, repeating recursively until it finally fits (which will probably happen on the first pass anyway). That is:

reallylongtext becomes reallylongte…
reallylongte…  becomes reallylongt…
reallylongt…   becomes reallylong…

So the 2 in this case isn't arbitrary; it's the minimum number we need to reduce the string by at each iteration to ensure the text shrinks at each pass. Does that make sense?

Comment 32

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #31)
> So the 2 in this case isn't arbitrary; it's the minimum number we need to
> reduce the string by at each iteration to ensure the text shrinks at each
> pass.

Of course, this isn't true if other localizers use a different string length, so rather than hardcoding 2, you should probably use ellipsis.length() - 1. And this still doesn't address the RTL issue....

Comment 33

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 8383974 [details] [diff] [review]
Use custom EllipsisTextView to handle bug on older devices

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

::: mobile/android/base/home/HomeBanner.java
@@ +136,5 @@
>              // Update the banner message on the UI thread.
>              ThreadUtils.postToUiThread(new Runnable() {
>                  @Override
>                  public void run() {
> +                    mTextView.ellipsizeAtLine(3);

Ideally this should be theme-able.

::: mobile/android/base/widget/EllipsisTextView.java
@@ +24,5 @@
> +      this(context, null);
> +    }
> +
> +    public EllipsisTextView(Context context, AttributeSet attrs) {
> +      this(context, attrs, 0);

I wouldn't do super(context, attrs); :P
I would do
this(context, attrs, R.attr.ellipsisTextViewStyle); :P

@@ +28,5 @@
> +      this(context, attrs, 0);
> +    }
> +
> +    public EllipsisTextView(Context context, AttributeSet attrs, int defStyle) {
> +        super(context, attrs, defStyle);

Get the themed value -- will be 3 in this case.
But something else can have a 5 or so!
"Re-usable"

@@ +36,5 @@
> +        mMaxLines = maxLines;
> +    }
> +
> +    @Override
> +    public void onLayout(boolean changed, int left, int top, int right, int bottom) {

Isn't the point of the patch to reduce the font size to fit 3 lines of text?
I didn't read past comment 10 or so.

Comment 34

[Francesco Lodolo [:flod]]

(looping in Pike in case he has some notes to add)

Gosh, I hate ellipsis. A perfectly fine word would easily become insulting if truncated at the wrong point.

The text-overflow page on MDN have some examples on RTL/LTR ellipsis, and AFAIK they're accurate
https://developer.mozilla.org/en-US/docs/Web/CSS/text-overflow

From a quick look at strings I don't think we have locales using a character different than "…", but having it exposed as a localizable string won't hurt.

Ignoring for a moment the ellipsis problem, any chance to get a different solution like fading the text, adding a "Tap to read" or similar if the text doesn't fit?

Comment 35

[Josh Dover [:jdover]]

Attachment: Use custom EllipsisTextView to handle bug on older devices

Addressed all comments from margaret, bnicholson, and sriram. And as far as I understand, I don't need to do anything special for processing an RTL string correct?

Comment 36

[Josh Dover [:jdover]]

Attachment: 0001-Bug-921557-Use-custom-EllipsisTextView-to-handle-bug.patch

Missed margaret's comment about using the actual ellipsis character in the strings.dtd

Comment 37

[:Margaret Leibovic]

Comment on attachment 8384907 [details] [diff] [review]
0001-Bug-921557-Use-custom-EllipsisTextView-to-handle-bug.patch

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

This looks like it's on the right track. However, testing this on my Nexus 4, I found that the text is ellipsizing earlier than where it does without this patch applied. We should figure out why that's happening.

However, I'm not giving this an r+ now because it doesn't recalculate the text on orientation change, which can result in text being cut off if you start in landscape and then rotate to portrait.

::: mobile/android/base/widget/EllipsisTextView.java
@@ +35,5 @@
> +        super.onLayout(changed, left, top, right, bottom);
> +        if (mMaxLines > 0 && getLineCount() > mMaxLines) {
> +            final int lineEndIndex = getLayout().getLineEnd(mMaxLines - 1);
> +            final String text = getText().subSequence(0, lineEndIndex - ellipsis.length())
> +                + ellipsis;

Nit: I would prefer pulling out |lineEndIndex - ellipsis.length()| to a local variable, then keeping the ellipsis concatenation on the same line.

Comment 38

[Josh Dover [:jdover]]

Attachment: patch

Fixed to show more text (when possible) for any resizing of the TextView. I had to make a new method (setOriginalText) to accomplish this. I couldn't think of a good way to override setText() and be able to know what the original text is.

Comment 39

[:Margaret Leibovic]

Comment on attachment 8385756 [details] [diff] [review]
patch

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

::: mobile/android/base/widget/EllipsisTextView.java
@@ +45,5 @@
> +        // There is extra space, start over with the original text
> +        if (getLineCount() < mMaxLines && mIsEllipsized) {
> +            setText(mOriginalText);
> +
> +            mIsEllipsized = false;

Why do you need this mIsEllipsized variable? To avoid re-setting the text? I don't feel like it's worth introducing a boolean flag for this.

@@ +49,5 @@
> +            mIsEllipsized = false;
> +        }
> +
> +        // If we are over the max line attribute, ellipsize
> +        while (mMaxLines > 0 && getLineCount() > mMaxLines) {

This mMaxLines > 0 check feels unnecessary, since it would be weird for a consumer to use this EllipsisTextView but explicitly set ellipsizeAtLine to 0 (that's what would need to happen, since you default this to 1 in the constructor).

@@ +50,5 @@
> +        }
> +
> +        // If we are over the max line attribute, ellipsize
> +        while (mMaxLines > 0 && getLineCount() > mMaxLines) {
> +            final int lineEndIndex = getLayout().getLineEnd(mMaxLines - 1);

I believe getLineEnd returns an offset, not an index, so you'll want to subtract 1 from this value. I believe this is why the text is getting ellipsized earlier than it is without this patch, and I think that's also why you decided to introduce the while loop.

@@ +53,5 @@
> +        while (mMaxLines > 0 && getLineCount() > mMaxLines) {
> +            final int lineEndIndex = getLayout().getLineEnd(mMaxLines - 1);
> +            final String text = getText().subSequence(0, lineEndIndex - ellipsis.length())
> +                + ellipsis;
> +            setText(text);

Because you're being so precise with calculating where the text ends, you shouldn't need a while loop. I was playing around with this myself, and I found that this snippet got the job done:

    @Override
    public void onLayout(boolean changed, int left, int top, int right, int bottom) {
        super.onLayout(changed, left, top, right, bottom);

        // There is extra space, start over with the original text
        if (getLineCount() < mMaxLines) {
            setText(mOriginalText);
        }

        // If we are over the max line attribute, ellipsize
        if (getLineCount() > mMaxLines) {
            final int lineEndIndex = getLayout().getLineEnd(mMaxLines - 1) - 1;
            final String text = mOriginalText.subSequence(0, lineEndIndex - ellipsis.length()) + ellipsis;
            setText(text);
        }
    }

Comment 40

[Josh Dover [:jdover]]

Attachment: patch

Simplified ellipsizing logic.

Comment 41

[:Margaret Leibovic]

Comment on attachment 8386978 [details] [diff] [review]
patch

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

I don't love that we need to do this, but if we need to do this, I think this is a fine solution. However, I want a second opinion from Brian before we land this :)

Comment 42

[Brian Nicholson (:bnicholson)]

Comment on attachment 8386978 [details] [diff] [review]
patch

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

Looks good, though I'd recommend overriding setText as opposed to introducing a new method.

::: mobile/android/base/widget/EllipsisTextView.java
@@ +31,5 @@
> +        mMaxLines = a.getInteger(R.styleable.EllipsisTextView_ellipsizeAtLine, 1);
> +        a.recycle();
> +    }
> +
> +    public void setOriginalText(CharSequence originalText) {

Rather than introducing setOriginalText, how about you just override setText?

@@ +33,5 @@
> +    }
> +
> +    public void setOriginalText(CharSequence originalText) {
> +        mOriginalText = originalText;
> +        setText(originalText);

...and call super.setText() here...

@@ +42,5 @@
> +        super.onLayout(changed, left, top, right, bottom);
> +
> +        // There is extra space, start over with the original text
> +        if (getLineCount() < mMaxLines) {
> +            setText(mOriginalText);

...and here...

@@ +49,5 @@
> +        // If we are over the max line attribute, ellipsize
> +        if (getLineCount() > mMaxLines) {
> +            final int endIndex = getLayout().getLineEnd(mMaxLines - 1) - 1 - ellipsis.length();
> +            final String text = getText().subSequence(0, endIndex) + ellipsis;
> +            setText(text);

...and here?

Comment 43

[Brian Nicholson (:bnicholson)]

Comment on attachment 8386978 [details] [diff] [review]
patch

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

Sorry, did that review a bit hastily and missed a few things. Downgrading to f+ since I think this needs one more iteration.

::: mobile/android/base/widget/EllipsisTextView.java
@@ +17,5 @@
> +/**
> + * Text view that correctly handles maxLines and ellipsizing for Android < 2.3.
> + */
> +public class EllipsisTextView extends TextView {
> +    private final String ellipsis = getResources().getString(R.string.ellipsis);

Nit: Use all caps since this is a constant (ELLIPSIS)

Also nit: Please initialize this in the constructor where you initialize the others.

@@ +18,5 @@
> + * Text view that correctly handles maxLines and ellipsizing for Android < 2.3.
> + */
> +public class EllipsisTextView extends TextView {
> +    private final String ellipsis = getResources().getString(R.string.ellipsis);
> +    private final int mMaxLines;

s/mMaxLines/maxLines/g. Front-end devs decided to drop the mX convention for new code.

@@ +20,5 @@
> +public class EllipsisTextView extends TextView {
> +    private final String ellipsis = getResources().getString(R.string.ellipsis);
> +    private final int mMaxLines;
> +
> +    private CharSequence mOriginalText;

s/mOriginalText/originalText/g

@@ +22,5 @@
> +    private final int mMaxLines;
> +
> +    private CharSequence mOriginalText;
> +
> +    public EllipsisTextView(Context context, AttributeSet attrs) {

Please define other constructors [View(Context context) and View(Context context, AttributeSet attrs, int defStyleAttr)] so this can safely be used in other situations.

Comment 44

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #43)
> ::: mobile/android/base/widget/EllipsisTextView.java
> @@ +17,5 @@
> > +/**
> > + * Text view that correctly handles maxLines and ellipsizing for Android < 2.3.
> > + */
> > +public class EllipsisTextView extends TextView {
> > +    private final String ellipsis = getResources().getString(R.string.ellipsis);
> 
> Nit: Use all caps since this is a constant (ELLIPSIS)

Argh, was too hasty again. Ignore this suggestion. I thought ellipsis was static, which it is not.

Comment 45

[Josh Dover [:jdover]]

Attachment: patch

Fixed to override setText() properly. FYI setText(CharSequence) is final, so I had to override setText(CharSequence, BufferType).

Addressed various nits and added other View constructors.

Comment 46

[Josh Dover [:jdover]]

Comment on attachment 8388727 [details] [diff] [review]
patch

Forgot that bnicholson was on vacation

Comment 47

[:Margaret Leibovic]

Comment on attachment 8388727 [details] [diff] [review]
patch

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

r- for the constructor logic, but I also want to push back against bnicholson's suggestion to override setText, since it turns out we can't fully implement his suggestion.

::: mobile/android/base/widget/EllipsisTextView.java
@@ +44,5 @@
> +        TypedArray a = context.getTheme()
> +            .obtainStyledAttributes(attrs, R.styleable.EllipsisTextView, 0, 0);
> +        maxLines = a.getInteger(R.styleable.EllipsisTextView_ellipsizeAtLine, 1);
> +        a.recycle();
> +    }

Instead of making this init method and duplicating code in all the constructors, you can just put the meat of your logic in the last constructor, and then call it from the other constructors.

Here is an example of that:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/FadedTextView.java#31

@@ +48,5 @@
> +    }
> +
> +    private String getEllipsis() {
> +        return getResources().getString(R.string.ellipsis);
> +    }

You won't need this helper method once you assign ellipsis in only one place.

@@ +70,5 @@
> +        if (getLineCount() > maxLines) {
> +            final int endIndex = getLayout().getLineEnd(maxLines - 1) - 1 - ellipsis.length();
> +            final String text = getText().subSequence(0, endIndex) + ellipsis;
> +            // Call super.setText() so that we don't change originalText
> +            super.setText(text, BufferType.NORMAL);

Since we're not overriding setText(CharSequence text), you could just call setText(text) here.

However, I think that if we can't properly override setText, we should just make our own setOriginalText method like your previous patch did. Otherwise, I feel like it's too confusing that specifying a BufferType means that we're setting the original text, but omitting it won't set the original text.

Comment 48

[Josh Dover [:jdover]]

Attachment: patch

(In reply to :Margaret Leibovic from comment #47)
> Instead of making this init method and duplicating code in all the
> constructors, you can just put the meat of your logic in the last
> constructor, and then call it from the other constructors.
> 
> Here is an example of that:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> FadedTextView.java#31

Ah yes I couldn't find what the default AttributeSet was when Googling around, this did the trick.

> 
> @@ +70,5 @@
> > +        if (getLineCount() > maxLines) {
> > +            final int endIndex = getLayout().getLineEnd(maxLines - 1) - 1 - ellipsis.length();
> > +            final String text = getText().subSequence(0, endIndex) + ellipsis;
> > +            // Call super.setText() so that we don't change originalText
> > +            super.setText(text, BufferType.NORMAL);
> 
> Since we're not overriding setText(CharSequence text), you could just call
> setText(text) here.
> 
> However, I think that if we can't properly override setText, we should just
> make our own setOriginalText method like your previous patch did. Otherwise,
> I feel like it's too confusing that specifying a BufferType means that we're
> setting the original text, but omitting it won't set the original text.

If you call any of the final super.setText() methods, our overriden setText(CharSequence, BufferType) will be called. The only way to not reset the originalText is to call super.setText(CharSequence, BufferType). I changed this to a private method called setTextAndKeepOriginal() that I think makes this distinction clear and allows us to keep the standard TextView API.

Comment 49

[:Margaret Leibovic]

Comment on attachment 8389522 [details] [diff] [review]
patch

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

::: mobile/android/base/widget/EllipsisTextView.java
@@ +41,5 @@
> +        a.recycle();
> +    }
> +
> +    @Override
> +    public void setText(CharSequence text, BufferType type) {

As I said in my last review, I think it's confusing if we override setText with a BufferType parameter, but not without one. I would prefer that we just implement a new setOriginalText method as you did in your previous patch.

@@ +52,5 @@
> +        super.onLayout(changed, left, top, right, bottom);
> +
> +        // There is extra space, start over with the original text
> +        if (getLineCount() < maxLines) {
> +            setTet(originalText);

I don't trust that you tested this, since it looks like this won't even build :)

Comment 50

[Josh Dover [:jdover]]

Attachment: patch

Reverted to use setOriginalText()

Comment 51

[:Margaret Leibovic]

Comment on attachment 8389948 [details] [diff] [review]
patch

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

Thanks, we can always iterate on this in the future (and perhaps retire it if we decide to permanently disable the banner for 2.3 and below).

::: mobile/android/base/home/HomeBanner.java
@@ +53,5 @@
>      // Tracks whether the user swiped the banner down, preventing us from autoshowing when the user
>      // switches back to the default page.
>      private boolean mUserSwipedDown = false;
>  
> +    private final EllipsisTextView mTextView;

Add a comment explaining that we use EllipsisTextView to work around an android bug on older devices (so that we can remember to get rid of this in the future when one day we drop support for those devices).

Comment 52

[Josh Dover [:jdover]]

Attachment: patch

Comment 53

[Josh Dover [:jdover]]

Clearing needinfo for ibarlow

Comment 54

[Ryan VanderMeulen [:RyanVM]]

This has conflicts. Please rebase.

Comment 55

[:Margaret Leibovic]

I wonder if we should even bother landing this if we're doing to disable the banner for pre-honeycomb devices. We should see what we end up deciding in bug 982181. At the very least if we decide to just disable it on aurora, we won't need to chase uplifting this patch.

Comment 56

[Josh Dover [:jdover]]

Attachment: rebased patch

Here is a rebased patch. Flagging needinfo for margaret pending any decisions in bug 982181 before landing.

Comment 57

[:Margaret Leibovic]

(In reply to Josh Dover from comment #56)
> Created attachment 8390909 [details] [diff] [review]
> rebased patch
> 
> Here is a rebased patch. Flagging needinfo for margaret pending any
> decisions in bug 982181 before landing.

We're not going to be disabling on this for pre-honeycomb devices. Sorry for the noise!

Comment 58

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing again :(

Comment 59

[Josh Dover [:jdover]]

Attachment: rebased patch

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/7381f6b0c4f2

Comment 61

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7381f6b0c4f2