Fix undo queue state for ReplaceSelection items
The DCHECK crash bug was introduced as a part of a bug fix in CL [1].
CL [1] introduced the scheme where Undo/Redo returns the number of items
remaining and everything but UndoReplaceSelection returned 0. The
original problem that needed fixing was that ReplaceSelection inserts
multiple items into the undo queue and all must be executed with undo
and redo operations.
Initially, the number of undo items for ReplaceSelection was hard-coded
to 3. However this was buggy because the number of items is actually
variable. So CL [2] added an additional complication in ReplaceSelection
where the internal logic of the other helpers was replicated to
determine the number of items inserted into the queue. This number was
intended to be set on both UndoReplaceSelection objects via
UndoStack::GetLastAddItem().
The fix in CL [2] had two problems:
1. The replication of the internal logic was imperfect, so the count
was sometimes wrong. It was even revised later but it was still
wrong.
2. GetLastAddItem() isn't a reliable way to access the
UndoReplaceSelection object. Sometimes set_undo_remaining() was
called on other item types that ignore it.
This CL takes a different approach: don't try to set these item counts
at all. Instead simply look for the two UndoReplaceSelection items and
execute every item in between.
[1] https://pdfium.googlesource.com/pdfium/+/3abc96a18bf41a55958e2fe99a3ce03882c3ecb5
[2] https://pdfium.googlesource.com/pdfium/+/6a34da391b15f5373247fda48c10a12bc4143c94
Bug: 445725426
Change-Id: I61fc85d022200574f93c981150471b27150ca492
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/135850
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: April Kallmeyer <ask@chromium.org>
diff --git a/fpdfsdk/fpdf_formfill_embeddertest.cpp b/fpdfsdk/fpdf_formfill_embeddertest.cpp
index 6f33fa5..e266474 100644
--- a/fpdfsdk/fpdf_formfill_embeddertest.cpp
+++ b/fpdfsdk/fpdf_formfill_embeddertest.cpp
@@ -3361,6 +3361,47 @@
EXPECT_EQ(Selection(), "");
}
+TEST_F(FPDFFormFillTextFormEmbedderTest, RedoCutSelection) {
+ ClickOnFormFieldAtPoint(RegularFormBegin());
+ EXPECT_FALSE(CanUndo());
+ EXPECT_FALSE(CanRedo());
+
+ TypeTextIntoTextField(2, RegularFormBegin());
+ EXPECT_EQ(FocusedFieldText(), "AB");
+ EXPECT_EQ(Selection(), "");
+ EXPECT_TRUE(CanUndo());
+ EXPECT_FALSE(CanRedo());
+
+ // ctrl+a
+ EXPECT_TRUE(FORM_SelectAllText(form_handle(), page()));
+ EXPECT_TRUE(CanUndo());
+ EXPECT_FALSE(CanRedo());
+ EXPECT_EQ(FocusedFieldText(), "AB");
+ EXPECT_EQ(Selection(), "AB");
+
+ // ctrl+x
+ ScopedFPDFWideString text_to_insert = GetFPDFWideString(L"");
+ FORM_ReplaceSelection(form_handle(), page(), text_to_insert.get());
+ EXPECT_TRUE(CanUndo());
+ EXPECT_FALSE(CanRedo());
+ EXPECT_EQ(FocusedFieldText(), "");
+ EXPECT_EQ(Selection(), "");
+
+ // ctrl+z
+ PerformUndo();
+ EXPECT_TRUE(CanUndo());
+ EXPECT_TRUE(CanRedo());
+ EXPECT_EQ(FocusedFieldText(), "AB");
+ EXPECT_EQ(Selection(), "AB");
+
+ // ctrl+shift+z
+ PerformRedo();
+ EXPECT_TRUE(CanUndo());
+ EXPECT_FALSE(CanRedo());
+ EXPECT_EQ(FocusedFieldText(), "");
+ EXPECT_EQ(Selection(), "");
+}
+
TEST_F(FPDFFormFillTextFormEmbedderTest, SelectAllWithKeyboardShortcut) {
// Start with a couple of letters in the text form.
TypeTextIntoTextField(2, RegularFormBegin());
diff --git a/fpdfsdk/pwl/cpwl_edit_impl.cpp b/fpdfsdk/pwl/cpwl_edit_impl.cpp
index a025eba..d8e44ac 100644
--- a/fpdfsdk/pwl/cpwl_edit_impl.cpp
+++ b/fpdfsdk/pwl/cpwl_edit_impl.cpp
@@ -176,6 +176,10 @@
refresh_rects_.push_back(new_rect);
}
+bool CPWL_EditImpl::UndoItemIface::IsSentinel() {
+ return false;
+}
+
CPWL_EditImpl::UndoStack::UndoStack() = default;
CPWL_EditImpl::UndoStack::~UndoStack() = default;
@@ -187,13 +191,23 @@
void CPWL_EditImpl::UndoStack::Undo() {
DCHECK(!working_);
working_ = true;
- int undo_remaining = 1;
- while (CanUndo() && undo_remaining > 0) {
- undo_remaining += undo_item_stack_[cur_undo_pos_ - 1]->Undo();
- cur_undo_pos_--;
- undo_remaining--;
+ bool first_undo = true;
+ while (CanUndo()) {
+ --cur_undo_pos_;
+ std::unique_ptr<UndoItemIface>& item = undo_item_stack_[cur_undo_pos_];
+ item->Undo();
+
+ if (first_undo) {
+ first_undo = false;
+ if (!item->IsSentinel()) {
+ break;
+ }
+ } else {
+ if (item->IsSentinel()) {
+ break;
+ }
+ }
}
- DCHECK_EQ(undo_remaining, 0);
DCHECK(working_);
working_ = false;
}
@@ -202,21 +216,26 @@
return cur_undo_pos_ < undo_item_stack_.size();
}
-CPWL_EditImpl::UndoItemIface* CPWL_EditImpl::UndoStack::GetLastAddItem() {
- CHECK(!undo_item_stack_.empty());
- return undo_item_stack_.back().get();
-}
-
void CPWL_EditImpl::UndoStack::Redo() {
DCHECK(!working_);
working_ = true;
- int nRedoRemain = 1;
- while (CanRedo() && nRedoRemain > 0) {
- nRedoRemain += undo_item_stack_[cur_undo_pos_]->Redo();
- cur_undo_pos_++;
- nRedoRemain--;
+
+ bool first_undo = true;
+ while (CanRedo()) {
+ std::unique_ptr<UndoItemIface>& item = undo_item_stack_[cur_undo_pos_];
+ ++cur_undo_pos_;
+ item->Redo();
+ if (first_undo) {
+ first_undo = false;
+ if (!item->IsSentinel()) {
+ break;
+ }
+ } else {
+ if (item->IsSentinel()) {
+ break;
+ }
+ }
}
- DCHECK_EQ(nRedoRemain, 0);
DCHECK(working_);
working_ = false;
}
@@ -258,8 +277,8 @@
~UndoInsertWord() override;
// UndoItemIface:
- int Redo() override;
- int Undo() override;
+ void Redo() override;
+ void Undo() override;
private:
UnownedPtr<CPWL_EditImpl> edit_;
@@ -285,18 +304,16 @@
CPWL_EditImpl::UndoInsertWord::~UndoInsertWord() = default;
-int CPWL_EditImpl::UndoInsertWord::Redo() {
+void CPWL_EditImpl::UndoInsertWord::Redo() {
edit_->SelectNone();
edit_->SetCaret(wp_old_);
edit_->InsertWord(word_, charset_, false);
- return 0;
}
-int CPWL_EditImpl::UndoInsertWord::Undo() {
+void CPWL_EditImpl::UndoInsertWord::Undo() {
edit_->SelectNone();
edit_->SetCaret(wp_new_);
edit_->Backspace(false);
- return 0;
}
class CPWL_EditImpl::UndoInsertReturn final
@@ -308,8 +325,8 @@
~UndoInsertReturn() override;
// UndoItemIface:
- int Redo() override;
- int Undo() override;
+ void Redo() override;
+ void Undo() override;
private:
UnownedPtr<CPWL_EditImpl> edit_;
@@ -328,67 +345,33 @@
CPWL_EditImpl::UndoInsertReturn::~UndoInsertReturn() = default;
-int CPWL_EditImpl::UndoInsertReturn::Redo() {
+void CPWL_EditImpl::UndoInsertReturn::Redo() {
edit_->SelectNone();
edit_->SetCaret(wp_old_);
edit_->InsertReturn(false);
- return 0;
}
-int CPWL_EditImpl::UndoInsertReturn::Undo() {
+void CPWL_EditImpl::UndoInsertReturn::Undo() {
edit_->SelectNone();
edit_->SetCaret(wp_new_);
edit_->Backspace(false);
- return 0;
}
class CPWL_EditImpl::UndoReplaceSelection final
: public CPWL_EditImpl::UndoItemIface {
public:
- UndoReplaceSelection(CPWL_EditImpl* pEdit, bool bIsEnd);
- ~UndoReplaceSelection() override;
+ UndoReplaceSelection() = default;
+ ~UndoReplaceSelection() override = default;
// UndoItemIface:
- int Redo() override;
- int Undo() override;
+ void Redo() override {}
+ void Undo() override {}
+ bool IsSentinel() override { return true; }
private:
- bool IsEnd() const { return end_; }
-
UnownedPtr<CPWL_EditImpl> edit_;
- const bool end_; // indicate whether this is the end of replace action
};
-CPWL_EditImpl::UndoReplaceSelection::UndoReplaceSelection(CPWL_EditImpl* pEdit,
- bool bIsEnd)
- : edit_(pEdit), end_(bIsEnd) {
- DCHECK(edit_);
- // Redo ClearSelection, InsertText and ReplaceSelection's end marker
- // Undo InsertText, ClearSelection and ReplaceSelection's beginning
- // marker
- set_undo_remaining(3);
-}
-
-CPWL_EditImpl::UndoReplaceSelection::~UndoReplaceSelection() = default;
-
-int CPWL_EditImpl::UndoReplaceSelection::Redo() {
- if (IsEnd()) {
- return 0;
- }
- // Redo ClearSelection, InsertText and ReplaceSelection's end
- // marker. (ClearSelection may not exist)
- return undo_remaining();
-}
-
-int CPWL_EditImpl::UndoReplaceSelection::Undo() {
- if (!IsEnd()) {
- return 0;
- }
- // Undo InsertText, ClearSelection and ReplaceSelection's beginning
- // marker. (ClearSelection may not exist)
- return undo_remaining();
-}
-
class CPWL_EditImpl::UndoBackspace final : public CPWL_EditImpl::UndoItemIface {
public:
UndoBackspace(CPWL_EditImpl* pEdit,
@@ -399,8 +382,8 @@
~UndoBackspace() override;
// UndoItemIface:
- int Redo() override;
- int Undo() override;
+ void Redo() override;
+ void Undo() override;
private:
UnownedPtr<CPWL_EditImpl> edit_;
@@ -426,14 +409,13 @@
CPWL_EditImpl::UndoBackspace::~UndoBackspace() = default;
-int CPWL_EditImpl::UndoBackspace::Redo() {
+void CPWL_EditImpl::UndoBackspace::Redo() {
edit_->SelectNone();
edit_->SetCaret(wp_old_);
edit_->Backspace(false);
- return 0;
}
-int CPWL_EditImpl::UndoBackspace::Undo() {
+void CPWL_EditImpl::UndoBackspace::Undo() {
edit_->SelectNone();
edit_->SetCaret(wp_new_);
if (wp_new_.nSecIndex != wp_old_.nSecIndex) {
@@ -441,7 +423,6 @@
} else {
edit_->InsertWord(word_, charset_, false);
}
- return 0;
}
class CPWL_EditImpl::UndoDelete final : public CPWL_EditImpl::UndoItemIface {
@@ -455,8 +436,8 @@
~UndoDelete() override;
// UndoItemIface:
- int Redo() override;
- int Undo() override;
+ void Redo() override;
+ void Undo() override;
private:
UnownedPtr<CPWL_EditImpl> edit_;
@@ -485,14 +466,13 @@
CPWL_EditImpl::UndoDelete::~UndoDelete() = default;
-int CPWL_EditImpl::UndoDelete::Redo() {
+void CPWL_EditImpl::UndoDelete::Redo() {
edit_->SelectNone();
edit_->SetCaret(wp_old_);
edit_->Delete(false);
- return 0;
}
-int CPWL_EditImpl::UndoDelete::Undo() {
+void CPWL_EditImpl::UndoDelete::Undo() {
edit_->SelectNone();
edit_->SetCaret(wp_new_);
if (sec_end_) {
@@ -500,7 +480,6 @@
} else {
edit_->InsertWord(word_, charset_, false);
}
- return 0;
}
class CPWL_EditImpl::UndoClear final : public CPWL_EditImpl::UndoItemIface {
@@ -511,8 +490,8 @@
~UndoClear() override;
// UndoItemIface:
- int Redo() override;
- int Undo() override;
+ void Redo() override;
+ void Undo() override;
private:
UnownedPtr<CPWL_EditImpl> edit_;
@@ -530,19 +509,17 @@
CPWL_EditImpl::UndoClear::~UndoClear() = default;
-int CPWL_EditImpl::UndoClear::Redo() {
+void CPWL_EditImpl::UndoClear::Redo() {
edit_->SelectNone();
edit_->SetSelection(wr_sel_.BeginPos, wr_sel_.EndPos);
edit_->Clear(false);
- return 0;
}
-int CPWL_EditImpl::UndoClear::Undo() {
+void CPWL_EditImpl::UndoClear::Undo() {
edit_->SelectNone();
edit_->SetCaret(wr_sel_.BeginPos);
edit_->InsertText(sw_text_, FX_Charset::kDefault, false);
edit_->SetSelection(wr_sel_.BeginPos, wr_sel_.EndPos);
- return 0;
}
class CPWL_EditImpl::UndoInsertText final
@@ -556,8 +533,8 @@
~UndoInsertText() override;
// UndoItemIface:
- int Redo() override;
- int Undo() override;
+ void Redo() override;
+ void Undo() override;
private:
UnownedPtr<CPWL_EditImpl> edit_;
@@ -583,18 +560,16 @@
CPWL_EditImpl::UndoInsertText::~UndoInsertText() = default;
-int CPWL_EditImpl::UndoInsertText::Redo() {
+void CPWL_EditImpl::UndoInsertText::Redo() {
edit_->SelectNone();
edit_->SetCaret(wp_old_);
edit_->InsertText(sw_text_, charset_, false);
- return 0;
}
-int CPWL_EditImpl::UndoInsertText::Undo() {
+void CPWL_EditImpl::UndoInsertText::Undo() {
edit_->SelectNone();
edit_->SetSelection(wp_old_, wp_new_);
edit_->Clear(false);
- return 0;
}
void CPWL_EditImpl::DrawEdit(CFX_RenderDevice* pDevice,
@@ -1849,42 +1824,27 @@
}
void CPWL_EditImpl::ReplaceAndKeepSelection(const WideString& text) {
- AddEditUndoItem(std::make_unique<UndoReplaceSelection>(this, false));
- bool is_insert_undo_clear = ClearSelection();
- // It is necessary to determine whether the value of `undo_remaining_` is 2 or
- // 3 based on ClearSelection().
- // Special case: when cutting (clearing selection and inserting empty text),
- // we need to ensure proper undo counting to restore all characters.
- if (!is_insert_undo_clear || text.IsEmpty()) {
- undo_.GetLastAddItem()->set_undo_remaining(2);
- }
- // Select the inserted text.
+ AddEditUndoItem(std::make_unique<UndoReplaceSelection>());
+ ClearSelection();
+
+ // Insert the text and then select it
CPVT_WordPlace caret_before_insert = wp_caret_;
InsertText(text, FX_Charset::kDefault);
CPVT_WordPlace caret_after_insert = wp_caret_;
sel_state_.Set(caret_before_insert, caret_after_insert);
- AddEditUndoItem(std::make_unique<UndoReplaceSelection>(this, true));
- if (!is_insert_undo_clear || text.IsEmpty()) {
- undo_.GetLastAddItem()->set_undo_remaining(2);
- }
+ AddEditUndoItem(std::make_unique<UndoReplaceSelection>());
}
void CPWL_EditImpl::ReplaceSelection(const WideString& text) {
- AddEditUndoItem(std::make_unique<UndoReplaceSelection>(this, false));
- bool is_insert_undo_clear = ClearSelection();
- // It is necessary to determine whether the value of `undo_remaining_` is 2 or
- // 3 based on ClearSelection().
- // Special case: when cutting (clearing selection and inserting empty text),
- // we need to ensure proper undo counting to restore all characters.
- if (!is_insert_undo_clear || text.IsEmpty()) {
- undo_.GetLastAddItem()->set_undo_remaining(2);
- }
+ // UndoReplaceSelection acts as a sentinel object in the undo queue. Since
+ // ClearSelection and InsertText only optionally add undo items there are a
+ // variable number of items in the queue for this action. These sentinel
+ // objects mark the start and the end of the series of related undo items.
+ AddEditUndoItem(std::make_unique<UndoReplaceSelection>());
+ ClearSelection();
InsertText(text, FX_Charset::kDefault);
- AddEditUndoItem(std::make_unique<UndoReplaceSelection>(this, true));
- if (!is_insert_undo_clear || text.IsEmpty()) {
- undo_.GetLastAddItem()->set_undo_remaining(2);
- }
+ AddEditUndoItem(std::make_unique<UndoReplaceSelection>());
}
bool CPWL_EditImpl::Redo() {
diff --git a/fpdfsdk/pwl/cpwl_edit_impl.h b/fpdfsdk/pwl/cpwl_edit_impl.h
index 99dcf97..653c525 100644
--- a/fpdfsdk/pwl/cpwl_edit_impl.h
+++ b/fpdfsdk/pwl/cpwl_edit_impl.h
@@ -183,26 +183,14 @@
public:
virtual ~UndoItemIface() = default;
- // Undo/Redo the current undo item and returns the number of additional
- // items to be processed in |undo_item_stack_| to fully undo/redo the
- // action. (An example is UndoReplaceSelection::Undo(), if
- // UndoReplaceSelection marks the end of a replace action,
- // UndoReplaceSelection::Undo() returns |undo_remaining_|. The default value
- // of |undo_remaining_| in UndoReplaceSelection is 3. because 3 more undo
- // items need to be processed to revert the replace action: insert text,
- // clear selection and the UndoReplaceSelection which marks the beginning of
- // replace action. If CPWL_EditImpl::ClearSelection() returns false, the
- // value of |undo_remaining_| in UndoReplaceSelection needs to be set to 2)
- // Implementations should return 0 by default.
- virtual int Undo() = 0;
- virtual int Redo() = 0;
- void set_undo_remaining(int undo_remaining) {
- undo_remaining_ = undo_remaining;
- }
- int undo_remaining() const { return undo_remaining_; }
+ virtual void Undo() = 0;
+ virtual void Redo() = 0;
- private:
- int undo_remaining_ = 0;
+ // Return true if this is a sentinel undo item, the undo stack will continue
+ // performing undo/redo operations until the next sentinel undo item.
+ //
+ // Returns false by default. This is used by UndoReplaceSelection.
+ virtual bool IsSentinel();
};
class UndoStack {
@@ -215,9 +203,6 @@
void Redo();
bool CanUndo() const;
bool CanRedo() const;
- // GetLastAddItem() will never return null, so it can only be called after
- // calling AddItem().
- UndoItemIface* GetLastAddItem();
private:
void RemoveHeads();
