Offset after autoplace - #34424
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (29)
💤 Files with no reviewable changes (8)
🚧 Files skipped from review as they are similar to previous changes (21)
📝 WalkthroughWalkthroughThe change removes offset-change tracking from engraving items, undo handling, dragging, flipping, and autoplace. Autoplace now applies calculated movement directly. A new 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Public OSS repositories can only analyze public repositories installed in this organization. No linked repositories were analyzed; skipped Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
c69ad87 to
2746cbd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/engraving/rendering/score/autoplace.cpp`:
- Line 72: Update the measure autoplace path around the shape construction and
skyline insertion to use an offset-free measurement pose: subtract
item->offset() before clearance measurement/padding, then restore the offset
exactly once when adding the result to the skyline. Apply the same adjustment to
the related logic at the referenced measure-path locations, while preserving the
segment path’s existing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 39137e54-e0e5-40ee-a620-f44358221701
📒 Files selected for processing (10)
src/engraving/dom/engravingitem.cppsrc/engraving/dom/engravingitem.hsrc/engraving/dom/engravingobject.cppsrc/engraving/dom/guitarbend.cppsrc/engraving/dom/image.cppsrc/engraving/dom/line.cppsrc/engraving/editing/flip.cppsrc/engraving/rendering/score/autoplace.cppsrc/engraving/rendering/score/autoplace.hsrc/engraving/rendering/score/tlayout.cpp
💤 Files with no reviewable changes (9)
- src/engraving/dom/image.cpp
- src/engraving/dom/line.cpp
- src/engraving/editing/flip.cpp
- src/engraving/dom/engravingobject.cpp
- src/engraving/rendering/score/autoplace.h
- src/engraving/dom/engravingitem.h
- src/engraving/dom/guitarbend.cpp
- src/engraving/rendering/score/tlayout.cpp
- src/engraving/dom/engravingitem.cpp
2746cbd to
5337da3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/notation/internal/notationinteraction.cpp (1)
6439-6451: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftAdd regression coverage for freeze and undo.
Cover an autoplaced item, an already non-autoplaced or generated item, an empty selection, and undo/redo. Assert that the item keeps its drawn position after relayout and that undo restores the previous autoplace and offset state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/notation/internal/notationinteraction.cpp` around lines 6439 - 6451, Extend regression tests for NotationInteraction::freezeSelectionPosition to cover autoplaced items, already non-autoplaced or generated items, and an empty selection. Verify frozen items retain their drawn position after relayout, and verify undo and redo restore and reapply the autoplace and offset states respectively.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/engraving/editing/editposition.cpp`:
- Around line 48-50: Update the offset property handling in the surrounding
edit-position logic before calling item->undoChangeProperty: convert offsetPf
from PropertyFlags::STYLED to PropertyFlags::UNSTYLED for the calculated freeze
offset, while preserving PropertyFlags::NOSTYLE for elements without a styled
offset. Keep the existing offset calculation and undoChangeProperty call
unchanged otherwise.
In `@src/engraving/editing/editposition.h`:
- Line 32: Include the standard <vector> header in editposition.h so the
std::vector type used by freezeItemsPositions is declared directly, removing
reliance on transitive or precompiled headers.
In
`@src/propertiespanel/qml/MuseScore/PropertiesPanel/general/appearance/internal/AppearanceOffsetSection.qml`:
- Around line 62-82: Update the navigation.row assignments for
freezePlacementButton and configureGridButton to use unique sequential rows in
visual order: place freezePlacementButton immediately after OffsetSection, then
place snapToGridCheckbox after the freeze button, with configureGridButton
following the appropriate preceding control. Ensure no controls share the same
navigation row.
---
Nitpick comments:
In `@src/notation/internal/notationinteraction.cpp`:
- Around line 6439-6451: Extend regression tests for
NotationInteraction::freezeSelectionPosition to cover autoplaced items, already
non-autoplaced or generated items, and an empty selection. Verify frozen items
retain their drawn position after relayout, and verify undo and redo restore and
reapply the autoplace and offset states respectively.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bfccc913-3955-4eab-b0e9-eafb92dbcc9b
📒 Files selected for processing (29)
src/app/configs/data/shortcuts.xmlsrc/app/configs/data/shortcuts_azerty.xmlsrc/app/configs/data/shortcuts_mac.xmlsrc/engraving/CMakeLists.txtsrc/engraving/dom/engravingitem.cppsrc/engraving/dom/engravingitem.hsrc/engraving/dom/engravingobject.cppsrc/engraving/dom/engravingobject.hsrc/engraving/dom/guitarbend.cppsrc/engraving/dom/image.cppsrc/engraving/dom/line.cppsrc/engraving/editing/editposition.cppsrc/engraving/editing/editposition.hsrc/engraving/editing/flip.cppsrc/engraving/rendering/score/autoplace.cppsrc/engraving/rendering/score/autoplace.hsrc/engraving/rendering/score/tlayout.cppsrc/notation/inotationinteraction.hsrc/notation/internal/notationinteraction.cppsrc/notation/internal/notationinteraction.hsrc/notation/tests/mocks/notationinteractionmock.hsrc/notationscene/internal/notationactioncontroller.cppsrc/notationscene/internal/notationcommandsregister.cppsrc/notationscene/internal/notationuiactions.cppsrc/notationscene/notationcommands.hsrc/propertiespanel/qml/MuseScore/PropertiesPanel/general/appearance/AppearanceSettings.qmlsrc/propertiespanel/qml/MuseScore/PropertiesPanel/general/appearance/appearancesettingsmodel.cppsrc/propertiespanel/qml/MuseScore/PropertiesPanel/general/appearance/appearancesettingsmodel.hsrc/propertiespanel/qml/MuseScore/PropertiesPanel/general/appearance/internal/AppearanceOffsetSection.qml
💤 Files with no reviewable changes (8)
- src/engraving/dom/engravingobject.cpp
- src/engraving/dom/guitarbend.cpp
- src/engraving/editing/flip.cpp
- src/engraving/dom/image.cpp
- src/engraving/dom/engravingitem.cpp
- src/engraving/dom/line.cpp
- src/engraving/rendering/score/tlayout.cpp
- src/engraving/dom/engravingitem.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/engraving/rendering/score/autoplace.cpp
5337da3 to
62600f2
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
62600f2 to
106a05c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
106a05c to
9439623
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
9439623 to
16ae913
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@miiizen Noticed that the shortcut for 'Freeze current placement' doesn't work when the appearance popup is open. Is that expected? Also, undoing the freeze does restore auto-place to being on, but does not reset the offset. Screen.Recording.2026-08-06.at.9.10.07.AM.mov |
|
@avvvvve None of our notation shortcuts work if a a text/spinbox field have focus, so I think this is expected. It works for me if nothing is selected in the popup. We don't want the offset to be reset, we want it to return to what it was before clicking freeze which it looks like it does in the video. When you click freeze in the video offset is zero because of this issue. It's actually (0.14, 3.7). |
|
Ah yeah, I think I got confused because the first time I tried the undo, I had the appearance popup open and the offset did not visually reset to its previous state (I didn't mean it should always reset to 0,0, my bad for not writing clearly). Anyway, yes, the behavior looks right otherwise. Also, are there some elements that don't need the 'Freeze' button at all? For example, it's available for notes—but when you press it, it doesn't actually turn off auto-place. Perhaps we should just not show it for notes/rests/note slashes. Same for all frame types, unless you can think of a reason why to include it for these. cc @davidstephengrant |
|
Yes - we could disable it for any element autoplace does nothing for. While we're at it, it might make sense to also disable the autoplace checkbox. |
|
That sounds good to me! |
3 participants
Resolves: #34411