Added "Repeat Last Action" command#32928
Added "Repeat Last Action" command#32928joxeankoret wants to merge 1 commit intomusescore:masterfrom
Conversation
It basically lets redo the last action. I find it annoying to have to go to the palette to choose again some articulation or have to find some menu to re-apply some action to some bar or selection, specially when I have to do it many-many things for the same composition or arrangement I'm working on. With these changes, now I only need to do "Shift + R" for my new selection and it makes it much more efficient for my workflow. Hopefully it's also useful for somebody else.
📝 WalkthroughWalkthroughThis change introduces a "repeat last action" feature across the application framework. It adds keyboard shortcut 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framework/actions/actiontypes.h`:
- Around line 44-48: The static uint64_t counter in nextActionSequence() is not
thread-safe; replace it with a std::atomic<uint64_t> (e.g., make counter an
atomic<uint64_t>) and perform an atomic increment (fetch_add(1) or ++ with
appropriate memory ordering) inside nextActionSequence() so concurrent calls are
safe and undefined behavior is avoided; also add the necessary <atomic> include
and keep the function signature nextActionSequence() unchanged.
In `@src/framework/actions/iactionsdispatcher.h`:
- Around line 58-61: Add a brief documentation comment to the dispatcher
interface clarifying how callers should detect absence of a repeatable action:
state that lastRepeatableAction() returns an empty/invalid ActionCode when none
exists and lastRepeatableActionSequence() returns 0 in that case, and mention
that repeatLastAction() should be called only when a repeatable action is
present; reference the existing methods lastRepeatableAction(),
lastRepeatableActionSequence(), and repeatLastAction() and mirror the intent of
INotationInteraction::canReapplyLastPaletteElement() for clarity.
In `@src/framework/actions/internal/actionsdispatcher.cpp`:
- Around line 217-281: ActionsDispatcher::isRepeatable currently keeps a
hardcoded static unordered_set repeatableActions which will drift; replace this
by consulting a single source of truth in the action registry/config (e.g., add
a repeatable metadata flag to your existing Action or ActionDescriptor and
expose a lookup like ActionRegistry::isRepeatable(const ActionCode&)). Change
isRepeatable to return ActionRegistry::isRepeatable(code) (or query
Action(code).descriptor().isRepeatable()) and remove the static
repeatableActions set; optionally keep the set as a temporary fallback only
while migrating, but ensure the canonical metadata lives in the
registry/ActionDescriptor so new actions automatically honor repeatability.
In `@src/notation/internal/notationinteraction.cpp`:
- Around line 2365-2367: Repeat currently loses original keyboard modifiers when
storing palette actions; extend NotationInteraction to save and replay
modifiers: add a member Qt::KeyboardModifiers m_lastPaletteModifiers (default
{}) and set it where you call m_lastPaletteElement.reset(element->clone()) and
m_lastPaletteElementSequence = muse::actions::nextActionSequence(); then update
the replay path that calls applyPaletteElement(m_lastPaletteElement.get()) to
pass or apply m_lastPaletteModifiers so the palette action is executed with the
original Ctrl/Shift state (ensure any calls to applyPaletteElement or its
wrapper accept/forward the modifiers).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 8bfd5fff-34e0-47b3-9ffd-21e6d4ecfa56
📒 Files selected for processing (13)
src/app/configs/data/shortcuts.xmlsrc/appshell/qml/MuseScore/AppShell/appmenumodel.cppsrc/framework/actions/actiontypes.hsrc/framework/actions/iactionsdispatcher.hsrc/framework/actions/internal/actionsdispatcher.cppsrc/framework/actions/internal/actionsdispatcher.hsrc/framework/actions/tests/mocks/actionsdispatchermock.hsrc/notation/inotationinteraction.hsrc/notation/internal/notationinteraction.cppsrc/notation/internal/notationinteraction.hsrc/notation/tests/mocks/notationinteractionmock.hsrc/notationscene/internal/notationactioncontroller.cppsrc/notationscene/internal/notationuiactions.cpp
| inline uint64_t nextActionSequence() | ||
| { | ||
| static uint64_t counter = 0; | ||
| return ++counter; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's confirm the file exists and check the actual content at lines 44-48
cd /repo && wc -l src/framework/actions/actiontypes.hRepository: musescore/MuseScore
Length of output: 118
🏁 Script executed:
# Read the relevant section of the file
cd /repo && sed -n '40,55p' src/framework/actions/actiontypes.hRepository: musescore/MuseScore
Length of output: 118
🏁 Script executed:
# Search for all usages of nextActionSequence to understand calling context
cd /repo && rg -n "nextActionSequence" --type cpp --type hRepository: musescore/MuseScore
Length of output: 118
🏁 Script executed:
# Check for any threading/concurrency includes or context in the file
cd /repo && head -40 src/framework/actions/actiontypes.hRepository: musescore/MuseScore
Length of output: 118
🏁 Script executed:
# Search for mutex, lock, or atomic usage patterns related to action sequences
cd /repo && rg -n "mutex|atomic|thread|lock" src/framework/actions/ --type cpp --type h -iRepository: musescore/MuseScore
Length of output: 118
🏁 Script executed:
# Check file size and read the relevant section
wc -l src/framework/actions/actiontypes.hRepository: musescore/MuseScore
Length of output: 102
🏁 Script executed:
# Read lines 40-55 to see the function context
sed -n '40,55p' src/framework/actions/actiontypes.hRepository: musescore/MuseScore
Length of output: 350
🏁 Script executed:
# Search for all usages of nextActionSequence
rg -n "nextActionSequence" --type cpp --type hRepository: musescore/MuseScore
Length of output: 375
🏁 Script executed:
# Check the beginning of the file for includes and context
head -40 src/framework/actions/actiontypes.hRepository: musescore/MuseScore
Length of output: 1275
🏁 Script executed:
# Search for threading/concurrency patterns in the framework actions directory
rg -n "mutex|atomic|thread|lock|std::thread" src/framework/actions/ --type cpp --type h -i | head -30Repository: musescore/MuseScore
Length of output: 45
🏁 Script executed:
# Check the actionsdispatcher context where nextActionSequence is called
sed -n '130,150p' src/framework/actions/internal/actionsdispatcher.cppRepository: musescore/MuseScore
Length of output: 596
🏁 Script executed:
# Check more context in actionsdispatcher.cpp - class definition
rg -n "class.*ActionDispatcher|nextActionSequence" src/framework/actions/internal/actionsdispatcher.cpp -B 5 -A 5Repository: musescore/MuseScore
Length of output: 408
🏁 Script executed:
# Check the notationinteraction context
sed -n '2360,2375p' src/notation/internal/notationinteraction.cppRepository: musescore/MuseScore
Length of output: 373
🏁 Script executed:
# Check if MuseScore has threading infrastructure - look for thread pools, worker threads
rg -n "QThread|std::thread|ThreadPool|Worker" src/framework/ --type cpp --type h | head -20Repository: musescore/MuseScore
Length of output: 2373
🏁 Script executed:
# Check if the actionsdispatcher or notationinteraction are used in threaded contexts
rg -n "moveToThread|emit|signal|Worker" src/framework/actions/internal/actionsdispatcher.cpp src/notation/internal/notationinteraction.cpp --type cpp -B 2 -A 2 | head -40Repository: musescore/MuseScore
Length of output: 670
🏁 Script executed:
# Check ActionDispatcher dispatch method signature and class
rg -n "class ActionDispatcher|void dispatch\(" src/framework/actions/internal/actionsdispatcher.h src/framework/actions/internal/actionsdispatcher.cpp -B 3 -A 8 | head -50Repository: musescore/MuseScore
Length of output: 1538
🏁 Script executed:
# Search for calls to dispatch() from async/worker thread contexts
rg -n "taskScheduler|async|thread|worker" src/framework/actions/internal/actionsdispatcher.cpp --type cpp -B 2 -A 2Repository: musescore/MuseScore
Length of output: 179
🏁 Script executed:
# Check NotationInteraction class for thread affinity hints
rg -n "class NotationInteraction|mainThreadOnly|threadSafe|mutex" src/notation/internal/notationinteraction.h -B 2 -A 5 | head -40Repository: musescore/MuseScore
Length of output: 570
🏁 Script executed:
# Search for how NotationInteraction methods are invoked - from slots, from main thread, etc.
rg -n "NotationInteraction|Q_SLOT|qobject_cast|QThread" src/notation/internal/notationinteraction.cpp --type cpp | head -20Repository: musescore/MuseScore
Length of output: 1374
🏁 Script executed:
# Check if there are any thread safety comments or annotations in the codebase
rg -n "thread.safe|thread-safe|not.thread.safe|data.race|synchroniz" src/framework/actions/ --type cpp --type h -i | head -20Repository: musescore/MuseScore
Length of output: 45
🏁 Script executed:
# Check if ActionDispatcher has thread affinity constraints or Qt thread enforcement
rg -n "mainThread|moveToThread|QThread|Q_OBJECT" src/framework/actions/internal/actionsdispatcher.h src/framework/actions/internal/actionsdispatcher.cpp --type cpp --type hRepository: musescore/MuseScore
Length of output: 45
🏁 Script executed:
# Search for how dispatch is invoked - look for calls to dispatch
rg -n "\.dispatch\(" src/ --type cpp | head -30Repository: musescore/MuseScore
Length of output: 45
🏁 Script executed:
# Check if there's any runtime thread ID verification in actionsdispatcher
rg -n "thread|Thread|THREAD" src/framework/actions/internal/actionsdispatcher.cpp --type cpp -i -B 2 -A 2Repository: musescore/MuseScore
Length of output: 45
🏁 Script executed:
# Check the async module for thread safety guarantees
rg -n "mainThread|affinity|singleThread" src/framework/global/async/ --type cpp --type h | head -20Repository: musescore/MuseScore
Length of output: 45
🏁 Script executed:
# Look for any Qt integration with ActionDispatcher (Qt objects are main-thread)
rg -n "QObject|Q_OBJECT|signals|slots" src/framework/actions/internal/actionsdispatcher.h src/framework/actions/internal/actionsdispatcher.cpp --type cpp --type hRepository: musescore/MuseScore
Length of output: 45
🏁 Script executed:
# Broader search for dispatch calls without the parenthesis
rg -n "dispatch" src/framework/actions/ --type cpp --type h | head -30Repository: musescore/MuseScore
Length of output: 3125
🏁 Script executed:
# Check ActionDispatcher instantiation and usage
rg -n "ActionsDispatcher\|ActionDispatcher" src/ --type cpp | head -20Repository: musescore/MuseScore
Length of output: 45
🏁 Script executed:
# Look at the header to see if there are any comments about thread safety
cat src/framework/actions/internal/actionsdispatcher.hRepository: musescore/MuseScore
Length of output: 2713
🏁 Script executed:
# Check the Module/initialization of ActionDispatcher - might show thread model
rg -n "ActionDispatcher\|actionsDispatcher" src/framework/actions/ --type cpp -B 2 -A 2 | head -40Repository: musescore/MuseScore
Length of output: 45
🏁 Script executed:
# Check if the async::Channel used for postDispatch has thread safety implications
rg -n "class Channel|postDispatch" src/framework/global/async/ --type h -B 2 -A 10 | head -50Repository: musescore/MuseScore
Length of output: 45
🏁 Script executed:
# Final check: look at async::Asyncable to understand thread affinity
rg -n "class Asyncable" src/framework/global/async/ --type h -A 15Repository: musescore/MuseScore
Length of output: 45
🏁 Script executed:
# Search for Asyncable class definition
rg -n "Asyncable" src/framework/global/ --type h | head -20Repository: musescore/MuseScore
Length of output: 2702
🏁 Script executed:
# Look for documentation or comments about thread safety in async module
rg -n "thread|Thread" src/framework/global/async/ --type h --type cpp -i | head -20Repository: musescore/MuseScore
Length of output: 163
🏁 Script executed:
# Check if there's explicit single-threaded enforcement or main thread checks
rg -n "mainThread\|isMainThread\|CHECK_IS_ON_MAIN_THREAD" src/framework/ --type h --type cpp | head -15Repository: musescore/MuseScore
Length of output: 45
🏁 Script executed:
# Check if there's documentation or usage patterns showing thread affinity
rg -n "dispatch" src/framework/actions/actionsmodule.cpp --type cpp -B 5 -A 10Repository: musescore/MuseScore
Length of output: 575
🏁 Script executed:
# Search for any thread safety comments in the codebase around action code
rg -n "thread.safe|single.thread|main.thread" src/framework/actions/ --type cpp --type h -iRepository: musescore/MuseScore
Length of output: 45
Use atomic increment for global sequence generation.
++counter on a shared static variable can race if nextActionSequence() is called from multiple threads, causing undefined behavior. Please make it atomic (or enforce and document strict single-thread access).
🔧 Proposed fix
+#include <atomic>
...
inline uint64_t nextActionSequence()
{
- static uint64_t counter = 0;
- return ++counter;
+ static std::atomic_uint64_t counter { 0 };
+ return counter.fetch_add(1, std::memory_order_relaxed) + 1;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framework/actions/actiontypes.h` around lines 44 - 48, The static
uint64_t counter in nextActionSequence() is not thread-safe; replace it with a
std::atomic<uint64_t> (e.g., make counter an atomic<uint64_t>) and perform an
atomic increment (fetch_add(1) or ++ with appropriate memory ordering) inside
nextActionSequence() so concurrent calls are safe and undefined behavior is
avoided; also add the necessary <atomic> include and keep the function signature
nextActionSequence() unchanged.
| virtual ActionCode lastRepeatableAction() const = 0; | ||
| virtual uint64_t lastRepeatableActionSequence() const = 0; | ||
| virtual void repeatLastAction() = 0; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Interface API asymmetry with INotationInteraction.
The dispatcher interface provides lastRepeatableAction() and lastRepeatableActionSequence() but lacks an explicit availability check method like INotationInteraction::canReapplyLastPaletteElement(). This is likely intentional, as callers can check for an empty ActionCode or sequence value of 0 to determine availability.
Consider adding a brief comment documenting how to determine if a repeatable action is available (e.g., "Returns empty ActionCode if no repeatable action exists").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framework/actions/iactionsdispatcher.h` around lines 58 - 61, Add a brief
documentation comment to the dispatcher interface clarifying how callers should
detect absence of a repeatable action: state that lastRepeatableAction() returns
an empty/invalid ActionCode when none exists and lastRepeatableActionSequence()
returns 0 in that case, and mention that repeatLastAction() should be called
only when a repeatable action is present; reference the existing methods
lastRepeatableAction(), lastRepeatableActionSequence(), and repeatLastAction()
and mirror the intent of INotationInteraction::canReapplyLastPaletteElement()
for clarity.
| bool ActionsDispatcher::isRepeatable(const ActionCode& code) const | ||
| { | ||
| // Query-style action codes (e.g., "action://...") cannot be safely | ||
| // re-dispatched with ActionData, so never treat them as repeatable. | ||
| if (ActionQuery(code).isValid()) { | ||
| return false; | ||
| } | ||
|
|
||
| static const std::unordered_set<ActionCode> repeatableActions = { | ||
| // Articulations | ||
| "add-marcato", "add-sforzato", "add-tenuto", "add-staccato", | ||
| // Ornaments | ||
| "add-turn", "add-turn-inverted", "add-turn-slash", | ||
| "add-turn-up", "add-turn-inverted-up", | ||
| "add-trill", "add-short-trill", "add-mordent", "add-haydn", | ||
| "add-tremblement", "add-prall-mordent", "add-shake", | ||
| "add-shake-muffat", "add-tremblement-couperin", | ||
| // Bowing | ||
| "add-up-bow", "add-down-bow", | ||
| // Slurs, ties, lines | ||
| "add-slur", "tie", "chord-tie", "lv", "hammer-on-pull-off", | ||
| // Hairpins, ottava | ||
| "add-hairpin", "add-hairpin-reverse", "add-8va", "add-8vb", | ||
| "add-dynamic", "add-noteline", | ||
| // Accidentals | ||
| "sharp2-post", "sharp-post", "nat-post", "flat-post", "flat2-post", | ||
| "flat2", "flat", "nat", "sharp", "sharp2", | ||
| // Brackets | ||
| "add-brackets", "add-parentheses", "add-braces", | ||
| // Beam modes | ||
| "beam-auto", "beam-none", "beam-break-left", | ||
| "beam-break-inner-8th", "beam-break-inner-16th", "beam-join", | ||
| // Grace notes | ||
| "acciaccatura", "appoggiatura", "grace4", "grace16", "grace32", | ||
| "grace8after", "grace16after", "grace32after", | ||
| // Layout breaks | ||
| "system-break", "page-break", "section-break", | ||
| // Visibility | ||
| "toggle-visible", "set-visible", "unset-visible", | ||
| // Flip | ||
| "flip", "flip-horizontally", | ||
| // Enharmonic | ||
| "enh-both", "enh-current", | ||
| // Transposition | ||
| "transpose-up", "transpose-down", | ||
| // Text | ||
| "system-text", "staff-text", "expression-text", | ||
| "rehearsalmark-text", "tempo", "fingering-text", | ||
| "sticking-text", "chord-text", "roman-numeral-text", | ||
| "nashville-number-text", "lyrics", | ||
| // Guitar bends | ||
| "standard-bend", "pre-bend", "grace-note-bend", "slight-bend", | ||
| "dive", "pre-dive", "dip", "scoop", | ||
| // Pitch | ||
| "pitch-up", "pitch-down", "pitch-up-octave", "pitch-down-octave", | ||
| "pitch-up-diatonic", "pitch-down-diatonic", | ||
| // Misc | ||
| "toggle-autoplace", "autoplace-enabled", "rest", | ||
| "toggle-snap-to-previous", "toggle-snap-to-next", | ||
| "mirror-note", "toggle-mmrest", "toggle-hide-empty", | ||
| "full-measure-rest", | ||
| }; | ||
|
|
||
| return repeatableActions.find(code) != repeatableActions.end(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Repeatability as a hardcoded whitelist will drift over time
isRepeatable() duplicates product behavior in a local static set. New repeat-worthy actions added elsewhere will silently stop being repeatable unless this list is manually updated.
Consider moving repeatability metadata to a single source of truth (e.g., action registry/config) and deriving this check from that metadata.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framework/actions/internal/actionsdispatcher.cpp` around lines 217 - 281,
ActionsDispatcher::isRepeatable currently keeps a hardcoded static unordered_set
repeatableActions which will drift; replace this by consulting a single source
of truth in the action registry/config (e.g., add a repeatable metadata flag to
your existing Action or ActionDescriptor and expose a lookup like
ActionRegistry::isRepeatable(const ActionCode&)). Change isRepeatable to return
ActionRegistry::isRepeatable(code) (or query
Action(code).descriptor().isRepeatable()) and remove the static
repeatableActions set; optionally keep the set as a temporary fallback only
while migrating, but ensure the canonical metadata lives in the
registry/ActionDescriptor so new actions automatically honor repeatability.
| m_lastPaletteElement.reset(element->clone()); | ||
| m_lastPaletteElementSequence = muse::actions::nextActionSequence(); | ||
|
|
There was a problem hiding this comment.
Repeat currently drops original keyboard modifiers
At Line 2387, replay calls applyPaletteElement(m_lastPaletteElement.get()) with default modifiers. Palette operations that depend on Ctrl/Shift will not reproduce the original action semantics.
💡 Suggested fix
- m_lastPaletteElement.reset(element->clone());
- m_lastPaletteElementSequence = muse::actions::nextActionSequence();
+ m_lastPaletteElement.reset(element->clone());
+ m_lastPaletteModifiers = modifiers;
+ m_lastPaletteElementSequence = muse::actions::nextActionSequence();
@@
- return applyPaletteElement(m_lastPaletteElement.get());
+ return applyPaletteElement(m_lastPaletteElement.get(), m_lastPaletteModifiers);Also add a member in NotationInteraction (header):
Qt::KeyboardModifiers m_lastPaletteModifiers = {};Also applies to: 2381-2388
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notation/internal/notationinteraction.cpp` around lines 2365 - 2367,
Repeat currently loses original keyboard modifiers when storing palette actions;
extend NotationInteraction to save and replay modifiers: add a member
Qt::KeyboardModifiers m_lastPaletteModifiers (default {}) and set it where you
call m_lastPaletteElement.reset(element->clone()) and
m_lastPaletteElementSequence = muse::actions::nextActionSequence(); then update
the replay path that calls applyPaletteElement(m_lastPaletteElement.get()) to
pass or apply m_lastPaletteModifiers so the palette action is executed with the
original Ctrl/Shift state (ensure any calls to applyPaletteElement or its
wrapper accept/forward the modifiers).
|
FWIW, I have nothing against this basic idea, but just wanted to point out that there is already the "apply current palette element" command (for which you can define a shortcut) that does the palette part of this - no need to special-case those that also have actions. Of course, having actions for palette items would be great if it allows them to be assigned shortcuts, but such a facility should ideally support all palette items, including custom ones. That's a more complicated design problem, though, which is the main reason this wasn't done long ago. |
mathesoncalum
added
the
needs design approval
There are more actions than palette elements that I like to have re-applied sometimes, like, for example, Paste double/half duration, transposing or switching voices (or even re-running plugins, something I haven't added support in this PR).
I would be happy adapting my changes to support whatever you might want, or doing so in alternate PRs. |
3 participants
Hi!
This PR basically lets redo the last action done. I find it annoying to have to go to the palette to choose again the same articulation or have to find some menu to re-apply some action to some bar or selection, specially when I have to do it many-many times for the same composition or arrangement I'm working on at many different points. With these changes, now I only need to do "Shift + R" for my new selection and it makes it much more efficient for my workflow. Hopefully it's also useful for somebody else.