Closed Bug 921557 Opened 8 years ago Closed 7 years ago

Banner text on small-screen devices is cut off

Categories

(Firefox for Android Graveyard :: General, defect)

27 Branch
ARM
Android
defect
Not set
normal

Tracking

(fennec29+)

RESOLVED FIXED
Firefox 31
Tracking Status
fennec 29+ ---

People

(Reporter: aaronmt, Assigned: jdover)

References

Details

Attachments

(3 files, 15 obsolete files)

44.93 KB, image/png
Details
49.56 KB, image/png
Details
12.55 KB, patch
jdover
: review+
Details | Diff | Splinter Review
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
(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?
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 :(
(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?
Flags: needinfo?(ibarlow)
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.
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.
Flags: needinfo?(ibarlow)
We should fix this before we ship a dynamic snippets add-on.
tracking-fennec: --- → ?
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → 28+
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?
tracking-fennec: 28+ → ?
tracking-fennec: ? → 28+
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.
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/ea722ce55e26
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
tracking-fennec: 28+ → 29+
Josh has been working on this :)
Assignee: margaret.leibovic → jdover
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.
Attachment #8367499 - Flags: review?(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.
Attachment #8367499 - Flags: review?(margaret.leibovic)
Depends on: 965552
Status: REOPENED → ASSIGNED
Stripped out unneeded code and simplified AutoResizeTextView.
Attachment #8367499 - Attachment is obsolete: true
Attachment #8377888 - Flags: review?(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?
Attachment #8377888 - Flags: review?(margaret.leibovic) → review-
(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 &#8230;) 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.
Flags: needinfo?(ibarlow)
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 :)
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
Attachment #8379971 - Flags: review?(margaret.leibovic)
Attachment #8377888 - Attachment description: Use AutoResizeTextView for home banner → 01 - Use AutoResizeTextView for home banner
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.
Attachment #8379971 - Flags: review?(margaret.leibovic) → feedback+
Attached image 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.
Also, I'm not actually seeing the ellipsis appear, even when building without your second patch... I feel a bit skeptical about this widget :/
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.
Flags: needinfo?(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.
Flags: needinfo?(margaret.leibovic)
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.
Attachment #8377888 - Attachment is obsolete: true
Attachment #8379971 - Attachment is obsolete: true
Attachment #8383346 - Flags: review?(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.
Attachment #8383346 - Flags: review?(margaret.leibovic) → feedback+
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.
Attachment #8383346 - Flags: feedback?(bnicholson)
(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.
Flags: needinfo?(francesco.lodolo)
Fixed to use onLayout(), need input from Francesco on RTL.
Attachment #8383346 - Attachment is obsolete: true
Attachment #8383346 - Flags: feedback?(bnicholson)
Attachment #8383974 - Flags: review?(margaret.leibovic)
Attachment #8383974 - Flags: feedback?(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?
Attachment #8383974 - Flags: feedback?(bnicholson) → feedback+
(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 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.
Attachment #8383974 - Flags: feedback-
(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?
Flags: needinfo?(francesco.lodolo)
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?
Attachment #8383974 - Attachment is obsolete: true
Attachment #8383974 - Flags: review?(margaret.leibovic)
Attachment #8384901 - Flags: review?(margaret.leibovic)
Missed margaret's comment about using the actual ellipsis character in the strings.dtd
Attachment #8384901 - Attachment is obsolete: true
Attachment #8384901 - Flags: review?(margaret.leibovic)
Attachment #8384907 - Flags: review?(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.
Attachment #8384907 - Flags: review?(margaret.leibovic) → feedback+
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8384907 - Attachment is obsolete: true
Attachment #8385756 - Flags: review?(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);
        }
    }
Attachment #8385756 - Flags: review?(margaret.leibovic) → review-
Attached patch patch (obsolete) — Splinter Review
Simplified ellipsizing logic.
Attachment #8385756 - Attachment is obsolete: true
Attachment #8386978 - Flags: review?(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 :)
Attachment #8386978 - Flags: review?(margaret.leibovic)
Attachment #8386978 - Flags: review?(bnicholson)
Attachment #8386978 - Flags: review+
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?
Attachment #8386978 - Flags: review?(bnicholson) → review+
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.
Attachment #8386978 - Flags: review+ → feedback+
(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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8386978 - Attachment is obsolete: true
Attachment #8388727 - Flags: review?(bnicholson)
Comment on attachment 8388727 [details] [diff] [review]
patch

Forgot that bnicholson was on vacation
Attachment #8388727 - Flags: review?(bnicholson) → review?(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.
Attachment #8388727 - Flags: review?(margaret.leibovic) → review-
Attached patch patch (obsolete) — Splinter Review
(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.
Attachment #8388727 - Attachment is obsolete: true
Attachment #8389522 - Flags: review?(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 :)
Attachment #8389522 - Flags: review?(margaret.leibovic) → review-
Attached patch patch (obsolete) — Splinter Review
Reverted to use setOriginalText()
Attachment #8388641 - Attachment is obsolete: true
Attachment #8389522 - Attachment is obsolete: true
Attachment #8389948 - Flags: review?(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).
Attachment #8389948 - Flags: review?(margaret.leibovic) → review+
Attached patch patch (obsolete) — Splinter Review
Attachment #8389948 - Attachment is obsolete: true
Attachment #8390068 - Flags: review+
Clearing needinfo for ibarlow
Flags: needinfo?(ibarlow)
Keywords: checkin-needed
This has conflicts. Please rebase.
Keywords: checkin-needed
Flags: needinfo?(jdover)
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.
Attached patch rebased patch (obsolete) — Splinter Review
Here is a rebased patch. Flagging needinfo for margaret pending any decisions in bug 982181 before landing.
Attachment #8390068 - Attachment is obsolete: true
Attachment #8390909 - Flags: review+
Flags: needinfo?(jdover) → needinfo?(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!
Flags: needinfo?(margaret.leibovic)
Keywords: checkin-needed
Needs rebasing again :(
Keywords: checkin-needed
Attached patch rebased patchSplinter Review
Attachment #8390909 - Attachment is obsolete: true
Attachment #8392330 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7381f6b0c4f2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: Firefox 29 → Firefox 31
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.