Repository navigation
Sort a chord's notes ordinally, so its hash doesn't change with the culture [patch] - #227
Open
matt-edmondson wants to merge 1 commit into
Open
matt-edmondson wants to merge 1 commit into
matt-edmondson wants to merge 1 commit into
Conversation
…ulture [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<Chord> and Dictionary<Chord, ...> 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 #225 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017vfJW3JF2xHwwm6qrw3Bh7
|
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
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.
Fixes #225
What was wrong
Chordsorted its notes with the current culture in two places: the constructor (OrderBy(n => n.Key.ToString())) andGetHashCode, which sorted again on every call. Some cultures order letters differently: et-EE sorts Z before T, and lt-LT sorts Y before J. So the same unchanged chord hashed differently on threads with different cultures, andHashSet<Chord>/Dictionary<Chord, …>lookups missed chords that compare equal. The stored note order, whichToString()and the saved file use, also depended on the culture of whichever thread built the chord.Change
StringComparer.Ordinal.GetHashCodehashes_notesin that stored order and no longer sorts on each call.Keybinding.Testsets<InvariantGlobalization>false</InvariantGlobalization>, because ktsu.Sdk turns invariant mode on and every culture would then sort like the invariant one. FuzzySearch, TextFilter and Semantics do the same in their test projects. Turning invariant mode off enables CA1308 on an existing test that lowercases an alias on purpose, so that test method gets aSuppressMessagewith a justification.Tests
ChordCultureInvarianceTests. It buildsCtrl+T+ZandCtrl+J+Yunder en-US, then under et-EE / lt-LT checks:HashSetandDictionarylookups still find the chordToString()are the same in both culturesMusicalTypes.cschange reverted, all 4 new cases fail. With it, the full suite passes: 385/385 on Linux / .NET 10.Keybinding.slnbuilds with 0 warnings.Generated by Claude Code