From 0264e97bceaca69d0b0232ac680bd7b26e2db66a Mon Sep 17 00:00:00 2001 From: Dan Sinclair Date: Tue, 19 Sep 2017 10:07:20 -0400 Subject: Convert selection to use count instead of end index This CL changes the Text Edit Engine code to use a count instead of an end index for the selection range. Using count lets us differentiate a selection at the beginning of 1 character and an empty selection. A few new tests were added to test unicode word break behaviour, some are not working yet and are commented out. Change-Id: Icce8f5003102ef0a850151ccdf16d3c2226d94bf Reviewed-on: https://pdfium-review.googlesource.com/13491 Commit-Queue: dsinclair Reviewed-by: Henrique Nakashima --- xfa/fde/cfde_texteditengine.cpp | 44 +++++---- xfa/fde/cfde_texteditengine.h | 10 +-- xfa/fde/cfde_texteditengine_unittest.cpp | 149 ++++++++++++++++++++----------- xfa/fwl/cfwl_datetimepicker.h | 2 +- xfa/fwl/cfwl_edit.cpp | 31 ++++--- xfa/fwl/cfwl_edit.h | 2 +- xfa/fxfa/cxfa_fftextedit.cpp | 5 +- 7 files changed, 149 insertions(+), 94 deletions(-) (limited to 'xfa') diff --git a/xfa/fde/cfde_texteditengine.cpp b/xfa/fde/cfde_texteditengine.cpp index 8075ea7a98..0c64e39aeb 100644 --- a/xfa/fde/cfde_texteditengine.cpp +++ b/xfa/fde/cfde_texteditengine.cpp @@ -588,31 +588,29 @@ void CFDE_TextEditEngine::SelectAll() { has_selection_ = true; selection_.start_idx = 0; - selection_.end_idx = text_length_ - 1; + selection_.count = text_length_; } void CFDE_TextEditEngine::ClearSelection() { has_selection_ = false; selection_.start_idx = 0; - selection_.end_idx = 0; + selection_.count = 0; } -void CFDE_TextEditEngine::SetSelection(size_t start_idx, size_t end_idx) { - // If the points are the same, then we pretend the selection doesn't exist - // anymore. - if (start_idx == end_idx) { +void CFDE_TextEditEngine::SetSelection(size_t start_idx, size_t count) { + if (count == 0) { ClearSelection(); return; } if (start_idx > text_length_) return; - if (end_idx > text_length_) - end_idx = text_length_ - 1; + if (start_idx + count > text_length_) + count = text_length_ - start_idx; has_selection_ = true; selection_.start_idx = start_idx; - selection_.end_idx = end_idx; + selection_.count = count; } WideString CFDE_TextEditEngine::GetSelectedText() const { @@ -621,22 +619,30 @@ WideString CFDE_TextEditEngine::GetSelectedText() const { WideString text; if (selection_.start_idx < gap_position_) { - if (selection_.end_idx < gap_position_) { + // Fully on left of gap. + if (selection_.start_idx + selection_.count < gap_position_) { text += WideStringView(content_.data() + selection_.start_idx, - selection_.end_idx - selection_.start_idx + 1); + selection_.count); return text; } + // Pre-gap text text += WideStringView(content_.data() + selection_.start_idx, gap_position_ - selection_.start_idx); - text += WideStringView( - content_.data() + gap_position_ + gap_size_, - selection_.end_idx - (gap_position_ - selection_.start_idx) + 1); + + if (selection_.count - (gap_position_ - selection_.start_idx) > 0) { + // Post-gap text + text += WideStringView( + content_.data() + gap_position_ + gap_size_, + selection_.count - (gap_position_ - selection_.start_idx)); + } + return text; } + // Fully right of gap text += WideStringView(content_.data() + gap_size_ + selection_.start_idx, - selection_.end_idx - selection_.start_idx + 1); + selection_.count); return text; } @@ -645,8 +651,7 @@ WideString CFDE_TextEditEngine::DeleteSelectedText( if (!has_selection_) return L""; - return Delete(selection_.start_idx, - selection_.end_idx - selection_.start_idx + 1, add_operation); + return Delete(selection_.start_idx, selection_.count, add_operation); } WideString CFDE_TextEditEngine::Delete(size_t start_idx, @@ -965,6 +970,9 @@ std::vector CFDE_TextEditEngine::GetCharacterRectsInRange( std::pair CFDE_TextEditEngine::BoundsForWordAt( size_t idx) const { + if (idx > text_length_) + return {0, 0}; + CFDE_TextEditEngine::Iterator iter(this); iter.SetAt(idx); iter.FindNextBreakPos(true); @@ -972,7 +980,7 @@ std::pair CFDE_TextEditEngine::BoundsForWordAt( iter.FindNextBreakPos(false); size_t end_idx = iter.GetAt(); - return {start_idx, end_idx}; + return {start_idx, end_idx - start_idx + 1}; } CFDE_TextEditEngine::Iterator::Iterator(const CFDE_TextEditEngine* engine) diff --git a/xfa/fde/cfde_texteditengine.h b/xfa/fde/cfde_texteditengine.h index 750b62a2e1..40f12e5f76 100644 --- a/xfa/fde/cfde_texteditengine.h +++ b/xfa/fde/cfde_texteditengine.h @@ -140,12 +140,12 @@ class CFDE_TextEditEngine { size_t GetIndexAtEndOfLine(size_t pos) { return 0; } void SelectAll(); - void SetSelection(size_t start_idx, size_t end_idx); + void SetSelection(size_t start_idx, size_t count); void ClearSelection(); bool HasSelection() const { return has_selection_; } - // Returns indices of the selection. + // Returns of the selection. std::pair GetSelection() const { - return {selection_.start_idx, selection_.end_idx}; + return {selection_.start_idx, selection_.count}; } WideString GetSelectedText() const; WideString DeleteSelectedText( @@ -159,7 +159,7 @@ class CFDE_TextEditEngine { size_t GetWidthOfChar(size_t idx); // Non-const so we can force a Layout() if needed. size_t GetIndexForPoint(const CFX_PointF& point); - // + // std::pair BoundsForWordAt(size_t idx) const; // Returns @@ -197,7 +197,7 @@ class CFDE_TextEditEngine { struct Selection { size_t start_idx; - size_t end_idx; + size_t count; }; CFX_RectF contents_bounding_box_; diff --git a/xfa/fde/cfde_texteditengine_unittest.cpp b/xfa/fde/cfde_texteditengine_unittest.cpp index 48ed647945..51940f21a8 100644 --- a/xfa/fde/cfde_texteditengine_unittest.cpp +++ b/xfa/fde/cfde_texteditengine_unittest.cpp @@ -244,10 +244,10 @@ TEST_F(CFDE_TextEditEngineTest, Selection) { engine()->SelectAll(); size_t start_idx; - size_t end_idx; - std::tie(start_idx, end_idx) = engine()->GetSelection(); + size_t count; + std::tie(start_idx, count) = engine()->GetSelection(); EXPECT_EQ(0U, start_idx); - EXPECT_EQ(10U, end_idx); + EXPECT_EQ(11U, count); // Selection before gap. EXPECT_STREQ(L"Hello World", engine()->GetSelectedText().c_str()); @@ -272,7 +272,7 @@ TEST_F(CFDE_TextEditEngineTest, Selection) { EXPECT_STREQ(L"", engine()->GetText().c_str()); engine()->Insert(0, L"Hello World"); - engine()->SetSelection(5, 9); + engine()->SetSelection(5, 5); EXPECT_STREQ(L" Worl", engine()->DeleteSelectedText().c_str()); EXPECT_FALSE(engine()->HasSelection()); EXPECT_STREQ(L"Hellod", engine()->GetText().c_str()); @@ -419,112 +419,155 @@ TEST_F(CFDE_TextEditEngineTest, GetIndexForPoint) { TEST_F(CFDE_TextEditEngineTest, BoundsForWordAt) { size_t start_idx; - size_t end_idx; + size_t count; - std::tie(start_idx, end_idx) = engine()->BoundsForWordAt(100); + std::tie(start_idx, count) = engine()->BoundsForWordAt(100); EXPECT_EQ(0U, start_idx); - EXPECT_EQ(0U, end_idx); - engine()->SetSelection(start_idx, end_idx); + EXPECT_EQ(0U, count); + engine()->SetSelection(start_idx, count); EXPECT_STREQ(L"", engine()->GetSelectedText().c_str()); engine()->Clear(); engine()->Insert(0, L"Hello"); - std::tie(start_idx, end_idx) = engine()->BoundsForWordAt(0); + std::tie(start_idx, count) = engine()->BoundsForWordAt(0); EXPECT_EQ(0U, start_idx); - EXPECT_EQ(4U, end_idx); - engine()->SetSelection(start_idx, end_idx); + EXPECT_EQ(5U, count); + engine()->SetSelection(start_idx, count); EXPECT_STREQ(L"Hello", engine()->GetSelectedText().c_str()); engine()->Clear(); engine()->Insert(0, L"Hello World"); - std::tie(start_idx, end_idx) = engine()->BoundsForWordAt(100); - EXPECT_EQ(11U, start_idx); - EXPECT_EQ(11U, end_idx); - engine()->SetSelection(start_idx, end_idx); + std::tie(start_idx, count) = engine()->BoundsForWordAt(100); + EXPECT_EQ(0U, start_idx); + EXPECT_EQ(0U, count); + engine()->SetSelection(start_idx, count); EXPECT_STREQ(L"", engine()->GetSelectedText().c_str()); - std::tie(start_idx, end_idx) = engine()->BoundsForWordAt(0); + std::tie(start_idx, count) = engine()->BoundsForWordAt(0); EXPECT_EQ(0U, start_idx); - EXPECT_EQ(4U, end_idx); - engine()->SetSelection(start_idx, end_idx); + EXPECT_EQ(5U, count); + engine()->SetSelection(start_idx, count); EXPECT_STREQ(L"Hello", engine()->GetSelectedText().c_str()); - std::tie(start_idx, end_idx) = engine()->BoundsForWordAt(1); + std::tie(start_idx, count) = engine()->BoundsForWordAt(1); EXPECT_EQ(0U, start_idx); - EXPECT_EQ(4U, end_idx); - engine()->SetSelection(start_idx, end_idx); + EXPECT_EQ(5U, count); + engine()->SetSelection(start_idx, count); EXPECT_STREQ(L"Hello", engine()->GetSelectedText().c_str()); - std::tie(start_idx, end_idx) = engine()->BoundsForWordAt(4); + std::tie(start_idx, count) = engine()->BoundsForWordAt(4); EXPECT_EQ(0U, start_idx); - EXPECT_EQ(4U, end_idx); - engine()->SetSelection(start_idx, end_idx); + EXPECT_EQ(5U, count); + engine()->SetSelection(start_idx, count); EXPECT_STREQ(L"Hello", engine()->GetSelectedText().c_str()); // Select the space - std::tie(start_idx, end_idx) = engine()->BoundsForWordAt(5); + std::tie(start_idx, count) = engine()->BoundsForWordAt(5); EXPECT_EQ(5U, start_idx); - EXPECT_EQ(5U, end_idx); - engine()->SetSelection(start_idx, end_idx); - EXPECT_STREQ(L"", engine()->GetSelectedText().c_str()); + EXPECT_EQ(1U, count); + engine()->SetSelection(start_idx, count); + EXPECT_STREQ(L" ", engine()->GetSelectedText().c_str()); - std::tie(start_idx, end_idx) = engine()->BoundsForWordAt(6); + std::tie(start_idx, count) = engine()->BoundsForWordAt(6); EXPECT_EQ(6U, start_idx); - EXPECT_EQ(10U, end_idx); - engine()->SetSelection(start_idx, end_idx); + EXPECT_EQ(5U, count); + engine()->SetSelection(start_idx, count); EXPECT_STREQ(L"World", engine()->GetSelectedText().c_str()); engine()->Clear(); engine()->Insert(0, L"123 456 789"); - std::tie(start_idx, end_idx) = engine()->BoundsForWordAt(5); - engine()->SetSelection(start_idx, end_idx); + std::tie(start_idx, count) = engine()->BoundsForWordAt(5); + engine()->SetSelection(start_idx, count); EXPECT_STREQ(L"456", engine()->GetSelectedText().c_str()); engine()->Clear(); engine()->Insert(0, L"123def789"); - std::tie(start_idx, end_idx) = engine()->BoundsForWordAt(5); - engine()->SetSelection(start_idx, end_idx); + std::tie(start_idx, count) = engine()->BoundsForWordAt(5); + engine()->SetSelection(start_idx, count); EXPECT_STREQ(L"123def789", engine()->GetSelectedText().c_str()); engine()->Clear(); engine()->Insert(0, L"abc456ghi"); - std::tie(start_idx, end_idx) = engine()->BoundsForWordAt(5); - engine()->SetSelection(start_idx, end_idx); + std::tie(start_idx, count) = engine()->BoundsForWordAt(5); + engine()->SetSelection(start_idx, count); EXPECT_STREQ(L"abc456ghi", engine()->GetSelectedText().c_str()); engine()->Clear(); engine()->Insert(0, L"hello, world"); - std::tie(start_idx, end_idx) = engine()->BoundsForWordAt(0); - engine()->SetSelection(start_idx, end_idx); + std::tie(start_idx, count) = engine()->BoundsForWordAt(0); + engine()->SetSelection(start_idx, count); EXPECT_STREQ(L"hello", engine()->GetSelectedText().c_str()); engine()->Clear(); engine()->Insert(0, L"hello, world"); - std::tie(start_idx, end_idx) = engine()->BoundsForWordAt(5); - engine()->SetSelection(start_idx, end_idx); - EXPECT_STREQ(L"", engine()->GetSelectedText().c_str()); + std::tie(start_idx, count) = engine()->BoundsForWordAt(5); + engine()->SetSelection(start_idx, count); + EXPECT_STREQ(L",", engine()->GetSelectedText().c_str()); engine()->Clear(); engine()->Insert(0, L"np-complete"); - std::tie(start_idx, end_idx) = engine()->BoundsForWordAt(6); - engine()->SetSelection(start_idx, end_idx); + std::tie(start_idx, count) = engine()->BoundsForWordAt(6); + engine()->SetSelection(start_idx, count); EXPECT_STREQ(L"complete", engine()->GetSelectedText().c_str()); engine()->Clear(); engine()->Insert(0, L"(123) 456-7890"); - std::tie(start_idx, end_idx) = engine()->BoundsForWordAt(0); - engine()->SetSelection(start_idx, end_idx); - EXPECT_STREQ(L"", engine()->GetSelectedText().c_str()); + std::tie(start_idx, count) = engine()->BoundsForWordAt(0); + engine()->SetSelection(start_idx, count); + EXPECT_STREQ(L"(", engine()->GetSelectedText().c_str()); - std::tie(start_idx, end_idx) = engine()->BoundsForWordAt(1); - engine()->SetSelection(start_idx, end_idx); + std::tie(start_idx, count) = engine()->BoundsForWordAt(1); + engine()->SetSelection(start_idx, count); EXPECT_STREQ(L"123", engine()->GetSelectedText().c_str()); - std::tie(start_idx, end_idx) = engine()->BoundsForWordAt(7); - engine()->SetSelection(start_idx, end_idx); + std::tie(start_idx, count) = engine()->BoundsForWordAt(7); + engine()->SetSelection(start_idx, count); EXPECT_STREQ(L"456", engine()->GetSelectedText().c_str()); - std::tie(start_idx, end_idx) = engine()->BoundsForWordAt(11); - engine()->SetSelection(start_idx, end_idx); + std::tie(start_idx, count) = engine()->BoundsForWordAt(11); + engine()->SetSelection(start_idx, count); EXPECT_STREQ(L"7890", engine()->GetSelectedText().c_str()); + + // Tests from: + // http://unicode.org/Public/UNIDATA/auxiliary/WordBreakTest.html#samples + struct bounds { + size_t start; + size_t end; + }; + struct { + const wchar_t* str; + std::vector results; + } tests[] = { + // {L"\r\na\n\u0308", {L"\r\n", L"a", L"\n", L"\u0308"}}, + // {L"a\u0308", {L"a\u0308"}}, + // {L" \u200d\u0646", {L" \u200d", L"\u0646"}}, + // {L"\u0646\u200d ", {L"\u0646\u200d", L" "}}, + {L"AAA", {L"AAA"}}, + {L"A:A", {L"A:A"}}, + {L"A::A", {L"A", L":", L":", L"A"}}, + // {L"\u05d0'", {L"\u05d0'"}}, + // {L"\u05d0\"\u05d0", {L"\u05d0\"\u05d0"}}, + {L"A00A", {L"A00A"}}, + {L"0,0", {L"0,0"}}, + {L"0,,0", {L"0", L",", L",", L"0"}}, + {L"\u3031\u3031", {L"\u3031\u3031"}}, + {L"A_0_\u3031_", {L"A_0_\u3031_"}}, + {L"A__A", {L"A__A"}}, + // {L"\u200d\u2640", {L"\u200d\u2640"}}, + // {L"a\u0308\u200b\u0308b", {L"a\u0308\u200b\u0308b"}}, + }; + + for (auto t : tests) { + engine()->Clear(); + engine()->Insert(0, t.str); + + size_t idx = 0; + for (const auto* res : t.results) { + std::tie(start_idx, count) = engine()->BoundsForWordAt(idx); + engine()->SetSelection(start_idx, count); + EXPECT_STREQ(res, engine()->GetSelectedText().c_str()) + << "Input: '" << t.str << "'"; + idx += count; + } + } } diff --git a/xfa/fwl/cfwl_datetimepicker.h b/xfa/fwl/cfwl_datetimepicker.h index 276fea451d..97c5cd196e 100644 --- a/xfa/fwl/cfwl_datetimepicker.h +++ b/xfa/fwl/cfwl_datetimepicker.h @@ -53,7 +53,7 @@ class CFWL_DateTimePicker : public CFWL_Widget { WideString GetEditText() const; bool HasSelection() const { return m_pEdit->HasSelection(); } - // Returns indices of the selection. + // Returns of the selection. std::pair GetSelection() const { return m_pEdit->GetSelection(); } diff --git a/xfa/fwl/cfwl_edit.cpp b/xfa/fwl/cfwl_edit.cpp index 1bec1503fc..426d46e761 100644 --- a/xfa/fwl/cfwl_edit.cpp +++ b/xfa/fwl/cfwl_edit.cpp @@ -481,10 +481,10 @@ void CFWL_Edit::DrawContent(CXFA_Graphics* pGraphics, bool bShowSel = !!(m_pProperties->m_dwStates & FWL_WGTSTATE_Focused); if (bShowSel && m_EdtEngine.HasSelection()) { size_t sel_start; - size_t sel_end; - std::tie(sel_start, sel_end) = m_EdtEngine.GetSelection(); - std::vector rects = m_EdtEngine.GetCharacterRectsInRange( - sel_start, sel_end - sel_start + 1); + size_t count; + std::tie(sel_start, count) = m_EdtEngine.GetSelection(); + std::vector rects = + m_EdtEngine.GetCharacterRectsInRange(sel_start, count); CXFA_Path path; for (auto& rect : rects) { @@ -1237,11 +1237,11 @@ void CFWL_Edit::OnLButtonUp(CFWL_MessageMouse* pMsg) { void CFWL_Edit::OnButtonDoubleClick(CFWL_MessageMouse* pMsg) { size_t click_idx = m_EdtEngine.GetIndexForPoint(DeviceToEngine(pMsg->m_pos)); size_t start_idx; - size_t end_idx; - std::tie(start_idx, end_idx) = m_EdtEngine.BoundsForWordAt(click_idx); + size_t count; + std::tie(start_idx, count) = m_EdtEngine.BoundsForWordAt(click_idx); - m_EdtEngine.SetSelection(start_idx, end_idx); - m_CursorPosition = end_idx; + m_EdtEngine.SetSelection(start_idx, count); + m_CursorPosition = start_idx + count; RepaintRect(m_rtEngine); } @@ -1260,10 +1260,13 @@ void CFWL_Edit::OnMouseMove(CFWL_MessageMouse* pMsg) { SetCursorPosition(length); size_t sel_start; - size_t sel_end; - std::tie(sel_start, sel_end) = m_EdtEngine.GetSelection(); - m_EdtEngine.SetSelection(std::min(sel_start, m_CursorPosition), - std::max(sel_end, m_CursorPosition)); + size_t count; + std::tie(sel_start, count) = m_EdtEngine.GetSelection(); + size_t original_end = sel_start + count; + sel_start = std::min(sel_start, m_CursorPosition); + m_EdtEngine.SetSelection( + std::min(sel_start, m_CursorPosition), + std::max(original_end, m_CursorPosition) - sel_start); } void CFWL_Edit::OnKeyDown(CFWL_MessageKey* pMsg) { @@ -1273,8 +1276,8 @@ void CFWL_Edit::OnKeyDown(CFWL_MessageKey* pMsg) { size_t sel_start = m_CursorPosition; if (m_EdtEngine.HasSelection()) { size_t start_idx; - size_t end_idx; - std::tie(start_idx, end_idx) = m_EdtEngine.GetSelection(); + size_t count; + std::tie(start_idx, count) = m_EdtEngine.GetSelection(); sel_start = start_idx; } diff --git a/xfa/fwl/cfwl_edit.h b/xfa/fwl/cfwl_edit.h index e85baaaa38..cfebb25146 100644 --- a/xfa/fwl/cfwl_edit.h +++ b/xfa/fwl/cfwl_edit.h @@ -74,7 +74,7 @@ class CFWL_Edit : public CFWL_Widget, public CFDE_TextEditEngine::Delegate { void SelectAll(); void ClearSelection(); bool HasSelection() const; - // Returns indices of the selection. + // Returns of the selection. std::pair GetSelection() const; int32_t GetLimit() const; diff --git a/xfa/fxfa/cxfa_fftextedit.cpp b/xfa/fxfa/cxfa_fftextedit.cpp index e27971d5df..82b6f63b59 100644 --- a/xfa/fxfa/cxfa_fftextedit.cpp +++ b/xfa/fxfa/cxfa_fftextedit.cpp @@ -299,8 +299,9 @@ void CXFA_FFTextEdit::OnTextChanged(CFWL_Widget* pWidget, CFWL_DateTimePicker* pDateTime = (CFWL_DateTimePicker*)pEdit; eParam.m_wsNewText = pDateTime->GetEditText(); if (pDateTime->HasSelection()) { - std::tie(eParam.m_iSelStart, eParam.m_iSelEnd) = - pDateTime->GetSelection(); + size_t count; + std::tie(eParam.m_iSelStart, count) = pDateTime->GetSelection(); + eParam.m_iSelEnd = eParam.m_iSelStart + count; } } else { eParam.m_wsNewText = pEdit->GetText(); -- cgit v1.2.3