[PDF Ink Signatures] Return discarded commands in PdfInkUndoRedoModel

Change StartDraw() and StartErase() to return the set of drawing
commands to discard, instead of returning a bool. The caller can use
this data to discard the actual strokes that corresponds to the
discarded IDs.

Bug: 335521182
Change-Id: Icf8d68a08ccefc37564b26599e6abdf6d1dfa9c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5594601
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Alan Screen <awscreen@chromium.org>
Reviewed-by: Andy Phan <andyphan@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1310705}

pdf/pdf_ink_undo_redo_model.cc

@@ -6,6 +6,7 @@
 
 #include <stddef.h>
 
+#include <optional>
 #include <set>
 #include <vector>
 
@@ -35,7 +36,8 @@ PdfInkUndoRedoModel::PdfInkUndoRedoModel() = default;
 
 PdfInkUndoRedoModel::~PdfInkUndoRedoModel() = default;
 
-bool PdfInkUndoRedoModel::StartDraw() {
+std::optional<PdfInkUndoRedoModel::DiscardedDrawCommands>
+PdfInkUndoRedoModel::StartDraw() {
   return StartImpl<DrawCommands>();
 }
 
@@ -77,7 +79,8 @@ bool PdfInkUndoRedoModel::FinishDraw() {
   return true;
 }
 
-bool PdfInkUndoRedoModel::StartErase() {
+std::optional<PdfInkUndoRedoModel::DiscardedDrawCommands>
+PdfInkUndoRedoModel::StartErase() {
   return StartImpl<EraseCommands>();
 }
 
@@ -202,23 +205,46 @@ const PdfInkUndoRedoModel::EraseCommands& PdfInkUndoRedoModel::GetEraseCommands(
 }
 
 template <typename T>
-bool PdfInkUndoRedoModel::StartImpl() {
+std::optional<PdfInkUndoRedoModel::DiscardedDrawCommands>
+PdfInkUndoRedoModel::StartImpl() {
   CHECK(!commands_stack_.empty());
   CHECK_LT(stack_position_, commands_stack_.size());
 
+  DiscardedDrawCommands discarded_commands;
   auto& commands = commands_stack_[stack_position_];
   const bool has_commands = GetCommandsType(commands) != CommandsType::kNone;
   if (stack_position_ == commands_stack_.size() - 1) {
     if (has_commands) {
-      return false;  // Cannot start when drawing/erasing already started.
+      // Cannot start when drawing/erasing already started.
+      return std::nullopt;
     }
   } else {
-    CHECK(has_commands);                          // Invariant 2.
-    commands_stack_.resize(stack_position_ + 1);  // Discard rest of stack.
+    CHECK(has_commands);  // Invariant 2.
+
+    // Record the draw commands to discard.
+    for (size_t i = stack_position_; i < commands_stack_.size(); ++i) {
+      if (GetCommandsType(commands_stack_[i]) == CommandsType::kDraw) {
+        for (size_t id : GetDrawCommands(commands_stack_[i]).value()) {
+          bool inserted = discarded_commands.insert(id).second;
+          CHECK(inserted);
+        }
+      }
+    }
+
+    // Discard rest of stack.
+    //
+    // The vector capacity is never reduced when resizing to smaller size. Thus
+    // references to `commands_stack_` elements are not invalidated and safe to
+    // use.
+    commands_stack_.resize(stack_position_ + 1);
   }
 
-  commands = T();  // Set the top of the stack to the appropriate command type.
-  return true;
+  // Set the top of the stack to the appropriate command type.
+  //
+  // Safe to reuse `commands`, which references an element inside of
+  // `commands_stack_`. See note above the resize() call regarding safety.
+  commands = T();
+  return discarded_commands;
 }
 
 bool PdfInkUndoRedoModel::IsAtTopOfStackWithGivenCommandType(

pdf/pdf_ink_undo_redo_model.h

@@ -7,6 +7,7 @@
 
 #include <stddef.h>
 
+#include <optional>
 #include <set>
 #include <vector>
 
@@ -37,6 +38,9 @@ class PdfInkUndoRedoModel {
 
   using Commands = absl::variant<absl::monostate, DrawCommands, EraseCommands>;
 
+  // Set of IDs used for drawing to discard.
+  using DiscardedDrawCommands = std::set<size_t>;
+
   PdfInkUndoRedoModel();
   PdfInkUndoRedoModel(const PdfInkUndoRedoModel&) = delete;
   PdfInkUndoRedoModel& operator=(const PdfInkUndoRedoModel&) = delete;
@@ -45,16 +49,20 @@ class PdfInkUndoRedoModel {
   // For all Draw / Erase methods:
   // - The expected usage is: 1 StartOp call, any number of Op calls, 1 FinishOp
   //   call.
-  // - Returns true on success. Returns false if any requirements are not met.
+  // - StartOp returns a non-null, but possible empty value on success. Returns
+  //   nullopt if any requirements are not met.
+  // - Op and FinishOp return true on success. Return false if any requirements
+  //   are not met.
   // - Must not return false in production code. Returning false is only allowed
   //   in tests to check failure modes without resorting to death tests.
 
   // Starts recording draw commands. If the current commands stack position is
   // not at the top of the stack, then this discards all entries from the
-  // current position to the top of the stack.
+  // current position to the top of the stack. The caller can discard its
+  // entries with IDs that match the returned values.
   // Must be called before Draw().
   // Must not be called while another draw/erase has been started.
-  [[nodiscard]] bool StartDraw();
+  [[nodiscard]] std::optional<DiscardedDrawCommands> StartDraw();
   // Records drawing a stroke identified by `id`.
   // Must be called between StartDraw() and FinishDraw().
   // `id` must not be on the commands stack.
@@ -65,10 +73,11 @@ class PdfInkUndoRedoModel {
 
   // Starts recording erase commands. If the current commands stack position is
   // not at the top of the stack, then this discards all entries from the
-  // current position to the top of the stack.
+  // current position to the top of the stack. The caller can discard its
+  // entries with IDs that match the returned values.
   // Must be called before Erase().
   // Must not be called while another draw/erase has been started.
-  [[nodiscard]] bool StartErase();
+  [[nodiscard]] std::optional<DiscardedDrawCommands> StartErase();
   // Records erasing a stroke identified by `id`.
   // Must be called between StartErase() and FinishErase().
   // `id` must be in a `DrawCommands` on the commands stack.
@@ -90,7 +99,7 @@ class PdfInkUndoRedoModel {
 
  private:
   template <typename T>
-  bool StartImpl();
+  std::optional<DiscardedDrawCommands> StartImpl();
 
   bool IsAtTopOfStackWithGivenCommandType(CommandsType type) const;
   bool HasIdInDrawCommands(size_t id) const;

pdf/pdf_ink_undo_redo_model_unittest.cc

@@ -6,13 +6,19 @@
 
 #include <stddef.h>
 
+#include <numeric>
+#include <optional>
+#include <set>
+
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::ElementsAreArray;
+using testing::Optional;
 
 namespace chrome_pdf {
 
+using DiscardedDrawCommands = PdfInkUndoRedoModel::DiscardedDrawCommands;
 using enum PdfInkUndoRedoModel::CommandsType;
 
 namespace {
@@ -20,7 +26,8 @@ namespace {
 // Shorthand for test setup that is expected to succeed.
 void DoDrawCommandsCycle(PdfInkUndoRedoModel& undo_redo,
                          const std::set<size_t>& ids) {
-  ASSERT_TRUE(undo_redo.StartDraw());
+  std::optional<DiscardedDrawCommands> discards = undo_redo.StartDraw();
+  ASSERT_THAT(discards, Optional(DiscardedDrawCommands()));
   for (size_t id : ids) {
     ASSERT_TRUE(undo_redo.Draw(id));
   }
@@ -29,7 +36,8 @@ void DoDrawCommandsCycle(PdfInkUndoRedoModel& undo_redo,
 
 TEST(PdfInkUndoRedoModelTest, BadActionDoubleStartDraw) {
   PdfInkUndoRedoModel undo_redo;
-  ASSERT_TRUE(undo_redo.StartDraw());
+  std::optional<DiscardedDrawCommands> discards = undo_redo.StartDraw();
+  ASSERT_THAT(discards, Optional(DiscardedDrawCommands()));
   ASSERT_FALSE(undo_redo.StartDraw());
 }
 
@@ -45,7 +53,8 @@ TEST(PdfInkUndoRedoModelTest, BadActionSpuriousFinishDraw) {
 
 TEST(PdfInkUndoRedoModelTest, BadActionEraseWhileDrawing) {
   PdfInkUndoRedoModel undo_redo;
-  ASSERT_TRUE(undo_redo.StartDraw());
+  std::optional<DiscardedDrawCommands> discards = undo_redo.StartDraw();
+  ASSERT_THAT(discards, Optional(DiscardedDrawCommands()));
   ASSERT_TRUE(undo_redo.Draw(1));
 
   ASSERT_FALSE(undo_redo.StartErase());
@@ -55,7 +64,8 @@ TEST(PdfInkUndoRedoModelTest, BadActionEraseWhileDrawing) {
 
 TEST(PdfInkUndoRedoModelTest, BadActionDoubleStartErase) {
   PdfInkUndoRedoModel undo_redo;
-  ASSERT_TRUE(undo_redo.StartErase());
+  std::optional<DiscardedDrawCommands> discards = undo_redo.StartErase();
+  ASSERT_THAT(discards, Optional(DiscardedDrawCommands()));
   ASSERT_FALSE(undo_redo.StartErase());
 }
 
@@ -73,7 +83,8 @@ TEST(PdfInkUndoRedoModelTest, BadActionDrawWhileErasing) {
   PdfInkUndoRedoModel undo_redo;
   DoDrawCommandsCycle(undo_redo, {1});
 
-  ASSERT_TRUE(undo_redo.StartErase());
+  std::optional<DiscardedDrawCommands> discards = undo_redo.StartErase();
+  ASSERT_THAT(discards, Optional(DiscardedDrawCommands()));
 
   ASSERT_FALSE(undo_redo.StartDraw());
   ASSERT_FALSE(undo_redo.Draw(2));
@@ -132,7 +143,8 @@ TEST(PdfInkUndoRedoModelTest, BadActionEraseUnknownId) {
   PdfInkUndoRedoModel undo_redo;
   DoDrawCommandsCycle(undo_redo, {1});
 
-  ASSERT_TRUE(undo_redo.StartErase());
+  std::optional<DiscardedDrawCommands> discards = undo_redo.StartErase();
+  ASSERT_THAT(discards, Optional(DiscardedDrawCommands()));
   ASSERT_FALSE(undo_redo.Erase(3));
 }
 
@@ -140,7 +152,8 @@ TEST(PdfInkUndoRedoModelTest, BadActionEraseTwice) {
   PdfInkUndoRedoModel undo_redo;
   DoDrawCommandsCycle(undo_redo, {0});
 
-  ASSERT_TRUE(undo_redo.StartErase());
+  std::optional<DiscardedDrawCommands> discards = undo_redo.StartErase();
+  ASSERT_THAT(discards, Optional(DiscardedDrawCommands()));
   ASSERT_TRUE(undo_redo.Erase(0));
   ASSERT_FALSE(undo_redo.Erase(0));
 }
@@ -173,7 +186,8 @@ TEST(PdfInkUndoRedoModelTest, EmptyDraw) {
 
 TEST(PdfInkUndoRedoModelTest, EmptyErase) {
   PdfInkUndoRedoModel undo_redo;
-  ASSERT_TRUE(undo_redo.StartErase());
+  std::optional<DiscardedDrawCommands> discards = undo_redo.StartErase();
+  ASSERT_THAT(discards, Optional(DiscardedDrawCommands()));
   ASSERT_TRUE(undo_redo.FinishErase());
 
   PdfInkUndoRedoModel::Commands commands = undo_redo.Undo();
@@ -187,7 +201,8 @@ TEST(PdfInkUndoRedoModelTest, DrawCannotRepeatId) {
   PdfInkUndoRedoModel undo_redo;
   DoDrawCommandsCycle(undo_redo, {1, 2, 3});
 
-  ASSERT_TRUE(undo_redo.StartDraw());
+  std::optional<DiscardedDrawCommands> discards = undo_redo.StartDraw();
+  ASSERT_THAT(discards, Optional(DiscardedDrawCommands()));
   ASSERT_FALSE(undo_redo.Draw(1));
   ASSERT_FALSE(undo_redo.Draw(3));
 
@@ -214,7 +229,8 @@ TEST(PdfInkUndoRedoModelTest, DrawCanRepeatIdAfterUndo) {
   EXPECT_THAT(PdfInkUndoRedoModel::GetEraseCommands(commands).value(),
               ElementsAreArray({1, 2, 3}));
 
-  ASSERT_TRUE(undo_redo.StartDraw());
+  std::optional<DiscardedDrawCommands> discards = undo_redo.StartDraw();
+  ASSERT_THAT(discards, Optional(DiscardedDrawCommands({1, 2, 3, 97, 98, 99})));
   ASSERT_TRUE(undo_redo.Draw(2));
   ASSERT_TRUE(undo_redo.Draw(98));
 }
@@ -245,7 +261,8 @@ TEST(PdfInkUndoRedoModelTest, DrawDrawEraseUndoRedo) {
   DoDrawCommandsCycle(undo_redo, {1, 2, 3});
   DoDrawCommandsCycle(undo_redo, {4});
 
-  ASSERT_TRUE(undo_redo.StartErase());
+  std::optional<DiscardedDrawCommands> discards = undo_redo.StartErase();
+  ASSERT_THAT(discards, Optional(DiscardedDrawCommands()));
   ASSERT_TRUE(undo_redo.Erase(1));
   ASSERT_TRUE(undo_redo.Erase(4));
   ASSERT_TRUE(undo_redo.FinishErase());
@@ -301,7 +318,8 @@ TEST(PdfInkUndoRedoModelTest, DrawDrawUndoEraseUndo) {
   EXPECT_THAT(PdfInkUndoRedoModel::GetEraseCommands(commands).value(),
               ElementsAreArray({4, 8}));
 
-  ASSERT_TRUE(undo_redo.StartErase());
+  std::optional<DiscardedDrawCommands> discards = undo_redo.StartErase();
+  ASSERT_THAT(discards, Optional(ElementsAreArray({4, 8})));
   ASSERT_TRUE(undo_redo.Erase(5));
   ASSERT_TRUE(undo_redo.FinishErase());
 
@@ -322,8 +340,10 @@ TEST(PdfInkUndoRedoModelTest, DISABLED_Stress) {
   }
 
   ASSERT_EQ(2 * kCycles, id);
+  ASSERT_TRUE(undo_redo.StartErase());
   for (size_t i = 0; i < kCycles; ++i) {
-    ASSERT_TRUE(undo_redo.StartErase());
+    std::optional<DiscardedDrawCommands> discards = undo_redo.StartErase();
+    ASSERT_THAT(discards, Optional(DiscardedDrawCommands()));
     ASSERT_TRUE(undo_redo.Erase(--id));
     ASSERT_TRUE(undo_redo.Erase(--id));
     ASSERT_TRUE(undo_redo.FinishErase());
@@ -347,7 +367,12 @@ TEST(PdfInkUndoRedoModelTest, DISABLED_Stress) {
                 ElementsAreArray({id, id + 1}));
   }
 
-  DoDrawCommandsCycle(undo_redo, {0});
+  std::vector<size_t> expected_discards(kCycles * 2);
+  std::iota(expected_discards.begin(), expected_discards.end(), 0);
+  std::optional<DiscardedDrawCommands> discards = undo_redo.StartDraw();
+  ASSERT_THAT(discards, Optional(ElementsAreArray(expected_discards)));
+  ASSERT_TRUE(undo_redo.Draw(0));
+  ASSERT_TRUE(undo_redo.FinishDraw());
 }
 
 }  // namespace