Implement move by word, fix rendering of Arabic shape joining

ui/gfx/render_text_unittest.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/gfx/render_text.h"
 
 #include "base/memory/scoped_ptr.h"
 #include "base/utf_string_conversions.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ skipping 160 matching lines @@
   EXPECT_EQ(ui::Range(0, 2), render_text->style_ranges()[0].range);
   EXPECT_FALSE(render_text->style_ranges()[0].strike);
 
   // Test that previously removed ranges don't return.
   render_text->SetText(ASCIIToUTF16("abcdef"));
   EXPECT_EQ(1U, render_text->style_ranges().size());
   EXPECT_EQ(ui::Range(0, 6), render_text->style_ranges()[0].range);
   EXPECT_FALSE(render_text->style_ranges()[0].strike);
 }
 
+#if defined(OS_LINUX)

[msw, 2011/09/15 01:26:00]

Add a TODO to make these work on Windows.

[xji, 2011/09/15 22:58:09]

Done.

+void MoveLeftRightByWordVerifier(RenderText* render_text,

[msw, 2011/09/15 01:26:00]

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.

[xji, 2011/09/15 22:58:09]

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.

+                                 const wchar_t* str) {
+  render_text->SetText(WideToUTF16(str));
+
+  // Test moving by word from left ro right.
+  render_text->MoveCursorLeft(LINE_BREAK, false);
+  bool first_word = true;
+  while (true) {
+    // First, test moving by word from a word break position, such as from
+    // "|abc def" to "abc| def".
+    SelectionModel start = render_text->selection_model();
+    render_text->MoveCursorRight(WORD_BREAK, false);
+    SelectionModel end = render_text->selection_model();
+    if (end.Equals(start))  // reach the end.
+      break;
+
+    // For testing simplicity, each word is a 3-character word.
+    int num_of_character_moves = first_word ? 3 : 4;
+    first_word = false;
+    render_text->MoveCursorTo(start);
+    for (int j = 0; j < num_of_character_moves; ++j)
+      render_text->MoveCursorRight(CHARACTER_BREAK, false);
+    EXPECT_TRUE(render_text->selection_model().Equals(end));
+
+    // Then, test moving by word from positions inside the word, such as from
+    // "a|bc def" to "abc| def", and from "ab|c def" to "abc| def".
+    for (int j = 1; j < num_of_character_moves; ++j) {
+      render_text->MoveCursorTo(start);
+      for (int k = 0; k < j; ++k)
+        render_text->MoveCursorRight(CHARACTER_BREAK, false);
+      render_text->MoveCursorRight(WORD_BREAK, false);
+      EXPECT_TRUE(render_text->selection_model().Equals(end));
+    }
+  }
+
+  // Test moving by word from right to left.
+  render_text->MoveCursorRight(LINE_BREAK, false);
+  first_word = true;
+  while (true) {
+    SelectionModel start = render_text->selection_model();
+    render_text->MoveCursorLeft(WORD_BREAK, false);
+    SelectionModel end = render_text->selection_model();
+    if (end.Equals(start))  // reach the end.
+      break;
+
+    int num_of_character_moves = first_word ? 3 : 4;
+    first_word = false;
+    render_text->MoveCursorTo(start);
+    for (int j = 0; j < num_of_character_moves; ++j)
+      render_text->MoveCursorLeft(CHARACTER_BREAK, false);
+    EXPECT_TRUE(render_text->selection_model().Equals(end));
+
+    for (int j = 1; j < num_of_character_moves - 1; ++j) {

[msw, 2011/09/15 01:26:00]

Why |num_of_character_moves - 1| here but not - 1 in the left-tor-right case
above? Comment?

[xji, 2011/09/15 22:58:09]

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

+      render_text->MoveCursorTo(start);
+      for (int k = 0; k < j; ++k)
+        render_text->MoveCursorLeft(CHARACTER_BREAK, false);
+      render_text->MoveCursorLeft(WORD_BREAK, false);
+      EXPECT_TRUE(render_text->selection_model().Equals(end));
+    }
+  }
+}
+
+TEST_F(RenderTextTest, MoveLeftRightByWordInBidiText) {
+  scoped_ptr<RenderText> render_text(RenderText::CreateRenderText());
+
+  // For testing simplicity, each word is a 3-character word.
+  std::vector<const wchar_t*> test;
+  test.push_back(L"abc");
+  test.push_back(L"abc def");
+  test.push_back(L"\x05E1\x05E2\x05E3");
+  test.push_back(L"\x05E1\x05E2\x05E3 \x05E4\x05E5\x05E6");
+  test.push_back(L"abc \x05E1\x05E2\x05E3");
+  test.push_back(L"abc def \x05E1\x05E2\x05E3 \x05E4\x05E5\x05E6");
+  test.push_back(L"abc def hij \x05E1\x05E2\x05E3 \x05E4\x05E5\x05E6"
+                 L" \x05E7\x05E8\x05E9");
+
+  test.push_back(L"abc \x05E1\x05E2\x05E3 hij");
+  test.push_back(L"abc def \x05E1\x05E2\x05E3 \x05E4\x05E5\x05E6 hij opq");
+  test.push_back(L"abc def hij \x05E1\x05E2\x05E3 \x05E4\x05E5\x05E6"
+                 L" \x05E7\x05E8\x05E9"L" opq rst uvw");
+
+  test.push_back(L"\x05E1\x05E2\x05E3 abc");
+  test.push_back(L"\x05E1\x05E2\x05E3 \x05E4\x05E5\x05E6 abc def");
+  test.push_back(L"\x05E1\x05E2\x05E3 \x05E4\x05E5\x05E6 \x05E7\x05E8\x05E9"
+                 L" abc def hij");
+
+  test.push_back(L"\x05D1\x05D2\x05D3 abc \x05E1\x05E2\x05E3");
+  test.push_back(L"\x05D1\x05D2\x05D3 \x05D4\x05D5\x05D6 abc def"
+                 L" \x05E1\x05E2\x05E3 \x05E4\x05E5\x05E6");
+  test.push_back(L"\x05D1\x05D2\x05D3 \x05D4\x05D5\x05D6 \x05D7\x05D8\x05D9"
+                 L" abc def hij \x05E1\x05E2\x05E3 \x05E4\x05E5\x05E6"
+                 L" \x05E7\x05E8\x05E9");
+
+  for (size_t i = 0; i < test.size(); ++i)
+    MoveLeftRightByWordVerifier(render_text.get(), test[i]);
+}
+
+TEST_F(RenderTextTest, MoveLeftRightByWordInBidiText_TestEndOfText) {

[msw, 2011/09/15 01:26:00]

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.

[xji, 2011/09/15 22:58:09]

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.

+  scoped_ptr<RenderText> render_text(RenderText::CreateRenderText());
+
+  render_text->SetText(WideToUTF16(L"ab\x05E1"));
+  render_text->MoveCursorRight(LINE_BREAK, false);
+  render_text->MoveCursorLeft(WORD_BREAK, false);
+  // TODO(xji): end of text is always treated as a word boundary.
+  // EXPECT_TRUE(render_text->selection_model().Equals(SelectionModel(0)));

[msw, 2011/09/15 01:26:00]

Why isn't this working now? Can you add "abC" comments with a '|' cursor to show
what's happening and what's expected?

[xji, 2011/09/15 22:58:09]

Done.

+
+  render_text->MoveCursorLeft(LINE_BREAK, false);
+  render_text->MoveCursorRight(WORD_BREAK, false);
+  EXPECT_TRUE(render_text->selection_model().Equals(
+      SelectionModel(3, 2, SelectionModel::LEADING)));
+
+  render_text->SetText(WideToUTF16(L"\x05E1\x05E2"L"a"));
+  render_text->MoveCursorRight(LINE_BREAK, false);
+  render_text->MoveCursorLeft(WORD_BREAK, false);
+  EXPECT_TRUE(render_text->selection_model().Equals(
+      SelectionModel(3, 2, SelectionModel::LEADING)));
+
+  render_text->MoveCursorLeft(LINE_BREAK, false);
+  render_text->MoveCursorRight(WORD_BREAK, false);
+  // TODO(xji): end of text is always treated as a word boundary.
+  // EXPECT_TRUE(render_text->selection_model().Equals(SelectionModel(0)));

[msw, 2011/09/15 01:26:00]

Same here, I don't get what's broken.

[xji, 2011/09/15 22:58:09]

explained.

+}
+
+TEST_F(RenderTextTest, MoveLeftRightByWordInTextWithMultiSpaces) {
+  scoped_ptr<RenderText> render_text(RenderText::CreateRenderText());
+  render_text->SetText(WideToUTF16(L"abc     def"));
+  render_text->MoveCursorTo(SelectionModel(5));
+  render_text->MoveCursorRight(WORD_BREAK, false);
+  EXPECT_EQ(11U, render_text->GetCursorPosition());
+
+  render_text->MoveCursorTo(SelectionModel(5));
+  render_text->MoveCursorLeft(WORD_BREAK, false);
+  EXPECT_EQ(0U, render_text->GetCursorPosition());
+}
+#endif
+
 }  // namespace gfx