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 bd590f086fe51fc771024c62dabd6adc7a2b15c5..c483c73cf070358cd1104820bde1631ca04d2038 100644
--- a/ui/gfx/render_text.cc
+++ b/ui/gfx/render_text.cc
@@ -10,6 +10,7 @@
 #include "base/i18n/break_iterator.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
+#include "base/strings/utf_string_conversions.h"
 #include "third_party/icu/source/common/unicode/rbbi.h"
 #include "third_party/icu/source/common/unicode/utf16.h"
 #include "third_party/skia/include/core/SkTypeface.h"
@@ -19,6 +20,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 {
@@ -370,7 +372,6 @@ void RenderText::SetText(const base::string16& text) {
 
   obscured_reveal_index_ = -1;
   UpdateLayoutText();
-  ResetLayout();
 }
 
 void RenderText::SetHorizontalAlignment(HorizontalAlignment alignment) {
@@ -416,7 +417,6 @@ void RenderText::SetObscured(bool obscured) {
     obscured_reveal_index_ = -1;
     cached_bounds_and_offset_valid_ = false;
     UpdateLayoutText();
-    ResetLayout();
   }
 }
 
@@ -427,7 +427,6 @@ void RenderText::SetObscuredRevealIndex(int index) {
   obscured_reveal_index_ = index;
   cached_bounds_and_offset_valid_ = false;
   UpdateLayoutText();
-  ResetLayout();
 }
 
 void RenderText::SetMultiline(bool multiline) {
@@ -438,11 +437,23 @@ void RenderText::SetMultiline(bool multiline) {
   }
 }
 
+void RenderText::SetElideBehavior(ElideBehavior elide_behavior) {
+  // TODO(skanuj) : Add a test for triggering layout change.
+  if (elide_behavior_ != elide_behavior) {
+    elide_behavior_ = elide_behavior;
+    UpdateLayoutText();
+  }
+}
+
 void RenderText::SetDisplayRect(const Rect& r) {
-  display_rect_ = r;
-  baseline_ = kInvalidBaseline;
-  cached_bounds_and_offset_valid_ = false;
-  lines_.clear();
+  if (r != display_rect_) {
+    display_rect_ = r;
+    baseline_ = kInvalidBaseline;
+    cached_bounds_and_offset_valid_ = false;
+    lines_.clear();
+    if (elide_behavior_ != gfx::NO_ELIDE)
+      UpdateLayoutText();
+  }
 }
 
 void RenderText::SetCursorPosition(size_t position) {
@@ -835,6 +846,7 @@ RenderText::RenderText()
       obscured_(false),
       obscured_reveal_index_(-1),
       truncate_length_(0),
+      elide_behavior_(NO_ELIDE),
       multiline_(false),
       fade_head_(false),
       fade_tail_(false),
@@ -1114,13 +1126,122 @@ void RenderText::UpdateLayoutText() {
     }
   }
 
-  const base::string16& text = obscured_ ? layout_text_ : text_;
+  const base::string16& text = GetLayoutText();
   if (truncate_length_ > 0 && truncate_length_ < text.length()) {
     // Truncate the text at a valid character break and append an ellipsis.
     icu::StringCharacterIterator iter(text.c_str());
     iter.setIndex32(truncate_length_ - 1);
     layout_text_.assign(text.substr(0, iter.getIndex()) + gfx::kEllipsisUTF16);
   }
+
+  if (elide_behavior_ != NO_ELIDE && display_rect_.width() > 0 &&
+      GetContentWidth() > display_rect_.width()) {
+    base::string16 elided_text = ElideText(GetLayoutText());
+
+    // This doesn't trim styles so ellipsis may get rendered as a different
+    // style than the preceding text. See crbug.com/327850.
+    layout_text_.assign(elided_text);
+  }
+  ResetLayout();
+}
+
+// TODO(skanuj): Fix code duplication with ElideText in ui/gfx/text_elider.cc
+// See crbug.com/327846
+base::string16 RenderText::ElideText(const base::string16& text) {
+  if (text.empty())

[msw, 2013/12/27 22:54:33]

nit: consider an early return (text or empty) for display_rect_.width() == 0.

[Anuj, 2013/12/30 22:46:58]

That check exists on line 1137. I think this is fine, although the early return
conditions are split in two places. If you feel strongly about it, I will like
to move all of them together at one place - here at the beginning of the method,
or before the call to this method.

[msw, 2013/12/30 23:17:59]

Please don't delete my original comments when replying; it's frustrating to read
replies without context, or to dig into each file to revive that context.
You can move this check out to UpdateLayoutText.

[Anuj, 2013/12/31 00:04:03]

I think that comments are insufficient context, and if you want code in context
too, you have to dig into each file. I will try not to remove your comments from
context, but no promises because I think they are ugly/redundant.

[msw, 2013/12/31 00:24:52]

ok, lol

[Peter Kasting, 2013/12/31 00:27:18]

FWIW, it also kinda drives me nuts when people remove all previous comments, or
reply above them instead of below.  It's really hard to recall the discussion
properly sometimes.

I usually compromise by removing all but one level of previous comments (or
sometimes more levels when more context is necessary), as I've done in this
case.

+    return text;
+
+  const bool insert_ellipsis = (elide_behavior_ != TRUNCATE_AT_END);
+  // Create a RenderText copy with attributes that affect the rendering width.
+  scoped_ptr<RenderText> render_text(CreateInstance());
+  render_text->SetFontList(font_list_);
+  render_text->SetDirectionalityMode(directionality_mode_);
+  render_text->SetCursorEnabled(cursor_enabled_);
+
+  render_text->styles_ = styles_;
+  render_text->colors_ = colors_;
+  render_text->SetText(text);
+  const int current_text_pixel_width = render_text->GetContentWidth();
+
+  const base::string16 ellipsis = base::string16(gfx::kEllipsisUTF16);
+  const bool elide_in_middle = false;
+  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())
+    return ElideText(slicer.CutString(text.length() / 2, insert_ellipsis));
+
+  if (current_text_pixel_width <= display_rect_.width())
+    return text;
+
+  render_text->SetText(base::string16());
+  render_text->SetText(ellipsis);
+  const int ellipsis_width = render_text->GetContentWidth();
+
+  if (insert_ellipsis && (ellipsis_width >= display_rect_.width()))
+    return base::string16();
+

[msw, 2013/12/27 22:54:33]

nit: remove the second blank line here.

[Anuj, 2013/12/30 22:46:58]

Done.

+
+  // Use binary search to compute the elided text.
+  size_t lo = 0;
+  size_t hi = text.length() - 1;
+  for (size_t guess = (lo + hi) / 2; (lo <= hi) && (guess < text.length());

[msw, 2013/12/27 22:54:33]

When would guess reasonably exceed the text length? Can you solve this correctly
rather than patching it over as such? A [D]CHECK + TODO inside the loop would be
better than this and perhaps sufficient for now.

[Anuj, 2013/12/30 22:46:58]

Given guess will be used as an index of text, I think this constraint is a good
idea.

+       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 new_text = slicer.CutString(guess, false);
+    render_text->SetText(new_text);
+
+    // 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) {
+      // 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 leading_text_direction =
+          base::i18n::GetFirstStrongCharacterDirection(new_text);
+      base::i18n::TextDirection trailing_text_direction =
+          base::i18n::GetLastStrongCharacterDirection(new_text);
+      new_text.append(ellipsis);
+      if (trailing_text_direction != leading_text_direction) {
+        if (trailing_text_direction == base::i18n::LEFT_TO_RIGHT)
+          new_text += base::i18n::kLeftToRightMark;
+        else
+          new_text += base::i18n::kRightToLeftMark;
+      }
+      render_text->SetText(new_text);
+    }
+
+    // We check the width of the whole desired string at once to ensure we
+    // handle kerning/ligatures/etc. correctly.
+    const int guess_width = render_text->GetContentWidth();
+    if (guess_width == display_rect_.width())
+      break;
+    if (guess_width > display_rect_.width()) {
+      // This shouldn't happen, but lets be paranoid.

[msw, 2013/12/27 22:54:33]

nit: let's, but I'd prefer a real fix (or perhaps a [D]CHECK + TODO for now).
I'm also confused as to how this occurs, were you able to repro this and the
ensuing underflow described at my my comment
<https://codereview.chromium.org/107513011/diff/140001/ui/gfx/render_text.cc#n...
With guess==0, the string should be empty (or just an ellipsis), and neither of
those should be wider than the available width (unless perhaps the
cursor_enabled_ is true and possibly when the display rect has 0 width). I
wonder if this should return |insert_ellipsis ? ellipsis : base::string16()|,
since the check at 1188 should ensure that an ellipsis on its own does not
exceed the available width.

[Anuj, 2013/12/30 22:46:58]

I was able to reproduce the issue you described in your earlier comment. This
won't happen anymore because of the check on line 1188 for ellipsis_width. For
some reason (likely cursor_enabled_, but I didn't confirm), the width of
ellipsis as determined by GetStringSize is different from that determined
through a RenderText instance. But now the ellipsis width is determined using
RenderText instance.

That said, for the completeness and correctness of this binary-search for-loop,
I think it is good to have this check. At this point, the guess_width would
correspond to a render_text which contains just an ellipsis when guess == 0 and
insert_ellipsis is true. So it should return an empty string.

I have modified how I check for guess being in correct bounds of the length of
text.

[msw, 2013/12/30 23:17:59]

I'm glad the revised ellipsis width checking fixed the underflow issue thanks.

[Anuj, 2013/12/31 00:04:03]

Done.

+      if (guess == 0)
+        return base::string16();
+      hi = guess - 1;
+      // Move back if we are on loop terminating condition, and guess is wider
+      // than available.
+      if (hi < lo)
+        lo = hi;
+    } else {
+      lo = guess + 1;
+    }
+  }
+
+  return render_text->text();
 }
 
 void RenderText::UpdateCachedBoundsAndOffset() {