Add dynamics markings (pp/p/mp/mf/f/ff) - #37
Merged
Merged
Conversation
Adds the discrete dynamic levels from ROADMAP.md phase 5. Hairpins
(cresc./dim., a gradual ramp) are a separate, harder follow-up --
this covers the step-function levels only.
Model: DynamicMarking (Text + NoteIndex/BeatPosition, mirroring
JianpuOrnament's note-index resolution) on a new JianpuMeasure.Dynamics
list, with "at most one marking per note" toggle/replace semantics via
DynamicMarkingService -- a note can't be both piano and forte, unlike
ornaments where several types can coexist. OnNoteRemoved is wired
alongside OrnamentService's existing call sites in
ScoreEditorViewModel so indices stay correct after note deletion.
UI: a new DynamicsEditorViewModel ("select a note, click a level")
wired through ribbon/Edit-menu/context-menu, using
ScoreSnapshotEditCommand for undo (the same generic whole-score-clone
mechanism TieEditorViewModel already uses) rather than a bespoke diff
command, since this is a low-frequency, deliberate action. Registered
in AppBootstrapper/MainViewModel alongside the other per-tab scoped
editor viewmodels.
Rendering got a real new row directly below the melody row (not the
ornament band above the note) -- this needed expanding
StaffBlockHeight, a constant used everywhere (block bounds,
hit-testing, PDF pagination content height, block stacking), touching
every place that assumed exactly three rows. This is the one part of
this change that carries the same cross-cutting layout risk flagged
for the accidental slash-convention work; done this way because the
user explicitly chose the real row over the lower-risk alternative
(reusing the ornament band) after being shown the tradeoff. Locked in
with a dedicated geometry test asserting every row boundary lines up
with no gap/overlap drift.
Playback: a velocity-scaling pass in ScoreMidiSchedule.BuildMelodyNotes
tracks the current dynamic level across notes and measures
(DynamicMarkingPlaybackService maps each level to a fixed velocity,
33-112), replacing the constant MelodyVelocity from the marked note
onward until the next marking or the end of the score. A score with
no dynamic markings schedules byte-identical output to before this
existed. Melody part only in this version, not chord markers.
Verified via the same sandbox pipeline as every prior change this
session: dotnet build succeeds against the real project/package
graph, dotnet format --verify-no-changes passes on both projects, and
new xUnit tests cover the service's toggle/replace/index-shifting
logic, the velocity mapping, the ViewModel's add/toggle/undo
behavior, the playback pass end-to-end (including a marking
overriding an earlier one), the new row's geometry, and a
render-to-bitmap smoke test. dotnet test itself cannot execute in
this sandbox; real confirmation comes from GitHub Actions CI.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pguj4XSScE141p1ScWoqEr
MainFormMultiTabTests.cs and DocumentTabTests.cs each build their own IServiceCollection mirroring AppBootstrapper's registration list rather than using AppBootstrapper itself, so the new DynamicsEditorViewModel scoped registration from the dynamics-markings change wasn't picked up automatically. This broke MainFormMultiTabTests (resolving MainViewModel throws "Unable to resolve service for type DynamicsEditorViewModel") on GitHub Actions -- the same class of gap this session hit before with OrnamentEditorViewModel's own registration. Added the missing services.AddScoped<DynamicsEditorViewModel>() line to both files, mirroring the existing OrnamentEditorViewModel registration in each. Verified via the same sandbox pipeline as every prior change this session: dotnet build succeeds against the real project/package graph, dotnet format --verify-no-changes passes. dotnet test itself cannot execute in this sandbox; real confirmation comes from GitHub Actions CI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pguj4XSScE141p1ScWoqEr
PlanPages_LineCount_MapsToExpectedPageCount(7, 1) expected 7 staff lines to fit on one PDF page. Adding the dynamics row increased StaffBlockHeight from 180 to 210px, so a page that used to fit 7 lines now fits 6 -- this is the correct, intended consequence of adding a real new row (more vertical space per line means fewer lines per page), not a pagination bug. Verified the exact page split against the real compiled PdfPagePlanner.PlanPages (7 lines -> 6+1 across 2 pages; 8 lines still splits 6+2, unchanged) before updating the fixture from (7, 1) to (7, 2). No other test hardcodes a StaffBlockHeight-derived value. Verified via the same sandbox pipeline as every prior change this session: dotnet build succeeds against the real project/package graph, dotnet format --verify-no-changes passes. dotnet test itself cannot execute in this sandbox; real confirmation comes from GitHub Actions CI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pguj4XSScE141p1ScWoqEr
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
2 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.
Summary
Continuing the Indonesian notasi angka gap list (
ROADMAP.md), phase 5 (dynamics markings) --the discrete levels (pp/p/mp/mf/f/ff). Hairpins (cresc./dim., a gradual ramp) are a separate,
harder follow-up, documented but not attempted here.
Before implementing, I flagged a real design fork to the user: real notation places dynamics
below the melody row, but that requires expanding
StaffBlockHeight(a constant usedeverywhere -- block bounds, hit-testing, PDF pagination, block stacking), the same category of
cross-cutting layout risk already flagged for the accidental slash-convention work, and something
this Linux sandbox can't visually verify. The user chose the real row over the lower-risk
alternative (reusing the ornament band above the note), so that's what this PR does.
DynamicMarking(Text+NoteIndex/BeatPosition, mirroringJianpuOrnament'snote-index resolution) on a new
JianpuMeasure.Dynamicslist, with "at most one marking pernote" toggle/replace semantics via
DynamicMarkingService-- a note can't be both piano andforte, unlike ornaments where several types can coexist on one note.
DynamicsEditorViewModel("select a note, click a level") wired throughribbon/Edit-menu/context-menu. Deliberately simpler than chord markers' click-to-add-at-a-
beat/drag/inline-text-edit interaction, since only a fixed vocabulary of levels is supported (no
free-text dynamics like "molto espress." in this version) -- avoids new canvas hit-testing code.
exactly three rows (both copies of the row-label drawing, the main
HitTestdispatcher, theGetSecondaryRowTop/GetLyricRowTophelper chain). Locked in with a dedicated geometry testasserting every row boundary lines up with no gap/overlap drift, on top of the usual
render-to-bitmap smoke test.
ScoreMidiSchedule.BuildMelodyNotestracks the currentdynamic level across notes and measures, replacing the constant
MelodyVelocityfrom the markednote onward until the next marking or the end of the score. A score with no dynamic markings
schedules byte-identical output to before this existed. Melody part only in this version, not
chord markers.
Test plan
Verified via the same sandbox pipeline as every prior change this session:
dotnet buildsucceeds against the real project/package graph (0 errors/warnings).dotnet format --verify-no-changespasses on both projects.the ViewModel's add/toggle/undo behavior, the playback pass end-to-end (including a marking
overriding an earlier one across measures), the new row's geometry invariant, and a
render-to-bitmap smoke test.
Not verified in this sandbox: the new row's actual on-screen appearance/position hasn't been
visually confirmed on a real Windows machine -- this is the biggest visual-verification gap of any
change this session, given the layout risk involved.
dotnet testitself cannot execute here (noWindowsDesktop runtime pack); real confirmation of the new tests comes from GitHub Actions CI.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Pguj4XSScE141p1ScWoqEr
Generated by Claude Code