From 17456782e2386bb794a2ec79021f5e5a831d5a52 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 6 Nov 2026 09:27:20 +0000 Subject: [PATCH] Sort a chord's notes ordinally, so its hash doesn't change with the culture [patch] Chord sorted its notes with the current culture in the constructor and again in GetHashCode. Under et-EE (Z before T) or lt-LT (Y before J) the same chord hashed differently, so HashSet and Dictionary lookups missed equal chords, and the stored/saved note order depended on the thread that built the chord. The constructor now sorts with StringComparer.Ordinal and GetHashCode hashes the stored order. The test project turns off invariant globalization so the culture tests see real culture data. Fixes ktsu-dev/Keybinding#225 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW3JF2xHwwm6qrw3Bh7 --- .../ChordCultureInvarianceTests.cs | 67 +++++++++++++++++++ Keybinding.Test/Keybinding.Test.csproj | 2 + .../ModifierAliasNormalizationTests.cs | 1 + Keybinding/Models/MusicalTypes.cs | 7 +- 4 files changed, 75 insertions(+), 2 deletions(-) create mode 100644 Keybinding.Test/ChordCultureInvarianceTests.cs diff --git a/Keybinding.Test/ChordCultureInvarianceTests.cs b/Keybinding.Test/ChordCultureInvarianceTests.cs new file mode 100644 index 0000000..1473e2c --- /dev/null +++ b/Keybinding.Test/ChordCultureInvarianceTests.cs @@ -0,0 +1,67 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Keybinding.Test; + +using System.Globalization; +using ktsu.Keybinding.Core.Models; + +[TestClass] +[DoNotParallelize] +public class ChordCultureInvarianceTests +{ + // et-EE sorts Z before T, and lt-LT sorts Y before J, so a culture-sensitive sort orders these pairs + // differently from en-US. + [TestMethod] + [DataRow("Ctrl+T+Z", "et-EE")] + [DataRow("Ctrl+J+Y", "lt-LT")] + public void HashCodeAndSetLookupDoNotDependOnTheThreadCulture(string text, string otherCulture) + { + CultureInfo original = CultureInfo.CurrentCulture; + try + { + CultureInfo.CurrentCulture = new CultureInfo("en-US"); + Chord chord = Chord.Parse(text); + HashSet set = [chord]; + Dictionary map = new() { [chord] = "handler" }; + int hash = chord.GetHashCode(); + + CultureInfo.CurrentCulture = new CultureInfo(otherCulture); + Chord reparsed = Chord.Parse(text); + + Assert.AreEqual(hash, chord.GetHashCode()); + Assert.AreEqual(hash, reparsed.GetHashCode()); + Assert.IsTrue(set.Contains(chord)); + Assert.IsTrue(set.Contains(reparsed)); + Assert.IsTrue(map.TryGetValue(reparsed, out string? handler)); + Assert.AreEqual("handler", handler); + } + finally + { + CultureInfo.CurrentCulture = original; + } + } + + [TestMethod] + [DataRow("Ctrl+T+Z", "et-EE")] + [DataRow("Ctrl+J+Y", "lt-LT")] + public void StoredNoteOrderDoesNotDependOnTheThreadCulture(string text, string otherCulture) + { + CultureInfo original = CultureInfo.CurrentCulture; + try + { + CultureInfo.CurrentCulture = new CultureInfo("en-US"); + Chord english = Chord.Parse(text); + + CultureInfo.CurrentCulture = new CultureInfo(otherCulture); + Chord other = Chord.Parse(text); + + CollectionAssert.AreEqual(english.Notes.ToList(), other.Notes.ToList()); + Assert.AreEqual(english.ToString(), other.ToString()); + Assert.AreEqual(text, other.ToString()); + } + finally + { + CultureInfo.CurrentCulture = original; + } + } +} diff --git a/Keybinding.Test/Keybinding.Test.csproj b/Keybinding.Test/Keybinding.Test.csproj index 439c95d..7a6b91f 100644 --- a/Keybinding.Test/Keybinding.Test.csproj +++ b/Keybinding.Test/Keybinding.Test.csproj @@ -6,6 +6,8 @@ true net10.0 + + false diff --git a/Keybinding.Test/ModifierAliasNormalizationTests.cs b/Keybinding.Test/ModifierAliasNormalizationTests.cs index 449fd09..4c01dbc 100644 --- a/Keybinding.Test/ModifierAliasNormalizationTests.cs +++ b/Keybinding.Test/ModifierAliasNormalizationTests.cs @@ -43,6 +43,7 @@ private static KeybindingService CreateService() [DataRow("Windows", "Meta")] [DataRow("Cmd", "Meta")] [DataRow("Command", "Meta")] + [System.Diagnostics.CodeAnalysis.SuppressMessage("Globalization", "CA1308:Normalize strings to uppercase", Justification = "The test feeds a lowercase alias on purpose.")] public void EveryConstructionPath_NormalizesModifierAliases(string alias, string canonical) { ArgumentNullException.ThrowIfNull(alias); diff --git a/Keybinding/Models/MusicalTypes.cs b/Keybinding/Models/MusicalTypes.cs index acbe933..48a027d 100644 --- a/Keybinding/Models/MusicalTypes.cs +++ b/Keybinding/Models/MusicalTypes.cs @@ -194,7 +194,9 @@ public Chord(IEnumerable notes) throw new ArgumentException("Chord cannot contain a null note", nameof(notes)); } - _notes = [.. given.Distinct().OrderBy(n => n.Key.ToString())]; + // Ordinal, so the stored order (which ToString, the saved file and GetHashCode rely on) is the same + // whatever the culture of the thread that built the chord. + _notes = [.. given.Distinct().OrderBy(n => n.Key.ToString(), StringComparer.Ordinal)]; if (_notes.Count == 0) { @@ -320,8 +322,9 @@ other is not null && /// public override int GetHashCode() { + // _notes is already in ordinal order, so equal chords hash their notes in the same order on any thread. HashCode hashCode = new(); - foreach (Note note in _notes.OrderBy(n => n.Key.ToString())) + foreach (Note note in _notes) { hashCode.Add(note); }