Separate selection highlight from pango layout

ui/gfx/render_text_linux.cc

Index: ui/gfx/render_text_linux.cc
===================================================================
--- ui/gfx/render_text_linux.cc	(revision 109708)
+++ ui/gfx/render_text_linux.cc	(working copy)
@@ -38,6 +38,7 @@
       num_log_attrs_(0),
       layout_text_(NULL),
       layout_text_len_(0) {
+  selection_visual_bounds_.clear();

[msw, 2011/11/28 20:01:17]

This isn't necessary.

[xji, 2011/11/29 01:37:02]

removed.

 }
 
 RenderTextLinux::~RenderTextLinux() {
@@ -65,37 +66,8 @@
 }
 
 void RenderTextLinux::Draw(Canvas* canvas) {
-  PangoLayout* layout = EnsureLayout();
-  Rect bounds(display_rect());
-
-  // Clip the canvas to the text display area.
-  SkCanvas* canvas_skia = canvas->GetSkCanvas();
-
-  skia::ScopedPlatformPaint scoped_platform_paint(canvas_skia);
-  cairo_t* cr = scoped_platform_paint.GetPlatformSurface();
-  cairo_save(cr);
-  cairo_rectangle(cr, bounds.x(), bounds.y(), bounds.width(), bounds.height());
-  cairo_clip(cr);
-
-  int text_width, text_height;
-  pango_layout_get_pixel_size(layout, &text_width, &text_height);
-  Point offset(ToViewPoint(Point()));
-  // Vertically centered.
-  int text_y = offset.y() + ((bounds.height() - text_height) / 2);
-  // TODO(xji): need to use SkCanvas->drawPosText() for gpu acceleration.
-  cairo_move_to(cr, offset.x(), text_y);
-  pango_cairo_show_layout(cr, layout);
-
-  cairo_restore(cr);
-
-  // Paint cursor.
-  bounds = GetUpdatedCursorBounds();
-  if (cursor_visible() && focused())
-      canvas->DrawRectInt(kCursorColor,
-                          bounds.x(),
-                          bounds.y(),
-                          bounds.width(),
-                          bounds.height());
+  EnsureLayout();
+  RenderText::Draw(canvas);
 }
 
 SelectionModel RenderTextLinux::FindCursorPosition(const Point& point) {
@@ -220,6 +192,37 @@
   return SelectionModel(0, 0, SelectionModel::LEADING);
 }
 
+std::vector<Rect> RenderTextLinux::GetSubstringBounds(size_t from, size_t to) {
+  DCHECK(from <= text().length() && to <= text().length());

[msw, 2011/11/28 20:01:17]

This should be two separate DCHECK statements.

[xji, 2011/11/29 01:37:02]

Done.

+  std::vector<Rect> bounds;
+  if (from == to)
+    return bounds;
+
+  EnsureLayout();
+
+  if (from == GetSelectionStart() && to == GetCursorPosition()) {
+    if (selection_visual_bounds_.empty())

[msw, 2011/11/28 20:01:17]

Does the inefficiency or recalculating selection bounds justify the extra
complexity of caching selection_visual_bounds_? I suspect it doesn't.

[xji, 2011/11/29 01:37:02]

I am not worried too much about the calculation part.
I am more concerned on the g_free part in CalculateStringBounds().
We constantly repaint textfield. if there is an selection and you leave the
screen as is, the selection is constantly repaint, and g_free() is constantly
called.

Yes you are right that the code will be much simpler without caching selection
visual bounds (it is patch 3). Maybe Behdad has better idea on the memory
management.

+      GetSubstringBounds(from, to, &selection_visual_bounds_);
+    return selection_visual_bounds_;

[Alexei Svitkine (slow), 2011/11/28 19:56:14]

I think it would be cleaner to have a function GetSelectionBounds() that has
this logic and would do the caching and call GetSubstringBounds() as a helper.

[xji, 2011/11/29 01:37:02]

Introduced GetSelectionBounds().

+  } else {
+    GetSubstringBounds(from, to, &bounds);
+    return bounds;
+  }
+}
+
+void RenderTextLinux::SetSelectionModel(const SelectionModel& selection_model) {
+  size_t start = GetSelectionStart();
+  size_t end = GetCursorPosition();
+  size_t new_start = selection_model.selection_start();
+  size_t new_end = selection_model.selection_end();
+  if (!(start == new_start && end == new_end) &&
+      !(start == new_end && end == new_start))

[msw, 2011/11/28 20:01:17]

Are the bounds always the same when the start and end are reversed?
How often will that happen? I'd reduce complexity to simplify code.

[xji, 2011/11/29 01:37:02]

I think they are.
it probably is not that often. So I simplified the code without considering such
case.

+    selection_visual_bounds_.clear();
+
+  RenderText::SetSelectionModel(selection_model);
+}
+
+
 bool RenderTextLinux::IsCursorablePosition(size_t position) {
   if (position == 0 && text().empty())
     return true;
@@ -233,6 +236,30 @@
   ResetLayout();
 }
 
+void RenderTextLinux::DrawVisualText(Canvas* canvas) {

[msw, 2011/11/28 20:01:17]

Sync up with Alexei, he's re-writing this to use Skia.

+  Rect bounds(display_rect());
+
+  // Clip the canvas to the text display area.
+  SkCanvas* canvas_skia = canvas->GetSkCanvas();
+
+  skia::ScopedPlatformPaint scoped_platform_paint(canvas_skia);
+  cairo_t* cr = scoped_platform_paint.GetPlatformSurface();
+  cairo_save(cr);
+  cairo_rectangle(cr, bounds.x(), bounds.y(), bounds.width(), bounds.height());
+  cairo_clip(cr);
+
+  int text_width, text_height;
+  pango_layout_get_pixel_size(layout_, &text_width, &text_height);
+  Point offset(ToViewPoint(Point()));
+  // Vertically centered.
+  int text_y = offset.y() + ((bounds.height() - text_height) / 2);
+  // TODO(xji): need to use SkCanvas->drawPosText() for gpu acceleration.
+  cairo_move_to(cr, offset.x(), text_y);
+  pango_cairo_show_layout(cr, layout_);
+
+  cairo_restore(cr);
+}
+
 size_t RenderTextLinux::IndexOfAdjacentGrapheme(size_t index, bool next) {
   EnsureLayout();
   return Utf16IndexOfAdjacentGrapheme(Utf16IndexToUtf8Index(index), next);
@@ -525,44 +552,25 @@
     log_attrs_ = NULL;
     num_log_attrs_ = 0;
   }
+  if (!selection_visual_bounds_.empty())
+    selection_visual_bounds_.clear();
   layout_text_ = NULL;
   layout_text_len_ = 0;
 }
 
 void RenderTextLinux::SetupPangoAttributes(PangoLayout* layout) {
   PangoAttrList* attrs = pango_attr_list_new();
-  // Set selection background color.
-  // TODO(xji): There's a bug in pango that it can't use two colors in one

[msw, 2011/11/28 20:01:17]

Is this no longer true?

-  // glyph. Please refer to https://bugzilla.gnome.org/show_bug.cgi?id=648157
-  // for detail. So for example, if a font has "ffi" ligature, but you select
-  // half of that glyph, you either get the entire "ffi" ligature
-  // selection-colored, or none of it.
-  // We could use clipping to render selection.
-  // Use pango_glyph_item_get_logical_widths to find the exact boundaries of
-  // selection, then cairo_clip that, paint background, set color to white and
-  // redraw the layout.
-  SkColor selection_color =
-      focused() ? kFocusedSelectionColor : kUnfocusedSelectionColor;
-  size_t start = std::min(MinOfSelection(), text().length());
-  size_t end = std::min(MaxOfSelection(), text().length());
-  PangoAttribute* pango_attr;
-  if (end > start) {
-    pango_attr = pango_attr_background_new(
-        ConvertColorFrom8BitTo16Bit(SkColorGetR(selection_color)),
-        ConvertColorFrom8BitTo16Bit(SkColorGetG(selection_color)),
-        ConvertColorFrom8BitTo16Bit(SkColorGetB(selection_color)));
-    AppendPangoAttribute(start, end, pango_attr, attrs);
-  }
 
   StyleRanges ranges_of_style(style_ranges());
   ApplyCompositionAndSelectionStyles(&ranges_of_style);
 
   PlatformFont* default_platform_font = default_style().font.platform_font();
 
+  PangoAttribute* pango_attr;
   for (StyleRanges::const_iterator i = ranges_of_style.begin();
        i < ranges_of_style.end(); ++i) {
-    start = std::min(i->range.start(), text().length());
-    end = std::min(i->range.end(), text().length());
+    size_t start = std::min(i->range.start(), text().length());
+    size_t end = std::min(i->range.end(), text().length());
     if (start >= end)
       continue;
 
@@ -657,4 +665,29 @@
   return utf16_index;
 }
 
+void RenderTextLinux::GetSubstringBounds(size_t from,
+                                         size_t to,
+                                         std::vector<Rect>* bounds) {
+  int* ranges;
+  int n_ranges;
+  size_t from_in_utf8 = Utf16IndexToUtf8Index(from);
+  size_t to_in_utf8 = Utf16IndexToUtf8Index(to);
+  pango_layout_line_get_x_ranges(current_line_,

[msw, 2011/11/28 20:01:17]

I believe this argument should be on a new line, indented by 4 spaces like the
rest of the args (indent all by 4 or indent all after the open paren).

[xji, 2011/11/29 01:37:02]

done.
but I think this is the style for function decl and definition, not func call.

+      std::min(from_in_utf8, to_in_utf8),
+      std::max(from_in_utf8, to_in_utf8), &ranges, &n_ranges);

[msw, 2011/11/28 20:01:17]

|&ranges| and |&n_ranges| should be on new lines.

[xji, 2011/11/29 01:37:02]

Done.

+
+  int height;
+  pango_layout_get_pixel_size(layout_, NULL, &height);
+
+  for (int i = 0; i < 2 * n_ranges; ++i) {

[msw, 2011/11/28 20:01:17]

Shouldn't |i| be less than n_ranges? Otherwise we access ranges at |2 * i| when
|i| is already |2 * n_ranges - 1|, which seems out of bounds.

[behdad_google, 2011/11/28 23:17:40]

Agreed.

[xji, 2011/11/29 01:37:02]

Thanks for catching this!

+    int x = PANGO_PIXELS(ranges[2 * i]);
+    int w = PANGO_PIXELS(ranges[2 * i + 1] - ranges[2 * i]);

[Alexei Svitkine (slow), 2011/11/28 19:56:14]

Nit: Rename to 'width'.

[msw, 2011/11/28 20:01:17]

Is ranges guaranteed to include an extra terminating entry?
Otherwise, we can't access ranges at |2 * (n_ranges - 1) + 1|.

Also, can you re-use the the PANGO_PIXELS(ranges[2 * i]) calculated as |x|
above?

[behdad_google, 2011/11/28 23:17:40]

The array is guaranteed to be of length 2*n_ranges, so we're fine.
In fact, "w = PANGO_PIXELS(ranges[2 * i + 1]) - x" is more correct
rounding-wise.

[xji, 2011/11/29 01:37:02]

Done.

[xji, 2011/11/29 01:37:02]

hm... why "w = PANGO_PIXELS(ranges[2 * i + 1]) - x" is more correct
rounding-wise? I thought the other way round.

+    Rect rect(x, 0, w, height);

[behdad_google, 2011/11/28 23:17:40]

You may need "w-1" instead of w, depending on what the definition of w is.  if
the rectangle is one pixel wide, do you expect w=1 or w=0?  

[xji, 2011/11/29 01:37:02]

Are you talking about skia's pixel model, such as we passed pixel width as 0 to
draw a 1-pixel cursor?
similarly, I need to pass 'w-1'? I am not sure here.

+    rect.Offset(0, (display_rect().height() - height) / 2);

[Alexei Svitkine (slow), 2011/11/28 19:56:14]

Nit: Move y = (display_rect().height() - height) / 2); to outside the loop.
Then, you can pass 'y' to the Rect constructor and remove the Offset() call.

[xji, 2011/11/29 01:37:02]

Done.

+    rect.set_origin(ToViewPoint(rect.origin()));
+    bounds->push_back(rect);
+  }
+  g_free(ranges);
+}
+
 }  // namespace gfx