Repository navigation
Count only the SetChords bindings that remain, and report the commands that lost their chord [minor] - #215
Merged
Conversation
…s that lost their chord [minor] SetChords counted every BindChord that succeeded, so a batch that gave one chord to two commands reported 2 although the second took the chord from the first. It now returns the bindings from the batch that remain, and a new SetChords(chords, out displacedCommandIds) overload names the commands whose chord was taken, so a settings UI can warn about a displaced binding. Fixes #207 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017vfJW2ncKULZZ8pQtc75wf
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Comment on lines
+311
to
+317
| foreach (string holder in holders) | ||
| { | ||
| if (!chord.Equals(activeProfile.GetChord(holder)) && !displaced.Contains(holder)) | ||
| { | ||
| displaced.Add(holder); | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Leaving this one as it is: it's a style suggestion with no change in behaviour. The guard also reads displaced, which the loop adds to, so the explicit if makes that dependency easier to see than a lazy Where would. Happy to fold it in if a later push touches this method.
Generated by Claude Code
|
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 #207
What was wrong
Since #152,
BindChordmoves a chord that is already in use to the new command.KeybindingManager.SetChordsstill counted everyBindChordcall that succeeded. SoSetChords({ a: Ctrl+S, b: Ctrl+S })returned 2 while the profile held 1 binding. Callers also had no way to find out thatahad lost its chord.Change
SetChords(chords)now returns the number of bindings from the batch that remain once it has run. The doc comment says so.SetChords(chords, out IReadOnlyList<string> displacedCommandIds)lists the commands whose chord a binding in the batch took. That covers commands outside the batch, and batch entries that a later entry displaced. A command that the batch itself rebinds is not listed if it keeps its new binding; for example, a swap wherebtakes Ctrl+S fromaandamoves to Ctrl+D.KeybindingManager, notIKeybindingService, so no interface member is added for other implementers to provide. It works through the existingBindChord(profileId, …)call, so it also works with anIKeybindingServicesupplied from outside.Tests
New cases in
ChordConflictTests:With the old single-argument body restored, the reproduction test fails (returns 2, expected 1). With this change, the whole suite passes: 333/333.
Generated by Claude Code