Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8725002:
Draw text via Skia in RenderTextLinux. (Closed)
|
Created:
9 years ago by Alexei Svitkine (slow) Modified:
9 years ago CC:
chromium-reviews Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionDraw text via Skia in RenderTextLinux.
Refactors some common Skia drawing code into RenderText internal
class SkiaTextRenderer. Rewrite RenderTextLinux::DrawVisualText()
to use SkiaTextRenderer and refactor RenderTextWin::DrawVisualText()
to also use SkiaTextRenderer, to re-use code.
BUG=103648
TEST=Run Linux views_examples_exe --use-pure-views. In the textfield
example tab, the text field should draw its text correctly.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113706
Patch Set 1 : '' #
Total comments: 1
Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 30
Patch Set 4 : '' #
Total comments: 1
Patch Set 5 : '' #
Total comments: 10
Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 12
Patch Set 8 : '' #
Total comments: 1
Patch Set 9 : '' #
Messages
Total messages: 38 (0 generated)
Oops, CL title was wrong. Changed it now to "Draw text via Skia in RenderTextLinux.". On Thu, Dec 1, 2011 at 3:21 PM, <asvitkine@chromium.org> wrote: > Reviewers: msw, xji, > > Description: > Draw text via Skia in RenderTextLinux. > > Refactors some common Skia drawing code into RenderText internal > class SkiaTextRenderer. Rewrite RenderTextLinux::DrawVisualText() > to use SkiaTextRenderer and refactor RenderTextWin::DrawVisualText() > to also use SkiaTextRenderer, to re-use code. > > BUG=103648 > TEST=Run Linux views_examples_exe --use-pure-views. In the textfield > example tab, the text field should draw its text correctly. > > Please review this at http://codereview.chromium.org/8725002/ > > SVN Base: http://src.chromium.org/svn/trunk/src/ > > Affected files: > M ui/gfx/render_text.h > M ui/gfx/render_text.cc > M ui/gfx/render_text_linux.cc > M ui/gfx/render_text_win.cc > >
http://codereview.chromium.org/8725002/diff/15001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8725002/diff/15001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:315: if (run_start + i >= style->range.end()) { There was an error here where the logic was using the glyph index rather than the character index when comparing against the style range. I've fixed this in the new patch.
Ping.
Rubber stamp LGTM from me. Make sure you wake for msw and xji to approve though.
sorry for the delay, i'm part way through, but trying to get something in for M17. On Mon, Dec 5, 2011 at 1:44 PM, <sky@chromium.org> wrote: > Rubber stamp LGTM from me. Make sure you wake for msw and xji to approve > though. > > http://codereview.chromium.**org/8725002/<http://codereview.chromium.org/8725... >
Thanks for the awesome work! And sorry for the delayed review. http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:120: size_t byte_length = glyph_count * sizeof(glyphs[0]); Will uint16 cause problem? PangoGlyph is a guint32. http://developer.gnome.org/pango/stable/pango-Glyph-Storage.html#PangoGlyph http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:276: y += default_style().font.GetFontSize(); The x/y initialization is the same for Win and Linux. Maybe they could be done in base? http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:283: you need to call ApplyCompositionAndSelectionStyle() before the loop so that composition text is drawn with underline and selection's foregound color is drawn correctly. I think it should be the same for Windows, but seems ApplyCompositionAndSelectionStyle() is not called in Windows, and the drawing part is different in windows (seems that each style breaks text in different run in windows). http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:308: glyphs[i] = static_cast<uint16>(glyph.glyph); will cast from uint32 to uint16 cause problem? http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:313: // If this glyph is the last for the current style, draw the glyphs so far this glyph is *beyond* the current style. http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:316: if (run_start + glyph_char_index >= style->range.end()) { glyph_char_index is *byte* index relative to run start. need rename it to glyph_byte_index, and do Utf8IndexToUtf16Index conversion. It should be: if (Utf8IndexToUtf16Index(run->item->offset + glyph_byte_index) >= style->range.end()) http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:338: x = glyph_x; Nice mechanism on drawing with styles. But seems that it works for LTR run, but not on a RTL run that has 2 different styles. For a RTL run whose logical text is ABC, it display as CBA, first glyph is 'C', assume run_start is 0, at line 316, Utf8IndexToUtf16Index(run->item->offset + glyph_byte_index) is calculating the char index using visual order, so the first glyph's index is 2. But style->range.end() is in logical order. For RTL run, maybe you need find the style using run's end (vs. line 291), and change the style when glyph's index < style_range.start() (vs. 316). But I do not know how to test it out (what RTL run can have 2 styles -- foregound, strike, underline)?
http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:94: paint_.setTextEncoding(SkPaint::kGlyphID_TextEncoding); Why not roll this into the ctor? http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:107: // |paint_| adds its own ref. Release the ref from |CreateFromName()|. Then shouldn't |typeface| be a scoped refptr or something? http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:118: const uint16* glyphs, Indent these args. http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.h#newcode15 ui/gfx/render_text.h:15: #include "third_party/skia/include/core/SkCanvas.h" Can you forward declare SkCanvas instead? http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.h#newcode17 ui/gfx/render_text.h:17: #include "third_party/skia/include/core/SkPoint.h" Can you forward declare SkPoint instead? http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.h#newcode32 ui/gfx/render_text.h:32: class SkiaTextRenderer { Does this belong in its own file? That might help split rendering from layout/shaping/metrics. http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:291: while (next < style_ranges().end() && next->range.start() <= run_start) This seems pretty inefficient, can we do better than O(N^2)? StylesRanges should be in logical order with the text. Is the loop that contains this code in logical or visual order? Maybe just add a TODO as a heads up for a low-hanging perf opportunity.
I've addressed some of the comments so far, but not all - still working through them all. Here's the initial set of responses/fixes - more to come. http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:94: paint_.setTextEncoding(SkPaint::kGlyphID_TextEncoding); On 2011/12/06 18:40:56, msw wrote: > Why not roll this into the ctor? I guess I followed our style that we shouldn't do too much work in the constructor. Though, since this is just setting properties, I see your point that it's not really doing serious work, so done. http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:107: // |paint_| adds its own ref. Release the ref from |CreateFromName()|. On 2011/12/06 18:40:56, msw wrote: > Then shouldn't |typeface| be a scoped refptr or something? The Chromium scoped_refptr won't work, since its a Skia ref-counted object. I've made it use SkAutoTUnref() instead. I'm not sure its much better, since it still needs the comment to explain why it uses .get() rather than .release(). http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:118: const uint16* glyphs, On 2011/12/06 18:40:56, msw wrote: > Indent these args. Done. http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:120: size_t byte_length = glyph_count * sizeof(glyphs[0]); On 2011/12/05 23:47:09, xji wrote: > Will uint16 cause problem? PangoGlyph is a guint32. > > http://developer.gnome.org/pango/stable/pango-Glyph-Storage.html#PangoGlyph It's a good question. |drawPosText()| seems to only take uint16 indices - I'll have to ask Mike Reed about this. http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.h#newcode15 ui/gfx/render_text.h:15: #include "third_party/skia/include/core/SkCanvas.h" On 2011/12/06 18:40:56, msw wrote: > Can you forward declare SkCanvas instead? Done. http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.h#newcode17 ui/gfx/render_text.h:17: #include "third_party/skia/include/core/SkPoint.h" On 2011/12/06 18:40:56, msw wrote: > Can you forward declare SkPoint instead? Done. http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.h#newcode32 ui/gfx/render_text.h:32: class SkiaTextRenderer { On 2011/12/06 18:40:56, msw wrote: > Does this belong in its own file? That might help split rendering from > layout/shaping/metrics. At the moment its not really a very general class - merely something that facilitates drawing by RenderText subclasses. This is why I didn't want to make it top level - it's not really meant for other classes. Although I was thinking it might make sense to have a |render_text| directory under ui/gfx to hold the different RenderText* classes. If that were the case, I'd be more inclined to have this in its own file, in that directory - so that it's still clear that it's not for general consumption. http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:291: while (next < style_ranges().end() && next->range.start() <= run_start) On 2011/12/06 18:40:56, msw wrote: > This seems pretty inefficient, can we do better than O(N^2)? > StylesRanges should be in logical order with the text. > Is the loop that contains this code in logical or visual order? > Maybe just add a TODO as a heads up for a low-hanging perf opportunity. I agree this is bad, I've added a TODO for now. http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:308: glyphs[i] = static_cast<uint16>(glyph.glyph); On 2011/12/05 23:47:09, xji wrote: > will cast from uint32 to uint16 cause problem? See my other comment. http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:313: // If this glyph is the last for the current style, draw the glyphs so far On 2011/12/05 23:47:09, xji wrote: > this glyph is *beyond* the current style. You're right, I've fixed the comment. http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:316: if (run_start + glyph_char_index >= style->range.end()) { On 2011/12/05 23:47:09, xji wrote: > glyph_char_index is *byte* index relative to run start. > need rename it to glyph_byte_index, and > do Utf8IndexToUtf16Index conversion. It should be: > if (Utf8IndexToUtf16Index(run->item->offset + glyph_byte_index) >= > style->range.end()) > Good catch! Rather than doing the conversion for every glyph, I've now made the code pre-calculate the style ranges in terms of UTF8 indices, and am using these now. Ideally, we could cache these precalculated ranges to avoid the extra work in Draw()
So, this generally works. However, it would be much much easier, cleaner, and more robust, to subclass PangoRenderer instead. That unfortunately wouldn't fit in with the refactored model introduced in this CL. Take a look at it and let me know if you need help. http://codereview.chromium.org/8725002/diff/25001/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8725002/diff/25001/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:312: renderer.SetFont(gfx::Font(native_font)); Do we know how to create a Skia font from a FT_Face, or font blob? If yes, it would be much better to use pango_fc_font_lock_face() kind of API. The current approach is extremely wasteful and slow.
(+Mike Reed) There were a couple questions in the review raised about some Skia APIs. Can you help answer them? Specifically: 1. Skia's |drawPosText()| takes uint16 glyph indices, but Pango uses 32-bit ones. Is there a way to use drawPosText() with pango indices that won't cause problems? (See xji@'s questions.) 2. Is there a cheaper way to get a SkTypeface from a pango font? (See behdad@'s question.) Thanks.
On 2011/12/06 19:31:57, Alexei Svitkine wrote: > (+Mike Reed) > > There were a couple questions in the review raised about some Skia APIs. Can you help answer them? > > Specifically: > > 1. Skia's |drawPosText()| takes uint16 glyph indices, but Pango uses 32-bit > ones. Is there a way to use drawPosText() with pango indices that won't cause > problems? (See xji@'s questions.) It's safe to do that. Note though, there are a few special glyph codes Pango uses that are NOT real glyph indices in the font: #define PANGO_GLYPH_EMPTY ((PangoGlyph)0x0FFFFFFF) #define PANGO_GLYPH_INVALID_INPUT ((PangoGlyph)0xFFFFFFFF) #define PANGO_GLYPH_UNKNOWN_FLAG ((PangoGlyph)0x10000000) #define PANGO_GET_UNKNOWN_GLYPH(wc) ((PangoGlyph)(wc)|PANGO_GLYPH_UNKNOWN_FLAG) So, PANGO_GLYPH_EMPTY codes shall be skipped. PANGO_GLYPH_INVALID_INPUT, means non-UTF-8 input. Pango draws a crossed box for it. Anything that &'s with PANGO_GLYPH_UNKNOWN_FLAG is a character the font doesn't handle. Pango draws a box with hexcodes in it. > 2. Is there a cheaper way to get a SkTypeface from a pango font? (See behdad@'s > question.) > > Thanks.
On Tue, Dec 6, 2011 at 2:23 PM, <behdad@google.com> wrote: > So, this generally works. However, it would be much much easier, cleaner, > and more robust, to subclass PangoRenderer instead. That unfortunately > wouldn't fit in with the refactored model introduced in this CL. Take a look > at it and let me know if you need help. Hmm. I see both upsides and downsides from doing it this way. The downsides I see: - No code sharing between Linux / Win implementations. - Potentially inconsistent output between Linux / Win implementations. - Need to implement a lot of PangoRenderer functions, such as: draw_trapezoid, draw_shape, etc. Doesn't seem like it would be "easier". Plus, essentially scrapping the work in this CL. - Some uglyness on the code side, i.e. having to expose functions via C struct with function pointers. The benefits I see: - Potentially more faithful to how Pango renders things, but inconsistent with Windows. - Easy to switch between Skia-backed and regular Pango path. Possibility of using the Skia path only for the GPU (Ganesh) case. At the moment, I'm leaning towards not going with the PangoRenderer approach - mainly because it seems like a lot of PangoRenderer functions would need to be implemented for this to work, and I'm not convinced the benefits are worth it, given the downsides. I'm curious to hear what others think, though.
http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:276: y += default_style().font.GetFontSize(); On 2011/12/05 23:47:09, xji wrote: > The x/y initialization is the same for Win and Linux. Maybe they could be done > in base? I've added a helper function for this. http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:283: On 2011/12/05 23:47:09, xji wrote: > you need to call ApplyCompositionAndSelectionStyle() before the loop so that > composition text is drawn with underline and selection's foregound color is > drawn correctly. > > I think it should be the same for Windows, but seems > ApplyCompositionAndSelectionStyle() is not called in Windows, and the drawing > part is different in windows (seems that each style breaks text in different run > in windows). Done. On the Windows side, this needed to be done in RenderTextWin::ItemizeLogicalText(), since runs/style ranges are merged at that point. I've made that change, removing a TODO from there. http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:338: x = glyph_x; On 2011/12/05 23:47:09, xji wrote: > Nice mechanism on drawing with styles. > But seems that it works for LTR run, but not on a RTL run that has 2 different > styles. > > For a RTL run whose logical text is ABC, it display as CBA, > first glyph is 'C', assume run_start is 0, > at line 316, Utf8IndexToUtf16Index(run->item->offset + glyph_byte_index) is > calculating the char index using visual order, so the first glyph's index is 2. > But style->range.end() is in logical order. > For RTL run, maybe you need find the style using run's end (vs. line 291), and > change the style when glyph's index < style_range.start() (vs. 316). But I do > not know how to test it out (what RTL run can have 2 styles -- foregound, > strike, underline)? Good catch. I've updated the code to hopefully handle RTL correctly per your comment - please take a look. I haven't yet tested it, though - I will try to do that tomorrow.
http://codereview.chromium.org/8725002/diff/27006/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/8725002/diff/27006/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:640: // Offset by the font size to account for Skia expecting y to be the bottom. The offset y is based on Skia draw. Maybe need to change the name to be more specific to Skia related? http://codereview.chromium.org/8725002/diff/27006/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8725002/diff/27006/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:278: // Pre-calculate UTF16 indeces style ranges to UTF16 indices. to *UTF8* indices. http://codereview.chromium.org/8725002/diff/27006/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:289: bool is_rtl = (GetTextDirection() == base::i18n::RIGHT_TO_LEFT); this should be run's directionality, not text's. http://codereview.chromium.org/8725002/diff/27006/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:301: if (is_rtl) { sorry for the confusion. I mean the style initialization should be based on a *run*'s directionality, not the whole text's directionality. you probably do not need to use the whole text's 'is_rtl' to guide searching from end to start or start to end. It probably does not have performance gain, and the code looks more complicated. http://codereview.chromium.org/8725002/diff/27006/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:337: if (!style_ranges_utf8[style].Contains(ui::Range(glyph_byte_index))) { isn't style_range's end exclusive? Then Contains() returns true if glyph_byte_index == style_range_utf8[style].end, which is not correct for this purpose.
RenderTextWin and base classes LGTM. I'll leave RenderTextLinux to others. http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:107: // |paint_| adds its own ref. Release the ref from |CreateFromName()|. On 2011/12/06 19:10:12, Alexei Svitkine wrote: > On 2011/12/06 18:40:56, msw wrote: > > Then shouldn't |typeface| be a scoped refptr or something? > > The Chromium scoped_refptr won't work, since its a Skia ref-counted object. I've > made it use SkAutoTUnref() instead. I'm not sure its much better, since it still > needs the comment to explain why it uses .get() rather than .release(). Thanks for enlightening me! http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.h File ui/gfx/render_text.h (right): http://codereview.chromium.org/8725002/diff/20002/ui/gfx/render_text.h#newcode32 ui/gfx/render_text.h:32: class SkiaTextRenderer { On 2011/12/06 19:10:12, Alexei Svitkine wrote: > On 2011/12/06 18:40:56, msw wrote: > > Does this belong in its own file? That might help split rendering from > > layout/shaping/metrics. > > At the moment its not really a very general class - merely something that > facilitates drawing by RenderText subclasses. This is why I didn't want to make > it top level - it's not really meant for other classes. > > Although I was thinking it might make sense to have a |render_text| directory > under ui/gfx to hold the different RenderText* classes. If that were the case, > I'd be more inclined to have this in its own file, in that directory - so that > it's still clear that it's not for general consumption. Fair enough, this is fine for now. A snazzy subdir for this code might eventually be warranted.
http://codereview.chromium.org/8725002/diff/27006/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/8725002/diff/27006/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:640: // Offset by the font size to account for Skia expecting y to be the bottom. On 2011/12/07 00:56:51, xji wrote: > The offset y is based on Skia draw. Maybe need to change the name to be more > specific to Skia related? Done. http://codereview.chromium.org/8725002/diff/27006/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8725002/diff/27006/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:278: // Pre-calculate UTF16 indeces style ranges to UTF16 indices. On 2011/12/07 00:56:51, xji wrote: > to *UTF8* indices. Done. http://codereview.chromium.org/8725002/diff/27006/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:289: bool is_rtl = (GetTextDirection() == base::i18n::RIGHT_TO_LEFT); On 2011/12/07 00:56:51, xji wrote: > this should be run's directionality, not text's. Done. I've also added a helper function IsRunLTR() to avoid repeating the obscure way to get this from Pango. http://codereview.chromium.org/8725002/diff/27006/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:301: if (is_rtl) { On 2011/12/07 00:56:51, xji wrote: > you probably do not need to use the whole text's 'is_rtl' to guide searching > from end to start or start to end. It probably does not have performance gain, > and the code looks more complicated. That version of the code was structured in a way that needed the two loops to be correct. You're right that it's too complex for dubious gain, so I've rewritten in a way that works for both cases. http://codereview.chromium.org/8725002/diff/27006/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:337: if (!style_ranges_utf8[style].Contains(ui::Range(glyph_byte_index))) { On 2011/12/07 00:56:51, xji wrote: > isn't style_range's end exclusive? Then Contains() returns true if > glyph_byte_index == style_range_utf8[style].end, which is not correct for this > purpose. You're right! Fixed.
I've tested the RTL side now (by hardcoding some RTL text and style ranges), and it seems to work correctly.
http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:171: if (IsRunLTR(item)) { thanks for making those changes. http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:315: if (IndexInRange(style_ranges_utf8[i], first_glyph_byte_index)) { Ah, sorry that I just realized does this (using glyph index to search for style, such as foreground) work for 'fi' ligature? Need to check with Behdad. The questions are: 'fi' ligature is one glyph, right? If it is, then, can it have different foreground for 'f' and 'i'? If it can, then, above logic is not correct for 'fi' ligature, then, what should be the alternatives? http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:320: DCHECK_GE(style, 0); nit: do you want to dcheck "style < style_ranges.utf8.size()" too? http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:344: if (!IndexInRange(style_ranges_utf8[style], glyph_byte_index)) { same question for whether glyph's style set works for 'fi' ligature.
http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:315: if (IndexInRange(style_ranges_utf8[i], first_glyph_byte_index)) { On 2011/12/07 19:52:26, xji wrote: > Ah, sorry that I just realized does this (using glyph index to search for style, > such as foreground) work for 'fi' ligature? Need to check with Behdad. > > The questions are: > 'fi' ligature is one glyph, right? If it is, then, can it have different > foreground for 'f' and 'i'? If it can, then, above logic is not correct for 'fi' > ligature, then, what should be the alternatives? I am not sure how its possible to render a single glyph using multiple styles. In the case of "fi", you might argue that you can split it with a vertical line and everything on the left of it would be one style and the right would be another. But I don't think you can say that in general - in other cases the combining glyph will have overlaps between the different parts. So as long as we choose one of the two styles in such a case, whatever we deem to be the right one - i.e. whatever is more intuitive, I think should be okay? Or is my mental model of this flawed? http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:320: DCHECK_GE(style, 0); On 2011/12/07 19:52:26, xji wrote: > nit: do you want to dcheck "style < style_ranges.utf8.size()" too? It's not possible given the above loop.
http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:315: if (IndexInRange(style_ranges_utf8[i], first_glyph_byte_index)) { On 2011/12/07 19:59:25, Alexei Svitkine wrote: > On 2011/12/07 19:52:26, xji wrote: > > Ah, sorry that I just realized does this (using glyph index to search for > style, > > such as foreground) work for 'fi' ligature? Need to check with Behdad. > > > > The questions are: > > 'fi' ligature is one glyph, right? If it is, then, can it have different > > foreground for 'f' and 'i'? If it can, then, above logic is not correct for > 'fi' > > ligature, then, what should be the alternatives? > > I am not sure how its possible to render a single glyph using multiple styles. > In the case of "fi", you might argue that you can split it with a vertical line > and everything on the left of it would be one style and the right would be > another. But I don't think you can say that in general - in other cases the > combining glyph will have overlaps between the different parts. So as long as we > choose one of the two styles in such a case, whatever we deem to be the right > one - i.e. whatever is more intuitive, I think should be okay? Or is my mental > model of this flawed? > I am not sure about this part. I remember that you can highlight part of 'fi' ligature glyph. then, can you draw part of it using different foreground color? For example, if the 'fi' ligature is green, when, select part of it, the foreground changed to kSelectedTextColor, then, are we able to draw the foreground correctly? It might be a rare case or not practical (say we never use different color to draw text) that we could address it later. But I'd like to check with Behdad. I just pinged him again. And he will look at it now. http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:320: DCHECK_GE(style, 0); On 2011/12/07 19:59:25, Alexei Svitkine wrote: > On 2011/12/07 19:52:26, xji wrote: > > nit: do you want to dcheck "style < style_ranges.utf8.size()" too? > > It's not possible given the above loop. Ah, you right.
http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:315: if (IndexInRange(style_ranges_utf8[i], first_glyph_byte_index)) { Yeah, this is broken. That's why I suggested that you highlight using Skia. But get the bounds from Pango. Pango knows how to handle the size. Well, it just linearly divides the cluster width by the number of cursor positions in the cluster.
On Wed, Dec 7, 2011 at 6:35 PM, <behdad@google.com> wrote: > > http://codereview.chromium.**org/8725002/diff/33005/ui/gfx/** > render_text_linux.cc<http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc> > File ui/gfx/render_text_linux.cc (right): > > http://codereview.chromium.**org/8725002/diff/33005/ui/gfx/** > render_text_linux.cc#**newcode315<http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc#newcode315> > ui/gfx/render_text_linux.cc:**315: if (IndexInRange(style_ranges_** > utf8[i], > first_glyph_byte_index)) { > Yeah, this is broken. That's why I suggested that you highlight using > Skia. But get the bounds from Pango. Pango knows how to handle the > size. Well, it just linearly divides the cluster width by the number of > cursor positions in the cluster. > I see. This is probably something we should do on Windows too. This is probably more broken there since the Windows side now splits runs according to styles, so "fi" would in that case span multiple runs and probably be very broken... Are you okay with adding a TODO about this for now?
On 2011/12/07 23:47:13, Alexei Svitkine wrote: > On Wed, Dec 7, 2011 at 6:35 PM, <mailto:behdad@google.com> wrote: > > > > > http://codereview.chromium.**org/8725002/diff/33005/ui/gfx/** > > > render_text_linux.cc<http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc> > > File ui/gfx/render_text_linux.cc (right): > > > > http://codereview.chromium.**org/8725002/diff/33005/ui/gfx/** > > > render_text_linux.cc#**newcode315<http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc#newcode315> > > ui/gfx/render_text_linux.cc:**315: if (IndexInRange(style_ranges_** > > utf8[i], > > first_glyph_byte_index)) { > > Yeah, this is broken. That's why I suggested that you highlight using > > Skia. But get the bounds from Pango. Pango knows how to handle the > > size. Well, it just linearly divides the cluster width by the number of > > cursor positions in the cluster. > > > > I see. This is probably something we should do on Windows too. This is > probably more broken there since the Windows side now splits runs according > to styles, so "fi" would in that case span multiple runs and probably be > very broken... > > Are you okay with adding a TODO about this for now? FYI: There's already a TODO for this on RenderTextWin's TextRun. http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/ui/gfx/render_text_wi...
http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:315: if (IndexInRange(style_ranges_utf8[i], first_glyph_byte_index)) { On 2011/12/07 23:35:05, behdad_google wrote: > Yeah, this is broken. That's why I suggested that you highlight using Skia. > But get the bounds from Pango. Pango knows how to handle the size. Well, it > just linearly divides the cluster width by the number of cursor positions in the > cluster. Does pango handle using 2 different foregrounds (not background) to draw 'fi' ligature? How? by "linearly divides", do you mean "evenly" divides? Skia draws glyph by glyph, if you look at loop starts at line 334, even if you can adjust pos[i] (at line 337) and glyph_x (at line 339) to count part of the 'fi' glyph, how can you pass half of the glyph to glyphs[i] (at line 336)? http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:315: if (IndexInRange(style_ranges_utf8[i], first_glyph_byte_index)) { On 2011/12/07 19:59:25, Alexei Svitkine wrote: > On 2011/12/07 19:52:26, xji wrote: > > Ah, sorry that I just realized does this (using glyph index to search for > style, > > such as foreground) work for 'fi' ligature? Need to check with Behdad. > > > > The questions are: > > 'fi' ligature is one glyph, right? If it is, then, can it have different > > foreground for 'f' and 'i'? If it can, then, above logic is not correct for > 'fi' > > ligature, then, what should be the alternatives? > > I am not sure how its possible to render a single glyph using multiple styles. > In the case of "fi", you might argue that you can split it with a vertical line > and everything on the left of it would be one style and the right would be > another. But I don't think you can say that in general - in other cases the > combining glyph will have overlaps between the different parts. So as long as we > choose one of the two styles in such a case, whatever we deem to be the right > one - i.e. whatever is more intuitive, I think should be okay? Or is my mental > model of this flawed? > 'fi' ligature is different. it is one glyph, but 2 graphemes , logically you can select part of it. http://en.wikipedia.org/wiki/Typographic_ligature Behdad, correct me if I am wrong.
On 2011/12/07 23:47:13, Alexei Svitkine wrote: > On Wed, Dec 7, 2011 at 6:35 PM, <mailto:behdad@google.com> wrote: > > > > > http://codereview.chromium.**org/8725002/diff/33005/ui/gfx/** > > > render_text_linux.cc<http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc> > > File ui/gfx/render_text_linux.cc (right): > > > > http://codereview.chromium.**org/8725002/diff/33005/ui/gfx/** > > > render_text_linux.cc#**newcode315<http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc#newcode315> > > ui/gfx/render_text_linux.cc:**315: if (IndexInRange(style_ranges_** > > utf8[i], > > first_glyph_byte_index)) { > > Yeah, this is broken. That's why I suggested that you highlight using > > Skia. But get the bounds from Pango. Pango knows how to handle the > > size. Well, it just linearly divides the cluster width by the number of > > cursor positions in the cluster. > > > > I see. This is probably something we should do on Windows too. This is > probably more broken there since the Windows side now splits runs according > to styles, so "fi" would in that case span multiple runs and probably be > very broken... > > Are you okay with adding a TODO about this for now? I am ok with adding TODO for now.
> > Skia draws glyph by glyph, if you look at loop starts at line 334, even > if you can adjust pos[i] (at line 337) and glyph_x (at line 339) to > count part of the 'fi' glyph, how can you pass half of the glyph to > glyphs[i] (at line 336)? We can play tricks by specifying a clip on the Skia context and drawing the glyph twice - each clipped to a different half of the glyph. It won't be pretty, but it is possible to do. -Alexei
> > It won't be pretty, but it is possible to do. > To be clear, I meant "the code for this won't be pretty".
On 2011/12/08 07:12:12, Alexei Svitkine wrote: > > > > Skia draws glyph by glyph, if you look at loop starts at line 334, even > > if you can adjust pos[i] (at line 337) and glyph_x (at line 339) to > > count part of the 'fi' glyph, how can you pass half of the glyph to > > glyphs[i] (at line 336)? > > > We can play tricks by specifying a clip on the Skia context and drawing the > glyph twice - each clipped to a different half of the glyph. It won't be > pretty, but it is possible to do. > > -Alexei make sense. pango might be doing the same thing, just like how it draws 'fi' highlight.
LGTM with a TODO and bug for drawing split glyph. You might want to check with Behdad too. Just a reminder to test bidi text against RTL UI (such as --lang=ar), ignore it if you have done so.
I've updated the patch with the TODO and added a loop to iterate over the styles in case a glyph spans more than one style, per our discussion. The loop is needed, else the styles would get out of sync with the glyphs in the "fi" case.
http://codereview.chromium.org/8725002/diff/46001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): http://codereview.chromium.org/8725002/diff/46001/ui/gfx/render_text.cc#newco... ui/gfx/render_text.cc:91: //paint_.setLCDRenderText(true); Did you mean to comment this out?
On 2011/12/08 19:40:26, msw wrote: > http://codereview.chromium.org/8725002/diff/46001/ui/gfx/render_text.cc > File ui/gfx/render_text.cc (right): > > http://codereview.chromium.org/8725002/diff/46001/ui/gfx/render_text.cc#newco... > ui/gfx/render_text.cc:91: //paint_.setLCDRenderText(true); > Did you mean to comment this out? No, that was something else I was playing with - it's not supposed to be in the CL. Good catch! Removed.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asvitkine@chromium.org/8725002/51001
Change committed as 113706
http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc File ui/gfx/render_text_linux.cc (right): http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc... ui/gfx/render_text_linux.cc:315: if (IndexInRange(style_ranges_utf8[i], first_glyph_byte_index)) { On 2011/12/08 00:27:56, xji wrote: > On 2011/12/07 23:35:05, behdad_google wrote: > > Yeah, this is broken. That's why I suggested that you highlight using Skia. > > But get the bounds from Pango. Pango knows how to handle the size. Well, it > > just linearly divides the cluster width by the number of cursor positions in > the > > cluster. > > Does pango handle using 2 different foregrounds (not background) to draw 'fi' > ligature? How? No. Pango doesn't by itself. But if you try selection in gedit it works. That's because gtk implements selection by clipping through cairo. > by "linearly divides", do you mean "evenly" divides? Yes, right. > Skia draws glyph by glyph, if you look at loop starts at line 334, even if you > can adjust pos[i] (at line 337) and glyph_x (at line 339) to count part of the > 'fi' glyph, how can you pass half of the glyph to glyphs[i] (at line 336)? The idea is that you draw the entire paragraph. Then clip to selection region only, and draw with different color. So, you are drawing twice.
Thanks for the explanation. On 2011/12/14 06:29:11, behdad_google wrote: > http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc > File ui/gfx/render_text_linux.cc (right): > > http://codereview.chromium.org/8725002/diff/33005/ui/gfx/render_text_linux.cc... > ui/gfx/render_text_linux.cc:315: if (IndexInRange(style_ranges_utf8[i], > first_glyph_byte_index)) { > On 2011/12/08 00:27:56, xji wrote: > > On 2011/12/07 23:35:05, behdad_google wrote: > > > Yeah, this is broken. That's why I suggested that you highlight using Skia. > > > > But get the bounds from Pango. Pango knows how to handle the size. Well, > it > > > just linearly divides the cluster width by the number of cursor positions in > > the > > > cluster. > > > > Does pango handle using 2 different foregrounds (not background) to draw 'fi' > > ligature? How? > > No. Pango doesn't by itself. But if you try selection in gedit it works. > That's because gtk implements selection by clipping through cairo. > > > by "linearly divides", do you mean "evenly" divides? > > Yes, right. > > > Skia draws glyph by glyph, if you look at loop starts at line 334, even if you > > can adjust pos[i] (at line 337) and glyph_x (at line 339) to count part of the > > 'fi' glyph, how can you pass half of the glyph to glyphs[i] (at line 336)? > > The idea is that you draw the entire paragraph. Then clip to selection region > only, and draw with different color. So, you are drawing twice. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
