Fix tie/slur pitch bug and add the missing Mordent button - #32
Merged
Merged
Conversation
Starting on the Indonesian notasi angka gap list, low-risk items first: - Tie tool no longer accepts different-pitch notes. TieEditorViewModel. TryCompleteTie previously validated only that the end note comes after the start note, never that the pitches match, and ScoreMidiSchedule.BuildTieEndSet unconditionally suppresses the tied end note's own NoteOn -- so a "tie" drawn between two different pitches silently dropped the second note's actual pitch during playback and MIDI export. This affected existing Chinese/Western scores too, independent of anything Indonesian-specific. Now rejects a pitch mismatch with a status message and treats the mismatched note as a new start candidate, mirroring the existing "must come after" rejection flow. Real slur support (connecting different pitches for legato phrasing, not sustain) is separate future work. - Added the missing Mordent button (ribbon + Edit menu + right-click context menu, plus a new RibbonIcon.Mordent glyph). Mordent's backend -- glyph stacking layout, playback expansion in OrnamentPlaybackService -- already existed from earlier work; only the UI entry point was missing, so this was pure wiring, unlike Staccato/Accent/Tenuto/Glissando which still need new glyphs and new playback effects each (left for a separate pass). Also updated ROADMAP.md: marked these two sub-items done, and flagged that the accidental slash-convention item (originally planned next) is more involved than first scoped -- JianpuRenderer has two separate accidental rendering paths, and the compact one positions the mark via a dedicated layout band this sandbox can't visually verify without a real Windows render. Recommending that one land as its own carefully reviewed PR rather than bundled with these lower-risk fixes. Verified via the same sandbox pipeline as every prior change this session: dotnet build and dotnet format --verify-no-changes both pass, and the full regression suite (20 tests across Harness.cs and MainFormHarness.cs) passes under Mono + Xvfb against the rebuilt assembly, including two new tests added for the tie pitch-validation fix (rejects a mismatched pair, accepts a matching pair). The Mordent icon's actual on-screen appearance hasn't been visually confirmed on a real Windows machine, same caveat as every prior UI change this session. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pguj4XSScE141p1ScWoqEr
TryCompleteTie_AddsTieWhenStartAndEndAreValid and TryCompleteTie_Undo_RemovesAddedTie used two notes with different pitches (1 and 2) incidentally, to test tie creation/undo behavior unrelated to pitch matching. The new same-pitch validation in TieEditorViewModel correctly rejects that setup, so both tests now use matching pitches to keep exercising their actual intent. Verified by cross-compiling the test project (net8.0-windows via a temporary EnableWindowsTargeting flag, reverted after) and confirming 0 errors/warnings; dotnet test itself cannot execute in this sandbox (no WindowsDesktop runtime pack), so 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
Starting on the Indonesian notasi angka gap list (
ROADMAP.md), low-risk items first.TieEditorViewModel.TryCompleteTiepreviously validated only that the end note comes after the start note, never that the pitches
match, and
ScoreMidiSchedule.BuildTieEndSetunconditionally suppresses the tied end note's ownNoteOn— so a "tie" drawn between two different pitches silently dropped the second note'sactual pitch during playback and MIDI export. This affected existing Chinese/Western scores too,
independent of anything Indonesian-specific. Now rejects a pitch mismatch with a status message
and treats the mismatched note as a new start candidate, mirroring the existing "must come
after" rejection flow. Real slur support (connecting different pitches for legato phrasing, not
sustain) is separate future work.
RibbonIcon.Mordentglyph). Mordent's backend — glyph stacking layout, playback expansion inOrnamentPlaybackService— already existed from earlier work; only the UI entry point wasmissing, so this was pure wiring, unlike Staccato/Accent/Tenuto/Glissando which still need new
glyphs and new playback effects each (left for a separate pass).
Also updated
ROADMAP.md: marked these two sub-items done, and flagged that the accidentalslash-convention item (originally planned next) is more involved than first scoped —
JianpuRendererhas two separate accidental rendering paths, and the compact one positions themark via a dedicated layout band this sandbox can't visually verify without a real Windows render.
Recommending that one land as its own carefully reviewed PR rather than bundled with these
lower-risk fixes.
Test plan
Verified via the same sandbox pipeline as every prior change this session:
dotnet buildsucceeds against the real project/package graph.dotnet format --verify-no-changespasses.Harness.csandMainFormHarness.cs) passes underMono + Xvfb against the rebuilt assembly, including two new tests added for the tie
pitch-validation fix (rejects a mismatched pair, accepts a matching pair).
Not verified in this sandbox: the Mordent icon's actual on-screen appearance hasn't been
visually confirmed on a real Windows machine, same caveat as every prior UI change this session.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Pguj4XSScE141p1ScWoqEr
Generated by Claude Code