Skip to content

Note(NoteName) doesn't uppercase before canonicalizing, so a chord built from NoteName "control" shows "Ctrl+S" but isn't equal to Chord.Parse("Ctrl+S") #121

Description

@matt-edmondson

What's wrong

The fix for #107 (bbbd73e) aimed to canonicalize modifier aliases "in both Note constructors so every path agrees". The [JsonConstructor] Note(NoteName key) overload (Keybinding/Models/MusicalTypes.cs, around lines 18–22) passes key.ToString() to CanonicalizeKey without Trim().ToUpperInvariant(), while the string overload does both. CanonicalizeKey only matches uppercase text, so:

  • "control" is not mapped to CTRL;
  • a lowercase key such as "s" is stored as s, not S.

NoteName doesn't enforce uppercase: ValidateNoteName isn't attached as a validation attribute. Chord.ToString() uppercases for display, so the bad chord prints exactly like the correct one. That's the same invisible-mismatch symptom #107 fixed for the other paths. The new tests only feed this constructor NoteName.Create(alias.ToUpperInvariant()), which is why they pass.

Reproduced

[TestMethod]
public void ChordFromNoteNameAlias()
{
	Chord built = new([new Note(NoteName.Create("control")), new Note("S")]);
	Chord parsed = Chord.Parse("Ctrl+S");
	Assert.AreEqual(parsed.ToString(), built.ToString()); // passes: both "Ctrl+S"
	Assert.AreEqual(parsed, built);                        // fails
}

[TestMethod]
public void NoteNameLowercaseAlias() => Assert.AreEqual(new Note("control"), new Note(NoteName.Create("control")));

Observed: ChordFromNoteNameAlias fails with expected: Ctrl+S actual: Ctrl+S. NoteNameLowercaseAlias fails because it gets control where it expects CTRL.

Consequence: a binding built this way never matches, and conflict detection misses it, even though every UI surface shows the same text.

Suggested fix

Route both constructors through one normalizing helper, e.g. CanonicalizeKey(key.ToString().Trim().ToUpperInvariant()), in the NoteName overload too.

Acceptance: both tests above pass, and new Note(NoteName.Create(x)) == new Note(x) for mixed-case aliases and letters.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions