Issue 7892044: Implement move by word, fix rendering of Arabic shape joining

Issue 7892044: Implement move by word, fix rendering of Arabic shape joining (Closed)

Created:
9 years, 3 months ago by xji

Modified:
9 years, 3 months ago

Reviewers:
msw, behdad_google, behdad

CC:
chromium-reviews, Paweł Hajdan Jr., jshin+watch_chromium.org, brettw-cc_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src/

Visibility:
Public.

Description

1. Implement move cursor left/right by word. 2. Fix pango rendering of Arabic joining shapes. In Pango, different font means different runs, and it breaks arabic shaping across run boundaries. Do not reset font attribute if it is the same as default. BUG=90426 TEST=RenderTextTest.MoveLeftRightByWord*. Build chromeos, open it with --use-pure-views, type in تسیک then select the 2nd and 3rd characters, check whether the characters' shapes changes (from joining to not joining). Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102359

Patch Set 1 #

Patch Set 2 : fix lint error #

Total comments: 25

Patch Set 3 : address comment #

Total comments: 5

Patch Set 4 : sync #

Patch Set 5 : sync #

Created: 9 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+263 lines, -10 lines)			Patch
M	ui/gfx/render_text_linux.h	View	1 2 3 4	1 chunk	+5 lines, -0 lines	0 comments	Download
M	ui/gfx/render_text_linux.cc	View	1 2 3 4	9 chunks	+93 lines, -10 lines	0 comments	Download
M	ui/gfx/render_text_unittest.cc	View	1 2 3 4	1 chunk	+165 lines, -0 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

msw

http://codereview.chromium.org/7892044/diff/4002/base/i18n/break_iterator.h File base/i18n/break_iterator.h (right): http://codereview.chromium.org/7892044/diff/4002/base/i18n/break_iterator.h#newcode91 base/i18n/break_iterator.h:91: // Under BREAK_WORD mode, returns true if |position| is ...

9 years, 3 months ago (2011-09-15 01:26:00 UTC) #2

behdad_google

LGTM. On 2011/09/15 01:26:00, msw wrote: > http://codereview.chromium.org/7892044/diff/4002/base/i18n/break_iterator.h > File base/i18n/break_iterator.h (right): > > http://codereview.chromium.org/7892044/diff/4002/base/i18n/break_iterator.h#newcode91 ...

9 years, 3 months ago (2011-09-15 17:14:03 UTC) #3

xji

9 years, 3 months ago (2011-09-15 22:58:09 UTC) #4

http://codereview.chromium.org/7892044/diff/4002/base/i18n/break_iterator.h
File base/i18n/break_iterator.h (right):

http://codereview.chromium.org/7892044/diff/4002/base/i18n/break_iterator.h#n...
base/i18n/break_iterator.h:91: // Under BREAK_WORD mode, returns true if
|position| is at the end of word or
On 2011/09/15 01:26:00, msw wrote:
> Same as your http://codereview.chromium.org/7841056 ...
> ditto for the code file.

before commit, both of them need those 2 files. the latter commit one needs to
do merge.

http://codereview.chromium.org/7892044/diff/4002/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

http://codereview.chromium.org/7892044/diff/4002/ui/gfx/render_text_linux.cc#...
ui/gfx/render_text_linux.cc:188: return LeftSelectionModelByWord(current);
On 2011/09/15 01:26:00, msw wrote:
> DCHECK(break_type == WORD_BREAK); before this to ensure someone doesn't
> introduce a different break type that gets inadvertantly handled as a word
here.
> Same for GetRightSelectionModel below.

Done.

http://codereview.chromium.org/7892044/diff/4002/ui/gfx/render_text_linux.cc#...
ui/gfx/render_text_linux.cc:581: if (font.platform_font() !=
default_platform_font) {
On 2011/09/15 01:26:00, msw wrote:
> I want to make sure this is correct. If you don't AppendPangoAttribute, it
just
> uses the default font right?

Yes.

> (If it uses the previous run's font, then this is
> incorrect.)
> 
> Still, this will break Arabic shaping if the two runs hosting that text have
the
> same non-default font. It may not occur in practice now, but this would
> AppendPangoAttribute and split up the text for each half with a custom font.
> 
You are right.

> I think we'll eventually need to split up StyleRange into components
> (ColorRange, FontRange, etc.) so that we can combine adjacent ranges with the
> same Fonts (to avoid unnecessarily splitting up runs).

You are right. I added a TODO.

http://codereview.chromium.org/7892044/diff/4002/ui/gfx/render_text_linux.cc#...
ui/gfx/render_text_linux.cc:582: // In pango, different fonts means different
runs, and it breaks Arabic
On 2011/09/15 01:26:00, msw wrote:
> Capitalize Pango?, also this comment probably belongs just above and outside
> this conditional block.

Done.

http://codereview.chromium.org/7892044/diff/4002/ui/gfx/render_text_linux.cc#...
ui/gfx/render_text_linux.cc:621: // TODO(xji): keep a vector of runs to avoid
use single-linked list.
On 2011/09/15 01:26:00, msw wrote:
> Nits: "*K*eep", "us*ing*", "singl*y*" and "list*s*"

done except "lists", it is one list.

http://codereview.chromium.org/7892044/diff/4002/ui/gfx/render_text_linux.h
File ui/gfx/render_text_linux.h (right):

http://codereview.chromium.org/7892044/diff/4002/ui/gfx/render_text_linux.h#n...
ui/gfx/render_text_linux.h:78: // word.
On 2011/09/15 01:26:00, msw wrote:
> " word." fits on the line above.

Done.

http://codereview.chromium.org/7892044/diff/4002/ui/gfx/render_text_unittest.cc
File ui/gfx/render_text_unittest.cc (right):

http://codereview.chromium.org/7892044/diff/4002/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:181: #if defined(OS_LINUX)
On 2011/09/15 01:26:00, msw wrote:
> Add a TODO to make these work on Windows.

Done.

http://codereview.chromium.org/7892044/diff/4002/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:182: void MoveLeftRightByWordVerifier(RenderText*
render_text,
On 2011/09/15 01:26:00, msw wrote:
> This function is rather complex (difficult to debug/maintain later). I'd
prefer
> that you followed the expected selectionmodel vector pattern of your
> MoveCursorLeftRight* tests in http://codereview.chromium.org/7841056. But if
you
> disagree (hopefully with support from behdad), then this is okay.

hm... I agree with you that this function is not easy enough, and that is why I
added comments.
But if using SelectionModel vector pattern, we will have a huge expectation
tables.
For example, the last test has 35 characters and 9 words, we will have 9
expectations for move word from word break position to another word break
position. And (35-9) expectations for moving from a non-word break positions to
a word break position. And that is just for moving in one direction. So total 70
expectations for moving bi-direction.
So, I'd prefer to leave it as is.
I think using expectation vector works better for small test, but not if you
want to test different variation combinations.

http://codereview.chromium.org/7892044/diff/4002/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:234: for (int j = 1; j < num_of_character_moves -
1; ++j) {
On 2011/09/15 01:26:00, msw wrote:
> Why |num_of_character_moves - 1| here but not - 1 in the left-tor-right case
> above? Comment?

Ah, should not "-1", forgot to change this part.

http://codereview.chromium.org/7892044/diff/4002/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:279: TEST_F(RenderTextTest,
MoveLeftRightByWordInBidiText_TestEndOfText) {
On 2011/09/15 01:26:00, msw wrote:
> Can you add comments as to what each of these blocks tests? It'd be nice to
see
> visual/logical "abC" comments with a '|' cursor to show what's happening.

Done.

I tried to do move by word the other way, from a position find the word break,
if the break is inside the run, just return, if it is outside the run, then,
look for work break in next/prev run. But still I am not able to make this part
correct. Even with find word break run by run, in a specific run, I need to
check whether the run boundary itself is a word break position, which will treat
text end as word break and have the same error.

http://codereview.chromium.org/7892044/diff/4002/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:286: //
EXPECT_TRUE(render_text->selection_model().Equals(SelectionModel(0)));
On 2011/09/15 01:26:00, msw wrote:
> Why isn't this working now? Can you add "abC" comments with a '|' cursor to
show
> what's happening and what's expected?

Done.

http://codereview.chromium.org/7892044/diff/4002/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:302: //
EXPECT_TRUE(render_text->selection_model().Equals(SelectionModel(0)));
On 2011/09/15 01:26:00, msw wrote:
> Same here, I don't get what's broken.

explained.

msw

9 years, 3 months ago (2011-09-17 00:27:10 UTC) #5

Just grammatical nits; LGTM.

http://codereview.chromium.org/7892044/diff/4002/ui/gfx/render_text_linux.cc
File ui/gfx/render_text_linux.cc (right):

http://codereview.chromium.org/7892044/diff/4002/ui/gfx/render_text_linux.cc#...
ui/gfx/render_text_linux.cc:621: // TODO(xji): keep a vector of runs to avoid
use single-linked list.
On 2011/09/15 22:58:09, xji wrote:
> On 2011/09/15 01:26:00, msw wrote:
> > Nits: "*K*eep", "us*ing*", "singl*y*" and "list*s*"
> 
> done except "lists", it is one list.

Technically that's grammatically incorrect, you'll need a preposition with a
singular noun in that case:
"... to avoid using *a* singly-linked list."

http://codereview.chromium.org/7892044/diff/9001/ui/gfx/render_text_unittest.cc
File ui/gfx/render_text_unittest.cc (right):

http://codereview.chromium.org/7892044/diff/9001/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:284: // Moving cursor by word from "abC|" to left
should return "|abC".
"Moving *the* cursor"... "to *the* left"
Same corrections below.

http://codereview.chromium.org/7892044/diff/9001/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:292: // Moving cursor by word from "|abC" to
right returns "abC|".
"Moving *the* cursor"... "to *the* right"

http://codereview.chromium.org/7892044/diff/9001/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:299: // For logical text "BCa", moving cursor by
word from "aCB|" to left returns
"moving *the* cursor"... "to *the* left"

http://codereview.chromium.org/7892044/diff/9001/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:306: // Moving cursor by word from "|aCB" to
right should return "returns "aCB|".
You have a stray bit of text ("returns)...
"*the* right"

http://codereview.chromium.org/7892044/diff/9001/ui/gfx/render_text_unittest....
ui/gfx/render_text_unittest.cc:307: // But since end of text is always treated
as a word breaker, it returns
"word break" I don't think 'breaker' is correct.

Expand Messages | Collapse Messages