added notation selection target#34247
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughSelection handling was consolidated around 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Public OSS repositories can only analyze public repositories installed in this organization. No linked repositories were analyzed; skipped Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🤖 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/notation/internal/notationinteraction.cpp`:
- Around line 1037-1040: Update the selection-change handling after
score->selectSimilarInRange(lastHit) to use notifyAboutSelectionChangedIfNeed()
instead of directly checking selectionChanged() and notifying. Preserve the
existing notification behavior while ensuring the helper clears
Score::selectionChanged() before or as part of notification.
In `@src/notationscene/internal/notationactioncontroller.cpp`:
- Around line 3174-3190: Update NotationActionController::registerAliases so the
dispatched alias checks the state for the alias command itself, rather than
query.uri() (which resolves to SELECT_COMMAND). Use the alias key captured for
each registration when consulting commandsState(), preserving the existing
NotSupported response and handler dispatch behavior.
🪄 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: 0629dcb0-03a3-4fea-8b50-6934509b6174
📒 Files selected for processing (11)
musesrc/notation/inotationinteraction.hsrc/notation/internal/notationinteraction.cppsrc/notation/internal/notationinteraction.hsrc/notation/notationtypes.hsrc/notation/tests/mocks/notationinteractionmock.hsrc/notationscene/internal/notationactioncontroller.cppsrc/notationscene/internal/notationactioncontroller.hsrc/notationscene/internal/notationcommandsregister.cppsrc/notationscene/internal/notationcommandsstate.cppsrc/notationscene/notationcommands.h
| void NotationActionController::registerAliases(const std::map<muse::rcommand::Command, muse::rcommand::CommandQuery>& aliases, | ||
| void (NotationActionController::*handler)(const muse::rcommand::CommandQuery&)) | ||
| { | ||
| for (auto it = aliases.cbegin(); it != aliases.cend(); ++it) { | ||
| auto alias = it->first; | ||
| auto query = it->second; | ||
|
|
||
| commandDispatcher()->onRequest(this, alias, [this, query, handler]() { | ||
| if (!commandsState()->commandState(query.uri()).enabled) { | ||
| return muse::make_ret(Ret::Code::NotSupported); | ||
| } | ||
|
|
||
| (this->*handler)(query); | ||
| return muse::make_ok(); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check the dispatched alias’s state, not SELECT_COMMAND.
Every stored query has SELECT_COMMAND as its URI, so selection-required aliases bypass their states from HAS_SELECTION_REQUIRED_COMMANDS. This also lets remapped legacy actions report success without a selection.
Proposed fix
- commandDispatcher()->onRequest(this, alias, [this, query, handler]() {
- if (!commandsState()->commandState(query.uri()).enabled) {
+ commandDispatcher()->onRequest(this, alias, [this, alias, query, handler]() {
+ if (!commandsState()->commandState(alias).enabled) {📝 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.
| void NotationActionController::registerAliases(const std::map<muse::rcommand::Command, muse::rcommand::CommandQuery>& aliases, | |
| void (NotationActionController::*handler)(const muse::rcommand::CommandQuery&)) | |
| { | |
| for (auto it = aliases.cbegin(); it != aliases.cend(); ++it) { | |
| auto alias = it->first; | |
| auto query = it->second; | |
| commandDispatcher()->onRequest(this, alias, [this, query, handler]() { | |
| if (!commandsState()->commandState(query.uri()).enabled) { | |
| return muse::make_ret(Ret::Code::NotSupported); | |
| } | |
| (this->*handler)(query); | |
| return muse::make_ok(); | |
| }); | |
| } | |
| } | |
| void NotationActionController::registerAliases(const std::map<muse::rcommand::Command, muse::rcommand::CommandQuery>& aliases, | |
| void (NotationActionController::*handler)(const muse::rcommand::CommandQuery&)) | |
| { | |
| for (auto it = aliases.cbegin(); it != aliases.cend(); ++it) { | |
| auto alias = it->first; | |
| auto query = it->second; | |
| commandDispatcher()->onRequest(this, alias, [this, alias, query, handler]() { | |
| if (!commandsState()->commandState(alias).enabled) { | |
| return muse::make_ret(Ret::Code::NotSupported); | |
| } | |
| (this->*handler)(query); | |
| return muse::make_ok(); | |
| }); | |
| } | |
| } |
🤖 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/notationscene/internal/notationactioncontroller.cpp` around lines 3174 -
3190, Update NotationActionController::registerAliases so the dispatched alias
checks the state for the alias command itself, rather than query.uri() (which
resolves to SELECT_COMMAND). Use the alias key captured for each registration
when consulting commandsState(), preserving the existing NotSupported response
and handler dispatch behavior.
| void findAndSelectChordRest(const Fraction& tick) override; | ||
| void moveSegmentSelection(MoveDirection d) override; | ||
|
|
||
| FilterElementsOptions elementsFilterOptions(const EngravingItem* element) const; |
There was a problem hiding this comment.
I think we can make it private
There was a problem hiding this comment.
In the next PR I will clean up this interface and class
ea126df to
b70d58c
Compare
2 participants
No description provided.