Implement eliding/truncating at end in RenderText

ui/gfx/render_text.cc

Index: ui/gfx/render_text.cc
diff --git a/ui/gfx/render_text.cc b/ui/gfx/render_text.cc
index e8e3436f11c0508303fe1515447752d7ce341936..201aff194f56f52a2847f0e7dd881314b106fe9c 100644
--- a/ui/gfx/render_text.cc
+++ b/ui/gfx/render_text.cc
@@ -19,6 +19,7 @@
 #include "ui/gfx/skia_util.h"
 #include "ui/gfx/text_constants.h"
 #include "ui/gfx/text_elider.h"
+#include "ui/gfx/text_utils.h"
 #include "ui/gfx/utf16_indexing.h"
 
 namespace gfx {
@@ -433,6 +434,14 @@ void RenderText::SetMultiline(bool multiline) {
   }
 }
 
+void RenderText::Elide(float available_pixel_width,
+                       ElideBehavior elide_behavior) {
+  available_pixel_width_ = available_pixel_width;
+  elide_behavior_ = elide_behavior;
+  UpdateLayoutText();
+  ResetLayout();

[msw, 2013/12/11 08:10:14]

nit: Would you mind adding a ResetLayout() call to the end of UpdateLayoutText()
call? Then remove all the explicitly paired ResetLayout() calls.

[Anuj, 2013/12/12 08:08:41]

Done.

[msw, 2013/12/12 19:28:29]

Thanks!

[Anuj, 2013/12/13 01:23:50]

Done.

+}
+
 void RenderText::SetDisplayRect(const Rect& r) {
   display_rect_ = r;
   baseline_ = kInvalidBaseline;
@@ -830,6 +839,8 @@ RenderText::RenderText()
       obscured_(false),
       obscured_reveal_index_(-1),
       truncate_length_(0),
+      available_pixel_width_(0.0),
+      elide_behavior_(NO_ELISION),
       multiline_(false),
       fade_head_(false),
       fade_tail_(false),
@@ -1116,6 +1127,111 @@ void RenderText::UpdateLayoutText() {
     iter.setIndex32(truncate_length_ - 1);
     layout_text_.assign(text.substr(0, iter.getIndex()) + gfx::kEllipsisUTF16);
   }
+
+  int content_width = GetContentWidth();
+  if (elide_behavior_ != NO_ELISION && available_pixel_width_ > 0 &&
+      content_width > available_pixel_width_) {
+    base::string16 elided_text = ElideText(text);

[msw, 2013/12/11 08:10:14]

This disregards the obscuring and truncation done above, which seems bad. You
should do something like the |text| const reference above that conditionally
uses |text_| or |layout_text_|. Both this and the |text| assignment above can
actually call GetLayoutText() to do the same.

[Anuj, 2013/12/12 08:08:41]

Done.

+
+    // This doesn't trim styles so ellipsis may get rendered as a different

[msw, 2013/12/11 08:10:14]

I suppose it'll be tough to temporarily apply styles based on the elided text
length, but otherwise you might get some weird styling of the ellipsis. I would
suggest trying to modify ApplyCompositionAndSelectionStyles to ApplyLayoutStyles
or similar, and extending the last style of the unelided text to the ellipsis
there. If you want, you can file an issue, add a comment here, just deal with
potentially buggy elided styles for now, and address this in a followup CL.

[Anuj, 2013/12/12 08:08:41]

Filed crbug.com/327850

+    // style than the preceding text.
+    // DISCUSS WITH REVIEWER.
+    layout_text_.assign(elided_text);
+  }
+}
+
+base::string16 RenderText::ElideText(const base::string16& text) {

[msw, 2013/12/11 08:10:14]

This is quite similar to ElideText from text_elider... please at least file a
bug against yourself to reduce the redundancy of these codepaths and add a TODO
comment with the bug number here.

[Anuj, 2013/12/12 08:08:41]

Done.

+  if (text.empty())
+    return text;
+
+  const float current_text_pixel_width = GetStringSizeF().width();

[msw, 2013/12/11 08:10:14]

GetStringSizeF will actually EnsureLayout and calculate the size of the current
GetLayoutText(), which should coincide with the supplied |text| argument if you
address the changes I suggest above, but I suspect that you can forego the
argument altogether and use GetLayoutText() below in this function.

[Anuj, 2013/12/12 08:08:41]

I can't forgo the argument because of the recursive call for width overflow. I
moved the render text creation up here to handle that.

[msw, 2013/12/12 19:28:29]

Hmm, nice!

[Anuj, 2013/12/13 01:23:50]

Done.

+  const bool elide_in_middle = false;
+  const bool insert_ellipsis = (elide_behavior_ != TRUNCATE_AT_END);
+
+  const base::string16 ellipsis = base::string16(kEllipsisUTF16);
+  StringSlicer slicer(text, ellipsis, elide_in_middle);
+
+  // Pango will return 0 width for absurdly long strings. Cut the string in
+  // half and try again.
+  // This is caused by an int overflow in Pango (specifically, in
+  // pango_glyph_string_extents_range). It's actually more subtle than just
+  // returning 0, since on super absurdly long strings, the int can wrap and
+  // return positive numbers again. Detecting that is probably not worth it
+  // (eliding way too much from a ridiculous string is probably still
+  // ridiculous), but we should check other widths for bogus values as well.
+  if (current_text_pixel_width <= 0 && !text.empty()) {
+    const base::string16 cut = slicer.CutString(text.length() / 2, false);

[msw, 2013/12/11 08:10:14]

Shouldn't this actually insert an ellipsis if |insert_ellipsis| is true?
Otherwise, if we cut the string in half, and that happens to fit in the
available space, we'd have a string that's truncated instead of elided, when the
next current_text_pixel_width <= available_pixel_width_ returns the halved text.
Ditto for the text_elider implemention from which this was copied.

[Anuj, 2013/12/12 08:08:41]

Done.

+    return ElideText(cut);
+  }
+
+  if (current_text_pixel_width <= available_pixel_width_)
+    return text;
+
+  if (insert_ellipsis &&
+      GetStringWidthF(ellipsis, font_list_) > available_pixel_width_)
+    return base::string16();
+
+  // Use binary search to compute the elided text.
+  size_t lo = 0;
+  size_t hi = text.length() - 1;
+  size_t guess;

[msw, 2013/12/11 08:10:14]

nit: declare guess in the for loop's control.

[Anuj, 2013/12/12 08:08:41]

Done.

+
+  // Create a copy render text. We copy the attributes which affect the
+  // rendering width.
+  scoped_ptr<RenderText> render_text(CreateInstance());
+  render_text->SetFontList(font_list_);
+  render_text->SetDirectionalityMode(directionality_mode_);
+  render_text->styles_ = styles_;
+  render_text->colors_ = colors_;
+  render_text->SetText(text);
+  for (guess = (lo + hi) / 2; lo <= hi; guess = (lo + hi) / 2) {
+    // Restore styles and colors. They will be truncated to size by SetText.
+    render_text->styles_ = styles_;
+    render_text->colors_ = colors_;
+    base::string16 newtext = slicer.CutString(guess, false);

[msw, 2013/12/11 08:10:14]

nit: use the unix_hacker naming convention |new_text|.

[Anuj, 2013/12/12 08:08:41]

Done.

+    render_text->SetText(newtext);
+
+    // This has to be an additional step so that the ellipsis is rendered with
+    // same style as trailing part of the text.
+    if (insert_ellipsis) {
+      newtext += ellipsis;
+
+      // When ellipsis follows text whose directionality is not the same as that
+      // of the whole text, it will be rendered with the directionality of the
+      // whole text. Since we want ellipsis to indicate continuation of the
+      // preceding text, we force the directionality of ellipsis to be same as
+      // the preceding text using LTR or RTL markers.
+      base::i18n::TextDirection trailing_text_direction =
+        base::i18n::GetLastStrongCharacterDirection(newtext);

[msw, 2013/12/11 08:10:14]

nit: indent two more spaces.

[Anuj, 2013/12/12 08:08:41]

Done.

+      if (trailing_text_direction != render_text->GetTextDirection()) {
+        if (trailing_text_direction == base::i18n::LEFT_TO_RIGHT)
+          newtext += base::i18n::kLeftToRightMark;

[msw, 2013/12/11 08:10:14]

I think you should use WrapStringWithLTRFormatting and
WrapStringWithRTLFormatting on the ellipsis substring, then append that to
|newtext|? Otherwise, don't you need to append the directionality control
character before appending the ellipsis, and probably also add a
kPopDirectionalFormatting in case text is ever appended to the result?

[Anuj, 2013/12/11 18:41:32]

See implicit markers here -
http://www.iamcal.com/understanding-bidirectional-text/. What you are referring
to are explicit markers, and they need a PDF (pop-directional-formatting).

I also wanted to check if similar change should be done in text_elider, probably
based on an additional param.

Finally, I have tested it manually, it works.

[msw, 2013/12/11 19:39:57]

Cool, using that mark seems correct, thanks for the info. Yes, you'll need to
update text_elider similarly, why do you need an extra param?

[Anuj, 2013/12/12 08:08:41]

I suggested an extra param because existing users of that method do not expect
RTL marker. And what if someone has chosen to inspect size of the returned
string from that method, or has an assumption that the last character will be
ellipsis?

So adding a variant in text_elider.cc

[msw, 2013/12/12 19:28:29]

I suggest making the default behavior add the directionality control marks
because it's the right thing to do; we can update callers as needed.

[Anuj, 2013/12/13 01:23:50]

Adding a TODO, because ellipsis in middle also needs some additional handling.

+        else
+          newtext += base::i18n::kRightToLeftMark;
+      }
+      render_text->SetText(newtext);
+    }
+
+    // We check the length of the whole desired string at once to ensure we

[msw, 2013/12/11 08:10:14]

nit: s/length/width/, s/guess_length/guess_width/; ditto for text_elider.

[Anuj, 2013/12/12 08:08:41]

Done.

+    // handle kerning/ligatures/etc. correctly.
+    const float guess_length = render_text->GetStringSizeF().width();
+    // Check again that we didn't hit a Pango width overflow. If so, cut the

[msw, 2013/12/11 08:10:14]

This shouldn't be necessary here or in the text_elider implementation; above,
|text| is under the Pango limit, then it's cut in half and we append an ellipsis
and perhaps directionality control characters. Halving a sufficiently short
string then adding a few characters should not exceed the Pango limit, right?

[Anuj, 2013/12/12 08:08:41]

I thought adding ellipsis can cause that behavior. But I think you are right.
The guess starts at ~length/2, and since we do not reach here until the pango
width is valid, this should not be needed.

Done.

+    // current string in half and start over.
+    if (guess_length <= 0) {

[msw, 2013/12/11 08:10:14]

nit: remove unnecessary curly braces.

[Anuj, 2013/12/12 08:08:41]

Done.

+      return ElideText(slicer.CutString(guess / 2, false));
+    }
+    if (guess_length > available_pixel_width_) {

[msw, 2013/12/11 08:10:14]

Shouldn't this break if guess_length == available_pixel_width_?

[Anuj, 2013/12/12 08:08:41]

Done.

+      // Move back if we are on loop terminating condition, and guess is longer
+      // than available.
+      if (hi == lo)

[msw, 2013/12/11 08:10:14]

Sorry, I'm failing to understand how this can happen, and the original
text_elider implementation doesn't do this; can you give an example of how this
might happen? Does this happen only because there's no break on equal widths as
I noted above? Should the original text_elider code be updated? Can you add a
unit test to ensure this doesn't happen and verify that without this workaround
the algorithm might yield a string wider than appropriate?

[Anuj, 2013/12/11 18:41:32]

I had a scenario where the following happened
available = 182
guess = 21, guess_length = 181
guess = 23, guess_length =193
guess = 22, guess_length=186 (hi = 22, lo = 22)

I don't remember the previous hi-lo values. So guess == available never happens.
I can add this to text_elider too if you think this is the right fix.

[msw, 2013/12/11 19:39:57]

Yeah, I worked through a few conceptual examples and I think you're right, but
I'm not sure this is a sufficient solution. Consider a 4-char string where two
characters will fit, but three won't "<<>>":
a: low=0, hi=3, guess=1 ("<<" is sufficiently short)
b: low=2, hi=3, guess=2 ("<<>" is too long)
c: low=2, hi=1... We terminate with "<<>", which is too long.
Yes, you'll need to update the text_elider implementation, that's one of the
fundamental problems with code duplication, one is fixed and the other isn't...

[Anuj, 2013/12/12 08:08:41]

I got a new interview question ;)

[msw, 2013/12/12 19:28:29]

I thought binary searches were supposed to be easy!
Your new solution looks good.

[Anuj, 2013/12/13 01:23:50]

Done.

+        lo = guess - 1;
+      hi = guess - 1;
+    } else {
+      lo = guess + 1;
+    }
+  }
+
+  return render_text->text();
 }
 
 void RenderText::UpdateCachedBoundsAndOffset() {