chromium/src
0
Fork 0
Code Activity

[PDF Ink Signatures] Support modeled shapes in PdfInkUndoRedoModel

Browse Source 
To make PdfInkUndoRedoModel work with modeled shapes in addition to
strokes:

1) Add PdfInkUndoRedoModel::IdType, which is a variant that can hold
   either an InkStrokeId or an InkModeledShapeId.
2) Switch PdfInkUndoRedoModel::DrawCommands and EraseCommands to use
   IdType.
3) Update PdfInkUndoRedoModel internals to support (1) and (2).
4) Rename PdfInkUndoRedoModel::Erase() to EraseStroke(). Update callers.
5) Add PdfInkUndoRedoModel::EraseShape(), which is similar to
   EraseStroke(), but not identical.
6) Add unit tests for EraseShape().

With PdfInkUndoRedoModel ready to support modeled shapes, update
PdfInkModule::ApplyUndoRedoCommandsHelper(). Prior to this CL, it only
had to deal with InkStrokeIds. Now it needs to separate out the
InkStrokeIds from the InkModeledShapeIds. The InkModeledShapeIds
handling remains a TODO for later, when PdfInkModule connects all the
pieces together.

Bug: 377820805
Change-Id: I87c158481a407c9401ceff1a4d78d3526a05fd33
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6004272
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Andy Phan <andyphan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1380772}
This commit is contained in:
Lei Zhang
2024-11-09 02:25:25 +00:00
committed by Chromium LUCI CQ
parent c81cdeb133
commit 36ca9de254
5 changed files with 140 additions and 39 deletions

30
pdf/pdf_ink_module.cc

@ -626,7 +626,7 @@ bool PdfInkModule::EraseHelper(const gfx::PointF& position, int page_index) {
invalidate_envelope.Add(shape.Bounds());
bool undo_redo_success = undo_redo_model_.Erase(stroke.id);
bool undo_redo_success = undo_redo_model_.EraseStroke(stroke.id);
CHECK(undo_redo_success);
}
@ -861,11 +861,25 @@ void PdfInkModule::ApplyUndoRedoCommands(
NOTREACHED();
}
void PdfInkModule::ApplyUndoRedoCommandsHelper(std::set<InkStrokeId> ids,
bool should_draw) {
void PdfInkModule::ApplyUndoRedoCommandsHelper(
std::set<PdfInkUndoRedoModel::IdType> ids,
bool should_draw) {
CHECK(!strokes_.empty());
CHECK(!ids.empty());
std::set<InkStrokeId> stroke_ids;
std::set<InkModeledShapeId> shape_ids;
for (PdfInkUndoRedoModel::IdType id : ids) {
bool inserted;
if (absl::holds_alternative<InkStrokeId>(id)) {
inserted = stroke_ids.insert(absl::get<InkStrokeId>(id)).second;
} else {
CHECK(absl::holds_alternative<InkModeledShapeId>(id));
inserted = shape_ids.insert(absl::get<InkModeledShapeId>(id)).second;
}
CHECK(inserted);
}
for (auto& [page_index, page_ink_strokes] : strokes_) {
std::vector<InkStrokeId> page_ids;
page_ids.reserve(page_ink_strokes.size());
@ -874,7 +888,7 @@ void PdfInkModule::ApplyUndoRedoCommandsHelper(std::set<InkStrokeId> ids,
}
std::vector<InkStrokeId> ids_to_apply_command;
base::ranges::set_intersection(ids, page_ids,
base::ranges::set_intersection(stroke_ids, page_ids,
std::back_inserter(ids_to_apply_command));
if (ids_to_apply_command.empty()) {
continue;
@ -895,7 +909,7 @@ void PdfInkModule::ApplyUndoRedoCommandsHelper(std::set<InkStrokeId> ids,
invalidate_envelope.Add(stroke.stroke.GetShape().Bounds());
ids.erase(id);
stroke_ids.erase(id);
}
client_->Invalidate(CanonicalInkEnvelopeToExpandedInvalidationScreenRect(
@ -903,10 +917,12 @@ void PdfInkModule::ApplyUndoRedoCommandsHelper(std::set<InkStrokeId> ids,
client_->GetPageContentsRect(page_index), client_->GetZoom()));
client_->UpdateThumbnail(page_index);
if (ids.empty()) {
return; // Return early if there is nothing left to apply.
if (stroke_ids.empty()) {
break; // Break out of loop if there is no stroke remaining to apply.
}
}
// TODO(crbug.com/377820805): Handle `shape_ids`.
}
void PdfInkModule::ApplyUndoRedoDiscards(

3
pdf/pdf_ink_module.h

@ -326,7 +326,8 @@ class PdfInkModule {
base::TimeTicks timestamp);
void ApplyUndoRedoCommands(const PdfInkUndoRedoModel::Commands& commands);
void ApplyUndoRedoCommandsHelper(std::set<InkStrokeId> ids, bool should_draw);
void ApplyUndoRedoCommandsHelper(std::set<PdfInkUndoRedoModel::IdType> ids,
bool should_draw);
void ApplyUndoRedoDiscards(
const PdfInkUndoRedoModel::DiscardedDrawCommands& discards);

31
pdf/pdf_ink_undo_redo_model.cc

@ -84,7 +84,7 @@ PdfInkUndoRedoModel::StartErase() {
return StartImpl<EraseCommands>();
}
bool PdfInkUndoRedoModel::Erase(InkStrokeId id) {
bool PdfInkUndoRedoModel::EraseStroke(InkStrokeId id) {
CHECK(!commands_stack_.empty());
if (!IsAtTopOfStackWithGivenCommandType(CommandsType::kErase)) {
@ -103,6 +103,22 @@ bool PdfInkUndoRedoModel::Erase(InkStrokeId id) {
return true;
}
bool PdfInkUndoRedoModel::EraseShape(InkModeledShapeId id) {
CHECK(!commands_stack_.empty());
if (!IsAtTopOfStackWithGivenCommandType(CommandsType::kErase)) {
// Can only erase at top of the stack, and the entry there must be for
// erasing.
return false;
}
if (HasIdInEraseCommands(id)) {
return false; // Failed invariant 5.
}
GetModifiableEraseCommands(commands_stack_.back())->insert(id);
return true;
}
bool PdfInkUndoRedoModel::FinishErase() {
CHECK(!commands_stack_.empty());
@ -140,14 +156,14 @@ PdfInkUndoRedoModel::Commands PdfInkUndoRedoModel::Undo() {
}
case CommandsType::kDraw: {
EraseCommands result;
for (InkStrokeId id : GetDrawCommands(commands).value()) {
for (IdType id : GetDrawCommands(commands).value()) {
result->insert(id);
}
return result;
}
case CommandsType::kErase: {
DrawCommands result;
for (InkStrokeId id : GetEraseCommands(commands).value()) {
for (IdType id : GetEraseCommands(commands).value()) {
result->insert(id);
}
return result;
@ -226,8 +242,9 @@ PdfInkUndoRedoModel::StartImpl() {
// 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 (InkStrokeId id : GetDrawCommands(commands_stack_[i]).value()) {
bool inserted = discarded_commands.insert(id).second;
for (IdType id : GetDrawCommands(commands_stack_[i]).value()) {
bool inserted =
discarded_commands.insert(absl::get<InkStrokeId>(id)).second;
CHECK(inserted);
}
}
@ -257,7 +274,7 @@ bool PdfInkUndoRedoModel::IsAtTopOfStackWithGivenCommandType(
return GetCommandsType(commands_stack_.back()) == type;
}
bool PdfInkUndoRedoModel::HasIdInDrawCommands(InkStrokeId id) const {
bool PdfInkUndoRedoModel::HasIdInDrawCommands(IdType id) const {
for (const auto& commands : commands_stack_) {
if (GetCommandsType(commands) == CommandsType::kDraw &&
GetDrawCommands(commands)->contains(id)) {
@ -267,7 +284,7 @@ bool PdfInkUndoRedoModel::HasIdInDrawCommands(InkStrokeId id) const {
return false;
}
bool PdfInkUndoRedoModel::HasIdInEraseCommands(InkStrokeId id) const {
bool PdfInkUndoRedoModel::HasIdInEraseCommands(IdType id) const {
for (const auto& commands : commands_stack_) {
if (GetCommandsType(commands) == CommandsType::kErase &&
GetEraseCommands(commands)->contains(id)) {

44
pdf/pdf_ink_undo_redo_model.h

@ -31,15 +31,21 @@ class PdfInkUndoRedoModel {
kErase,
};
// Set of IDs to draw/erase.
// Set of IDs to draw/erase. There are multiple types of IDs:
// - `InkStrokeId` is for strokes that are first drawn, and maybe erased
// later.
// - `InkModeledShapeId` is for modeled shapes that are pre-existing and can
// be erased.
using IdType = absl::variant<InkStrokeId, InkModeledShapeId>;
using DrawCommands =
base::StrongAlias<class DrawCommandsTag, std::set<InkStrokeId>>;
base::StrongAlias<class DrawCommandsTag, std::set<IdType>>;
using EraseCommands =
base::StrongAlias<class EraseCommandsTag, std::set<InkStrokeId>>;
base::StrongAlias<class EraseCommandsTag, std::set<IdType>>;
using Commands = absl::variant<absl::monostate, DrawCommands, EraseCommands>;
// Set of IDs used for drawing to discard.
// Set of IDs used for drawing to discard. This does not use `IdType`, because
// model shapes are pre-existing and cannot be discarded.
using DiscardedDrawCommands = std::set<InkStrokeId>;
PdfInkUndoRedoModel();
@ -48,12 +54,12 @@ class PdfInkUndoRedoModel {
~PdfInkUndoRedoModel();
// For all Draw / Erase methods:
// - The expected usage is: 1 StartOp call, any number of Op calls, 1 FinishOp
// call.
// - The expected usage is: 1 StartOp call, any number of Op(Variant) calls,
// 1 FinishOp call.
// - 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.
// - Op(Variant) 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.
@ -76,14 +82,22 @@ class PdfInkUndoRedoModel {
// not at the top of the stack, then this discards all entries from the
// 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 be called before EraseStroke() and EraseShape().
// Must not be called while another draw/erase has been started.
[[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.
// `id` must not be in any `EraseCommands` on the commands stack.
[[nodiscard]] bool Erase(InkStrokeId id);
[[nodiscard]] bool EraseStroke(InkStrokeId id);
// Records erasing a shape identified by `id`.
// Must be called between StartErase() and FinishErase().
// `id` must not be in any `EraseCommands` on the commands stack.
// Unlike EraseStroke(), EraseShape() has no corresponding draw method, so it
// relies on the caller to pass in valid `id` values. If the caller passes in
// invalid values, `PdfInkUndoRedoModel` will faithfully give them back during
// undo/redo operations.
[[nodiscard]] bool EraseShape(InkModeledShapeId id);
// Finishes recording erase commands and pushes a new element onto the stack.
// Must be called after StartErase().
[[nodiscard]] bool FinishErase();
@ -103,8 +117,8 @@ class PdfInkUndoRedoModel {
std::optional<DiscardedDrawCommands> StartImpl();
bool IsAtTopOfStackWithGivenCommandType(CommandsType type) const;
bool HasIdInDrawCommands(InkStrokeId id) const;
bool HasIdInEraseCommands(InkStrokeId id) const;
bool HasIdInDrawCommands(IdType id) const;
bool HasIdInEraseCommands(IdType id) const;
// Invariants:
// (1) Never empty.
@ -116,10 +130,14 @@ class PdfInkUndoRedoModel {
// `EraseCommands` elements.
// (6) IDs added to a `EraseCommands` must exist in some `DrawCommands`
// element.
// (7) `DrawCommands` only contains `InkStrokeId` elements here. The reason
// `DrawCommands` can hold `InkModeledShapeId` is to undo an
// `InkModeledShapeId` erasure, where the caller needs to know they need
// to draw the shape.
std::vector<Commands> commands_stack_ = {absl::monostate()};
// Invariants:
// (7) Always less than the size of `commands_stack_`.
// (8) Always less than the size of `commands_stack_`.
size_t stack_position_ = 0;
};

71
pdf/pdf_ink_undo_redo_model_unittest.cc

@ -14,6 +14,7 @@
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
using testing::ElementsAre;
using testing::ElementsAreArray;
using testing::Optional;
@ -91,7 +92,7 @@ TEST(PdfInkUndoRedoModelTest, BadActionEraseWhileDrawing) {
ASSERT_TRUE(undo_redo.Draw(InkStrokeId(1)));
ASSERT_FALSE(undo_redo.StartErase());
ASSERT_FALSE(undo_redo.Erase(InkStrokeId(1)));
ASSERT_FALSE(undo_redo.EraseStroke(InkStrokeId(1)));
ASSERT_FALSE(undo_redo.FinishErase());
}
@ -104,7 +105,8 @@ TEST(PdfInkUndoRedoModelTest, BadActionDoubleStartErase) {
TEST(PdfInkUndoRedoModelTest, BadActionSpuriousErase) {
PdfInkUndoRedoModel undo_redo;
ASSERT_FALSE(undo_redo.Erase(InkStrokeId(1)));
ASSERT_FALSE(undo_redo.EraseStroke(InkStrokeId(1)));
ASSERT_FALSE(undo_redo.EraseShape(InkModeledShapeId(2)));
}
TEST(PdfInkUndoRedoModelTest, BadActionSpuriousFinishErase) {
@ -157,7 +159,8 @@ TEST(PdfInkUndoRedoModelTest, BadActionSpuriousEraseAfterUndo) {
EXPECT_THAT(PdfInkUndoRedoModel::GetEraseCommands(commands).value(),
ElementsAreArray({InkStrokeId(4)}));
ASSERT_FALSE(undo_redo.Erase(InkStrokeId(4)));
ASSERT_FALSE(undo_redo.EraseStroke(InkStrokeId(4)));
ASSERT_FALSE(undo_redo.EraseShape(InkModeledShapeId(9)));
}
TEST(PdfInkUndoRedoModelTest, BadActionSpuriousFinishEraseAfterUndo) {
@ -178,7 +181,7 @@ TEST(PdfInkUndoRedoModelTest, BadActionEraseUnknownId) {
std::optional<DiscardedDrawCommands> discards = undo_redo.StartErase();
ASSERT_THAT(discards, Optional(DiscardedDrawCommands()));
ASSERT_FALSE(undo_redo.Erase(InkStrokeId(3)));
ASSERT_FALSE(undo_redo.EraseStroke(InkStrokeId(3)));
}
TEST(PdfInkUndoRedoModelTest, BadActionEraseTwice) {
@ -187,8 +190,10 @@ TEST(PdfInkUndoRedoModelTest, BadActionEraseTwice) {
std::optional<DiscardedDrawCommands> discards = undo_redo.StartErase();
ASSERT_THAT(discards, Optional(DiscardedDrawCommands()));
ASSERT_TRUE(undo_redo.Erase(InkStrokeId(0)));
ASSERT_FALSE(undo_redo.Erase(InkStrokeId(0)));
ASSERT_TRUE(undo_redo.EraseStroke(InkStrokeId(0)));
ASSERT_FALSE(undo_redo.EraseStroke(InkStrokeId(0)));
ASSERT_TRUE(undo_redo.EraseShape(InkModeledShapeId(0)));
ASSERT_FALSE(undo_redo.EraseShape(InkModeledShapeId(0)));
}
TEST(PdfInkUndoRedoModelTest, Empty) {
@ -308,8 +313,8 @@ TEST(PdfInkUndoRedoModelTest, DrawDrawEraseUndoRedo) {
std::optional<DiscardedDrawCommands> discards = undo_redo.StartErase();
ASSERT_THAT(discards, Optional(DiscardedDrawCommands()));
ASSERT_TRUE(undo_redo.Erase(InkStrokeId(1)));
ASSERT_TRUE(undo_redo.Erase(InkStrokeId(4)));
ASSERT_TRUE(undo_redo.EraseStroke(InkStrokeId(1)));
ASSERT_TRUE(undo_redo.EraseStroke(InkStrokeId(4)));
ASSERT_TRUE(undo_redo.FinishErase());
PdfInkUndoRedoModel::Commands commands = undo_redo.Undo();
@ -368,7 +373,7 @@ TEST(PdfInkUndoRedoModelTest, DrawDrawUndoEraseUndo) {
std::optional<DiscardedDrawCommands> discards = undo_redo.StartErase();
ASSERT_THAT(discards,
Optional(ElementsAreArray({InkStrokeId(4), InkStrokeId(8)})));
ASSERT_TRUE(undo_redo.Erase(InkStrokeId(5)));
ASSERT_TRUE(undo_redo.EraseStroke(InkStrokeId(5)));
ASSERT_TRUE(undo_redo.FinishErase());
commands = undo_redo.Undo();
@ -377,6 +382,50 @@ TEST(PdfInkUndoRedoModelTest, DrawDrawUndoEraseUndo) {
ElementsAreArray({InkStrokeId(5)}));
}
TEST(PdfInkUndoRedoModelTest, EraseShapesUndoRedo) {
PdfInkUndoRedoModel undo_redo;
std::optional<DiscardedDrawCommands> discards = undo_redo.StartErase();
ASSERT_THAT(discards, Optional(DiscardedDrawCommands()));
ASSERT_TRUE(undo_redo.EraseShape(InkModeledShapeId(0)));
ASSERT_TRUE(undo_redo.EraseShape(InkModeledShapeId(1)));
ASSERT_TRUE(undo_redo.FinishErase());
PdfInkUndoRedoModel::Commands commands = undo_redo.Undo();
ASSERT_EQ(kDraw, PdfInkUndoRedoModel::GetCommandsType(commands));
EXPECT_THAT(PdfInkUndoRedoModel::GetDrawCommands(commands).value(),
ElementsAre(InkModeledShapeId(0), InkModeledShapeId(1)));
commands = undo_redo.Redo();
ASSERT_EQ(kErase, PdfInkUndoRedoModel::GetCommandsType(commands));
EXPECT_THAT(PdfInkUndoRedoModel::GetEraseCommands(commands).value(),
ElementsAre(InkModeledShapeId(0), InkModeledShapeId(1)));
}
TEST(PdfInkUndoRedoModelTest, DrawDrawEraseStrokesAndShapesUndoRedo) {
PdfInkUndoRedoModel undo_redo;
DoDrawCommandsCycle(undo_redo, {InkStrokeId(5)});
DoDrawCommandsCycle(undo_redo, {InkStrokeId(4), InkStrokeId(8)});
std::optional<DiscardedDrawCommands> discards = undo_redo.StartErase();
ASSERT_THAT(discards, Optional(DiscardedDrawCommands()));
ASSERT_TRUE(undo_redo.EraseShape(InkModeledShapeId(0)));
ASSERT_TRUE(undo_redo.EraseStroke(InkStrokeId(4)));
ASSERT_TRUE(undo_redo.EraseShape(InkModeledShapeId(1)));
ASSERT_TRUE(undo_redo.FinishErase());
PdfInkUndoRedoModel::Commands commands = undo_redo.Undo();
ASSERT_EQ(kDraw, PdfInkUndoRedoModel::GetCommandsType(commands));
EXPECT_THAT(
PdfInkUndoRedoModel::GetDrawCommands(commands).value(),
ElementsAre(InkStrokeId(4), InkModeledShapeId(0), InkModeledShapeId(1)));
commands = undo_redo.Redo();
ASSERT_EQ(kErase, PdfInkUndoRedoModel::GetCommandsType(commands));
EXPECT_THAT(
PdfInkUndoRedoModel::GetEraseCommands(commands).value(),
ElementsAre(InkStrokeId(4), InkModeledShapeId(0), InkModeledShapeId(1)));
}
TEST(PdfInkUndoRedoModelTest, Stress) {
#if defined(NDEBUG)
constexpr size_t kCycles = 10000;
@ -396,8 +445,8 @@ TEST(PdfInkUndoRedoModelTest, Stress) {
for (size_t i = 0; i < kCycles; ++i) {
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.EraseStroke(--id));
ASSERT_TRUE(undo_redo.EraseStroke(--id));
ASSERT_TRUE(undo_redo.FinishErase());
}