Conversation
miiizen
force-pushed
the
22622-offsetRefactor
branch
from
889a68b to
50019b0
Compare
src/engraving/dom/excerpt.cpp
Outdated
| @@ -775,10 +775,7 @@ static void processLinkedClone(EngravingItem* ne, Score* score, track_idx_t stra | |||
| { | |||
| // reset offset as most likely it will not fit | |||
| PropertyFlags f = ne->propertyFlags(Pid::OFFSET); | |||
| } | ||
|
|
||
| void TLayout::layoutFermata(const Fermata* item, Fermata::LayoutData* ldata) | ||
| void TLayout::layoutFermata(const Fermata* item, Fermata::LayoutData* ldata, const LayoutContext& ctx) |
There was a problem hiding this comment.
LayoutContext no longer needed
miiizen
force-pushed
the
22622-offsetRefactor
branch
from
50019b0 to
6b2b1bb
Compare
| if (id == Pid::PLACEMENT || id == Pid::HAIRPIN_TYPE) { | ||
| // first set property, then set offset for above/below if styled | ||
| changeProperties(this, id, v, ps); |
There was a problem hiding this comment.
This whole if block now looks redundant
|
I was wondering, since almost none of the offset style values use the X coordinate, how would it be to replace them with something like "(vertical) distance to staff"? |
miiizen
force-pushed
the
22622-offsetRefactor
branch
from
6b2b1bb to
99a0b1f
Compare
📝 WalkthroughWalkthroughThis PR refactors how MuseScore calculates default item positioning by introducing Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/notationscene/qml/MuseScore/NotationScene/styledialog/measurenumberspagemodel.cpp (1)
69-80:⚠️ Potential issue | 🟠 MajorEmit
measureNumberPosBelowChanged()when the text style changes.
measureNumberPosBelow()now returns a differentStyleItem*based onmeasureNumberTextStyle, but the model still only notifiesmeasureNumberPosAboveChanged(). After switching text styles, the below-position control can stay bound to the previous item until the page is rebuilt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e4d00f33-2367-44f3-8adb-ebf371299f16
📒 Files selected for processing (100)
src/engraving/compat/engravingcompat.cppsrc/engraving/compat/engravingcompat.hsrc/engraving/dom/articulation.cppsrc/engraving/dom/chord.cppsrc/engraving/dom/chordrest.cppsrc/engraving/dom/dynamic.cppsrc/engraving/dom/engravingitem.cppsrc/engraving/dom/engravingitem.hsrc/engraving/dom/engravingobject.cppsrc/engraving/dom/excerpt.cppsrc/engraving/dom/expression.cppsrc/engraving/dom/fermata.cppsrc/engraving/dom/fermata.hsrc/engraving/dom/fret.cppsrc/engraving/dom/gradualtempochange.cppsrc/engraving/dom/gradualtempochange.hsrc/engraving/dom/hairpin.cppsrc/engraving/dom/hairpin.hsrc/engraving/dom/harmonicmark.cppsrc/engraving/dom/harmonicmark.hsrc/engraving/dom/harmony.cppsrc/engraving/dom/letring.cppsrc/engraving/dom/letring.hsrc/engraving/dom/line.cppsrc/engraving/dom/lyrics.cppsrc/engraving/dom/measure.cppsrc/engraving/dom/noteline.cppsrc/engraving/dom/noteline.hsrc/engraving/dom/ottava.cppsrc/engraving/dom/ottava.hsrc/engraving/dom/palmmute.cppsrc/engraving/dom/palmmute.hsrc/engraving/dom/pedal.cppsrc/engraving/dom/pedal.hsrc/engraving/dom/pickscrape.cppsrc/engraving/dom/pickscrape.hsrc/engraving/dom/rasgueado.cppsrc/engraving/dom/rasgueado.hsrc/engraving/dom/segment.cppsrc/engraving/dom/spanner.cppsrc/engraving/dom/spanner.hsrc/engraving/dom/stafftype.cppsrc/engraving/dom/textbase.cppsrc/engraving/dom/textbase.hsrc/engraving/dom/textline.cppsrc/engraving/dom/textline.hsrc/engraving/dom/trill.cppsrc/engraving/dom/trill.hsrc/engraving/dom/vibrato.cppsrc/engraving/dom/vibrato.hsrc/engraving/dom/volta.cppsrc/engraving/dom/volta.hsrc/engraving/dom/whammybar.cppsrc/engraving/dom/whammybar.hsrc/engraving/editing/edit.cppsrc/engraving/rendering/score/autoplace.cppsrc/engraving/rendering/score/dynamicslayout.cppsrc/engraving/rendering/score/harmonylayout.cppsrc/engraving/rendering/score/lyricslayout.cppsrc/engraving/rendering/score/measurenumberlayout.cppsrc/engraving/rendering/score/systemlayout.cppsrc/engraving/rendering/score/textlayout.cppsrc/engraving/rendering/score/tlayout.cppsrc/engraving/rw/compat/compatutils.cppsrc/engraving/rw/compat/compatutils.hsrc/engraving/rw/read206/read206.cppsrc/engraving/rw/read400/tread.cppsrc/engraving/rw/read410/tread.cppsrc/engraving/rw/read460/tread.cppsrc/engraving/rw/write/twrite.cppsrc/engraving/style/refactorts.pysrc/engraving/style/textstyle.cppsrc/engraving/style/textstyle.hsrc/engraving/tests/chordsymbol_data/realize-concert-pitch-ref.mscxsrc/engraving/tests/chordsymbol_data/realize-jazz-ref.mscxsrc/engraving/tests/compat206_data/fermata-ref.mscxsrc/engraving/tests/compat206_data/frame_text2-ref.mscxsrc/engraving/tests/compat206_data/textstyles-ref.mscxsrc/engraving/tests/expression_data/expression-1-ref.mscxsrc/engraving/tests/measure_data/mmrest-ref.mscxsrc/engraving/tests/parts_data/part-all-appendmeasures.mscxsrc/engraving/tests/parts_data/part-all-insertmeasures.mscxsrc/engraving/tests/parts_data/part-all-parts.mscxsrc/engraving/tests/parts_data/part-all-uappendmeasures.mscxsrc/engraving/tests/parts_data/part-all-uinsertmeasures.mscxsrc/engraving/tests/parts_data/voices-ref.mscxsrc/engraving/tests/spanners_data/linecolor01-ref.mscxsrc/engraving/tests/spanners_data/smallstaff01-ref.mscxsrc/importexport/capella/tests/data/testText1.capx-ref.mscxsrc/importexport/capella/tests/data/testVolta1.capx-ref.mscxsrc/importexport/musicxml/tests/data/testPlacementOffsetDefaults_ref.mscxsrc/importexport/musicxml/tests/data/testSticking_ref.mscxsrc/notationscene/qml/MuseScore/NotationScene/styledialog/measurenumberspagemodel.cppsrc/notationscene/widgets/editstyle.cppsrc/playback/internal/playbackconfiguration.cppvtest/scores/default-position-1.mscxvtest/scores/default-position-2.mscxvtest/scores/default-position-3.mscxvtest/scores/default-position-4.mscxvtest/scores/default-position-5.mscx
💤 Files with no reviewable changes (11)
- src/engraving/dom/excerpt.cpp
- src/engraving/dom/chordrest.cpp
- src/engraving/rendering/score/autoplace.cpp
- src/engraving/dom/chord.cpp
- src/engraving/dom/articulation.cpp
- src/engraving/dom/segment.cpp
- src/engraving/dom/measure.cpp
- src/engraving/dom/line.cpp
- src/engraving/dom/vibrato.h
- src/engraving/dom/vibrato.cpp
- src/engraving/dom/harmony.cpp
| if (!staffText->offset().isNull()) { | ||
| setOffset(staffText->offset()); | ||
| setSnapToDynamics(false); | ||
| setPropertyFlags(Pid::OFFSET, PropertyFlags::UNSTYLED); | ||
| setPropertyFlags(Pid::SNAP_TO_DYNAMICS, PropertyFlags::UNSTYLED); | ||
| } |
There was a problem hiding this comment.
Don't key legacy offset migration off PointF::isNull().
If an old score explicitly wrote (0, 0) to cancel the old styled default offset, this now gets treated as “no offset”, so the override is skipped and snapToDynamics stays enabled. Please key this off legacy property state / serialized presence instead of coordinate zero.
| void Lyrics::setYRelativeToStaff(double y) | ||
| { | ||
| const double yOff = staffOffsetY(); | ||
| PointF offsetPos = defaultOffset(); | ||
| y += offsetPos.y(); | ||
| mutldata()->setPosY(y - chordRest()->pos().y() - yOff); |
There was a problem hiding this comment.
setYRelativeToStaff() and yRelativeToStaff() are now asymmetric.
Line 533 adds defaultOffset().y() in the setter, but the getter does not compensate for it. This breaks round-trip consistency and can shift values during edit/serialization flows.
💡 Proposed fix (restore inverse conversion)
double Lyrics::yRelativeToStaff() const
{
const double yOff = staffOffsetY();
- return pos().y() + chordRest()->pos().y() + yOff;
+ return pos().y() + chordRest()->pos().y() + yOff - defaultOffset().y();
}| if (item->score()->nstaves() <= 1 || item->anchorToEndOfPrevious() || !item->offset().isNull()) { | ||
| return; |
There was a problem hiding this comment.
Keep barline avoidance active for Y-only dynamic offsets.
This guard now disables horizontal barline-collision handling for any user offset. A dynamic nudged only vertically will therefore stop getting X clearance from surrounding barlines, even though manageBarlineCollisions() only changes X.
Suggested fix
- if (item->score()->nstaves() <= 1 || item->anchorToEndOfPrevious() || !item->offset().isNull()) {
+ if (item->score()->nstaves() <= 1 || item->anchorToEndOfPrevious()
+ || !muse::RealIsNull(item->offset().x())) {
return;
}// Add this include if the helper is not already visible transitively:
`#include` "realfn.h"
src/engraving/style/refactorts.py
Outdated
| import re | ||
| from pathlib import Path | ||
|
|
||
| FILE_PATH = "textstyle.cpp" # 👈 change this | ||
|
|
||
| def refactor_textstyle_block(block: str) -> str: | ||
| # Extract Offset Sid | ||
| offset_pattern = re.compile( | ||
| r'\{\s*TextStylePropertyType::Offset\s*,\s*(Sid::[A-Za-z0-9_]+)\s*,[^}]+\},?\n?' | ||
| ) | ||
| match = offset_pattern.search(block) | ||
|
|
||
| if not match: | ||
| return block # nothing to do | ||
|
|
||
| offset_sid = match.group(1) | ||
|
|
||
| # Remove Offset line | ||
| block = offset_pattern.sub('', block) | ||
|
|
||
| # Extract property list | ||
| inner_pattern = re.compile(r'\{\s*\{(.*?)\}\s*\}', re.DOTALL) | ||
| inner_match = inner_pattern.search(block) | ||
|
|
||
| if not inner_match: | ||
| return block | ||
|
|
||
| properties = inner_match.group(1).rstrip() | ||
|
|
||
| # Rebuild block | ||
| new_inner = ( | ||
| "{\n" | ||
| " { {\n" | ||
| f"{properties}\n" | ||
| " } },\n" | ||
| f" {{ {offset_sid}, {offset_sid} }}\n" | ||
| "}" | ||
| ) | ||
|
|
||
| return inner_pattern.sub(new_inner, block) | ||
|
|
||
|
|
||
| def main(): | ||
| path = Path(FILE_PATH) | ||
| content = path.read_text(encoding="utf-8") | ||
|
|
||
| # Match each TextStyle definition | ||
| pattern = re.compile( | ||
| r'const\s+TextStyle\s+\w+\s*\{\s*\{.*?\}\s*\};', | ||
| re.DOTALL | ||
| ) | ||
|
|
||
| new_content = pattern.sub( | ||
| lambda m: refactor_textstyle_block(m.group(0)), | ||
| content | ||
| ) | ||
|
|
||
| # Write back (or change to .write_text(...) for overwrite) | ||
| output_path = path.with_suffix(".refactored.cpp") | ||
| output_path.write_text(new_content, encoding="utf-8") | ||
|
|
||
| print(f"Done. Output written to: {output_path}") | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| main() No newline at end of file |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider removing or relocating this one-time migration script.
This Python script appears to be a one-time development tool for refactoring textstyle.cpp. Committing it to src/engraving/style/ may cause confusion since:
- It's not part of the runtime codebase
- The hardcoded
FILE_PATH = "textstyle.cpp"suggests single-use - After the refactor is complete, it serves no purpose
Consider either:
- Moving it to a
tools/orscripts/directory with documentation - Removing it from the commit if the refactoring is already applied
| textStyleOffset->setOffset(styleValue(ts->offsetSids.above).value<PointF>()); | ||
| resetTextStyleOffset->setEnabled(styleValue(ts->offsetSids.above) != defaultStyleValue(ts->offsetSids.above)); |
There was a problem hiding this comment.
Don't hard-code text-style offset editing to offsetSids.above.
These paths always read, write, and reset ts->offsetSids.above, but the dialog still has below-specific text styles such as measure-number-alternate. On those pages, the offset widget will reflect and update the wrong StyleId. Resolve the editable offset SID for the selected TextStyleType instead of assuming above.
Also applies to: 2488-2490, 2514-2516
| if (sourceType == AudioSourceType::MuseSampler) { | ||
| if (index == REVERB_CHANNEL_IDX) { | ||
| float lvl = musesamplerInfo()->defaultReverbLevel(instrumentSoundId); | ||
| float lvl = musesamplerInfo() ? musesamplerInfo()->defaultReverbLevel(instrumentSoundId) : 0.0; | ||
| return muse::RealIsNull(lvl) ? DEFAULT_VALUE : lvl; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good defensive null check; consider caching the pointer to avoid the double call.
The null check prevents a potential null pointer dereference, which is a good fix. However, musesamplerInfo() is called twice in the ternary expression. While likely inexpensive, storing the result in a local variable would be cleaner and safer.
♻️ Suggested improvement
if (sourceType == AudioSourceType::MuseSampler) {
if (index == REVERB_CHANNEL_IDX) {
- float lvl = musesamplerInfo() ? musesamplerInfo()->defaultReverbLevel(instrumentSoundId) : 0.0;
+ auto info = musesamplerInfo();
+ float lvl = info ? info->defaultReverbLevel(instrumentSoundId) : 0.0f;
return muse::RealIsNull(lvl) ? DEFAULT_VALUE : lvl;
}Note: Also changed 0.0 to 0.0f for type consistency with float lvl.
| <Expression> | ||
| <eid>jiez/v+5/XJ_1m9WGkuLIaN</eid> | ||
| <snapToDynamics>0</snapToDynamics> | ||
| <text>expression</text> | ||
| </Expression> | ||
| <Expression> | ||
| <direction>up</direction> | ||
| <eid>RTU3fI7NeoH_kfnUclpHawG</eid> | ||
| <snapToDynamics>0</snapToDynamics> | ||
| <text>expression</text> | ||
| </Expression> | ||
| <Rest> | ||
| <eid>LtyeAnb16mF_PGuIDPoe3nN</eid> | ||
| <durationType>measure</durationType> | ||
| <duration>4/4</duration> | ||
| </Rest> | ||
| </voice> | ||
| </Measure> | ||
| <Measure> | ||
| <eid>FlL/NX00iiJ_byYc3TP9ItL</eid> | ||
| <voice> | ||
| <Spanner type="HairPin"> | ||
| <HairPin> | ||
| <subtype>0</subtype> | ||
| <snapBefore>0</snapBefore> | ||
| <snapAfter>0</snapAfter> | ||
| <eid>Lz2zxh3ZHsE_PX3+/1LbHDM</eid> | ||
| </HairPin> | ||
| <next> | ||
| <location> | ||
| <measures>1</measures> | ||
| </location> | ||
| </next> | ||
| </Spanner> | ||
| <Spanner type="HairPin"> | ||
| <HairPin> | ||
| <subtype>0</subtype> | ||
| <direction>up</direction> | ||
| <snapBefore>0</snapBefore> | ||
| <snapAfter>0</snapAfter> | ||
| <eid>g6GkswO8l8K_Kro/I/eWfPB</eid> | ||
| </HairPin> | ||
| <next> | ||
| <location> | ||
| <measures>1</measures> | ||
| </location> | ||
| </next> | ||
| </Spanner> | ||
| <Rest> | ||
| <eid>w7D+1q5phFL_jTPvlHpVo5D</eid> | ||
| <durationType>measure</durationType> | ||
| <duration>4/4</duration> | ||
| </Rest> | ||
| </voice> | ||
| </Measure> | ||
| <Measure> | ||
| <eid>iB/GisSI5UE_i3pIjGJeXIB</eid> | ||
| <voice> | ||
| <Dynamic> | ||
| <subtype>mp</subtype> | ||
| <velocity>64</velocity> | ||
| <eid>2zNSZGrSVSM_//awvq7XUfL</eid> | ||
| </Dynamic> | ||
| <Dynamic> | ||
| <subtype>mp</subtype> | ||
| <velocity>64</velocity> | ||
| <direction>up</direction> | ||
| <eid>rpR/ZyRmfHD_CoP6G2gZR2D</eid> | ||
| </Dynamic> | ||
| <Spanner type="HairPin"> | ||
| <prev> | ||
| <location> | ||
| <measures>-1</measures> | ||
| </location> | ||
| </prev> | ||
| </Spanner> | ||
| <Spanner type="HairPin"> | ||
| <prev> | ||
| <location> | ||
| <measures>-1</measures> | ||
| </location> | ||
| </prev> | ||
| </Spanner> | ||
| <Rest> | ||
| <eid>1kCpOKMB0NI_GDnexI4eJqE</eid> | ||
| <durationType>measure</durationType> | ||
| <duration>4/4</duration> | ||
| </Rest> | ||
| </voice> | ||
| </Measure> | ||
| <Measure> | ||
| <eid>e1gSdK1yOxC_Nz9sGur42DO</eid> | ||
| <voice> | ||
| <Spanner type="HairPin"> | ||
| <HairPin> | ||
| <subtype>2</subtype> | ||
| <eid>Ar83OOycfoO_1O7VFDxOCLK</eid> | ||
| </HairPin> | ||
| <next> | ||
| <location> | ||
| <fractions>1/1</fractions> | ||
| </location> | ||
| </next> | ||
| </Spanner> | ||
| <Spanner type="HairPin"> | ||
| <HairPin> | ||
| <subtype>2</subtype> | ||
| <direction>up</direction> | ||
| <eid>RG9fa0lvNXD_iM2Nf4BMfoH</eid> | ||
| </HairPin> | ||
| <next> | ||
| <location> | ||
| <fractions>1/1</fractions> | ||
| </location> | ||
| </next> | ||
| </Spanner> | ||
| <Rest> | ||
| <eid>v0hbUkGHgyP_h8rldg8BXkE</eid> | ||
| <durationType>measure</durationType> | ||
| <duration>4/4</duration> | ||
| </Rest> | ||
| <Spanner type="HairPin"> | ||
| <prev> | ||
| <location> | ||
| <fractions>-1/1</fractions> | ||
| </location> | ||
| </prev> | ||
| </Spanner> | ||
| <Spanner type="HairPin"> | ||
| <prev> | ||
| <location> | ||
| <fractions>-1/1</fractions> | ||
| </location> | ||
| </prev> | ||
| </Spanner> | ||
| <Spanner type="HairPin"> | ||
| <prev> | ||
| <location> | ||
| <fractions>-1/1</fractions> | ||
| </location> | ||
| </prev> | ||
| </Spanner> | ||
| <Spanner type="HairPin"> | ||
| <prev> | ||
| <location> | ||
| <fractions>-1/1</fractions> | ||
| </location> | ||
| </prev> | ||
| </Spanner> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add at least one explicit non-zero user offset case in this fixture.
The current score validates default placement behavior well, but it doesn’t directly validate the new contract that user offset is independent from internal/default positioning. Add one element with explicit <offset><x>..</x><y>..</y></offset> (e.g., one Dynamic and one HairPin), so this test can catch regressions where migration or layout accidentally rebases user offsets.
| <Spanner type="PalmMute"> | ||
| <prev> | ||
| <location> | ||
| <fractions>-1/1</fractions> | ||
| </location> | ||
| </prev> | ||
| </Spanner> | ||
| <Spanner type="PalmMute"> | ||
| <prev> | ||
| <location> | ||
| <fractions>-1/1</fractions> | ||
| </location> | ||
| </prev> | ||
| </Spanner> | ||
| <Spanner type="PalmMute"> | ||
| <prev> | ||
| <location> | ||
| <fractions>-1/1</fractions> | ||
| </location> | ||
| </prev> | ||
| </Spanner> | ||
| <Spanner type="PalmMute"> | ||
| <prev> | ||
| <location> | ||
| <fractions>-1/1</fractions> | ||
| </location> | ||
| </prev> | ||
| </Spanner> |
There was a problem hiding this comment.
Fix mismatched PalmMute spanner closures (orphan prev entries).
There are 4 closing PalmMute <prev> entries, but only 2 opening PalmMute spanners (Line 443-462). This leaves unmatched closures and can make the fixture semantically invalid/noisy for regression assertions.
Suggested fix (remove extra unmatched closures)
<Spanner type="PalmMute">
<prev>
<location>
<fractions>-1/1</fractions>
</location>
</prev>
</Spanner>
<Spanner type="PalmMute">
<prev>
<location>
<fractions>-1/1</fractions>
</location>
</prev>
</Spanner>
- <Spanner type="PalmMute">
- <prev>
- <location>
- <fractions>-1/1</fractions>
- </location>
- </prev>
- </Spanner>
- <Spanner type="PalmMute">
- <prev>
- <location>
- <fractions>-1/1</fractions>
- </location>
- </prev>
- </Spanner>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Spanner type="PalmMute"> | |
| <prev> | |
| <location> | |
| <fractions>-1/1</fractions> | |
| </location> | |
| </prev> | |
| </Spanner> | |
| <Spanner type="PalmMute"> | |
| <prev> | |
| <location> | |
| <fractions>-1/1</fractions> | |
| </location> | |
| </prev> | |
| </Spanner> | |
| <Spanner type="PalmMute"> | |
| <prev> | |
| <location> | |
| <fractions>-1/1</fractions> | |
| </location> | |
| </prev> | |
| </Spanner> | |
| <Spanner type="PalmMute"> | |
| <prev> | |
| <location> | |
| <fractions>-1/1</fractions> | |
| </location> | |
| </prev> | |
| </Spanner> | |
| <Spanner type="PalmMute"> | |
| <prev> | |
| <location> | |
| <fractions>-1/1</fractions> | |
| </location> | |
| </prev> | |
| </Spanner> | |
| <Spanner type="PalmMute"> | |
| <prev> | |
| <location> | |
| <fractions>-1/1</fractions> | |
| </location> | |
| </prev> | |
| </Spanner> |
Offsets which used to be "styled" are now just zero
Offset is not styled and its default is zero
TODO: this is being calculated at a bad time - before layout of the line, so we have no shape. The best we can do is use the info we already have (offset, min distance, staffheight) to check a single point. Previously, we were only checking offset
This is now decoupled from the property value and can have values for above and below. Currently, where two values are available, only the posAbove value is used in the style dialog. However, now it will be easy to expose posBelow in the text styles menu as and when we want to.
miiizen
force-pushed
the
22622-offsetRefactor
branch
from
99a0b1f to
3bd8309
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
src/notationscene/widgets/editstyle.cpp (1)
2457-2458:⚠️ Potential issue | 🟠 MajorResolve the active text-style offset SID instead of hard-coding
above.These paths still bind the offset widget to
ts->offsetSids.aboveunconditionally, so below-oriented text-style pages keep reading, writing, and resetting the wrong style value.measure-number-alternateis still selectable here, and editing its offset will hit the above SID instead of the below SID. Use one helper to resolve the correct offset SID for the selected text-style page and reuse it in all three places.Also applies to: 2488-2490, 2514-2516
src/engraving/rendering/score/systemlayout.cpp (1)
1848-1849:⚠️ Potential issue | 🟡 MinorOnly let Y nudges opt out of Y alignment.
This block only rewrites
posY. With the current guard, a horizontal-only user nudge skips alignment entirely, so aligned pedal segments can still end up vertically inconsistent even though their Y was never touched.Suggested fix
- if (!ss->offset().isNull()) { + if (!muse::RealIsNull(ss->offset().y())) { continue; }src/engraving/rw/read400/tread.cpp (1)
2958-2960:⚠️ Potential issue | 🟠 MajorPre-302 spanner parent offsets still bypass the fallback.
Line 2959 only migrates
LineSegment. ParentSLine/TextLineBaseobjects still deserializePid::OFFSETthroughreadItemProperties(), butTRead::read(SLine*)andTRead::read(TextLineBase*)never runcompat::CompatUtils::migrateOffsetPre302()after their last property is parsed. Pre-302 Hairpin/Pedal/Ottava/TextLine parents will therefore remain in legacy default+user space and load shifted.🩹 Suggested fix
void TRead::read(SLine* l, XmlReader& e, ReadContext& ctx) { l->eraseSpannerSegments(); @@ while (e.readNextStartElement()) { if (!readProperties(l, e, ctx)) { e.unknown(); } } + + compat::CompatUtils::migrateOffsetPre302(l, ctx.mscVersion()); } @@ void TRead::read(TextLineBase* b, XmlReader& e, ReadContext& ctx) { b->eraseSpannerSegments(); @@ while (e.readNextStartElement()) { if (!readProperties(b, e, ctx)) { e.unknown(); } } + + compat::CompatUtils::migrateOffsetPre302(b, ctx.mscVersion()); compat::CompatUtils::resetHookHeightSign(b); compat::CompatUtils::setTextLineTextPositionFromAlign(b); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d923f184-00f1-496a-802a-3963f2502c29
📒 Files selected for processing (60)
src/engraving/compat/engravingcompat.cppsrc/engraving/compat/engravingcompat.hsrc/engraving/dom/chordrest.cppsrc/engraving/dom/dynamic.cppsrc/engraving/dom/engravingitem.cppsrc/engraving/dom/engravingitem.hsrc/engraving/dom/engravingobject.cppsrc/engraving/dom/excerpt.cppsrc/engraving/dom/expression.cppsrc/engraving/dom/fermata.cppsrc/engraving/dom/fermata.hsrc/engraving/dom/fret.cppsrc/engraving/dom/gradualtempochange.cppsrc/engraving/dom/harmony.cppsrc/engraving/dom/line.cppsrc/engraving/dom/measure.cppsrc/engraving/dom/noteline.cppsrc/engraving/dom/noteline.hsrc/engraving/dom/segment.cppsrc/engraving/dom/spanner.cppsrc/engraving/dom/spanner.hsrc/engraving/dom/stafftype.cppsrc/engraving/dom/textbase.cppsrc/engraving/dom/textbase.hsrc/engraving/dom/trill.cppsrc/engraving/dom/trill.hsrc/engraving/editing/edit.cppsrc/engraving/rendering/score/autoplace.cppsrc/engraving/rendering/score/lyricslayout.cppsrc/engraving/rendering/score/measurenumberlayout.cppsrc/engraving/rendering/score/systemlayout.cppsrc/engraving/rendering/score/tlayout.cppsrc/engraving/rw/compat/compatutils.cppsrc/engraving/rw/compat/compatutils.hsrc/engraving/rw/read206/read206.cppsrc/engraving/rw/read400/tread.cppsrc/engraving/rw/write/twrite.cppsrc/engraving/style/textstyle.cppsrc/engraving/style/textstyle.hsrc/engraving/tests/chordsymbol_data/realize-concert-pitch-ref.mscxsrc/engraving/tests/chordsymbol_data/realize-jazz-ref.mscxsrc/engraving/tests/compat206_data/fermata-ref.mscxsrc/engraving/tests/compat206_data/frame_text2-ref.mscxsrc/engraving/tests/compat206_data/textstyles-ref.mscxsrc/engraving/tests/expression_data/expression-1-ref.mscxsrc/engraving/tests/measure_data/mmrest-ref.mscxsrc/engraving/tests/parts_data/part-all-appendmeasures.mscxsrc/engraving/tests/parts_data/part-all-insertmeasures.mscxsrc/engraving/tests/parts_data/part-all-parts.mscxsrc/engraving/tests/parts_data/part-all-uappendmeasures.mscxsrc/engraving/tests/parts_data/part-all-uinsertmeasures.mscxsrc/engraving/tests/parts_data/voices-ref.mscxsrc/engraving/tests/spanners_data/linecolor01-ref.mscxsrc/engraving/tests/spanners_data/smallstaff01-ref.mscxsrc/importexport/capella/tests/data/testText1.capx-ref.mscxsrc/importexport/capella/tests/data/testVolta1.capx-ref.mscxsrc/importexport/musicxml/tests/data/testPlacementOffsetDefaults_ref.mscxsrc/importexport/musicxml/tests/data/testSticking_ref.mscxsrc/notationscene/qml/MuseScore/NotationScene/styledialog/measurenumberspagemodel.cppsrc/notationscene/widgets/editstyle.cpp
💤 Files with no reviewable changes (7)
- src/engraving/dom/chordrest.cpp
- src/engraving/dom/excerpt.cpp
- src/engraving/rendering/score/autoplace.cpp
- src/engraving/dom/harmony.cpp
- src/engraving/dom/measure.cpp
- src/engraving/dom/line.cpp
- src/engraving/dom/segment.cpp
| double staffHeight = staff() && placeBelow() ? staff()->staffHeight(tick()) : 0.0; | ||
| double tempoChangePos = staffHeight + defaultOffset().y() + (autoplace() ? absoluteFromSpatium(minDistance()) : 0.0); | ||
| RectF rehearsalMarkBbox = rehearsalMark ? rehearsalMark->ldata()->bbox().translated(rehearsalMark->pos()) : RectF(); | ||
|
|
||
| const bool sameSide = placeAbove() == rehearsalMark->placeAbove(); | ||
| const bool collision = placeAbove() ? muse::RealIsEqualOrMore(rehearsalMarkBbox.bottom(), thisBbox.top()) : muse::RealIsEqualOrLess( | ||
| rehearsalMarkBbox.top(), thisBbox.bottom()); | ||
| const bool collision = muse::RealIsEqualOrMore(rehearsalMarkBbox.bottom(), tempoChangePos) && muse::RealIsEqualOrLess( | ||
| rehearsalMarkBbox.top(), tempoChangePos); |
There was a problem hiding this comment.
Use a placement-aware sign for minDistance() in the rehearsal-mark probe.
For above tempo changes, autoplace moves the segment farther up, but this code always adds minDistance(). That evaluates the collision at a lower Y than the actual segment, so linePos() can miss or misapply the horizontal dodge around rehearsal marks.
Suggested fix
- double staffHeight = staff() && placeBelow() ? staff()->staffHeight(tick()) : 0.0;
- double tempoChangePos = staffHeight + defaultOffset().y() + (autoplace() ? absoluteFromSpatium(minDistance()) : 0.0);
+ double staffHeight = staff() && placeBelow() ? staff()->staffHeight(tick()) : 0.0;
+ double minDistanceOffset = autoplace() ? absoluteFromSpatium(minDistance()) : 0.0;
+ if (placeAbove()) {
+ minDistanceOffset = -minDistanceOffset;
+ }
+ double tempoChangePos = staffHeight + defaultOffset().y() + minDistanceOffset;| PointF SpannerSegment::defaultOffset() const | ||
| { | ||
| return spanner()->defaultOffset(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "== offset-scaling hooks =="
rg -n -C2 'offsetIsSpatiumDependent|defaultOffset\(\) const' src/engraving/dom
echo
echo "== spanner/segment constructors and flags =="
rg -n -C3 'Spanner::Spanner|SLine::SLine|LineSegment::LineSegment|SpannerSegment::SpannerSegment|ElementFlag::ON_STAFF' src/engraving/dom
echo
echo "== current delegation =="
sed -n '284,310p' src/engraving/dom/engravingitem.cpp | cat -n
sed -n '206,214p' src/engraving/dom/spanner.cpp | cat -nRepository: musescore/MuseScore
Length of output: 44997
Fix segment offset scaling: use segment context instead of parent spanner.
EngravingItem::defaultOffset() scales offsets based on the receiver's offsetIsSpatiumDependent(). However, SpannerSegment::defaultOffset() delegates to spanner()->defaultOffset(), which applies scaling in the parent's context rather than the segment's.
This is a problem because:
SpannerSegmentconstructors default toElementFlag::ON_STAFF | ElementFlag::MOVABLESpannerconstructors do not setElementFlag::ON_STAFFby default- Therefore, a segment's
offsetIsSpatiumDependent()may return true while the parent's returns false - Delegating causes the offset to be scaled incorrectly (with or without spatium depending on parent, not segment)
Compute the default offset from the spanner's offset SID in SpannerSegment instead of delegating:
PointF SpannerSegment::defaultOffset() const
{
Sid styleId = spanner()->offsetSid();
if (styleId == Sid::NOSTYLE) {
return PointF();
}
PointF offsetPos = style().value(styleId).value<PointF>();
if (offsetIsSpatiumDependent()) {
offsetPos *= spatium();
} else {
offsetPos *= DPMM;
}
return offsetPos;
}| PointF TextBase::defaultOffset() const | ||
| { | ||
| if (parent()->isTextLineBaseSegment()) { | ||
| return PointF(); | ||
| } | ||
| return Sid::NOSTYLE; | ||
|
|
||
| return EngravingItem::defaultOffset(); | ||
| } |
There was a problem hiding this comment.
Potential null pointer dereference when checking parent type.
The condition at line 2844 calls parent()->isTextLineBaseSegment() without first checking if parent() is null. If this method is called on a TextBase item that has no parent, it will crash.
🛡️ Proposed fix to add null check
PointF TextBase::defaultOffset() const
{
- if (parent()->isTextLineBaseSegment()) {
+ if (parent() && parent()->isTextLineBaseSegment()) {
return PointF();
}
return EngravingItem::defaultOffset();
}| if (melisma || !endLyrics) { | ||
| Lyrics* nextLyrics = findNextLyrics(endChordRest, item->verse()); | ||
| ldata->setPosY(nextLyrics ? nextLyrics->offset().y() : startLyrics->offset().y()); | ||
| const Lyrics* nextLyrics = findNextLyrics(endChordRest, item->verse()); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check all usages of findNextLyrics to verify const-correctness
rg -n "findNextLyrics" --type cpp -C 2Repository: musescore/MuseScore
Length of output: 1867
🏁 Script executed:
# Get more context around both usages of nextLyrics
sed -n '840,850p' src/engraving/rendering/score/lyricslayout.cpp
echo "---"
sed -n '858,870p' src/engraving/rendering/score/lyricslayout.cppRepository: musescore/MuseScore
Length of output: 1079
🏁 Script executed:
# Search for defaultOffset method definition to check if it's const
rg -n "defaultOffset\s*\(" src/engraving -A 1 --type cpp | head -20Repository: musescore/MuseScore
Length of output: 1340
🏁 Script executed:
# Verify all usages are accounted for and whether any modify the pointer
rg -n "findNextLyrics" --type cpp -B 3 -A 3Repository: musescore/MuseScore
Length of output: 2620
Update findNextLyrics return type to const Lyrics* and update both call sites for consistency.
The function returns Lyrics* (non-const), but is stored as const Lyrics* at line 861 and Lyrics* at line 842. Both call sites only read from the pointer (calling const methods like defaultOffset() and offset()), so updating the return type to const Lyrics* would better reflect intent. This requires updating both lines 842 and 861 to declare the variable as const Lyrics*.
Suggested changes
-Lyrics* LyricsLayout::findNextLyrics(const ChordRest* endChordRest, int verseNumber)
+const Lyrics* LyricsLayout::findNextLyrics(const ChordRest* endChordRest, int verseNumber)Also update line 842:
- Lyrics* nextLyrics = findNextLyrics(endChordRest, item->verse());
+ const Lyrics* nextLyrics = findNextLyrics(endChordRest, item->verse());| Staff* staff = item->staff(); | ||
| PointF offsetPos = item->defaultOffset() * (staff ? staff->staffMag(item->tick()) : 1.0); | ||
|
|
||
| ldata->move(offsetPos); |
There was a problem hiding this comment.
Use the effective staff consistently for measure-number offsets.
layoutMeasureNumberBase() already resolves the effective staff, but Lines 94-95 and 123-124 still scale defaultOffset() from item->staff(), and Line 145 still uses item->staff()->staffHeight(). When a measure number/MM-rest range is rerouted to a different visible staff, the horizontal/vertical default offset and below-staff baseline come from the wrong staff.
Suggested fix
- Staff* staff = item->staff();
- PointF offsetPos = item->defaultOffset() * (staff ? staff->staffMag(item->tick()) : 1.0);
+ staff_idx_t effectiveStaffIdx = item->effectiveStaffIdx();
+ Staff* staff = effectiveStaffIdx != muse::nidx ? item->score()->staff(effectiveStaffIdx) : nullptr;
+ PointF offsetPos = item->defaultOffset() * (staff ? staff->staffMag(item->tick()) : 1.0);- yoff += item->staff() ? item->staff()->staffHeight() : 0.0;
+ yoff += staff ? staff->staffHeight() : 0.0;Also applies to: 123-126, 135-145
| void TLayout::doLayoutGradualTempoChangeSegment(GradualTempoChangeSegment* item, LayoutContext& ctx) | ||
| { | ||
| GradualTempoChangeSegment::LayoutData* ldata = item->mutldata(); | ||
| ldata->setPosY(0.0); |
There was a problem hiding this comment.
Reset X before re-laying out snapped tempo-change segments.
manageTempoChangeSnapping() can re-enter doLayoutGradualTempoChangeSegment() for the preceding segment. Line 2757 only clears Y, but Line 5812 now reapplies the full defaultOffset(). Any non-zero default X offset, or stale X from the prior pass, will accumulate on each relayout and shift the snapped segment horizontally.
Also applies to: 5812-5813
|
|
||
| compat::CompatUtils::migrateOffsetPre302(l, ctx.mscVersion()); | ||
| } |
There was a problem hiding this comment.
Use the new default-position API in the legacy lyrics fallback.
The pre-3.1 branch immediately above Line 2948 still reads propertyDefault(Pid::OFFSET). After this refactor that default is always (0, 0), so old autoplace lyrics will stop restoring their historical vertical baseline and load incorrectly. Read the legacy default from defaultOffset() (or the new internal-position helper) instead.
🩹 Suggested fix
- if (l->autoplace() && !version.isEmpty() && version < u"3.1") {
- PointF off = l->propertyDefault(Pid::OFFSET).value<PointF>();
+ if (l->autoplace() && !version.isEmpty() && version < u"3.1") {
+ PointF off = l->defaultOffset();
l->ryoffset() = off.y();
}| if (!item->offset().isNull()) { | ||
| writeProperty(item, xml, Pid::OFFSET); | ||
| } |
There was a problem hiding this comment.
Avoid serializing OFFSET twice for line-based items.
This generic branch now also hits objects that still have bespoke offset writers. Line 3287 already writes Pid::OFFSET for TextLine, and Lines 1629-1632 already serialize segment offsets inside writeProperties(const SLine*), so nudged text lines and user-modified line segments will emit duplicate <offset> tags in the same XML node.
| for (TextStyleType textStyleType : allTextStyles()) { | ||
| const TextStyle* style = textStyle(textStyleType); | ||
| int offsetPropertyIdx = static_cast<int>(TextStylePropertyType::Offset) - 1; | ||
| addStyleId(style->at(offsetPropertyIdx).sid); | ||
| addStyleId(style->offsetSids.above); | ||
| } |
There was a problem hiding this comment.
Wire the below offset SID end-to-end.
measureNumberPosBelow() now resolves through ts->offsetSids.below, but the constructor still preloads only offsetSids.above. With the picker still exposing all text styles, switching to a style with a distinct below SID leaves the below property pointing at an item the model never registered up front; make sure the measureNumberTextStyle change path also notifies measureNumberPosBelowChanged().
Suggested fix
for (TextStyleType textStyleType : allTextStyles()) {
const TextStyle* style = textStyle(textStyleType);
- addStyleId(style->offsetSids.above);
+ if (style->offsetSids.above != Sid::NOSTYLE) {
+ addStyleId(style->offsetSids.above);
+ }
+ if (style->offsetSids.below != Sid::NOSTYLE && style->offsetSids.below != style->offsetSids.above) {
+ addStyleId(style->offsetSids.below);
+ }
}Also update the measureNumberTextStyle subscription in buildStyleItem() to emit measureNumberPosBelowChanged() together with measureNumberPosAboveChanged().
Also applies to: 76-80
4 participants
Resolves: #22622
This PR resolves the core problem we have with offsets at the moment - the offset property is used to determine the default position of items. The offset property is now only set by the user, and it's default value is always (0, 0). It is no longer a styled proeprty. Style values which used to be linked to the offset property are still available, but contribute to the item's internal position, not it's offset.
Follow up PRs can now deal with longstanding issues we have had with item alignment.
Summary by CodeRabbit
Bug Fixes
Refactor