Skip to content

Rename profiles in place and keep their description - #115

Merged
matt-edmondson merged 1 commit into
mainfrom
fix/rename-profile-in-place-113
Sep 26, 2026
Merged

matt-edmondson merged 1 commit into
mainfrom
fix/rename-profile-in-place-113

Conversation

@matt-edmondson

Copy link
Copy Markdown
Contributor

Fixes #113

What was wrong

ProfileManager.RenameProfile built a new Profile, copied the chords across, and swapped it in with TryRemove followed by TryAdd. That caused three problems:

  1. The description was erased. newDescription is documented as optional, but it was passed straight through, so a call without it set Description = null.
  2. References already held were detached. This includes the objects returned by CreateProfile, CreateDefaultProfile and GetActiveProfile. Chords set through such a reference never reached GetProfile or SaveAsync, so those bindings were silently lost.
  3. There was a window where the profile didn't exist. Between the remove and the add, GetProfile returned null. A concurrent CreateProfile(id) in that window would make the TryAdd fail and drop the renamed profile.

Change

  • Profile.Name and Profile.Description now have private set.
  • A new internal Rename(name, description) method validates and trims the values the same way the constructor does. The public surface of Profile stays read-only.
  • RenameProfile now renames the stored instance under the manager's existing _lock, and passes newDescription ?? profile.Description. Since nothing is removed from the dictionary, the window in (3) is gone.
  • The IProfileManager.RenameProfile docs now state that a null description keeps the current one, and that held references stay valid.

Profile equality and hashing only use Id, so renaming in place can't disturb dictionary keys or equality anywhere.

Tests

The new file Keybinding.Test/ProfileRenameTests.cs adds five tests:

test covers
RenameProfile_WithoutDescription_KeepsTheExistingDescription defect 1
RenameProfile_KeepsHeldReferencesAttached defect 2: AreSame(held, GetProfile(id)), and a chord set through held is visible
RenameProfile_ChordsSetThroughAHeldReference_ArePersisted defect 2 end to end: rename, bind through the held reference, SaveAsync, reload in a new manager
RenameProfile_WithDescription_ReplacesIt an explicit description still replaces the old one, and the values are trimmed
RenameProfile_UnknownProfile_ReturnsFalse the missing-profile contract is unchanged

With the production changes reverted, the first three fail. With the fix, the full suite passes, 57 of 57 (dotnet test Keybinding.Test -c Release).

This PR doesn't overlap with the open #110 or #111, and it leaves the CreateProfile race in #112 for its own change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01QPAgvLThqmty1U7AaebQMQ


Generated by Claude Code

ProfileManager.RenameProfile built a new Profile and swapped it in with
TryRemove/TryAdd. Leaving out the optional description erased it, and
every reference a caller already held was detached, so chords set
through it never reached GetProfile or SaveAsync. The rename now updates
the stored instance under the manager's lock and keeps the current
description when none is passed.

Fixes #113

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QPAgvLThqmty1U7AaebQMQ
@sonarqubecloud

Copy link
Copy Markdown

@matt-edmondson
matt-edmondson merged commit e5b2bd0 into main Sep 26, 2026
12 checks passed
@matt-edmondson
matt-edmondson deleted the fix/rename-profile-in-place-113 branch September 26, 2026 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RenameProfile clears the description when none is passed, and replaces the Profile object so held references silently stop persisting

2 participants