Skip to content

follow up to #395: placeholders in CommaSeparatedText, and now unnecessary OneOrMore workarounds in mx::impl #396

Description

@webern

Background

PR #395 taught mx::core::OneOrMore to track its implicit default placeholder, so the first
add() on a default-constructed (or setItems({})-repaired) collection replaces the placeholder
instead of appending after it.

That change is safe by construction — OneOrMore exposes no non-const access to its items
(items() and front() are both const, there is no operator[]), so no caller could ever have
populated slot 0 in place and then appended. The only affected code is code that default-constructs
and then adds, which previously produced a fabricated leading element. That is always a bug.

Three things are left over.

1. The same bug in CommaSeparatedText and FontFamily

Identical shape, and the only other instance in the tree:

CommaSeparatedText::CommaSeparatedText() { repair(); }   // repair() -> m_items = {"-"}

void CommaSeparatedText::addItem(std::string item)
{
    m_items.push_back(std::move(item));
    m_text.clear();
    repair();
}

FontFamily f{}; f.addItem("Arial"); serializes as font-family="-, Arial". This is worse than
the OneOrMore case: the fabricated placeholder is a visible sentinel string rather than an empty
element. setItems({}) re-fabricates it as well, so both halves of the #395 treatment are needed.

Currently latent — mx::impl only ever uses the vector constructor (FontFunctions.h:164 and
NoteWriter.cpp:669, both guarded by an empty check) and never calls addItem. But it is the same
trap for the next caller.

The fix belongs in gen/cpp/templates/comma_separated_text.cpp.tmpl and then make gen-cpp;
editing the two generated files directly would be overwritten.

The sibling templates were checked and are not affected: nmtoken_string, smufl_prefixed,
and smufl_wavy repair to non-empty as well, but expose only setValue / setPrefix / setPart
with no append method, so there is no placeholder-then-append hazard.

2. Retire the now-dead first-item workarounds in mx::impl

Every guard that existed only to dodge the placeholder is dead weight after #395, and each one is a
trap for the next reader who assumes it is load-bearing:

  • ScoreWriter.cpp:246-258 — the partIndex == 0 && parts.size() == 2 branch is now unreachable.
  • DirectionWriter::addDirectionType (DirectionWriter.cpp:1416-1428) and the
    myIsFirstDirectionTypeAdded member.
  • The isFirstX dances in DirectionWriter.cpp: isFirstItemAdded (726/776),
    isFirstTuningAdded (1096), isFirstAccordAdded (1147), isFirstHarmonyChordGroup (1742),
    isFirstFigure (1804).
  • PartWriter.cpp:319-325 and makeMetronomeNotes (DirectionWriter.cpp:604-608) — the front()
    plus index-from-1 loops collapse to setItems(std::move(vec)) or a plain range-for.

Each must be removed as a unit. Dropping a guard while keeping the explicit-first-item constructor
(or the reverse) is how a regression sneaks in here.

Not a candidate: DirectionWriter.cpp:1767-1770, which skips figured-bass entries with zero
figures. That guard is about the zero-item case, which OneOrMore still cannot represent. Keep it.

3. Pin the frame-note fix with a round-trip fixture

frame.addFrameNote (DirectionWriter.cpp:1736) is the bug that motivated #395 — a default-constructed
core::Frame holds a placeholder FrameNote, so every fretboard diagram gets a spurious leading
<frame-note>. Nothing pins it. The tests added in #395 (ShapeTest.cpp) prove OneOrMore<int>
semantics but nothing proves a fretboard diagram round-trips correctly end to end.

Per AGENTS.md, that wants a fixture in make api-roundtrip / roundtrip-baseline.txt;
ApiLoadSmokeTest only proves the file imports without crashing and will not catch this.

References

Metadata

Metadata

Assignees

No one assigned

    Labels

    aiIssues opened by, or through, a coding agent.bugsoftware defectcoreAffects the mx::core layernon-breakingfixes or implementation that do not require breaking changestesting

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions