Fix undo/redo when typing over selected text The return key also, in some cases before this CL, created multiple undo items so undo/redo didn't work correctly for return either. Backspace incidentally did work correctly before this. This CL adds CPWL_EditImpl::TypeChar() to encapsulate the logic of handling the backspace and return key which was previously in CPWL_Edit::OnCharInternal(). This way CPWL_Edit isn't responsible for managing the tricky undo queue state. The logic in TypeChar() could be simpler but I went to extra lengths to minimize the number of items in the undo queue. Mainly I did this because the queue limit test expects typing to be exactly one undo item. I also didn't want to effectively divide the undo limit by 3 by adding 3 undo items for every character typed. Also deleted some now-dead code for forwarding calls to CPWL_EditImpl. Fixed: 446727801 Change-Id: Id9a82cb40aedca7ca2605a7362a83050fca0da51 Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/136992 Reviewed-by: Lei Zhang <thestig@chromium.org> Commit-Queue: April Kallmeyer <ask@chromium.org> Reviewed-by: Tom Sepez <tsepez@chromium.org>
diff --git a/fpdfsdk/pwl/cpwl_edit.cpp b/fpdfsdk/pwl/cpwl_edit.cpp index 8e9a3f2..d6ff46f 100644 --- a/fpdfsdk/pwl/cpwl_edit.cpp +++ b/fpdfsdk/pwl/cpwl_edit.cpp
@@ -617,26 +617,7 @@ return true; } - if (edit_impl_->IsSelected() && word == pdfium::ascii::kBackspace) { - word = pdfium::ascii::kNul; - } - - ClearSelection(); - - switch (word) { - case pdfium::ascii::kBackspace: - Backspace(); - break; - case pdfium::ascii::kReturn: - InsertReturn(); - break; - case pdfium::ascii::kNul: - break; - default: - InsertWord(word, GetCharSet()); - break; - } - + edit_impl_->TypeChar(word, GetCharSet()); return true; } @@ -780,30 +761,12 @@ void CPWL_Edit::PasteText() {} -void CPWL_Edit::InsertWord(uint16_t word, FX_Charset nCharset) { - if (!IsReadOnly()) { - edit_impl_->InsertWord(word, nCharset); - } -} - -void CPWL_Edit::InsertReturn() { - if (!IsReadOnly()) { - edit_impl_->InsertReturn(); - } -} - void CPWL_Edit::Delete() { if (!IsReadOnly()) { edit_impl_->Delete(); } } -void CPWL_Edit::Backspace() { - if (!IsReadOnly()) { - edit_impl_->Backspace(); - } -} - bool CPWL_Edit::CanUndo() { return !IsReadOnly() && edit_impl_->CanUndo(); }
diff --git a/fpdfsdk/pwl/cpwl_edit.h b/fpdfsdk/pwl/cpwl_edit.h index 0ca52dc..d409a32 100644 --- a/fpdfsdk/pwl/cpwl_edit.h +++ b/fpdfsdk/pwl/cpwl_edit.h
@@ -107,11 +107,8 @@ void CopyText(); void PasteText(); - void InsertWord(uint16_t word, FX_Charset nCharset); - void InsertReturn(); bool IsWndHorV() const; void Delete(); - void Backspace(); void GetCaretInfo(CFX_PointF* ptHead, CFX_PointF* ptFoot) const; void SetEditCaret(bool bVisible);
diff --git a/fpdfsdk/pwl/cpwl_edit_embeddertest.cpp b/fpdfsdk/pwl/cpwl_edit_embeddertest.cpp index f6f178d..4f3ee1c 100644 --- a/fpdfsdk/pwl/cpwl_edit_embeddertest.cpp +++ b/fpdfsdk/pwl/cpwl_edit_embeddertest.cpp
@@ -735,14 +735,11 @@ EXPECT_EQ(L"", GetCPWLEdit()->GetSelectedText()); EXPECT_TRUE(GetCPWLEdit()->CanUndo()); - // TODO(crbug.com/446727801): Only one undo/redo should be needed - EXPECT_TRUE(GetCPWLEdit()->Undo()); EXPECT_TRUE(GetCPWLEdit()->Undo()); EXPECT_EQ(L"ABC", GetCPWLEdit()->GetSelectedText()); EXPECT_TRUE(GetCPWLEdit()->CanRedo()); EXPECT_TRUE(GetCPWLEdit()->Redo()); - EXPECT_TRUE(GetCPWLEdit()->Redo()); EXPECT_EQ(L"Z", GetCPWLEdit()->GetText()); EXPECT_EQ(L"", GetCPWLEdit()->GetSelectedText()); EXPECT_FALSE(GetCPWLEdit()->CanRedo()); @@ -784,14 +781,11 @@ EXPECT_EQ(L"\r\n", GetCPWLEdit()->GetText()); EXPECT_TRUE(GetCPWLEdit()->CanUndo()); - // TODO(crbug.com/446727801): Only one undo/redo should be needed - EXPECT_TRUE(GetCPWLEdit()->Undo()); EXPECT_TRUE(GetCPWLEdit()->Undo()); EXPECT_EQ(L"ABC", GetCPWLEdit()->GetSelectedText()); EXPECT_TRUE(GetCPWLEdit()->CanRedo()); EXPECT_TRUE(GetCPWLEdit()->Redo()); - EXPECT_TRUE(GetCPWLEdit()->Redo()); EXPECT_EQ(L"\r\n", GetCPWLEdit()->GetText()); EXPECT_FALSE(GetCPWLEdit()->CanRedo()); }
diff --git a/fpdfsdk/pwl/cpwl_edit_impl.cpp b/fpdfsdk/pwl/cpwl_edit_impl.cpp index f5a73d0..bf05094 100644 --- a/fpdfsdk/pwl/cpwl_edit_impl.cpp +++ b/fpdfsdk/pwl/cpwl_edit_impl.cpp
@@ -10,6 +10,7 @@ #include <memory> #include <utility> +#include "constants/ascii.h" #include "core/fpdfapi/font/cpdf_font.h" #include "core/fpdfapi/render/cpdf_renderoptions.h" #include "core/fpdfapi/render/cpdf_textrenderer.h" @@ -1865,6 +1866,41 @@ AddEditUndoItem(std::make_unique<UndoReplaceSelection>()); } +void CPWL_EditImpl::TypeChar(uint16_t word, FX_Charset charset) { + bool was_selected = IsSelected(); + + // Backspace is special because it always needs only one undo item. + // ClearSelection() deletes the selected text so Backspace() isn't needed in + // that case. + if (word == pdfium::ascii::kBackspace) { + if (was_selected) { + ClearSelection(); + } else { + Backspace(); + } + return; + } + + // Don't add the UndoReplaceSelection sentinel items if there's no selection + // so that in the normal "typing one character" case only one undo item goes + // into the queue. + if (was_selected) { + AddEditUndoItem(std::make_unique<UndoReplaceSelection>()); + ClearSelection(); + } + + if (word == pdfium::ascii::kReturn) { + InsertReturn(); + } else { + // Not a special character, just typing letters + InsertWord(word, charset); + } + + if (was_selected) { + AddEditUndoItem(std::make_unique<UndoReplaceSelection>()); + } +} + bool CPWL_EditImpl::Redo() { if (enable_undo_) { if (undo_.CanRedo()) {
diff --git a/fpdfsdk/pwl/cpwl_edit_impl.h b/fpdfsdk/pwl/cpwl_edit_impl.h index 51f0855..e8154ab 100644 --- a/fpdfsdk/pwl/cpwl_edit_impl.h +++ b/fpdfsdk/pwl/cpwl_edit_impl.h
@@ -101,6 +101,7 @@ void InsertText(const WideString& sText, FX_Charset charset); void ReplaceAndKeepSelection(const WideString& text); void ReplaceSelection(const WideString& text); + void TypeChar(uint16_t word, FX_Charset charset); bool Redo(); bool Undo(); void SetMaxUndoItemsForTest(size_t items);