Skip to content

Progression.ToString() puts bar lines in the wrong place: after a sub-beat chord | lands mid-bar, and a chord held across a barline gets none #475

Closed

Description

@matt-edmondson

What's wrong

According to the spec, Progression.ToString() writes | at bar boundaries. 2026-07-02-music-type-safe-factories-design.md L121:``

Bar lines | are cosmetic: ToString inserts them at bar boundaries; Parse derives duration from the slashes and treats | as validation.

The implementation (Progression.Parse.cs L14-L56) gets this wrong in two ways:

  1. After a sub-beat chord. Any Chord@n/d (sub-beat) event sets beatCursor = 0, marked "cosmetic only". The next chord therefore always gets a | in front of it, wherever it really falls in the bar.
  2. Across a barline. A | is written only when a chord starts exactly on a boundary. A chord held across a barline gets none, even though Parse already accepts G | /.

Reproduction (4/4)

Events ToString() Correct chart
C 1/8, G 3/8, F 1/2, E 1/1 (2 bars) 4/4 C@1/8 | G@3/8 | F / E / / / 4/4 C@1/8 G@3/8 F / | E / / /
C 3/4, G 1/2, F 3/4 (2 bars) 4/4 C / / G / F / / (no bar line at all) 4/4 C / / G | / F / /

Round-trip equality still holds, because Parse skips every | token (L112). But the text is what Section.ToString() and Arrangement.ToString() show people, and it misstates the bar structure.

Parse also accepts "4/4 C | G / / / / / | | F", so the "| as validation" half of the spec was never implemented either.

Acceptance criteria

  • ToString tracks the position in the bar as an exact fraction (a Duration), not a whole-beat counter.
  • It writes | at each multiple of TimeSignature.BarDuration, including inside a held chord (G | /).
  • Both examples above produce the "Correct chart" column.
  • Existing round-trip tests stay green.
  • Decide separately whether Parse should reject a | that is not on a bar boundary, as the spec says, or whether the spec should be amended to say | is ignored.

Related, not a duplicate: #328 (Parse drops slashes after a Chord@n/d cell).

Activity

  1. matt-edmondson commented on Oct 20, 2026

    @matt-edmondson
    ContributorAuthor

    Triage

    • Category: Bug
    • Priority: Medium. Round-trip equality still holds, so no data is lost. But the text that Section/Arrangement.ToString() show people misstates the bar structure, and the | validation half of the spec was never implemented.
    • Area / suggested assignee: Progression.ToString and Parse in Semantics.Music/Progression.Parse.cs (L14-56 for writing, L112 for skipping |). Owner: @matt-edmondson
    • Duplicates / in progress: not a duplicate. It is related to Progression.Parse silently drops beat slashes after a Chord@n/d cell: "4/4 C@1/2 / / | G" parses as C for 2 beats with G on beat 3 #328 (Parse drops slashes after a Chord@n/d cell), and both touch the sub-beat handling in the same file, so they are worth fixing together. No open PR covers it.
    • Next step: track the bar position as an exact Duration and emit | at each multiple of BarDuration, including inside held chords. Decide whether to enforce | on Parse or to amend the spec, and do that as a separate change so this fix stays non-breaking.

    Generated by Claude Code

  2. matt-edmondson commented on Oct 29, 2026

    @matt-edmondson
    ContributorAuthor

    Fixed by #487 (commit 47dffbb, now on main). The commit cites this issue inline as "(#475)" rather than with a "Fixes" keyword, so GitHub didn't close it. Closing by hand.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

bugSomething isn't working

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions