Refactor undoable commands - step 2#33785
Open
cbjeukendrup wants to merge 4 commits into
Open
Conversation
cbjeukendrup
force-pushed
the
undo-transactionsmanager
branch
2 times, most recently
from
d056909 to
4d51824
Compare
cbjeukendrup
force-pushed
the
undo-transactionsmanager
branch
2 times, most recently
from
f73a640 to
978e09e
Compare
miiizen
approved these changes
Jun 12, 2026
cbjeukendrup
force-pushed
the
undo-transactionsmanager
branch
from
978e09e to
7112a0b
Compare
miiizen
reviewed
Jun 12, 2026
| if (locked == isLocked()) { | ||
| return; | ||
| } | ||
| Transaction& tx = system()->score()->transactionManager()->currentOrDummyTransaction(); |
Contributor
There was a problem hiding this comment.
Just so I'm clear - eventually we will remove this line. Where will the transaction come from - will it have to be passed around every function that uses an UndoableCommand, or will we get it from somewhere else?
Collaborator
Author
There was a problem hiding this comment.
The idea is indeed to pass the Transaction around (hence the short "tx" name), to guarantee at compile time that a Transaction exists.
But there are exceptions. For example, here in the plugin API, we cannot pass it around, and there is actually no guarantee that the plugin author has started a transaction, so in this case specifically we probably do need a permanent successor to currentOrDummyTransaction.
In the layout code, we could make the transaction part of LayoutCtx, for convenience. (Then we do need to be sure that the Transaction outlives the LayoutCtx... I wish this was Rust.)
And something similar could be done in the reading code with ReadCtx.
Contributor
There was a problem hiding this comment.
Perfect, this sounds great!
mike-spa
approved these changes
Jun 15, 2026
This method allows safely performing an undoable transaction, without the possibility to forget ending the transaction or to start a nested transaction. The aim is to eliminate Score::undo, so that all undoable commands must be performed inside a transaction. When the new method is adopted widely, it will directly or indirectly help eliminate a lot of bad patterns currently seen around undoable transactions: * forgetting to start/end a transaction * performing undoable commands outside a transaction * not being able to tell whether a method is undoable or not * calling an undoable method without being aware, leading to overhead or unexpected references in the UndoStack * complex unsafe direct UndoStack manipulation, for example in text editing; by replacing ad-hoc manipulation with proper conceptual solutions, we can introduce safety for more complex behaviour too * there are too many sources of undo changes information, especially inside the Notation module Two temporary methods remain for compatibility purposes: * Score::undo for performing an undoable command * TransactionManager::currentOrDummyTransaction for calling methods that do need a `Transaction&` in a context that does not have it yet Concepts not yet (completely) covered: * failing transactions, i.e. a replacement for `endTransaction(rollback: true)`; ideally, this would ensure that after failing a transaction, no further commands can be pushed to it, and it * "long-running" transactions, for example like in the style dialog or during text editing or dragging; these will still have to use begin/endTransaction. That’s basically fine, but those methods can be made more strict, for example beginTransaction could return some object that must be given back to endTransaction, ensuring correctness. * the aforementioned complex unsafe direct UndoStack manipulations * “dummy transactions” will have to be promoted from a temporary compatibility solution into a real concept, for operations during score reading or in situations where one does not care about the undo stack at all * we need to figure out how the plugins/extensions framework can fit into safely, given that we must consider user plugins/extensions unsafe. Potential opportunities to explore in the future * Allowing to change the description/actionName mid-transaction via the Transaction type * Moving CmdState etc. out of MasterScore into Transaction * Moving changes channels into TransactionManager * Moving UndoStack ownership to TransactionManager * Nested UndoStacks for "long-running” operations, for example in the case of text editing: this should eliminate the need for UndoStack::mergeTransactions Shortcomings The new `transaction` method and `Transaction` type do help prevent calling undoable methods outside an undoable transaction, but they do not help prevent calling non-undoable methods at unexpected moments. For that, we would need another type that can only be constructed by a few trusted friend classes, and acts as a ‘password’ for calling non-undoable methods. The question is how far we should go with this though, because adding such thing to literally every simple setter anywhere in the module is not feasible. We might reserve it for a few ambiguous places only.
cbjeukendrup
force-pushed
the
undo-transactionsmanager
branch
from
7112a0b to
2f9c428
Compare
Contributor
📝 WalkthroughWalkthroughThis pull request introduces ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/engraving/editing/transaction/transaction.cpp (1)
34-62: ⚡ Quick win
std::moveon const reference members performs copies, not moves.The
changesparameter isconst UndoableTransaction::ChangesInfo&, sostd::moveon its members (lines 58-61) will not actually move—they will copy instead, sincestd::moveof a const lvalue yields a const rvalue reference, which binds to copy constructors.If the intent is to transfer ownership and avoid copies, the parameter should be passed by value or non-const rvalue reference.
♻️ Suggested fix
-static ScoreChanges buildScoreChanges(const CmdState& cmdState, const UndoableTransaction::ChangesInfo& changes) +static ScoreChanges buildScoreChanges(const CmdState& cmdState, UndoableTransaction::ChangesInfo changes)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engraving/editing/transaction/transaction.cpp` around lines 34 - 62, The `buildScoreChanges` function receives the `changes` parameter as a const reference, so calling `std::move` on its members (changedObjects, changedObjectTypes, changedPropertyIdSet, changedStyleIdSet) will result in copies rather than moves because const rvalue references bind to copy constructors. To enable actual move semantics and transfer ownership, change the parameter type from `const UndoableTransaction::ChangesInfo&` to `UndoableTransaction::ChangesInfo` (pass by value), allowing the function to take ownership of a local copy and move its members without further copies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/engraving/tests/measure_tests.cpp`:
- Around line 586-589: The test contains pointer chain dereferences that lack
proper fatal guards. Replace the non-fatal EXPECT_TRUE checks on lines 587 and
589 with fatal assertions using ASSERT_TRUE or ASSERT_NE to stop execution
immediately if a pointer is null. Specifically, add a fatal assertion to guard
the Measure pointer m before it is dereferenced in the chain call to
m->first(SegmentType::ChordRest)->next()->nextChordRest(0) on line 588, and add
another fatal assertion to guard the ChordRest pointer cr after it is assigned.
This ensures that null pointers are caught and prevent crashes from
dereferencing null values in the pointer chain.
In `@src/notation/internal/notationinteraction.cpp`:
- Around line 2358-2360: The `doFinishAddFretboardDiagram()` function is being
called within the outer `transaction()` block but internally uses `apply()`
which commits the transaction and starts a new one, creating nested transactions
and breaking the unified transaction boundary. Refactor the code to defer the
`doFinishAddFretboardDiagram()` follow-up logic until after the outer palette
transaction completes, or restructure the fretboard-diagram handling path to
perform all operations within the single transaction without internal
commit/apply calls in between.
- Around line 6143-6148: The issue is that within the Transpose::transpose
function called inside the transaction lambda, when transposeNote() returns
false (indicating invalid pitch), the result flag is set to false but the
function continues executing and pushing mutations via
score->undoChangeKeySig(), transposeFretDiagram(), transposeHarmony(), and
score->undoAddElement(). These partial mutations then get committed when the
transaction closes, even though the operation failed. To fix this, modify the
Transpose::transpose function to return early as soon as a failure is detected
(when transposeNote() returns false or when any validation fails), rather than
continuing to push additional mutations. This ensures that either all
transposition mutations are successfully committed together, or none are
committed if an error occurs.
---
Nitpick comments:
In `@src/engraving/editing/transaction/transaction.cpp`:
- Around line 34-62: The `buildScoreChanges` function receives the `changes`
parameter as a const reference, so calling `std::move` on its members
(changedObjects, changedObjectTypes, changedPropertyIdSet, changedStyleIdSet)
will result in copies rather than moves because const rvalue references bind to
copy constructors. To enable actual move semantics and transfer ownership,
change the parameter type from `const UndoableTransaction::ChangesInfo&` to
`UndoableTransaction::ChangesInfo` (pass by value), allowing the function to
take ownership of a local copy and move its members without further copies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a03c2b8c-c9db-43c3-adf8-d32a02a0f527
📒 Files selected for processing (53)
src/engraving/CMakeLists.txtsrc/engraving/api/v1/elements.cppsrc/engraving/api/v1/score.cppsrc/engraving/dom/chordrest.cppsrc/engraving/dom/engravingobject.cppsrc/engraving/dom/excerpt.cppsrc/engraving/dom/instrchange.cppsrc/engraving/dom/masterscore.cppsrc/engraving/dom/masterscore.hsrc/engraving/dom/measure.cppsrc/engraving/dom/score.cppsrc/engraving/dom/score.hsrc/engraving/dom/slur.cppsrc/engraving/dom/unrollrepeats.cppsrc/engraving/editing/cmd.cppsrc/engraving/editing/edit.cppsrc/engraving/editing/editnote.cppsrc/engraving/editing/editpart.cppsrc/engraving/editing/editstavesharing.cppsrc/engraving/editing/editstavesharing.hsrc/engraving/editing/editstyle.cppsrc/engraving/editing/editsystemlocks.cppsrc/engraving/editing/editsystemlocks.hsrc/engraving/editing/splitjoinmeasure.cppsrc/engraving/editing/splitjoinmeasure.hsrc/engraving/editing/transaction/transaction.cppsrc/engraving/editing/transaction/transaction.hsrc/engraving/editing/transaction/undostack.cppsrc/engraving/editing/transaction/undostack.hsrc/engraving/editing/transpose.cppsrc/engraving/editing/transpose.hsrc/engraving/qml/MuseScore/Engraving/devtools/corruptscoredevtoolsmodel.cppsrc/engraving/rendering/score/layoutcontext.cppsrc/engraving/rw/read400/read400.cppsrc/engraving/rw/read410/read410.cppsrc/engraving/rw/read460/read460.cppsrc/engraving/rw/read500/read500.cppsrc/engraving/tests/chordsymbol_tests.cppsrc/engraving/tests/join_tests.cppsrc/engraving/tests/keysig_tests.cppsrc/engraving/tests/measure_tests.cppsrc/engraving/tests/split_tests.cppsrc/engraving/tests/stavesharing_tests.cppsrc/engraving/tests/system_locks_tests.cppsrc/engraving/tests/tab_transpose_tests.cppsrc/engraving/tests/transpose_tests.cppsrc/notation/internal/inotationundostack.hsrc/notation/internal/masternotationparts.cppsrc/notation/internal/notationinteraction.cppsrc/notation/internal/notationinteraction.hsrc/notation/internal/notationparts.cppsrc/notation/internal/notationundostack.cppsrc/notation/internal/notationundostack.h
💤 Files with no reviewable changes (2)
- src/engraving/editing/transaction/undostack.cpp
- src/engraving/editing/transaction/undostack.h
Comment on lines
+586
to
+589
| Measure* m = score->firstMeasure()->nextMeasure(); | ||
| EXPECT_TRUE(m); | ||
| ChordRest* cr = m->first(SegmentType::ChordRest)->next()->nextChordRest(0); | ||
| EXPECT_TRUE(cr); |
Contributor
There was a problem hiding this comment.
Use fatal guards before pointer-chain dereferences in this test block.
Line 588 dereferences a chain before hard null guards, and the EXPECT_TRUE checks on Lines 587 and 589 are non-fatal, so execution can continue into a crash. Use ASSERT_NE (or equivalent) with stepwise checks here.
Proposed fix
- Measure* m = score->firstMeasure()->nextMeasure();
- EXPECT_TRUE(m);
- ChordRest* cr = m->first(SegmentType::ChordRest)->next()->nextChordRest(0);
- EXPECT_TRUE(cr);
+ Measure* m = score->firstMeasure()->nextMeasure();
+ ASSERT_NE(m, nullptr);
+ Segment* firstCrSeg = m->first(SegmentType::ChordRest);
+ ASSERT_NE(firstCrSeg, nullptr);
+ Segment* nextSeg = firstCrSeg->next();
+ ASSERT_NE(nextSeg, nullptr);
+ ChordRest* cr = nextSeg->nextChordRest(0);
+ ASSERT_NE(cr, nullptr);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Measure* m = score->firstMeasure()->nextMeasure(); | |
| EXPECT_TRUE(m); | |
| ChordRest* cr = m->first(SegmentType::ChordRest)->next()->nextChordRest(0); | |
| EXPECT_TRUE(cr); | |
| Measure* m = score->firstMeasure()->nextMeasure(); | |
| ASSERT_NE(m, nullptr); | |
| Segment* firstCrSeg = m->first(SegmentType::ChordRest); | |
| ASSERT_NE(firstCrSeg, nullptr); | |
| Segment* nextSeg = firstCrSeg->next(); | |
| ASSERT_NE(nextSeg, nullptr); | |
| ChordRest* cr = nextSeg->nextChordRest(0); | |
| ASSERT_NE(cr, nullptr); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/engraving/tests/measure_tests.cpp` around lines 586 - 589, The test
contains pointer chain dereferences that lack proper fatal guards. Replace the
non-fatal EXPECT_TRUE checks on lines 587 and 589 with fatal assertions using
ASSERT_TRUE or ASSERT_NE to stop execution immediately if a pointer is null.
Specifically, add a fatal assertion to guard the Measure pointer m before it is
dereferenced in the chain call to
m->first(SegmentType::ChordRest)->next()->nextChordRest(0) on line 588, and add
another fatal assertion to guard the ChordRest pointer cr after it is assigned.
This ensures that null pointers are caught and prevent crashes from
dereferencing null values in the pointer chain.
Collaborator
Author
There was a problem hiding this comment.
Although this makes sense, this is an existing and more widespread problem, so not really in scope for this PR. But we should clean it up in the future.
Contributor
There was a problem hiding this comment.
@cbjeukendrup, understood! That's a fair call — addressing it piecemeal in every PR would be noisy. Would you like me to open a GitHub issue to track the cleanup of non-fatal EXPECT_TRUE pointer guards in the engraving test suite, so it doesn't get forgotten?
🧠 Learnings used
Learnt from: igorkorsukov
Repo: musescore/MuseScore PR: 32874
File: src/appshell/widgets/splashscreen/loadingscreenview.cpp:32-32
Timestamp: 2026-04-01T07:34:44.168Z
Learning: In the MuseScore C++ codebase, `IApplication::fullVersion()` (non-static virtual instance method in `src/framework/global/iapplication.h`) must only be called after application/services initialization. Do not use it in code paths that run before services are ready (e.g., splash/loading screens). If you need the app version via service-free static access in early UI/boot code, call `muse::BaseApplication::appFullVersion()` instead (static method declared in `src/framework/global/internal/baseapplication.h`), which reads compile-time constants.
Learnt from: igorkorsukov
Repo: musescore/MuseScore PR: 32950
File: src/framework/testing/environment.cpp:65-65
Timestamp: 2026-04-09T08:32:46.263Z
Learning: When using MuseScore’s IoC container (e.g., `muse::modularity::globalIoc()->registerExport<T>(name, impl)`), treat the `name` argument as the *registering module* identifier used for informational/tracing purposes. It does not have to match the key that consumers use to resolve the service later. In reviews, do not flag cases where the registration key (module name) differs from the resolution/lookup key as a bug, as this mismatch can be intentional (e.g., `IApplication` registered under `"testing"` in `src/framework/testing/environment.cpp`).
Learnt from: igorkorsukov
Repo: musescore/MuseScore PR: 33000
File: src/framework/audio/main/audiomodule.cpp:154-154
Timestamp: 2026-04-14T09:30:22.267Z
Learning: When registering exports via MuseScore’s IoC container (`muse::modularity`), it is acceptable to pass a raw pointer created with `new` to `ioc()->registerExport<T>(name, ...)`. The container will wrap it internally into a `std::shared_ptr`, so calls like `ioc()->registerExport<T>(name, new Impl(...))` are not a bug and should not be flagged for ownership/style issues. Prefer clarity, but do not treat raw `new` in `registerExport` arguments as an error; `ioc()->registerExport<T>(name, std::make_shared<Impl>(...))` is also valid.
Learnt from: Jojo-Schmitz
Repo: musescore/MuseScore PR: 33028
File: src/project/internal/opensaveprojectscenario.cpp:477-477
Timestamp: 2026-04-16T10:04:41.682Z
Learning: In the MuseScore C++ codebase, `configuration()->supportForumUrl()` intentionally returns a MuseScore Studio/app support URL on `musescore.org` (e.g., `/forum/...`, language-specific). When adding user-facing help text or help-button links for cloud-related errors (issues involving saving/uploading via `musescore.com`), ensure the help URL points to `musescore.com` (not `musescore.org`). Use `musescore.org` for Studio/app support problems and `musescore.com` for cloud-saving/upload issues.
Learnt from: saintmatthieu
Repo: musescore/MuseScore PR: 33036
File: src/framework/uicomponents/qml/Muse/UiComponents/tablesortfilterproxymodel.cpp:101-105
Timestamp: 2026-04-21T09:36:52.350Z
Learning: This repository guarantees Qt 6.10 as the minimum supported version. Therefore, when reviewing C++ code, do not require compatibility guards (e.g., QT_VERSION_CHECK) or fallback paths for Qt 6.10+ APIs/classes/enums. For example, do not flag uses of QSortFilterProxyModel::endFilterChange or QSortFilterProxyModel::Direction::Rows as needing guards for older Qt versions; such guards are unnecessary in this codebase.
Learnt from: Jojo-Schmitz
Repo: musescore/MuseScore PR: 33499
File: src/engraving/rendering/single/singledraw.cpp:1167-1169
Timestamp: 2026-05-20T11:29:20.381Z
Learning: When reviewing MSVC compiler warning fixes (notably warning C5054) in the engraving rendering C++ code, only flag `drawText`/painter call sites that bitwise-OR (`operator|`) `muse::draw` flags coming from *different* enum types unless the combination uses an explicit `static_cast<int>`. If the OR’d enumerators are all from the *same* enum type, they do not trigger C5054, so don’t require preemptive consistency casts—limit changes/flags to patterns that actually generate the compiler warning.
Comment on lines
+2358
to
+2360
| transaction( | ||
| TranslatableString("undoableAction", "Apply palette element: %1").arg(element->typeUserName()), | ||
| [&, this](engraving::Transaction&) { |
Contributor
There was a problem hiding this comment.
Keep the fretboard follow-up out of the active palette transaction.
applyPaletteElement() now runs inside transaction(...), but doFinishAddFretboardDiagram() still commits via apply() and then starts a second transaction(...). When palette application reaches this path, it can end the active transaction early and/or create a nested transaction, breaking the single transaction boundary this refactor is meant to guarantee. Defer the chord-symbol follow-up until after the outer palette transaction completes, or keep this fretboard-diagram path on an explicit two-step transaction flow.
Also applies to: 3421-3431
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/notation/internal/notationinteraction.cpp` around lines 2358 - 2360, The
`doFinishAddFretboardDiagram()` function is being called within the outer
`transaction()` block but internally uses `apply()` which commits the
transaction and starts a new one, creating nested transactions and breaking the
unified transaction boundary. Refactor the code to defer the
`doFinishAddFretboardDiagram()` follow-up logic until after the outer palette
transaction completes, or restructure the fretboard-diagram handling path to
perform all operations within the single transaction without internal
commit/apply calls in between.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds on #33658, #33784