Add note coloring scheme options to Style dialog#32994
Open
faiyazchowdhury wants to merge 1 commit intomusescore:masterfrom
Open
Add note coloring scheme options to Style dialog#32994faiyazchowdhury wants to merge 1 commit intomusescore:masterfrom
faiyazchowdhury wants to merge 1 commit intomusescore:masterfrom
Conversation
Author
|
@avvvvve This is the PR for this task. I also added the coloration of accidentals and beams and stems, as you requested as part of the "stretch goal". |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a note-coloring subsystem: new header 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 12
🤖 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/dom/articulation.cpp`:
- Around line 335-356: The chordRootNote() implementation is duplicated across
articulation.cpp, beam.cpp and stem.cpp; extract its logic into a single shared
helper (e.g., a free function or static function in a small engraving/utils
header such as engraving::chordRootNote or ArticulationUtils::chordRootNote)
keeping the same signature Note* chordRootNote(Chord*), move the existing body
there, update articulation.cpp, beam.cpp and stem.cpp to remove their local
definitions and include the new header, and ensure the new helper pulls the same
types (Note, Chord, NoteColoringScheme, Key) and helper functions
(tonicPitchClassFromKey, getMeasureChordRoot) so callers behave identically.
In `@src/engraving/dom/beam.cpp`:
- Around line 648-669: The chordRootNote function is duplicated; extract it into
a single shared utility (e.g., make it a static helper in note.cpp or a member
on Chord like Chord::rootNote()/Note::chordRoot()) and replace the copies in
articulation.cpp and stem.cpp to call that one implementation; ensure the new
location has access to Note, Chord, getMeasureChordRoot, tonicPitchClassFromKey,
Key and staff()/style()/upNote() APIs and update includes/visibility accordingly
so behavior remains identical for chordRootNote/Chord::rootNote.
In `@src/engraving/dom/note.cpp`:
- Around line 3310-3326: The propertyDefault(Pid::COLOR) logic currently
computes colorIdx from pitchClass for AbsolutePitchSimple and MoveableDo
variants, which diverges from the actual note painting in color(); update the
block in note.cpp that computes colorSid so it uses the same step/degree mapping
as color() for NoteColoringScheme::AbsolutePitchSimple,
NoteColoringScheme::MoveableDo and MoveableDoRelative (i.e. derive the scale
degree/step using tonicPitchClassFromKey(staff()->key(tick())),
getMeasureChordRoot(this, tonicPC) and the same chordDegree/step-to-color
mapping (chordDegreeColorIndex or the step-based branch used in color()) instead
of directly using pitchClass; keep existing behavior for OneColor and
ChordDegrees branches and then compute Sid from the resolved color index as
before.
- Around line 3353-3384: The MoveableDo* and ChordDegrees branches must honor
colorNotesByConcertPitch: replace uses of tpc() with colorTpc and epitch()/pC
with colorEpitch, and derive the tonic/chord root basis from the color pitch
values rather than the raw staff()->key() when computing
tonicPitchClass/getMeasureChordRoot/chordDegreeColorIndex; specifically, in the
MoveableDoSimple branch use colorTpc with tpc2degree(colorTpc, key) (and
normalize), in the MoveableDoChromatic branch compute pC from colorEpitch%12,
and in ChordDegrees compute tonicPC/rootChroma from colorTpc/colorEpitch (use
tonicPitchClassFromKey or compute tonic PC from colorTpc) before calling
getMeasureChordRoot(this, tonicPC) and chordDegreeColorIndex(chroma, rootChroma,
tonicPC).
In `@src/engraving/dom/notedot.cpp`:
- Around line 101-105: Remove the trailing blank lines after the final closing
braces in src/engraving/dom/notedot.cpp so the file ends immediately after the
closing brace of the color() method and its enclosing namespace; locate the
color() method definition and the namespace block and delete any extra empty
lines following their closing '}' so the file ends with the final brace only.
In `@src/engraving/dom/stem.cpp`:
- Around line 188-209: The chordRootNote helper is duplicated across files (this
copy in src/engraving/dom/stem.cpp plus copies in beam.cpp and
articulation.cpp); refactor by extracting chordRootNote into a single shared
utility (e.g., a new function in a common helper/utility module or existing
shared namespace) and replace the three local implementations with calls to that
shared function; ensure the signature and behavior (uses Note, Chord,
NoteColoringScheme, getMeasureChordRoot, tonicPitchClassFromKey) remain
identical and update includes/headers in stem.cpp, beam.cpp and articulation.cpp
to import the new utility.
In `@src/engraving/types/notecoloringscheme.h`:
- Line 138: The sentinel value 9999 used to initialize lowestPitch is a magic
number; change it to a self-documenting constant such as
std::numeric_limits<int>::max() or a named constant (e.g. kInvalidPitch) and
update the declaration of lowestPitch accordingly; also add the required include
(<limits>) if you choose std::numeric_limits and ensure any comparisons that
rely on the sentinel still behave correctly for the variable lowestPitch.
- Line 151: The CI Uncrustify error is caused by C++03-style spacing required
for nested template angle brackets: change the declaration of scored
(std::vector<std::pair<int, int>>) to use a space between the two closing angle
brackets so it becomes std::vector<std::pair<int, int> > scored; — update the
variable declaration in notecoloringscheme.h where scored is defined.
In `@src/notationscene/widgets/editstyle.cpp`:
- Around line 1111-1128: The lambda connected to schemeCombo currently clears
all swatches by setting colorLabels[i] and defaultColorLabel to Qt::black and
firing valueChanged, which wipes user-selected colors; remove the blocks that
blockSignals()/setColor(QColor(Qt::black))/blockSignals(false) for colorLabels
and defaultColorLabel and the corresponding valueChanged calls for colorSids and
StyleId::defaultNoteColor so the colors are preserved when only the scheme
changes; keep calling updateSwatchesUI(scheme), updatePresetFromState(), and
emit valueChanged(static_cast<int>(StyleId::noteColorTheme)) so the
mapping/count logic updates without resetting the actual swatch colors.
- Around line 954-960: The "Reset all color settings" button currently only
zeroes noteColor0..11; update its click handler (the slot connected to the
resetColorsBtn in editstyle.cpp) to also reset defaultNoteColor, noteColorTheme,
all colorApplyTo* flags (e.g., colorApplyToPart, colorApplyToScore if present),
and colorNotesByConcertPitch to their intended defaults (the same defaults used
when a new section/style is created or when loading a factory default), and
ensure any per-section stored defaults are updated so a section-level one-color
default can be restored; locate the resetColorsBtn setup and its connected slot
and add code to restore these additional fields and persist/update UI state
accordingly.
- Around line 1189-1239: The selection-coloring currently uses raw n->pitch() %
12 which ignores the same transposition/concert-pitch basis used by
Note::color(), causing divergence from the automatic scheme when
colorNotesByConcertPitch is set; change the chroma computation to use the same
basis as Note::color() (i.e. respect the colorNotesByConcertPitch setting and
convert to concert pitch when that flag is enabled) — for example, obtain the
pitch to use for coloring the same way Note::color() does (call the same helper
or compute concert vs written pitch accordingly) and then compute chroma from
that value instead of n->pitch().
🪄 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: 3b6f57af-b3aa-4e51-8af9-11a3be659b4e
📒 Files selected for processing (16)
src/engraving/dom/accidental.cppsrc/engraving/dom/accidental.hsrc/engraving/dom/articulation.cppsrc/engraving/dom/articulation.hsrc/engraving/dom/beam.cppsrc/engraving/dom/beam.hsrc/engraving/dom/note.cppsrc/engraving/dom/note.hsrc/engraving/dom/notedot.cppsrc/engraving/dom/notedot.hsrc/engraving/dom/stem.cppsrc/engraving/dom/stem.hsrc/engraving/style/styledef.cppsrc/engraving/style/styledef.hsrc/engraving/types/notecoloringscheme.hsrc/notationscene/widgets/editstyle.cpp
faiyazchowdhury
changed the title
faiyazchowdhury
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/engraving/dom/note.cpp (2)
3298-3311:⚠️ Potential issue | 🟠 MajorUse the concert key for Moveable Do when concert coloring is enabled.
This branch switches the note basis to
tpc1()/ concertepitch, but it still derives the tonic fromstaff()->key(). On transposing instruments that shifts Moveable Do colors by the transposition, so concert-pitch coloring is still wrong for the relative schemes.🔧 Suggested fix
case NoteColoringScheme::MoveableDoSimple: case NoteColoringScheme::MoveableDoChromatic: { - Key key = note->staff() ? note->staff()->key(note->tick()) : Key::C; + Key key = Key::C; + if (note->staff()) { + key = byConcertPitch ? note->staff()->concertKey(note->tick()) + : note->staff()->key(note->tick()); + } if (scheme == NoteColoringScheme::MoveableDoSimple) { int idx = tpc2degree(colorTpc, key); return idx < 0 ? idx + 7 : idx; } int pC = colorEpitch % 12;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/dom/note.cpp` around lines 3298 - 3311, The Moveable Do branch currently uses staff()->key(note->tick()) while the coloring basis has been switched to concert pitch (tpc1()/concert epitch); change it to use the concert key and concert tpc/epitch: use note->tpc1() (or the concert TPC variable passed in) instead of colorTpc and use note->epitch() (or colorEpitch concert value) and derive the tonic via staff()->concertKey(note->tick()) (falling back to Key::C if no staff) before calling tonicPitchClassFromKey; update the MoveableDoSimple and MoveableDoChromatic paths to compute idx/tonic relative to that concert key so transposing instruments get correct concert-pitch Moveable Do coloring.
3221-3256:⚠️ Potential issue | 🟠 MajorAvoid doing full measure analysis from the color hot path.
getMeasureChordRoot()rescans every chord/rest segment in the measure and rebuilds the analysis input on each call. UnderChordDegrees,noteColoringSwatchIndex()andchordRootNote()now reach this fromNote::color()/propertyDefault(Pid::COLOR), so repainting a dense measure turns into repeated whole-measure work for every note. Please move this behind a measure-scoped memoization/invalidation strategy instead of recomputing it per lookup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/dom/note.cpp` around lines 3221 - 3256, getMeasureChordRoot currently recomputes whole-measure chord analysis for every Note lookup (called from Note::color/propertyDefault(Pid::COLOR), noteColoringSwatchIndex(), chordRootNote()), so add a measure-scoped cache: compute noteInfos and analyzeChordRoot once per Measure and store the result (e.g. add a cached field on Measure or a central MeasureAnalysisCache keyed by Measure*), have getMeasureChordRoot first check the cache and return the cached tonicPC if present, and ensure cache invalidation when the measure changes (segment/chord/note edits, ticks changes) by hooking into existing mutation points or an explicit invalidateMeasureAnalysis(Measure*) call so repainting uses the cached root instead of rescanning each call.
🤖 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/dom/beam.cpp`:
- Around line 654-667: The code currently inspects only m_elements.front() (in
the Beam COLOR case) and assumes it's a ChordRest/chord; instead iterate over
m_elements to find the first element that is a chord (check cr &&
cr->isChord()), then use chordRootNote(toChord(cr)) and the existing logic
(reading noteColorTheme and colorApplyToBeam and returning
n->getProperty(Pid::COLOR)) on that chord; apply the same fix to the other
similar block handling COLOR (the duplicate code around the later lines) so any
leading rests are skipped and the first chord is used.
In `@src/notationscene/widgets/editstyle.cpp`:
- Around line 855-961: The UI strings created locally (presetCombo, schemeCombo,
schemeDescription, oneColorDescription, colorLabelText, applyToLabel, the
QCheckBoxes, rbWrittenPitch/rbConcertPitch, resetColorsBtn, colorSelectionBtn,
etc.) won't update on language change; make them member widgets (or store them
in styleWidgets) and move all setText/addItem calls into
EditStyle::retranslate() so translations are applied at runtime: replace
constructor-only muse::qtrc(...) uses with creating the widgets
(QComboBox/QLabel/QPushButton/QRadioButton/QCheckBox) as members, then implement
retranslate() to clear and repopulate combo items (clear()+addItem(...) with
translated strings), and call setText(...) / setVisible(...) / setWordWrap(...)
for labels and buttons there; ensure QButtonGroup ids (bgPitch) remain but radio
button labels are updated in retranslate().
- Around line 1285-1290: The one-shot sync after inserting gbColors leaves the
UI stale because subsequent setValues() calls block note-color widget signals
and never refresh updatePresetFromState() or updateSwatchesUI(); modify
setValues() (and any code paths that block signals on the note-color widgets) to
explicitly call updatePresetFromState() and
updateSwatchesUI(static_cast<NoteColoringScheme>(schemeCombo->currentData().toInt()))
after restoring values (or after re-enabling signals), so the preset dropdown,
swatch labels/descriptions, and chord-root button visibility are updated
whenever styles are loaded or reset rather than only once during initialization.
- Around line 1118-1165: The presetCombo handler currently only updates
schemeCombo for ChordDegrees, leaving the coloring scheme stale for Default,
Boomwhackers and FigureNotes; update those branches to set schemeCombo to the
appropriate NoteColoringScheme for each preset by calling
schemeCombo->findData(static_cast<int>(NoteColoringScheme::...)), check idx>=0,
blockSignals(true)/setCurrentIndex(idx)/blockSignals(false), then call
updateSwatchesUI(the same NoteColoringScheme) and proceed with existing
valueChanged calls (e.g., still call
valueChanged(static_cast<int>(StyleId::noteColorTheme)) at the end).
- Around line 938-945: The style entry for StyleId::colorNotesByConcertPitch is
registered with the wrong boolean flag, causing setValues()/getValue() to treat
bgPitch as a checkable widget; update the registration so the style is
recognized as a boolean represented by a button group (e.g. change the
styleWidgets.append call that currently uses bgPitch and false to use true), or
alternatively add StyleId::colorNotesByConcertPitch to
isBoolStyleRepresentedByButtonGroup(); this ensures bgPitch is handled correctly
by setValues() and getValue().
---
Duplicate comments:
In `@src/engraving/dom/note.cpp`:
- Around line 3298-3311: The Moveable Do branch currently uses
staff()->key(note->tick()) while the coloring basis has been switched to concert
pitch (tpc1()/concert epitch); change it to use the concert key and concert
tpc/epitch: use note->tpc1() (or the concert TPC variable passed in) instead of
colorTpc and use note->epitch() (or colorEpitch concert value) and derive the
tonic via staff()->concertKey(note->tick()) (falling back to Key::C if no staff)
before calling tonicPitchClassFromKey; update the MoveableDoSimple and
MoveableDoChromatic paths to compute idx/tonic relative to that concert key so
transposing instruments get correct concert-pitch Moveable Do coloring.
- Around line 3221-3256: getMeasureChordRoot currently recomputes whole-measure
chord analysis for every Note lookup (called from
Note::color/propertyDefault(Pid::COLOR), noteColoringSwatchIndex(),
chordRootNote()), so add a measure-scoped cache: compute noteInfos and
analyzeChordRoot once per Measure and store the result (e.g. add a cached field
on Measure or a central MeasureAnalysisCache keyed by Measure*), have
getMeasureChordRoot first check the cache and return the cached tonicPC if
present, and ensure cache invalidation when the measure changes
(segment/chord/note edits, ticks changes) by hooking into existing mutation
points or an explicit invalidateMeasureAnalysis(Measure*) call so repainting
uses the cached root instead of rescanning each call.
🪄 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: c7627466-bf16-4e27-bea3-cc434aab3474
📒 Files selected for processing (8)
src/engraving/dom/articulation.cppsrc/engraving/dom/beam.cppsrc/engraving/dom/note.cppsrc/engraving/dom/note.hsrc/engraving/dom/notedot.cppsrc/engraving/dom/stem.cppsrc/engraving/types/notecoloringscheme.hsrc/notationscene/widgets/editstyle.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/engraving/dom/note.cpp (1)
3330-3340:⚠️ Potential issue | 🟠 MajorUse the concert key for
MoveableDo*when concert-based coloring is enabled.These branches switch pitch/TPC with
Sid::colorNotesByConcertPitch, but they still compute degrees fromstaff()->key(). On transposing instruments, concert-pitch coloring will therefore map to the written key instead of the sounding key.🔧 Local fix
- Key key = note->staff() ? note->staff()->key(note->tick()) : Key::C; + Key key = Key::C; + if (note->staff()) { + key = byConcertPitch ? note->staff()->concertKey(note->tick()) + : note->staff()->key(note->tick()); + } if (scheme == NoteColoringScheme::MoveableDoSimple) { int idx = tpc2degree(colorTpc, key); return idx < 0 ? idx + 7 : idx; } int pC = colorEpitch % 12;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/dom/note.cpp` around lines 3330 - 3340, The MoveableDo branches use staff()->key(note->tick()) which gives the written key; when Sid::colorNotesByConcertPitch is set we must compute the key in concert pitch before calling tpc2degree or tonicPitchClassFromKey. Update the code paths handling NoteColoringScheme::MoveableDoSimple and the tonicPitchClass calculation to obtain the concert key (e.g., derive the sounding key from the note/instrument/staff using the existing concert-transposition helper or API) instead of staff()->key(...), then pass that concert key into tpc2degree(colorTpc, ...) and tonicPitchClassFromKey(...). Ensure symbols referenced: NoteColoringScheme::MoveableDoSimple, tpc2degree, colorTpc, colorEpitch, tonicPitchClassFromKey, and the Sid::colorNotesByConcertPitch flag are used to gate this behavior.
🤖 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/dom/note.cpp`:
- Around line 3292-3302: chordRootNote() currently returns the original note
when the measure root chroma isn't present, which causes coloring to fall back
to top-note behavior; instead, after computing rootChroma via
getMeasureChordRoot(n, tonicPC) and scanning ch->notes() using
noteColoringEpitch(), if no exact pc match is found, derive a target epitch
using tonicPC + rootChroma in the same octave as the input note (e.g., compute
targetEpitch = (n->epitch()/12)*12 + (tonicPC + rootChroma) or equivalent) and
then return either an existing chord Note with that epitch or a lightweight
clone of n with its epitch set to targetEpitch so coloring follows the measure
root (do not call upNote() as fallback). Ensure you reference chordRootNote(),
getMeasureChordRoot(), noteColoringEpitch(), and avoid using upNote() for the
fallback.
---
Duplicate comments:
In `@src/engraving/dom/note.cpp`:
- Around line 3330-3340: The MoveableDo branches use staff()->key(note->tick())
which gives the written key; when Sid::colorNotesByConcertPitch is set we must
compute the key in concert pitch before calling tpc2degree or
tonicPitchClassFromKey. Update the code paths handling
NoteColoringScheme::MoveableDoSimple and the tonicPitchClass calculation to
obtain the concert key (e.g., derive the sounding key from the
note/instrument/staff using the existing concert-transposition helper or API)
instead of staff()->key(...), then pass that concert key into
tpc2degree(colorTpc, ...) and tonicPitchClassFromKey(...). Ensure symbols
referenced: NoteColoringScheme::MoveableDoSimple, tpc2degree, colorTpc,
colorEpitch, tonicPitchClassFromKey, and the Sid::colorNotesByConcertPitch flag
are used to gate this 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: ASSERTIVE
Plan: Pro
Run ID: d9ab2155-8e3a-4bac-aea2-54c1d523e4f2
📒 Files selected for processing (3)
src/engraving/dom/note.cppsrc/engraving/dom/note.hsrc/engraving/types/notecoloringscheme.h
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/dom/notedot.cpp`:
- Around line 75-80: The dot color propagation currently skips the OneColor
scheme because the condition in the Pid::COLOR branch checks "theme !=
NoteColoringScheme::OneColor && colorApplyToDot", which prevents "apply to dots"
from taking effect for OneColor; update the condition in the note() Pid::COLOR
check (and the analogous block at the other occurrence) to simply test the
colorApplyToDot flag (styleV(Sid::colorApplyToDot).toBool()) and return
note()->getProperty(Pid::COLOR) when that flag is true, so OneColor is included
in dot color propagation while preserving existing behavior for other themes.
In `@src/engraving/types/notecoloringscheme.h`:
- Around line 80-104: The scoring function scoreNoteForChord uses hardcoded
magic numbers (100 for on-beat bonus, 150 for lowest-pitch bonus, 20 for chroma
count multiplier, and the implicit 4 for quarter division); extract these into
clearly named constants (e.g., kOnBeatBonus, kLowestPitchBonus,
kChromaCountWeight, kQuarterDivisor) declared as constexpr or static const ints
near the top of the header or in an unnamed namespace, then replace the literal
values in scoreNoteForChord with those constants so weights are self-documenting
and easy to tune.
🪄 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: d2250d9b-fc44-47e7-8a39-7429cc336bac
📒 Files selected for processing (2)
src/engraving/dom/notedot.cppsrc/engraving/types/notecoloringscheme.h
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/notationscene/widgets/editstyle.cpp (3)
941-948:⚠️ Potential issue | 🟠 MajorRegister
colorNotesByConcertPitchas a bool button-group style.Line 948 stores a
BOOLstyle onbgPitch, butEditStyle::isBoolStyleRepresentedByButtonGroup()does not includeStyleId::colorNotesByConcertPitch.getValue()/setValues()therefore fall back to reading and writing a nonexistentcheckedproperty onQButtonGroup, so the written/concert-pitch choice will not load or save correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notationscene/widgets/editstyle.cpp` around lines 941 - 948, The style for the pitch radio-button group (styleWidgets.append using StyleId::colorNotesByConcertPitch and bgPitch) was registered as a BOOL button-group but EditStyle::isBoolStyleRepresentedByButtonGroup() does not include StyleId::colorNotesByConcertPitch, causing getValue()/setValues() to try to read/write a non-existent QButtonGroup::checked property; update EditStyle::isBoolStyleRepresentedByButtonGroup() to include StyleId::colorNotesByConcertPitch (or alternatively change the registration to a non-bool style) so the button-group boolean handling matches the registration and getValue()/setValues() correctly load and save the written/concert-pitch choice.
1306-1309:⚠️ Potential issue | 🟠 MajorRe-sync the derived note-color UI after
setValues().
setValues()blocks signals onschemeCombo, the swatches, and the pitch widgets, so the lambdas that recompute preset inference, swatch visibility/labels, descriptions, and the chord-root button never run. Reopening the dialog or callingsetValues()from “Reset styles” can therefore leave this section showing stale state from the previous style.Also applies to: 2385-2515
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notationscene/widgets/editstyle.cpp` around lines 1306 - 1309, After setValues() finishes, explicitly re-run the UI recompute lambdas so the preset inference, swatch visibility/labels, descriptions, and chord-root button are updated even when signals were blocked: add a posted call (QMetaObject::invokeMethod with Qt::QueuedConnection) that calls updatePresetFromState() and updateSwatchesUI(static_cast<NoteColoringScheme>(schemeCombo->currentData().toInt())) (the same pattern shown around the current invokeMethod) immediately after each setValues() invocation (including the other occurrence in the 2385-2515 region) so the derived note-color UI is always re-synced.
853-959:⚠️ Potential issue | 🟠 MajorMove the note-color strings into
retranslate().All labels, combo items, button texts, and scheme descriptions in this block are constructor-local, so
EditStyle::retranslate()cannot refresh them onQEvent::LanguageChange. After a runtime language switch, this whole section will stay in the old locale.Also applies to: 1113-1127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notationscene/widgets/editstyle.cpp` around lines 853 - 959, The UI text in the note-color block is set only in the constructor, so move all translatable strings into EditStyle::retranslate(): make the widgets (presetCombo, schemeCombo, schemeDescription, oneColorDescription, colorLabelText, applyToLabel, cbApplyToAccidental, cbApplyToStem, cbApplyToArticulation, cbApplyToDot, cbApplyToBeam, pitchLabel, rbWrittenPitch, rbConcertPitch, resetColorsBtn, colorSelectionBtn and any other labels/buttons) accessible as members (or stored in styleWidgets) and in retranslate() call setText()/setItemText()/setWindowTitle()/setToolTip() as appropriate; for combo boxes update items by setItemText (or clear+addItems) to refresh localized entries (e.g., the preset and scheme item texts), and update schemeDescription/oneColorDescription via setText so language changes update correctly (also apply same change to the other block flagged at lines 1113-1127).
🤖 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/dom/stem.cpp`:
- Around line 197-226: The stem color logic currently blocks
NoteColoringScheme::OneColor from propagating to stems; in both the Pid::COLOR
branch (in Stem::property) and in Color Stem::color() remove the "theme !=
NoteColoringScheme::OneColor" check so that when
n->style().styleV(Sid::colorApplyToStem).toBool() is true the code returns the
note's color (n->getProperty(Pid::COLOR) in the property switch and n->color()
in Stem::color()) regardless of the OneColor theme; keep the rest of the guards
(chord(), chordRootNote, colorApplyToStem) unchanged.
---
Duplicate comments:
In `@src/notationscene/widgets/editstyle.cpp`:
- Around line 941-948: The style for the pitch radio-button group
(styleWidgets.append using StyleId::colorNotesByConcertPitch and bgPitch) was
registered as a BOOL button-group but
EditStyle::isBoolStyleRepresentedByButtonGroup() does not include
StyleId::colorNotesByConcertPitch, causing getValue()/setValues() to try to
read/write a non-existent QButtonGroup::checked property; update
EditStyle::isBoolStyleRepresentedByButtonGroup() to include
StyleId::colorNotesByConcertPitch (or alternatively change the registration to a
non-bool style) so the button-group boolean handling matches the registration
and getValue()/setValues() correctly load and save the written/concert-pitch
choice.
- Around line 1306-1309: After setValues() finishes, explicitly re-run the UI
recompute lambdas so the preset inference, swatch visibility/labels,
descriptions, and chord-root button are updated even when signals were blocked:
add a posted call (QMetaObject::invokeMethod with Qt::QueuedConnection) that
calls updatePresetFromState() and
updateSwatchesUI(static_cast<NoteColoringScheme>(schemeCombo->currentData().toInt()))
(the same pattern shown around the current invokeMethod) immediately after each
setValues() invocation (including the other occurrence in the 2385-2515 region)
so the derived note-color UI is always re-synced.
- Around line 853-959: The UI text in the note-color block is set only in the
constructor, so move all translatable strings into EditStyle::retranslate():
make the widgets (presetCombo, schemeCombo, schemeDescription,
oneColorDescription, colorLabelText, applyToLabel, cbApplyToAccidental,
cbApplyToStem, cbApplyToArticulation, cbApplyToDot, cbApplyToBeam, pitchLabel,
rbWrittenPitch, rbConcertPitch, resetColorsBtn, colorSelectionBtn and any other
labels/buttons) accessible as members (or stored in styleWidgets) and in
retranslate() call setText()/setItemText()/setWindowTitle()/setToolTip() as
appropriate; for combo boxes update items by setItemText (or clear+addItems) to
refresh localized entries (e.g., the preset and scheme item texts), and update
schemeDescription/oneColorDescription via setText so language changes update
correctly (also apply same change to the other block flagged at lines
1113-1127).
🪄 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: 525204e1-0269-4a39-a40b-10b135a8f3de
📒 Files selected for processing (2)
src/engraving/dom/stem.cppsrc/notationscene/widgets/editstyle.cpp
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/engraving/dom/beam.cpp (1)
659-671:⚠️ Potential issue | 🟠 MajorDon't stop at
m_elements.front()when resolving beam color.
Beamalready allows rests inm_elements. If a beamed group starts with a rest, both blocks fall back to the base/default color even when a later chord should drive the beam color. Scan for the first chord before callingchordRootNote().Suggested fix
- if (!m_elements.empty()) { - ChordRest* cr = m_elements.front(); - if (cr && cr->isChord()) { - Note* n = chordRootNote(toChord(cr)); + auto it = std::find_if(m_elements.begin(), m_elements.end(), [](ChordRest* cr) { + return cr && cr->isChord(); + }); + if (it != m_elements.end()) { + Note* n = chordRootNote(toChord(*it)); if (n) { ... } - } }Apply the same search in
Beam::color().Also applies to: 683-695
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/dom/beam.cpp` around lines 659 - 671, The color resolution in Beam::color() currently uses m_elements.front() which can be a rest; change both places that call m_elements.front() (the block handling Pid::COLOR and the later similar block) to scan m_elements for the first ChordRest* that returns true for isChord() (e.g., iterate m_elements until finding cr && cr->isChord()), then call chordRootNote(toChord(foundCr)) and proceed as before; ensure you still respect NoteColoringScheme and colorApplyToBeam checks before returning n->getProperty(Pid::COLOR).src/engraving/dom/accidental.cpp (1)
508-515:⚠️ Potential issue | 🟠 Major
OneColorstill won't propagate to the remaining apply-to targets.These branches still short-circuit on
theme != NoteColoringScheme::OneColor, so enabling “apply color to accidentals” has no effect in the one-color scheme even thoughNote::color()already resolves that swatch. Stems and dots were already fixed in this PR, and the analogous checks still left inarticulation.cppandbeam.cppwill make those targets inconsistent too.Suggested fix
- NoteColoringScheme theme = static_cast<NoteColoringScheme>(note()->style().styleV(Sid::noteColorTheme).toInt()); - if (theme != NoteColoringScheme::OneColor && note()->style().styleV(Sid::colorApplyToAccidental).toBool()) { + if (note()->style().styleV(Sid::colorApplyToAccidental).toBool()) { return note()->getProperty(Pid::COLOR); } @@ - NoteColoringScheme theme = static_cast<NoteColoringScheme>(note()->style().styleV(Sid::noteColorTheme).toInt()); - if (theme != NoteColoringScheme::OneColor && note()->style().styleV(Sid::colorApplyToAccidental).toBool()) { + if (note()->style().styleV(Sid::colorApplyToAccidental).toBool()) { return note()->color(); }Also applies to: 531-539
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/dom/accidental.cpp` around lines 508 - 515, The code currently short-circuits on theme != NoteColoringScheme::OneColor so enabling the "apply to accidentals" flag is ignored for the OneColor scheme; change the Pid::COLOR branch so that if note() exists and note()->style().styleV(Sid::colorApplyToAccidental).toBool() is true you return note()->getProperty(Pid::COLOR) regardless of the NoteColoringScheme value (i.e., remove the theme != OneColor guard), falling back to EngravingItem::propertyDefault(propertyId) otherwise; apply the same fix to the analogous blocks in the nearby Pid::COLOR range (531-539) and to the similar checks in articulation.cpp and beam.cpp so stems/dots/articulations behave consistently.
🤖 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/types/notecoloringscheme.h`:
- Around line 137-143: Replace the magic numeric weights in the interval scoring
block with named constants for consistency with existing CHORD_ROOT_SCORE_*
symbols: define CHORD_ROOT_SCORE_FIFTH = 3, CHORD_ROOT_SCORE_THIRD = 2,
CHORD_ROOT_SCORE_SEVENTH = 1 (or similar names), then use those constants in the
conditional that checks interval (the block using interval == 7 / 4 / 3 / 10 /
11) so the code reads score += CHORD_ROOT_SCORE_FIFTH, score +=
CHORD_ROOT_SCORE_THIRD, and score += CHORD_ROOT_SCORE_SEVENTH respectively; keep
the interval checks unchanged and add the new constant declarations alongside
the other CHORD_ROOT_SCORE_* constants.
In `@src/notationscene/widgets/editstyle.cpp`:
- Around line 1075-1080: The swatch label arrays (absoluteSimpleNames,
absoluteChromaticNames, moveableSimpleNames, moveableChromaticNames,
chordDegreeNames) are hard-coded in updateSwatchesUI() so they don't retranslate
on language change; replace those literal QStrings by routing each caption
through the translation helper (e.g. muse::qtrc(...) or the existing translated
note-name utilities) before populating colorTextLabels, and ensure the same
change is applied to the other label block around the 1104-1108 region so
retranslateNoteColorSection() will update the UI labels correctly.
- Around line 834-1323: The note-color UI and logic currently live inside
classBegin() and capture many locals (defaultColorLabel, colorLabels[],
updatePresetFromState, updateSwatchesUI, plus handlers like the
m_noteColorChordRootBtn click lambda), making the method large and fragile;
refactor by extracting this whole block into one or more members: create a
dedicated NoteColorWidget (or member methods) that owns the widgets
(m_noteColorGroup, m_noteColorPresetCombo, m_noteColorSchemeCombo,
defaultColorLabel, colorLabels[], m_noteColorResetBtn, m_noteColorChordRootBtn,
etc.), move updatePresetFromState and updateSwatchesUI into private member
functions of that class (or EditStyle as private methods) and change the lambdas
to use member functions/signals so they no longer capture local variables; also
move the chord-root scoring logic (the lambda connected to
m_noteColorChordRootBtn) into a member method (e.g., handleChordRootColoring) so
it can access score APIs without capturing stack locals; finally update
classBegin() to simply construct the widget(s) and connect top-level signals,
keeping behavior identical while removing heavy local captures.
---
Duplicate comments:
In `@src/engraving/dom/accidental.cpp`:
- Around line 508-515: The code currently short-circuits on theme !=
NoteColoringScheme::OneColor so enabling the "apply to accidentals" flag is
ignored for the OneColor scheme; change the Pid::COLOR branch so that if note()
exists and note()->style().styleV(Sid::colorApplyToAccidental).toBool() is true
you return note()->getProperty(Pid::COLOR) regardless of the NoteColoringScheme
value (i.e., remove the theme != OneColor guard), falling back to
EngravingItem::propertyDefault(propertyId) otherwise; apply the same fix to the
analogous blocks in the nearby Pid::COLOR range (531-539) and to the similar
checks in articulation.cpp and beam.cpp so stems/dots/articulations behave
consistently.
In `@src/engraving/dom/beam.cpp`:
- Around line 659-671: The color resolution in Beam::color() currently uses
m_elements.front() which can be a rest; change both places that call
m_elements.front() (the block handling Pid::COLOR and the later similar block)
to scan m_elements for the first ChordRest* that returns true for isChord()
(e.g., iterate m_elements until finding cr && cr->isChord()), then call
chordRootNote(toChord(foundCr)) and proceed as before; ensure you still respect
NoteColoringScheme and colorApplyToBeam checks before returning
n->getProperty(Pid::COLOR).
🪄 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: 9d78a7a1-c143-46f5-adab-5d93623f2cc1
📒 Files selected for processing (10)
src/engraving/dom/accidental.cppsrc/engraving/dom/articulation.cppsrc/engraving/dom/beam.cppsrc/engraving/dom/note.cppsrc/engraving/dom/note.hsrc/engraving/dom/notedot.cppsrc/engraving/dom/stem.cppsrc/engraving/types/notecoloringscheme.hsrc/notationscene/widgets/editstyle.cppsrc/notationscene/widgets/editstyle.h
faiyazchowdhury
force-pushed
the
color_notes_with_moveabe_do_re_me
branch
3 times, most recently
from
a0cd56a to
20553d0
Compare
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engraving/dom/accidental.cpp (1)
495-511:⚠️ Potential issue | 🟡 MinorClarify doc wording for
Pid::COLORdefault behavior.At Line 497, “returns the parent note's color” is ambiguous with rendered
note()->color(). The code returnsnote()->getProperty(Pid::COLOR), which can differ under theme-derived coloring.✏️ Proposed doc fix
- * When `@p` propertyId is `@c` Pid::COLOR and `@c` Sid::colorApplyToAccidental is set, returns the parent note's color. + * When `@p` propertyId is `@c` Pid::COLOR and `@c` Sid::colorApplyToAccidental is set, + * returns the parent note's `@c` Pid::COLOR property value. + * (This may differ from rendered `@c` note()->color() in theme-derived coloring paths.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/dom/accidental.cpp` around lines 495 - 511, The docstring for Accidental::propertyDefault when handling Pid::COLOR is ambiguous; update it to state that when Sid::colorApplyToAccidental is set the function returns the parent note's COLOR property via note()->getProperty(Pid::COLOR) (which may be theme-derived) rather than the rendered color from note()->color(), and otherwise falls back to EngravingItem::propertyDefault; edit the comment to explicitly mention getProperty vs color() to avoid confusion.
🤖 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/tests/notecoloring_tests.cpp`:
- Around line 110-133: Add two regression tests: one using a non-4/4 window
(e.g., a 3/4 windowTicks value) that calls analyzeChordRoot with a chord whose
durations/positions exercise meter-dependent weighting, and a second test that
constructs a tied-score scenario for scoreNoteForChord (equal chromaCounts and
durations) to assert a stable/tiebreak ordering. Locate and modify tests using
analyzeChordRoot and scoreNoteForChord (tests analyzeChordRoot_simpleTriad and
scoreNoteForChord_lowestPitchBonus) to add these cases so they exercise the
meter-weighting and equal-score logic in notecoloringscheme.h.
In `@src/engraving/types/notecoloringscheme.h`:
- Around line 100-105: The code incorrectly derives quarter-note length from
windowTicks / CHORD_ROOT_SCORE_QUARTER_DIVISOR (quarterTicks) which only matches
4/4; modify the helper that computes chord-root scoring (the function that uses
quarterTicks, info.tickInWindow and CHORD_ROOT_SCORE_ON_BEAT_BONUS) to accept an
explicit beat length or meter info (e.g., beatTicks or timeSignature
numerator/denominator) as an additional parameter and use that to compute beat =
info.tickInWindow / beatTicks instead of inferring from windowTicks; update
callers to pass the actual beat length (derived from the score's time signature)
so the on-beat bonus is applied to true strong beats in meters like 3/4.
- Around line 17-19: The header notecoloringscheme.h uses std::pair (around the
code referencing the pair at line ~211) but does not include <utility>, making
the header non-self-contained; fix this by adding `#include` <utility> to the
include list at the top of notecoloringscheme.h (alongside <algorithm>,
<limits>, <vector>) so declarations and any functions or types that use
std::pair compile without relying on transitive includes.
- Around line 220-222: The sort of the scored vector in notecoloringscheme.h
uses std::sort which can reorder equal-score elements non-deterministically;
change the call in the lambda that sorts scored (the sort surrounding the lambda
comparing a.first) to use std::stable_sort instead so equal a.first values
preserve original order, ensuring deterministic behavior downstream in
topPitchClasses and findChordRoot.
In `@src/notationscene/widgets/editstyle.cpp`:
- Around line 1304-1308: The click handler connected to m_noteColorChordRootBtn
dereferences globalContext()->currentNotation() without checking for nullptr
which can crash; update the lambda in the connect call to first retrieve auto*
notation = globalContext()->currentNotation() and return early if notation is
null before calling notation->elements()->msScore(), then proceed to get Score*
score and keep the existing score null check; ensure you follow the same
defensive pattern used elsewhere in this file (e.g., lines around the other
handlers) so the lambda safely exits when currentNotation() is null.
---
Outside diff comments:
In `@src/engraving/dom/accidental.cpp`:
- Around line 495-511: The docstring for Accidental::propertyDefault when
handling Pid::COLOR is ambiguous; update it to state that when
Sid::colorApplyToAccidental is set the function returns the parent note's COLOR
property via note()->getProperty(Pid::COLOR) (which may be theme-derived) rather
than the rendered color from note()->color(), and otherwise falls back to
EngravingItem::propertyDefault; edit the comment to explicitly mention
getProperty vs color() to avoid confusion.
🪄 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: 3ea5a302-aac7-430b-912e-cbbc8dd64b3c
📒 Files selected for processing (19)
src/engraving/dom/accidental.cppsrc/engraving/dom/accidental.hsrc/engraving/dom/articulation.cppsrc/engraving/dom/articulation.hsrc/engraving/dom/beam.cppsrc/engraving/dom/beam.hsrc/engraving/dom/note.cppsrc/engraving/dom/note.hsrc/engraving/dom/notedot.cppsrc/engraving/dom/notedot.hsrc/engraving/dom/stem.cppsrc/engraving/dom/stem.hsrc/engraving/style/styledef.cppsrc/engraving/style/styledef.hsrc/engraving/tests/CMakeLists.txtsrc/engraving/tests/notecoloring_tests.cppsrc/engraving/types/notecoloringscheme.hsrc/notationscene/widgets/editstyle.cppsrc/notationscene/widgets/editstyle.h
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engraving/dom/note.cpp (1)
3493-3504:⚠️ Potential issue | 🟠 MajorKeep
propertyDefault(Pid::COLOR)aligned withNote::color()forOneColor.
Note::color()usesSid::defaultNoteColorforNoteColoringScheme::OneColor, but this branch still falls back toEngravingItem::propertyDefault(Pid::COLOR). Reset/default-color paths will therefore treat a different value as the canonical default than the renderer does.💡 Proposed fix
case Pid::COLOR: { const StaffType* st = staffType(); if (st && st->isTabStaff() && st->fretUseTextStyle()) { return style().styleV(Sid::tabFretNumberColor); } NoteColoringScheme theme = static_cast<NoteColoringScheme>(style().styleV(Sid::noteColorTheme).toInt()); - if (theme != NoteColoringScheme::OneColor) { - int colorIdx = noteColoringSwatchIndex(this); - Sid colorSid = static_cast<Sid>(static_cast<int>(Sid::noteColor0) + colorIdx); - return style().styleV(colorSid); + if (theme == NoteColoringScheme::OneColor) { + return style().styleV(Sid::defaultNoteColor); } - return EngravingItem::propertyDefault(propertyId); + int colorIdx = noteColoringSwatchIndex(this); + Sid colorSid = static_cast<Sid>(static_cast<int>(Sid::noteColor0) + colorIdx); + return style().styleV(colorSid); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/dom/note.cpp` around lines 3493 - 3504, The Pid::COLOR branch should match Note::color() for NoteColoringScheme::OneColor: instead of falling back to EngravingItem::propertyDefault(propertyId) when theme == NoteColoringScheme::OneColor, return style().styleV(Sid::defaultNoteColor). Update the case handling in note.cpp (the Pid::COLOR block that also uses noteColoringSwatchIndex and style().styleV(colorSid)) so the OneColor path returns style().styleV(Sid::defaultNoteColor) to keep reset/default-color behavior consistent with Note::color().
♻️ Duplicate comments (2)
src/engraving/tests/notecoloring_tests.cpp (1)
149-163:⚠️ Potential issue | 🟡 MinorThis still doesn't cover the
stable_sortregression.Line 156 makes C the lowest pitch, so
scoreCandscoreGare different by design. That means this test only rechecks the bass bonus already covered above, and it would still pass if equal-score ordering inanalyzeChordRoot()regressed.🧪 Replace it with a real equal-score ordering case
-//! `@test` Equal-score notes produce a deterministic root via stable_sort ordering. -TEST_F(Engraving_NoteColoringTests, scoreNoteForChord_tiebreakDeterministic) +//! `@test` Equal-score competing chromas keep input order through stable_sort(). +TEST_F(Engraving_NoteColoringTests, analyzeChordRoot_equalScoreOrderDeterministic) { - int chromaCounts[12] = {}; - chromaCounts[0] = 1; - chromaCounts[7] = 1; const int windowTicks = 960; - int lowestPitch = 48; - ChordDegreeNoteInfo noteC { 48, 240, 0 }; // C3, beat 1 - ChordDegreeNoteInfo noteG { 55, 240, 0 }; // G3, beat 1, same duration/position - int scoreC = scoreNoteForChord(noteC, windowTicks, lowestPitch, chromaCounts); - int scoreG = scoreNoteForChord(noteG, windowTicks, lowestPitch, chromaCounts); - // C is lowest pitch so it gets the bass bonus; scores must differ - EXPECT_GT(scoreC, scoreG); + std::vector<ChordDegreeNoteInfo> infos = { + { 54, 200, 0 }, // F#3 + { 66, 200, 0 }, // F#4 + { 78, 200, 0 }, // F#5 + { 48, 90, 0 }, // C3: bass bonus ties the F# score + }; + + EXPECT_EQ(analyzeChordRoot(infos, windowTicks), 6); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/tests/notecoloring_tests.cpp` around lines 149 - 163, Test currently makes C the lowest pitch so bass bonus differs scores; replace it with a true equal-score scenario by ensuring neither note receives the bass bonus and all other scoring inputs match so scoreNoteForChord(noteC, ...) == scoreNoteForChord(noteG, ...). Concretely: in the TEST_F (Engraving_NoteColoringTests, scoreNoteForChord_tiebreakDeterministic) set lowestPitch to a value below both notes (so neither is treated as the bass), keep identical duration/position/chromaCounts for both ChordDegreeNoteInfo entries, assert EXPECT_EQ(scoreC, scoreG), then construct the chord vector in the original order (noteC then noteG) and call analyzeChordRoot (or the routine that uses stable_sort) to verify the chosen root/resolved ordering is deterministic and preserves the original relative order (i.e., prefers the first item) — reference scoreNoteForChord, analyzeChordRoot, and the test name to locate and update the code.src/engraving/dom/note.cpp (1)
3358-3379:⚠️ Potential issue | 🟠 MajorDon't fall back to
upNote()for rootless voicings.Line 3379 still returns
nwhen the inferred measure root is absent from the current voicing. UnderNoteColoringScheme::ChordDegrees, that makes stems, beams, and articulations inherit the top-note color again instead of the chord-root color the PR is introducing. Return a root-colored surrogate/source instead of falling back toch->upNote().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/dom/note.cpp` around lines 3358 - 3379, chordRootNote currently falls back to ch->upNote() when the inferred measure root (rootChroma from getMeasureChordRoot) is not present in the chord, which causes top-note coloring to be used instead of the intended chord-root color under NoteColoringScheme::ChordDegrees; in chordRootNote(Chord* ch) detect the case where no candidate matches rootChroma and, instead of returning ch->upNote(), construct and return a root-colored surrogate/source note (a Note instance or source reference with the tonicPC/rootChroma pitch and the same styling/context as n so coloring logic applies correctly); use the existing variables (n, tonicPC, rootChroma) and style attributes (n->style(), Sid::noteColorTheme, Sid::colorNotesByConcertPitch) to create the surrogate so stems/beams/articulations inherit the chord-root color rather than the top-note color.
🤖 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/dom/notedot.cpp`:
- Around line 78-87: The propertyDefault logic in NoteDot::propertyDefault only
checks note() and thus misses dots attached to rests; update the method (and the
analogous method around the second block) to also check rest() and, if
rest()->style().styleV(Sid::colorApplyToDot).toBool(), return
rest()->getProperty(Pid::COLOR) so rest-owned dots inherit color the same way as
note-owned dots, otherwise fall back to
EngravingItem::propertyDefault(propertyId).
In `@src/notationscene/widgets/editstyle.cpp`:
- Around line 1147-1153: The OneColor branch currently hides the full-section
reset control (m_noteColorResetBtn) which prevents restoring the apply-to
checkboxes; update the OneColor handling in the schemeMode check (when
schemeMode == NoteColoringScheme::OneColor) to keep m_noteColorResetBtn visible
(do not call setVisible(false)) and ensure the reset handler invoked by
m_noteColorResetBtn restores the full note-color state including the apply-to
checkbox flags (same behavior as the general reset used in other schemes); apply
the same visibility fix to the equivalent block around the other occurrence (the
block covering the lines similar to 1208-1221) so the reset button remains
available in OneColor mode.
In `@src/notationscene/widgets/editstyle.h`:
- Around line 191-192: The comment for hasDefaultStyleValue(StyleId id) is
inverted: update the doc to reflect the implementation (which returns
defaultStyleValue(id) == styleValue(id)); change the description to say it
returns true when the score value for id equals the style default (not when it
differs), so any UI/reset-button logic reads correctly.
---
Outside diff comments:
In `@src/engraving/dom/note.cpp`:
- Around line 3493-3504: The Pid::COLOR branch should match Note::color() for
NoteColoringScheme::OneColor: instead of falling back to
EngravingItem::propertyDefault(propertyId) when theme ==
NoteColoringScheme::OneColor, return style().styleV(Sid::defaultNoteColor).
Update the case handling in note.cpp (the Pid::COLOR block that also uses
noteColoringSwatchIndex and style().styleV(colorSid)) so the OneColor path
returns style().styleV(Sid::defaultNoteColor) to keep reset/default-color
behavior consistent with Note::color().
---
Duplicate comments:
In `@src/engraving/dom/note.cpp`:
- Around line 3358-3379: chordRootNote currently falls back to ch->upNote() when
the inferred measure root (rootChroma from getMeasureChordRoot) is not present
in the chord, which causes top-note coloring to be used instead of the intended
chord-root color under NoteColoringScheme::ChordDegrees; in chordRootNote(Chord*
ch) detect the case where no candidate matches rootChroma and, instead of
returning ch->upNote(), construct and return a root-colored surrogate/source
note (a Note instance or source reference with the tonicPC/rootChroma pitch and
the same styling/context as n so coloring logic applies correctly); use the
existing variables (n, tonicPC, rootChroma) and style attributes (n->style(),
Sid::noteColorTheme, Sid::colorNotesByConcertPitch) to create the surrogate so
stems/beams/articulations inherit the chord-root color rather than the top-note
color.
In `@src/engraving/tests/notecoloring_tests.cpp`:
- Around line 149-163: Test currently makes C the lowest pitch so bass bonus
differs scores; replace it with a true equal-score scenario by ensuring neither
note receives the bass bonus and all other scoring inputs match so
scoreNoteForChord(noteC, ...) == scoreNoteForChord(noteG, ...). Concretely: in
the TEST_F (Engraving_NoteColoringTests,
scoreNoteForChord_tiebreakDeterministic) set lowestPitch to a value below both
notes (so neither is treated as the bass), keep identical
duration/position/chromaCounts for both ChordDegreeNoteInfo entries, assert
EXPECT_EQ(scoreC, scoreG), then construct the chord vector in the original order
(noteC then noteG) and call analyzeChordRoot (or the routine that uses
stable_sort) to verify the chosen root/resolved ordering is deterministic and
preserves the original relative order (i.e., prefers the first item) — reference
scoreNoteForChord, analyzeChordRoot, and the test name to locate and update the
code.
🪄 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: a3752b2e-4600-4ca1-9904-3de2c865bd28
📒 Files selected for processing (18)
src/engraving/dom/accidental.cppsrc/engraving/dom/accidental.hsrc/engraving/dom/articulation.cppsrc/engraving/dom/articulation.hsrc/engraving/dom/beam.cppsrc/engraving/dom/beam.hsrc/engraving/dom/note.cppsrc/engraving/dom/note.hsrc/engraving/dom/notedot.cppsrc/engraving/dom/notedot.hsrc/engraving/dom/stem.cppsrc/engraving/dom/stem.hsrc/engraving/style/styledef.cppsrc/engraving/style/styledef.hsrc/engraving/tests/notecoloring_tests.cppsrc/engraving/types/notecoloringscheme.hsrc/notationscene/widgets/editstyle.cppsrc/notationscene/widgets/editstyle.h
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/engraving/dom/notedot.cpp (1)
78-88:⚠️ Potential issue | 🟡 MinorHandle rest-owned dots in color inheritance paths.
The new logic only checks
note(). Dots attached to rests won’t inherit the parent rest color/default, so dotted rests can render inconsistently.🔧 Proposed fix
PropertyValue NoteDot::propertyDefault(Pid propertyId) const { if (propertyId == Pid::COLOR) { - if (note()) { - if (note()->style().styleV(Sid::colorApplyToDot).toBool()) { - return note()->getProperty(Pid::COLOR); - } + if (note() && note()->style().styleV(Sid::colorApplyToDot).toBool()) { + return note()->getProperty(Pid::COLOR); + } + if (rest()) { + return rest()->getProperty(Pid::COLOR); } } return EngravingItem::propertyDefault(propertyId); } @@ Color NoteDot::color() const { if (m_color == configuration()->defaultColor()) { - if (note()) { - if (note()->style().styleV(Sid::colorApplyToDot).toBool()) { - return note()->color(); - } + if (note() && note()->style().styleV(Sid::colorApplyToDot).toBool()) { + return note()->color(); + } + if (rest()) { + return rest()->color(); } } return m_color; }Also applies to: 97-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/dom/notedot.cpp` around lines 78 - 88, The NoteDot::propertyDefault currently only checks note() for color inheritance; modify it to also handle dots owned by rests by checking owner() (cast or query whether owner is a Rest) when note() is null, and if that Rest's style().styleV(Sid::colorApplyToDot).toBool() is true return the Rest's getProperty(Pid::COLOR); otherwise fall back to EngravingItem::propertyDefault(propertyId). Apply the identical change to the other similar propertyDefault block later in the file that implements the same color logic.src/notationscene/widgets/editstyle.h (1)
191-192:⚠️ Potential issue | 🟡 MinorClarify the
hasDefaultStyleValue()contract comment.The current doc says the value differs from the default, which reads opposite to the method name and makes reset-related code easy to misinterpret.
Proposed fix
- //! `@c` true when the score value for `@p` id differs from the style default. + //! `@c` true when the score value for `@p` id matches the style default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notationscene/widgets/editstyle.h` around lines 191 - 192, The comment for hasDefaultStyleValue(StyleId id) const is misleading (it currently says "true when the score value ... differs from the style default")—update the docstring to state the actual contract: that the method returns true when the score value for the given StyleId equals the style default (i.e., the value is the default and does not need resetting), and false when it differs from the default; reference hasDefaultStyleValue(StyleId id) const in the comment so callers and reset-related code read the intent unambiguously.src/notationscene/widgets/editstyle.cpp (1)
1147-1153:⚠️ Potential issue | 🟡 MinorKeep the reset button visible in
OneColormode.The reset button (
m_noteColorResetBtn) is hidden whenschemeMode == NoteColoringScheme::OneColor, but this button is the only control that restores apply-to checkboxes to their defaults. TheDefaultpreset does not reset these flags. Users who customize the apply-to checkboxes inOneColormode have no in-section way to restore defaults.🛡️ Proposed fix to keep reset button visible
+ m_noteColorResetBtn->setVisible(true); if (schemeMode == NoteColoringScheme::OneColor) { m_noteColorSwatchesContainer->setVisible(false); m_noteColorSwatchColorLabel->setVisible(true); defaultColorLabel->setVisible(true); - m_noteColorResetBtn->setVisible(false); m_noteColorChordRootBtn->setVisible(false); m_noteColorOneColorDescription->setVisible(true); } else { m_noteColorSwatchesContainer->setVisible(true); m_noteColorSwatchColorLabel->setVisible(false); defaultColorLabel->setVisible(false); - m_noteColorResetBtn->setVisible(true); m_noteColorChordRootBtn->setVisible(schemeMode == NoteColoringScheme::ChordDegrees); m_noteColorOneColorDescription->setVisible(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notationscene/widgets/editstyle.cpp` around lines 1147 - 1153, The reset button is being hidden in the OneColor branch, removing users' only way to restore apply-to checkboxes; in the NoteColoringScheme::OneColor branch (in editstyle.cpp) stop hiding m_noteColorResetBtn — either remove the m_noteColorResetBtn->setVisible(false) line or set it to true so the reset control remains visible, ensuring users can restore defaults for the apply-to checkboxes; keep the other visibility changes (m_noteColorSwatchesContainer, m_noteColorSwatchColorLabel, defaultColorLabel, m_noteColorChordRootBtn, m_noteColorOneColorDescription) as-is.
🤖 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/style/styledef.cpp`:
- Line 240: Replace the raw literal 0 used to initialize noteColorTheme in the
styleDef call with the explicit NoteColoringScheme enum constant that represents
the intended default (e.g., NoteColoringScheme::Default or whatever the project
defines) so the default does not depend on enum ordering; locate the styleDef
invocation for noteColorTheme and change the initializer from 0 to the
appropriate NoteColoringScheme::<DefaultConstant> value.
---
Duplicate comments:
In `@src/engraving/dom/notedot.cpp`:
- Around line 78-88: The NoteDot::propertyDefault currently only checks note()
for color inheritance; modify it to also handle dots owned by rests by checking
owner() (cast or query whether owner is a Rest) when note() is null, and if that
Rest's style().styleV(Sid::colorApplyToDot).toBool() is true return the Rest's
getProperty(Pid::COLOR); otherwise fall back to
EngravingItem::propertyDefault(propertyId). Apply the identical change to the
other similar propertyDefault block later in the file that implements the same
color logic.
In `@src/notationscene/widgets/editstyle.cpp`:
- Around line 1147-1153: The reset button is being hidden in the OneColor
branch, removing users' only way to restore apply-to checkboxes; in the
NoteColoringScheme::OneColor branch (in editstyle.cpp) stop hiding
m_noteColorResetBtn — either remove the m_noteColorResetBtn->setVisible(false)
line or set it to true so the reset control remains visible, ensuring users can
restore defaults for the apply-to checkboxes; keep the other visibility changes
(m_noteColorSwatchesContainer, m_noteColorSwatchColorLabel, defaultColorLabel,
m_noteColorChordRootBtn, m_noteColorOneColorDescription) as-is.
In `@src/notationscene/widgets/editstyle.h`:
- Around line 191-192: The comment for hasDefaultStyleValue(StyleId id) const is
misleading (it currently says "true when the score value ... differs from the
style default")—update the docstring to state the actual contract: that the
method returns true when the score value for the given StyleId equals the style
default (i.e., the value is the default and does not need resetting), and false
when it differs from the default; reference hasDefaultStyleValue(StyleId id)
const in the comment so callers and reset-related code read the intent
unambiguously.
🪄 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: 0f37b965-0405-4efb-83a4-f643fa3e3393
📒 Files selected for processing (19)
src/engraving/dom/accidental.cppsrc/engraving/dom/accidental.hsrc/engraving/dom/articulation.cppsrc/engraving/dom/articulation.hsrc/engraving/dom/beam.cppsrc/engraving/dom/beam.hsrc/engraving/dom/note.cppsrc/engraving/dom/note.hsrc/engraving/dom/notedot.cppsrc/engraving/dom/notedot.hsrc/engraving/dom/stem.cppsrc/engraving/dom/stem.hsrc/engraving/style/styledef.cppsrc/engraving/style/styledef.hsrc/engraving/tests/CMakeLists.txtsrc/engraving/tests/notecoloring_tests.cppsrc/engraving/types/notecoloringscheme.hsrc/notationscene/widgets/editstyle.cppsrc/notationscene/widgets/editstyle.h
faiyazchowdhury
force-pushed
the
color_notes_with_moveabe_do_re_me
branch
from
2d78353 to
d6d9cab
Compare
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/engraving/dom/accidental.cpp (1)
501-535:⚠️ Potential issue | 🔴 CriticalResetting color breaks inheritance for all affected elements when parent has explicit color.
propertyDefault(Pid::COLOR)returnsparent->getProperty(Pid::COLOR), which is the parent's stored color—not necessarily the theme default. WhenresetProperty(Pid::COLOR)is called, it stores this parent color directly intom_colorviasetColor(). This breaks the dynamic inheritance gate incolor(): oncem_coloris no longer equal toconfiguration()->defaultColor(), the method returns the stored value instead of callingparent->color(), permanently freezing the child's color even if the parent is later recolored.This affects all five elements that follow this pattern: Accidental, Articulation, Beam, NoteDot, and Stem. The inherited color will only be reestablished if the child's color is explicitly reset to the configuration default, which defeats the intent of color inheritance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/dom/accidental.cpp` around lines 501 - 535, propertyDefault currently returns the parent's stored COLOR (note()->getProperty(Pid::COLOR)), which causes resetProperty(Pid::COLOR) to store that concrete color via setColor and break dynamic inheritance used by color() (which only inherits when m_color == configuration()->defaultColor()). Change Accidental::propertyDefault(Pid::COLOR) to return the true default (i.e. call EngravingItem::propertyDefault(propertyId) or the configuration default) instead of returning the parent's stored property so that resetting COLOR restores the configuration default and re-enables note()->color() inheritance in Accidental::color(). Apply the same fix to the other classes using this pattern (Articulation, Beam, NoteDot, Stem) where propertyDefault returns parent->getProperty(Pid::COLOR).src/engraving/dom/note.cpp (1)
3569-3580:⚠️ Potential issue | 🟠 MajorPreserve explicit TAB fret color overrides during style changes.
This unconditionally writes
Sid::tabFretNumberColorback intoPid::COLOR. Any manually overridden note color on a text-style TAB fret will be lost wheneverstyleChanged()runs. Only restyle notes that are still styled/default forPid::COLOR.Proposed fix
void Note::styleChanged() { const StaffType* st = staffType(); - if (st && st->isTabStaff() && st->fretUseTextStyle()) { + if (st && st->isTabStaff() && st->fretUseTextStyle() && isStyled(Pid::COLOR)) { setProperty(Pid::COLOR, style().styleV(Sid::tabFretNumberColor)); } EngravingItem::styleChanged(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/dom/note.cpp` around lines 3569 - 3580, Note::styleChanged currently unconditionally overwrites Pid::COLOR with Sid::tabFretNumberColor for text-style TAB frets, which clobbers any manual color overrides; change Note::styleChanged so it only sets Pid::COLOR when the note's color is still the default/style-derived value (i.e., not a user override). In practice, before calling setProperty(Pid::COLOR, ...), check the note's current color/property state (using the existing property inspection API on Note, e.g., property(Pid::COLOR) / hasPropertyOverride or propertyIsDefault) and skip the setProperty when an explicit/user override exists; otherwise keep the existing flow and then call EngravingItem::styleChanged().
♻️ Duplicate comments (1)
src/notationscene/widgets/editstyle.cpp (1)
1206-1253:⚠️ Potential issue | 🟠 MajorKeep
noteColorThemesynchronized for every preset.Only the
ChordDegreesbranch updatesm_noteColorSchemeCombo. SelectingDefault,Boomwhackers, orFigurenotesrepaints the swatches but leaves the previous scheme active, so the preset dropdown can disagree with the actual coloring behavior saved inStyleId::noteColorTheme.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notationscene/widgets/editstyle.cpp` around lines 1206 - 1253, The note-color preset handler (connected to m_noteColorPresetCombo) updates swatches but only synchronizes m_noteColorSchemeCombo for ChordDegrees; fix by setting m_noteColorSchemeCombo to the matching NoteColoringScheme for every preset (Default, Boomwhackers, FigureNotes) using blockSignals(true)/setCurrentIndex(...)/blockSignals(false), call updateSwatchesUI(...) with the same NoteColoringScheme, and then call valueChanged(static_cast<int>(StyleId::noteColorTheme)) so the StyleId::noteColorTheme always reflects the chosen preset; change this logic inside the lambda connected to m_noteColorPresetCombo.
🤖 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/dom/note.cpp`:
- Around line 3552-3567: Note::color() currently returns a themed swatch when
m_color equals configuration()->defaultColor(), breaking callers that treat
color() as the raw "unstyled" sentinel (e.g., Note::setupAfterRead()). Restore
color() to return m_color unchanged when it equals
configuration()->defaultColor(); move the themed-lookup logic (using
Sid::noteColorTheme, noteColoringSwatchIndex, Sid::noteColor0 and
Sid::defaultNoteColor) into a new method (e.g., Note::effectiveColor() or
Note::themedColor()) that callers can use when they want the resolved theme
color, and update call sites that need the resolved color to use the new method
while leaving tests that check for the sentinel comparing against
configuration()->defaultColor() intact.
- Around line 3241-3276: The collector currently only picks segments whose
tick() lies inside [rangeStart,rangeEnd) and uses chord->actualTicks() as the
weight, which drops notes that overlap the slice from before rangeStart and
overweights ones that extend past rangeEnd; update
appendChordDegreeNoteInfosFromMeasure (and the similar block around the other
function) to include any Segment that overlaps the selection (i.e., seg.tick() <
rangeEnd && segEnd > rangeStart), compute segEnd = st + chord->actualTicks(),
compute overlap = min(segEnd, rangeEnd) - max(st, rangeStart) and convert that
overlap to ticks (use overlap.ticks()) as the weight instead of durTicks, and
set tickInWindow based on max(st, rangeStart) - windowOrigin so carried-in notes
are placed at the selection start rather than dropped or mis-positioned.
---
Outside diff comments:
In `@src/engraving/dom/accidental.cpp`:
- Around line 501-535: propertyDefault currently returns the parent's stored
COLOR (note()->getProperty(Pid::COLOR)), which causes resetProperty(Pid::COLOR)
to store that concrete color via setColor and break dynamic inheritance used by
color() (which only inherits when m_color == configuration()->defaultColor()).
Change Accidental::propertyDefault(Pid::COLOR) to return the true default (i.e.
call EngravingItem::propertyDefault(propertyId) or the configuration default)
instead of returning the parent's stored property so that resetting COLOR
restores the configuration default and re-enables note()->color() inheritance in
Accidental::color(). Apply the same fix to the other classes using this pattern
(Articulation, Beam, NoteDot, Stem) where propertyDefault returns
parent->getProperty(Pid::COLOR).
In `@src/engraving/dom/note.cpp`:
- Around line 3569-3580: Note::styleChanged currently unconditionally overwrites
Pid::COLOR with Sid::tabFretNumberColor for text-style TAB frets, which clobbers
any manual color overrides; change Note::styleChanged so it only sets Pid::COLOR
when the note's color is still the default/style-derived value (i.e., not a user
override). In practice, before calling setProperty(Pid::COLOR, ...), check the
note's current color/property state (using the existing property inspection API
on Note, e.g., property(Pid::COLOR) / hasPropertyOverride or propertyIsDefault)
and skip the setProperty when an explicit/user override exists; otherwise keep
the existing flow and then call EngravingItem::styleChanged().
---
Duplicate comments:
In `@src/notationscene/widgets/editstyle.cpp`:
- Around line 1206-1253: The note-color preset handler (connected to
m_noteColorPresetCombo) updates swatches but only synchronizes
m_noteColorSchemeCombo for ChordDegrees; fix by setting m_noteColorSchemeCombo
to the matching NoteColoringScheme for every preset (Default, Boomwhackers,
FigureNotes) using blockSignals(true)/setCurrentIndex(...)/blockSignals(false),
call updateSwatchesUI(...) with the same NoteColoringScheme, and then call
valueChanged(static_cast<int>(StyleId::noteColorTheme)) so the
StyleId::noteColorTheme always reflects the chosen preset; change this logic
inside the lambda connected to m_noteColorPresetCombo.
🪄 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: 30106215-0400-4c8c-97b0-3aead67ec6fd
📒 Files selected for processing (21)
src/engraving/dom/accidental.cppsrc/engraving/dom/accidental.hsrc/engraving/dom/articulation.cppsrc/engraving/dom/articulation.hsrc/engraving/dom/beam.cppsrc/engraving/dom/beam.hsrc/engraving/dom/note.cppsrc/engraving/dom/note.hsrc/engraving/dom/notedot.cppsrc/engraving/dom/notedot.hsrc/engraving/dom/stem.cppsrc/engraving/dom/stem.hsrc/engraving/style/styledef.cppsrc/engraving/style/styledef.hsrc/engraving/tests/CMakeLists.txtsrc/engraving/tests/notecoloring_tests.cppsrc/engraving/types/notecoloringscheme.hsrc/importexport/mnx/internal/import/mnximporter.cppsrc/notationscene/widgets/editstyle.cppsrc/notationscene/widgets/editstyle.hsrc/notationscene/widgets/editstyleutils.cpp
💤 Files with no reviewable changes (1)
- src/importexport/mnx/internal/import/mnximporter.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engraving/dom/note.cpp (1)
3573-3578:⚠️ Potential issue | 🟠 MajorDon't reset explicit TAB note colors on every style change.
This unconditionally writes
Pid::COLORfor every text-style TAB note. Any manually colored fret note will be overwritten the next time the user changes an unrelated style setting.Proposed fix
void Note::styleChanged() { const StaffType* st = staffType(); - if (st && st->isTabStaff() && st->fretUseTextStyle()) { + if (st && st->isTabStaff() && st->fretUseTextStyle() && isStyled(Pid::COLOR)) { setProperty(Pid::COLOR, style().styleV(Sid::tabFretNumberColor)); } EngravingItem::styleChanged(); }
♻️ Duplicate comments (3)
src/engraving/dom/note.cpp (2)
3241-3276:⚠️ Potential issue | 🟠 MajorCount overlapping notes, not just note onsets, in range analysis.
For the selection-based chord-root action, Line 3257 drops notes that started before
rangeStartbut still sound inside the slice, and Line 3270 weights notes with their full duration even when they extend pastrangeEnd. Partial-measure selections can therefore resolve to the wrong root.Proposed fix
static void appendChordDegreeNoteInfosFromMeasure(const Measure* measure, const Fraction& rangeStart, const Fraction& rangeEnd, const Fraction& windowOrigin, std::vector<ChordDegreeNoteInfo>& noteInfos) { @@ for (const Segment& seg : measure->segments()) { if (!seg.isChordRestType()) { continue; } Fraction st = seg.tick(); - if (st < rangeStart || st >= rangeEnd) { - continue; - } - int tickInWindow = (st - windowOrigin).ticks(); for (staff_idx_t staffIdx = 0; staffIdx < nstaves; ++staffIdx) { track_idx_t startTrack = staffIdx * VOICES; track_idx_t endTrack = startTrack + VOICES; for (track_idx_t t = startTrack; t < endTrack; ++t) { EngravingItem* el = seg.element(t); @@ } const Chord* chord = toChord(el); - int durTicks = chord->actualTicks().ticks(); + Fraction segEnd = st + chord->actualTicks(); + if (st >= rangeEnd || segEnd <= rangeStart) { + continue; + } + Fraction overlapStart = std::max(st, rangeStart); + Fraction overlapEnd = std::min(segEnd, rangeEnd); + int tickInWindow = (overlapStart - windowOrigin).ticks(); + int durTicks = (overlapEnd - overlapStart).ticks(); + if (durTicks <= 0) { + continue; + } for (const Note* n : chord->notes()) { noteInfos.push_back({ noteColoringEpitch(n), durTicks, tickInWindow }); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/dom/note.cpp` around lines 3241 - 3276, The function appendChordDegreeNoteInfosFromMeasure currently only includes notes whose segment start st is within [rangeStart, rangeEnd) and uses chord->actualTicks() unclipped, which ignores notes that began before rangeStart but sustain into the selection and overcounts notes that extend past rangeEnd; update the logic to (1) treat a chord as relevant if its sounding interval overlaps [rangeStart, rangeEnd) (i.e., if st + duration > rangeStart && st < rangeEnd), (2) compute the note's overlap duration as the intersection of [st, st + chord->actualTicks()) and [rangeStart, rangeEnd) and use that clipped duration (in ticks) instead of chord->actualTicks(), and (3) compute tickInWindow relative to the clipped onset used for window placement (e.g., max(st, rangeStart) - windowOrigin) before pushing into noteInfos; modify the loop in appendChordDegreeNoteInfosFromMeasure to apply these checks and replacements when building noteInfos.
3552-3567:⚠️ Potential issue | 🟠 MajorKeep
color()'s default-color sentinel semantics intact.
setupAfterRead()still usescolor() == configuration()->defaultColor()on Line 1602 to detect an unstyled note. Once this override returns themed swatches instead, TAB text-style frets can stop reapplyingSid::tabFretNumberColoron load and pick up note-theme colors instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/engraving/dom/note.cpp` around lines 3552 - 3567, The current color() replaces the default-color sentinel with themed swatches, breaking setupAfterRead()'s check (color() == configuration()->defaultColor()) used to detect unstyled notes; restore sentinel semantics by making color() always return m_color when set (i.e., do not substitute theme swatches), then move the themed-swatches logic into a new method (e.g., themedColor() or effectiveColorForRendering()) that contains the existing noteColoringSwatchIndex() + Sid::noteColor0/Sid::noteColorTheme/Sid::defaultNoteColor logic; update rendering/painting code to call the new themed method while leaving setupAfterRead() (and any logic comparing color() to configuration()->defaultColor()) unchanged so Sid::tabFretNumberColor handling still works.src/notationscene/widgets/editstyle.cpp (1)
1206-1253:⚠️ Potential issue | 🟠 MajorPreset selection still leaves the scheme stale for most presets.
Only the
ChordDegreesbranch updatesm_noteColorSchemeCombo. PickingDefault,Boomwhackers, orFigurenotesafter a different scheme keeps the oldnoteColorTheme, so the preset dropdown/palette can disagree with the actual automatic coloring mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notationscene/widgets/editstyle.cpp` around lines 1206 - 1253, The preset handler updates swatches but doesn't update m_noteColorSchemeCombo for Default, Boomwhackers, and FigureNotes, leaving the combo and automatic coloring mode out of sync; modify each preset branch to set m_noteColorSchemeCombo to the corresponding NoteColoringScheme value (use m_noteColorSchemeCombo->findData(static_cast<int>(...)), blockSignals(true)/setCurrentIndex(...)/blockSignals(false)) and then call updateSwatchesUI(...) with that same NoteColoringScheme (mirror the pattern used in the ChordDegrees branch that uses NoteColoringScheme::ChordDegrees).
🤖 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/notationscene/widgets/editstyle.cpp`:
- Around line 1276-1295: The reset routine incorrectly forces some "apply-to"
checkboxes to true (e.g. m_noteColorCbAccidental and m_noteColorCbDot) instead
of restoring the style's factory defaults; update the reset code around
m_noteColorCbAccidental, m_noteColorCbStem, m_noteColorCbArticulation,
m_noteColorCbDot, and m_noteColorCbBeam to read and apply the style defaults
(StyleId::colorApplyToAccidental, ::colorApplyToStem,
::colorApplyToArticulation, ::colorApplyToDot, ::colorApplyToBeam) or
setChecked(false) where the factory default is false, keep
blockSignals(true)/blockSignals(false) and still call
valueChanged(static_cast<int>(StyleId::...)) for each so the UI and underlying
style state stay consistent.
- Line 888: The code appends { StyleId::noteColorTheme, false,
m_noteColorSchemeCombo, nullptr } to styleWidgets which already registers
m_noteColorSchemeCombo with the generic signal mapper, so calling
valueChanged(...) again causes duplicate writes/undo entries; remove the
redundant valueChanged(...) call for StyleId::noteColorTheme where styleWidgets
is appended (and apply the same removal/guard in the analogous block that
touches m_noteColorSchemeCombo at the later occurrence) so the mapper-driven
currentIndexChanged remains the single source of truth (alternatively, if
needed, suppress emission via blockSignals or check for existing mapping before
calling valueChanged).
---
Duplicate comments:
In `@src/engraving/dom/note.cpp`:
- Around line 3241-3276: The function appendChordDegreeNoteInfosFromMeasure
currently only includes notes whose segment start st is within [rangeStart,
rangeEnd) and uses chord->actualTicks() unclipped, which ignores notes that
began before rangeStart but sustain into the selection and overcounts notes that
extend past rangeEnd; update the logic to (1) treat a chord as relevant if its
sounding interval overlaps [rangeStart, rangeEnd) (i.e., if st + duration >
rangeStart && st < rangeEnd), (2) compute the note's overlap duration as the
intersection of [st, st + chord->actualTicks()) and [rangeStart, rangeEnd) and
use that clipped duration (in ticks) instead of chord->actualTicks(), and (3)
compute tickInWindow relative to the clipped onset used for window placement
(e.g., max(st, rangeStart) - windowOrigin) before pushing into noteInfos; modify
the loop in appendChordDegreeNoteInfosFromMeasure to apply these checks and
replacements when building noteInfos.
- Around line 3552-3567: The current color() replaces the default-color sentinel
with themed swatches, breaking setupAfterRead()'s check (color() ==
configuration()->defaultColor()) used to detect unstyled notes; restore sentinel
semantics by making color() always return m_color when set (i.e., do not
substitute theme swatches), then move the themed-swatches logic into a new
method (e.g., themedColor() or effectiveColorForRendering()) that contains the
existing noteColoringSwatchIndex() +
Sid::noteColor0/Sid::noteColorTheme/Sid::defaultNoteColor logic; update
rendering/painting code to call the new themed method while leaving
setupAfterRead() (and any logic comparing color() to
configuration()->defaultColor()) unchanged so Sid::tabFretNumberColor handling
still works.
In `@src/notationscene/widgets/editstyle.cpp`:
- Around line 1206-1253: The preset handler updates swatches but doesn't update
m_noteColorSchemeCombo for Default, Boomwhackers, and FigureNotes, leaving the
combo and automatic coloring mode out of sync; modify each preset branch to set
m_noteColorSchemeCombo to the corresponding NoteColoringScheme value (use
m_noteColorSchemeCombo->findData(static_cast<int>(...)),
blockSignals(true)/setCurrentIndex(...)/blockSignals(false)) and then call
updateSwatchesUI(...) with that same NoteColoringScheme (mirror the pattern used
in the ChordDegrees branch that uses NoteColoringScheme::ChordDegrees).
🪄 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: 1f6759b7-d31d-4da5-8b2c-4731d6a16bfd
📒 Files selected for processing (21)
src/engraving/dom/accidental.cppsrc/engraving/dom/accidental.hsrc/engraving/dom/articulation.cppsrc/engraving/dom/articulation.hsrc/engraving/dom/beam.cppsrc/engraving/dom/beam.hsrc/engraving/dom/note.cppsrc/engraving/dom/note.hsrc/engraving/dom/notedot.cppsrc/engraving/dom/notedot.hsrc/engraving/dom/stem.cppsrc/engraving/dom/stem.hsrc/engraving/style/styledef.cppsrc/engraving/style/styledef.hsrc/engraving/tests/CMakeLists.txtsrc/engraving/tests/notecoloring_tests.cppsrc/engraving/types/notecoloringscheme.hsrc/importexport/mnx/internal/import/mnximporter.cppsrc/notationscene/widgets/editstyle.cppsrc/notationscene/widgets/editstyle.hsrc/notationscene/widgets/editstyleutils.cpp
💤 Files with no reviewable changes (1)
- src/importexport/mnx/internal/import/mnximporter.cpp
|
@faiyazchowdhury Thank you for starting on this! Very nice to see custom color schemes in action. Here's some feedback on what I've noticed. UI inconsistencies with the Figma spec:Please refer to Figma for exact sizes, paddings, and layout of this UI, because it does not match the spec yet.
Functional issues:
|
|
@faiyazchowdhury Ah, just noticed you've added commits since I downloaded a build. That feedback above is based on what I downloaded yesterday morning, just FYI, so maybe some of it is moot now. |
faiyazchowdhury
force-pushed
the
color_notes_with_moveabe_do_re_me
branch
from
8c3a01f to
1cc736d
Compare
Add OneColor, AbsolutePitch, MoveableDo coloring schemes with palette swatches (Sid::noteColor0-11), apply-to flags for accidentals, dots, stems, beams, and articulations, concert-pitch toggle, Boomwhackers and FigureNotes presets, reset button, and QScrollArea wrapping for the Notes page in the Style dialog. Made-with: Cursor
faiyazchowdhury
force-pushed
the
color_notes_with_moveabe_do_re_me
branch
from
1cc736d to
51ec8b0
Compare
Author
|
@avvvvve I added all your suggested changes. coderrabbitai keeps complaining about docstring coverage not being at 80%, but I am struggling to get it over 78.21% without adding unnecessary comments nor comments outside of my changes. I hope that's fine. |
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
3 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.
Resolves: #32778
Adds a Color section on the Notes page of Format → Style so users can choose note coloring schemes (including movable-do coloring), configure swatch colors, and control whether color applies to accidentals, stems, beams, articulations, and dots.