Repaint every row sharing a toggled profile's name, not just one - #28
Merged
Conversation
Profile suppression is keyed by ApplicationSetting.Name; the list repaint was keyed by FileName. RefreshToggledListItemAppearance resolved a single setting through FindMatch and repainted the one row whose Tag held that setting's path. So when two profiles shared a Name, one hotkey press suppressed both and only one row changed appearance -- the other kept advertising a state it no longer had. The collision is trivially reachable, which was not obvious when this was deferred. Name is Path.GetFileNameWithoutExtension of the executable path, nothing more, and the manual Add Program path dedupes only on the full FileName. Adding D:\SteamLibrary\GameA\game.exe and D:\GOG\GameB\game.exe through the ordinary UI produces two profiles both named "game", with no warning. The game-finder's bulk path is the one place that already guards against this, which is likely why the gap survived: it only opens on manual adds. The decision is now a pure static, FindApplicationSettingsByName, that the real refresh path calls -- the same shape as DescribeListItem and ShouldRefreshListItemForToggleResult, and for the same reason: a fixture cannot construct a Form, so the testable part has to be the decision rather than the painting. The two comparisons are now one comparison. StringComparer. OrdinalIgnoreCase moved out of the suppression HashSet into ProfileToggleHelper.NameComparer, and the lookup compares through that same field instead of restating the intent. Flipping it to Ordinal now fails a pre-existing suppression check and a new one together, from both directions -- which is the property that stops these two drifting apart again. Seven checks, 417 to 424. One gap stays open and is worth stating. Reverting the call site to repaint a single row, while leaving the pure function intact, still passes all 424 checks: the loop, the FindApplicationListItem lookup and ApplyApplicationListItemAppearance all touch a real ListView on a real Form, which no fixture here can build. That wiring is verified by reading -- lvi.Tag and FileName are the same key on both sides -- not by a check. It is the same boundary ApplyApplicationListItemAppearance has always had. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0187tGqyEw4frZzDYPPJfUMd
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.
Profile suppression (the toggle hotkey from #14) is keyed by
ApplicationSetting.Name. The list repaint was keyed byFileName.RefreshToggledListItemAppearanceresolved a single setting throughFindMatch, then repainted the one row whoseTagheld that setting's path. So when two profiles shared aName, one hotkey press suppressed both and only one row changed appearance — the other kept advertising a state it no longer had.The collision is trivially reachable
This wasn't obvious when the item was deferred, and it's the reason to fix it rather than leave it.
NameisPath.GetFileNameWithoutExtension(_filePath)— nothing more. The manual Add Program flow dedupes only on the fullFileName. So addingD:\SteamLibrary\GameA\game.exeandD:\GOG\GameB\game.exethrough the ordinary UI produces two profiles both namedgame, with no warning.The game-finder's bulk-add path is the one place that already guards against a
Namecollision — which is likely why this survived unnoticed. The gap only opens on manual adds.The fix
The decision is now a pure static,
FindApplicationSettingsByName, that the real refresh path calls — the same shape asDescribeListItemandShouldRefreshListItemForToggleResult, and for the same reason: a fixture cannot construct aForm, so the testable part has to be the decision rather than the painting.FindApplicationListItemis unchanged and needed no sibling. Its other caller (OnForegroundChangedConfirmExecutable) legitimately wants exactly one row, since the unconfirmed-flag clear is per-FileName.The two comparisons are now one comparison
StringComparer.OrdinalIgnoreCasemoved out of the suppressionHashSetintoProfileToggleHelper.NameComparer, and the new lookup compares through that same field rather than restating the intent.That this matters was proved, not assumed: flipping
NameComparertoOrdinalfails a pre-existing cross-case suppression check and the newO4simultaneously. The two are structurally one comparison now, and any future edit is caught from both directions.Testing
424 checks, up seven (
O1–O7). Each mutation-proved individually — including reproducing the original defect exactly (returnafter the first match), which failsO2alone.One gap stays open, and it was found by testing for it
Reverting the call site to repaint a single row, while leaving the pure function intact, still passes all 424 checks. The loop, the
FindApplicationListItemlookup andApplyApplicationListItemAppearanceall touch a realListViewon a realFormthat no fixture here can build.That wiring is verified by direct code reading —
lvi.TagandFileNameare the same key on both sides — not by a check. It is the same boundaryApplyApplicationListItemAppearancehas always had, stated rather than assumed.