diff --git a/src/odb/src/db/dbBlock.h b/src/odb/src/db/dbBlock.h index 8c0eba09e05..65ef5e51290 100644 --- a/src/odb/src/db/dbBlock.h +++ b/src/odb/src/db/dbBlock.h @@ -328,8 +328,17 @@ class _dbBlock : public _dbObject std::list callbacks_; void* extmi_; + struct JournalStackEntry + { + dbJournal* journal; + // True when a nested beginEco suspended this journal; it resumes + // recording once the nested ECO is committed or undone. False when + // endEco stopped it. + bool resume; + }; + dbJournal* journal_; - std::stack journal_stack_; + std::stack journal_stack_; }; dbOStream& operator<<(dbOStream& stream, const _dbBlock& block); diff --git a/src/odb/src/db/dbDatabase.cpp b/src/odb/src/db/dbDatabase.cpp index 32dd8d48fb8..c03fceb2ed0 100644 --- a/src/odb/src/db/dbDatabase.cpp +++ b/src/odb/src/db/dbDatabase.cpp @@ -940,11 +940,27 @@ void dbDatabase::write(std::ostream& file) file.flush(); } +// Make the enclosing ECO the active journal again if a nested beginEco +// suspended it, so later edits are recorded and can be undone with it. +static void resumeSuspendedEco(_dbBlock* block) +{ + if (block->journal_stack_.empty() || !block->journal_stack_.top().resume) { + return; + } + assert(block->journal_ == nullptr); + assert(block->journal_stack_.top().journal != nullptr); + block->journal_ = block->journal_stack_.top().journal; + block->journal_stack_.pop(); +} + void dbDatabase::beginEco(dbBlock* block_) { _dbBlock* block = (_dbBlock*) block_; if (block->journal_) { - endEco(block_); + // Suspend the enclosing ECO; it resumes when this one is committed + // or undone. + block->journal_stack_.push({block->journal_, true}); + block->journal_ = nullptr; } block->journal_ = new dbJournal(block_); assert(block->journal_); @@ -960,7 +976,7 @@ void dbDatabase::endEco(dbBlock* block_) { _dbBlock* block = (_dbBlock*) block_; assert(block->journal_); - block->journal_stack_.push(block->journal_); + block->journal_stack_.push({block->journal_, false}); block->journal_ = nullptr; debugPrint(block_->getImpl()->getLogger(), utl::ODB, @@ -968,7 +984,7 @@ void dbDatabase::endEco(dbBlock* block_) 2, "ECO: Ended ECO #{} (size {}) and pushed to ECO stack", block->journal_stack_.size() - 1, - block->journal_stack_.top()->size()); + block->journal_stack_.top().journal->size()); } void dbDatabase::commitEco(dbBlock* block_) @@ -977,11 +993,11 @@ void dbDatabase::commitEco(dbBlock* block_) // Commit the current ECO or the last ECO into stack assert(block->journal_ || !block->journal_stack_.empty()); if (!block->journal_) { - block->journal_ = block->journal_stack_.top(); + block->journal_ = block->journal_stack_.top().journal; block->journal_stack_.pop(); } if (!block->journal_stack_.empty()) { - dbJournal* prev_journal = block->journal_stack_.top(); + dbJournal* prev_journal = block->journal_stack_.top().journal; int old_size = prev_journal->size(); prev_journal->append(block->journal_); debugPrint(block_->getImpl()->getLogger(), @@ -993,7 +1009,7 @@ void dbDatabase::commitEco(dbBlock* block_) block->journal_->size(), block->journal_stack_.size() - 1, old_size, - block->journal_stack_.top()->size()); + prev_journal->size()); } else { debugPrint(block_->getImpl()->getLogger(), utl::ODB, @@ -1005,6 +1021,7 @@ void dbDatabase::commitEco(dbBlock* block_) } delete block->journal_; block->journal_ = nullptr; + resumeSuspendedEco(block); } void dbDatabase::undoEco(dbBlock* block_) @@ -1012,7 +1029,7 @@ void dbDatabase::undoEco(dbBlock* block_) _dbBlock* block = (_dbBlock*) block_; assert(block->journal_ || !block->journal_stack_.empty()); if (!block->journal_) { - block->journal_ = block->journal_stack_.top(); + block->journal_ = block->journal_stack_.top().journal; block->journal_stack_.pop(); } debugPrint(block_->getImpl()->getLogger(), @@ -1026,6 +1043,7 @@ void dbDatabase::undoEco(dbBlock* block_) block->journal_ = nullptr; journal->undo(); delete journal; + resumeSuspendedEco(block); } bool dbDatabase::ecoEmpty(dbBlock* block_) @@ -1084,7 +1102,7 @@ void dbDatabase::writeEco(dbBlock* block_, const char* filename) stream.flush(); } else if (!block->journal_stack_.empty()) { dbOStream stream(block->getDatabase(), file); - stream << *block->journal_stack_.top(); + stream << *block->journal_stack_.top().journal; stream.flush(); } } diff --git a/src/odb/test/cpp/TestJournal.cpp b/src/odb/test/cpp/TestJournal.cpp index d7c3bd1cca9..890a4956892 100644 --- a/src/odb/test/cpp/TestJournal.cpp +++ b/src/odb/test/cpp/TestJournal.cpp @@ -286,5 +286,59 @@ TEST_F(JournalFixture, RenameModNet) dbModule::destroy(module); } +TEST_F(JournalFixture, NestedUndoThenEditIsUndoneByOuter) +{ + dbInst::create(block, and2, "a"); + + dbDatabase::beginEco(block); // outer + dbDatabase::beginEco(block); // inner + dbInst::destroy(block->findInst("a")); + dbDatabase::undoEco(block); // inner undo recreates "a" + ASSERT_NE(block->findInst("a"), nullptr); + + // The outer ECO must record edits made after the inner one ends. + dbInst::destroy(block->findInst("a")); + dbInst::create(block, or2, "b"); + EXPECT_FALSE(dbDatabase::ecoEmpty(block)); + dbDatabase::undoEco(block); // outer undo + + EXPECT_NE(block->findInst("a"), nullptr); + EXPECT_EQ(block->findInst("b"), nullptr); + EXPECT_TRUE(dbDatabase::ecoStackEmpty(block)); +} + +TEST_F(JournalFixture, NestedCommitThenEditIsUndoneByOuter) +{ + dbDatabase::beginEco(block); // outer + dbDatabase::beginEco(block); // inner + dbInst::create(block, and2, "a"); + dbDatabase::commitEco(block); // inner merges into outer + + dbInst::create(block, or2, "b"); + dbDatabase::undoEco(block); // outer undo + + EXPECT_EQ(block->findInst("a"), nullptr); + EXPECT_EQ(block->findInst("b"), nullptr); + EXPECT_TRUE(dbDatabase::ecoStackEmpty(block)); +} + +TEST_F(JournalFixture, EndedEcoStaysStoppedAfterNestedCommit) +{ + dbDatabase::beginEco(block); + dbDatabase::endEco(block); // stop recording the first ECO + dbDatabase::beginEco(block); + dbInst::create(block, and2, "a"); + dbDatabase::commitEco(block); // merges into the stopped ECO + + // The stopped ECO does not resume recording. + EXPECT_FALSE(dbDatabase::ecoStackEmpty(block)); + dbInst::create(block, or2, "b"); + dbDatabase::undoEco(block); + + EXPECT_EQ(block->findInst("a"), nullptr); + EXPECT_NE(block->findInst("b"), nullptr); + EXPECT_TRUE(dbDatabase::ecoStackEmpty(block)); +} + } // namespace } // namespace odb