Move setDotRelativeLine to ChordLayout#33022
Move setDotRelativeLine to ChordLayout#33022miiizen wants to merge 2 commits intomusescore:masterfrom
Conversation
📝 WalkthroughWalkthroughThe pull request refactors dot positioning logic by removing the 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 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/engraving/rendering/score/chordlayout.cpp`:
- Around line 2574-2583: The assignment to y inside the !tab->onLines() branch
is dead because y is immediately overwritten by the subsequent dotMove
conditional; remove the unreachable branch or refactor so the onLines() check
actually affects the dot positioning. Specifically, update the logic around
tab->onLines(), dotMove and note->voice() (the y variable assignment in this
block of chordlayout.cpp) so either (a) delete the if (!tab->onLines()) { y =
-0.5; } line, or (b) wrap the dotMove/voice-based assignment in an outer if/else
that differentiates behavior when tab->onLines() is false versus true,
preserving the intended fret-mark adjustment while ensuring y is not
overwritten.
🪄 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: 8ccc3848-4ec2-42c1-a80a-f15d4b499c43
📒 Files selected for processing (4)
src/engraving/dom/note.cppsrc/engraving/dom/note.hsrc/engraving/rendering/score/chordlayout.cppsrc/engraving/rendering/score/chordlayout.h
💤 Files with no reviewable changes (2)
- src/engraving/dom/note.h
- src/engraving/dom/note.cpp
| if (!tab->onLines()) { | ||
| // if fret marks above lines, raise the dots by half line distance | ||
| y = -0.5; | ||
| } | ||
| if (dotMove == 0) { | ||
| bool oddVoice = note->voice() & 1; | ||
| y = oddVoice ? 0.5 : -0.5; | ||
| } else { | ||
| y = 0.5; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for any history or comments about the onLines() TAB dot logic
# to understand if this dead code was intentional
# Search for related comments or TODOs
rg -n -C3 'onLines|fret marks above' --type=cpp src/engraving/Repository: musescore/MuseScore
Length of output: 12165
🏁 Script executed:
# Search for the original Note::setDotRelativeLine
rg -n 'Note::setDotRelativeLine' --type=cpp src/Repository: musescore/MuseScore
Length of output: 45
🏁 Script executed:
# Search for other dot-related positioning logic
rg -n 'setDotRelativeLine|dotMove|NoteDot' --type=cpp src/engraving/rendering/ -A2 -B2 | head -100
# Check if there's a prior version of this function or related logic
git log --oneline -n 20 src/engraving/rendering/score/chordlayout.cppRepository: musescore/MuseScore
Length of output: 8044
🏁 Script executed:
# Show the commit that moved setDotRelativeLine to ChordLayout
git show a65e0990e9 --stat
# Also search for where this function came from before the move
git show a65e0990e9 -- src/engraving/rendering/score/chordlayout.cpp | head -200Repository: musescore/MuseScore
Length of output: 50376
🏁 Script executed:
# Get the specific diff for the setDotRelativeLine function in the commit
git show a65e0990e9 -- src/engraving/rendering/score/chordlayout.cpp | grep -A 40 "setDotRelativeLine"Repository: musescore/MuseScore
Length of output: 11084
🏁 Script executed:
# Search for where setDotRelativeLine existed before this commit
git log --all --source --reverse -S "setDotRelativeLine" -- src/dom/note.cpp | head -20
# Also check if it existed in note.cpp at all
git show a65e0990e9^:src/dom/note.cpp 2>/dev/null | grep -A 50 "setDotRelativeLine" | head -60Repository: musescore/MuseScore
Length of output: 45
🏁 Script executed:
# Check the parent commit to see what was there before
git show a65e0990e9^:src/engraving/rendering/score/chordlayout.cpp 2>/dev/null | wc -l
# Search for setDotRelativeLine in all files in the parent commit
git show a65e0990e9^ | grep -l "setDotRelativeLine" 2>/dev/null || echo "Not found in parent commit"
# Let's check the specific lines in the current code to understand the TAB logic
git show HEAD:src/engraving/rendering/score/chordlayout.cpp | sed -n '2564,2615p'Repository: musescore/MuseScore
Length of output: 2064
Remove or refactor the unreachable !tab->onLines() branch.
The assignment y = -0.5 when !tab->onLines() is true (line 2576) is immediately overwritten by the subsequent if (dotMove == 0) ... else ... block (lines 2578-2583). This branch is dead code and should be either removed or refactored if the intent was to condition the entire if/else block on the stemThrough() and onLines() checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/engraving/rendering/score/chordlayout.cpp` around lines 2574 - 2583, The
assignment to y inside the !tab->onLines() branch is dead because y is
immediately overwritten by the subsequent dotMove conditional; remove the
unreachable branch or refactor so the onLines() check actually affects the dot
positioning. Specifically, update the logic around tab->onLines(), dotMove and
note->voice() (the y variable assignment in this block of chordlayout.cpp) so
either (a) delete the if (!tab->onLines()) { y = -0.5; } line, or (b) wrap the
dotMove/voice-based assignment in an outer if/else that differentiates behavior
when tab->onLines() is false versus true, preserving the intended fret-mark
adjustment while ensuring y is not overwritten.
1 participant
Some misc tidying. This is only ever called from
ChordLayoutand removes one morerenderer()->layoutItemcall.