What's wrong
The #106 fix (9e92fb5) made KeybindingManager.SaveAsync (Keybinding/KeybindingManager.cs:111-115) prune profiles:
IReadOnlyCollection<Profile> storedProfiles = await Repository.LoadAllProfilesAsync();
IEnumerable<string> deletedProfileIds = storedProfiles.Select(p => p.Id).Where(id => !profileIds.Contains(id));
await AsyncBatchHelper.ForEachAsync(deletedProfileIds, Repository.DeleteProfileAsync);
It treats every stored profile that is not in memory as deleted. On a manager that never loaded from disk, that is every stored profile. Before 9e92fb5, saving merged with what was on disk, so those profiles survived.
Reproduction
This was executed as a scratch MSTest:
- Manager A: call
InitializeAsync() and CreateDefaultProfile(), create profile "vim", then SaveAsync().
- Manager B on the same data directory, without calling
InitializeAsync(): call CreateDefaultProfile("other", "Other"), then SaveAsync().
- Manager C on the same directory: call
InitializeAsync().
Observed: C loads only other. The default and vim profiles, and their bindings, are permanently deleted.
Expected: Profiles the manager never loaded are not deleted. Either keep the earlier merge behaviour, or refuse to save until the manager has loaded.
Why it matters
Forgetting InitializeAsync() is an easy mistake, for example in a code path that creates a manager only to add one profile. The penalty is silent loss of every user's custom keymaps. Deleting a profile needs proof that someone meant to delete it, and "not in memory" is not that proof when memory was never populated.
Suggested fix / acceptance criteria
Pick one:
- Record the ids removed through
DeleteProfile and delete only those in SaveAsync. This is the most precise option and doesn't depend on initialization.
- Track whether
InitializeAsync has run, and prune only if it has.
- Throw
InvalidOperationException from SaveAsync when the manager has not been initialized.
Add a regression test for the three-manager scenario above.
commands.json has always been overwritten wholesale in this case. That can reasonably be called misuse, but profiles are user data, and this regression is new.
What's wrong
The #106 fix (9e92fb5) made
KeybindingManager.SaveAsync(Keybinding/KeybindingManager.cs:111-115) prune profiles:It treats every stored profile that is not in memory as deleted. On a manager that never loaded from disk, that is every stored profile. Before 9e92fb5, saving merged with what was on disk, so those profiles survived.
Reproduction
This was executed as a scratch MSTest:
InitializeAsync()andCreateDefaultProfile(), create profile"vim", thenSaveAsync().InitializeAsync(): callCreateDefaultProfile("other", "Other"), thenSaveAsync().InitializeAsync().Observed: C loads only
other. Thedefaultandvimprofiles, and their bindings, are permanently deleted.Expected: Profiles the manager never loaded are not deleted. Either keep the earlier merge behaviour, or refuse to save until the manager has loaded.
Why it matters
Forgetting
InitializeAsync()is an easy mistake, for example in a code path that creates a manager only to add one profile. The penalty is silent loss of every user's custom keymaps. Deleting a profile needs proof that someone meant to delete it, and "not in memory" is not that proof when memory was never populated.Suggested fix / acceptance criteria
Pick one:
DeleteProfileand delete only those inSaveAsync. This is the most precise option and doesn't depend on initialization.InitializeAsynchas run, and prune only if it has.InvalidOperationExceptionfromSaveAsyncwhen the manager has not been initialized.Add a regression test for the three-manager scenario above.
commands.jsonhas always been overwritten wholesale in this case. That can reasonably be called misuse, but profiles are user data, and this regression is new.