diff --git a/src/modules/keyboardmanager/KeyboardManagerEditor/KeyboardManagerEditor.cpp b/src/modules/keyboardmanager/KeyboardManagerEditor/KeyboardManagerEditor.cpp index d1c4aef8cd..ce18fb0ce8 100644 --- a/src/modules/keyboardmanager/KeyboardManagerEditor/KeyboardManagerEditor.cpp +++ b/src/modules/keyboardmanager/KeyboardManagerEditor/KeyboardManagerEditor.cpp @@ -148,12 +148,11 @@ KeyboardManagerEditor::KeyboardManagerEditor(HINSTANCE hInst) : { std::this_thread::sleep_for(std::chrono::milliseconds(500)); - // Retry only file-level or transient failures. Partial data is deterministic - // and must never be opened and subsequently saved as a truncated profile. + // Retry only file-level or transient failures. loadResult = mappingConfiguration.LoadSettingsWithResult(); } - configurationLoaded = loadResult == MappingConfigurationLoadResult::Success; + configurationLoaded = loadResult != MappingConfigurationLoadResult::Failure; } KeyboardManagerEditor::~KeyboardManagerEditor() diff --git a/src/modules/keyboardmanager/KeyboardManagerEditorTest/LoadingAndSavingRemappingTests.cpp b/src/modules/keyboardmanager/KeyboardManagerEditorTest/LoadingAndSavingRemappingTests.cpp index 86b1f586d7..8f8b56ecb3 100644 --- a/src/modules/keyboardmanager/KeyboardManagerEditorTest/LoadingAndSavingRemappingTests.cpp +++ b/src/modules/keyboardmanager/KeyboardManagerEditorTest/LoadingAndSavingRemappingTests.cpp @@ -710,7 +710,7 @@ namespace RemappingUITests Assert::IsTrue(configuration.textExpansions.empty()); } - TEST_METHOD (LoadSettingsFromFile_ShouldKeepSnapshotWhenExistingProfileCannotBeParsed) + TEST_METHOD (LoadSettingsFromFile_ShouldLeaveStateUnchangedWhenExistingProfileCannotBeParsed) { ScopedProfileTestPath corruptProfile{ L"corrupt.json" }; { @@ -731,6 +731,75 @@ namespace RemappingUITests Assert::AreEqual(std::wstring(TextExpansionId1), configuration.textExpansions[0].id); } + TEST_METHOD (LoadSettingsFromFile_ShouldApplyValidSubsetAndUpdateConfigurationOnPartial) + { + ScopedProfileTestPath partialProfile{ L"partial.json" }; + auto profile = CreateEmptyMappingProfile(); + json::JsonObject keyRemap; + keyRemap.SetNamedValue(KeyboardManagerConstants::OriginalKeysSettingName, json::value(L"65")); + keyRemap.SetNamedValue(KeyboardManagerConstants::NewRemapKeysSettingName, json::value(L"66")); + profile.GetNamedObject(KeyboardManagerConstants::RemapKeysSettingName) + .GetNamedArray(KeyboardManagerConstants::InProcessRemapKeysSettingName) + .Append(keyRemap); + auto rules = GetTextExpansionArray(profile); + rules.Append(CreateTextExpansionJson( + TextExpansionId1, + L"valid", + CreateActivationJson({ VK_SPACE }), + L"loaded", + true)); + auto invalidRule = CreateTextExpansionJson( + TextExpansionId2, + L"invalid", + CreateActivationJson({ VK_SPACE }), + L"skipped", + true); + invalidRule.Remove(KeyboardManagerConstants::TextExpansionReplacementTextSettingName); + rules.Append(invalidRule); + json::to_file(partialProfile.path.wstring(), profile); + + MappingConfiguration configuration; + configuration.currentConfig = L"previous"; + Assert::IsTrue(configuration.AddSingleKeyRemap(L'C', static_cast(L'D'))); + Assert::IsTrue(configuration.AddTextExpansion(CreateTextExpansionRule(TextExpansionId3))); + + const auto result = configuration.LoadSettingsFromFile(L"partial-profile", partialProfile.path.wstring()); + + Assert::AreEqual(static_cast(MappingConfigurationLoadResult::Partial), static_cast(result)); + Assert::AreEqual(std::wstring(L"partial-profile"), configuration.currentConfig); + Assert::AreEqual(static_cast(1), configuration.singleKeyReMap.size()); + Assert::IsTrue(configuration.singleKeyReMap.contains(L'A')); + Assert::AreEqual(static_cast(L'B'), std::get(configuration.singleKeyReMap.at(L'A'))); + Assert::AreEqual(static_cast(1), configuration.textExpansions.size()); + Assert::AreEqual(std::wstring(TextExpansionId1), configuration.textExpansions[0].id); + Assert::AreEqual(std::wstring(L"loaded"), configuration.textExpansions[0].replacementText); + } + + TEST_METHOD (LoadSettingsFromJson_ShouldLoadValidAppSpecificShortcutsWhenGlobalShortcutIsInvalid) + { + auto profile = CreateEmptyMappingProfile(); + auto shortcuts = profile.GetNamedObject(KeyboardManagerConstants::RemapShortcutsSettingName); + shortcuts.GetNamedArray(KeyboardManagerConstants::GlobalRemapShortcutsSettingName) + .Append(json::value(L"invalid")); + + json::JsonObject appSpecificShortcut; + appSpecificShortcut.SetNamedValue(KeyboardManagerConstants::OriginalKeysSettingName, json::value(L"17;65")); + appSpecificShortcut.SetNamedValue(KeyboardManagerConstants::NewRemapKeysSettingName, json::value(L"66")); + appSpecificShortcut.SetNamedValue(KeyboardManagerConstants::TargetAppSettingName, json::value(L"test.exe")); + shortcuts.GetNamedArray(KeyboardManagerConstants::AppSpecificRemapShortcutsSettingName) + .Append(appSpecificShortcut); + + MappingConfiguration configuration; + const auto result = configuration.LoadSettingsFromJson(profile); + + Assert::AreEqual(static_cast(MappingConfigurationLoadResult::Partial), static_cast(result)); + Assert::AreEqual(static_cast(1), configuration.appSpecificShortcutReMap.size()); + Assert::IsTrue(configuration.appSpecificShortcutReMap.contains(L"test.exe")); + const auto& appMappings = configuration.appSpecificShortcutReMap.at(L"test.exe"); + Assert::AreEqual(static_cast(1), appMappings.size()); + Assert::IsTrue(appMappings.contains(Shortcut(L"17;65"))); + } + TEST_METHOD (LoadTextExpansions_ShouldAcceptCanonicalSchema_NormalizeActivationAndPreserveDuplicates) { auto profile = CreateEmptyMappingProfile(); @@ -780,7 +849,7 @@ namespace RemappingUITests Assert::IsTrue(configuration.textExpansions.empty()); } - TEST_METHOD (LoadTextExpansions_ShouldRejectNonCanonicalOrDuplicateGuidsWithoutCommittingPartialSnapshot) + TEST_METHOD (LoadTextExpansions_ShouldSkipInvalidOrDuplicateGuidsAndApplyValidSubset) { const auto existingRule = CreateTextExpansionRule(TextExpansionId3, L"existing", Shortcut(VK_TAB), L"unchanged", false); MappingConfiguration configuration; @@ -792,7 +861,14 @@ namespace RemappingUITests std::wstring(L"{11111111-1111-4111-8111-111111111111}") }) { auto profile = CreateEmptyMappingProfile(); - GetTextExpansionArray(profile).Append(CreateTextExpansionJson( + auto rules = GetTextExpansionArray(profile); + rules.Append(CreateTextExpansionJson( + TextExpansionId2, + L"valid", + CreateActivationJson({ VK_TAB }), + L"loaded", + true)); + rules.Append(CreateTextExpansionJson( invalidId, L"brb", CreateActivationJson({ VK_SPACE }), @@ -802,7 +878,8 @@ namespace RemappingUITests const auto result = configuration.LoadSettingsFromJson(profile); Assert::AreEqual(static_cast(MappingConfigurationLoadResult::Partial), static_cast(result)); Assert::AreEqual(static_cast(1), configuration.textExpansions.size()); - Assert::AreEqual(existingRule.id, configuration.textExpansions[0].id); + Assert::AreEqual(std::wstring(TextExpansionId2), configuration.textExpansions[0].id); + Assert::AreEqual(std::wstring(L"loaded"), configuration.textExpansions[0].replacementText); } auto duplicateProfile = CreateEmptyMappingProfile(); @@ -823,10 +900,11 @@ namespace RemappingUITests const auto duplicateResult = configuration.LoadSettingsFromJson(duplicateProfile); Assert::AreEqual(static_cast(MappingConfigurationLoadResult::Partial), static_cast(duplicateResult)); Assert::AreEqual(static_cast(1), configuration.textExpansions.size()); - Assert::AreEqual(existingRule.id, configuration.textExpansions[0].id); + Assert::AreEqual(std::wstring(TextExpansionId1), configuration.textExpansions[0].id); + Assert::AreEqual(std::wstring(L"first"), configuration.textExpansions[0].sourceText); } - TEST_METHOD (LoadTextExpansions_ShouldRejectMissingRequiredFieldsWithoutCommittingPartialSnapshot) + TEST_METHOD (LoadTextExpansions_ShouldSkipEntriesWithMissingRequiredFieldsAndApplyValidSubset) { MappingConfiguration configuration; Assert::IsTrue(configuration.AddTextExpansion(CreateTextExpansionRule(TextExpansionId3))); @@ -839,6 +917,13 @@ namespace RemappingUITests KeyboardManagerConstants::TextExpansionEnabledSettingName }) { auto profile = CreateEmptyMappingProfile(); + auto rules = GetTextExpansionArray(profile); + rules.Append(CreateTextExpansionJson( + TextExpansionId2, + L"valid", + CreateActivationJson({ VK_TAB }), + L"loaded", + true)); auto rule = CreateTextExpansionJson( TextExpansionId1, L"brb", @@ -846,16 +931,17 @@ namespace RemappingUITests L"replacement", true); rule.Remove(missingField); - GetTextExpansionArray(profile).Append(rule); + rules.Append(rule); const auto result = configuration.LoadSettingsFromJson(profile); Assert::AreEqual(static_cast(MappingConfigurationLoadResult::Partial), static_cast(result)); Assert::AreEqual(static_cast(1), configuration.textExpansions.size()); - Assert::AreEqual(std::wstring(TextExpansionId3), configuration.textExpansions[0].id); + Assert::AreEqual(std::wstring(TextExpansionId2), configuration.textExpansions[0].id); + Assert::AreEqual(std::wstring(L"loaded"), configuration.textExpansions[0].replacementText); } } - TEST_METHOD (LoadTextExpansions_ShouldRejectInvalidActivationShapesWithoutCommittingPartialSnapshot) + TEST_METHOD (LoadTextExpansions_ShouldSkipInvalidActivationShapesAndApplyValidSubset) { MappingConfiguration configuration; Assert::IsTrue(configuration.AddTextExpansion(CreateTextExpansionRule(TextExpansionId3))); @@ -874,7 +960,14 @@ namespace RemappingUITests for (const auto& activation : invalidActivations) { auto profile = CreateEmptyMappingProfile(); - GetTextExpansionArray(profile).Append(CreateTextExpansionJson( + auto rules = GetTextExpansionArray(profile); + rules.Append(CreateTextExpansionJson( + TextExpansionId2, + L"valid", + CreateActivationJson({ VK_TAB }), + L"loaded", + true)); + rules.Append(CreateTextExpansionJson( TextExpansionId1, L"brb", activation, @@ -884,10 +977,18 @@ namespace RemappingUITests const auto result = configuration.LoadSettingsFromJson(profile); Assert::AreEqual(static_cast(MappingConfigurationLoadResult::Partial), static_cast(result)); Assert::AreEqual(static_cast(1), configuration.textExpansions.size()); - Assert::AreEqual(std::wstring(TextExpansionId3), configuration.textExpansions[0].id); + Assert::AreEqual(std::wstring(TextExpansionId2), configuration.textExpansions[0].id); + Assert::AreEqual(std::wstring(L"loaded"), configuration.textExpansions[0].replacementText); } auto nonArrayProfile = CreateEmptyMappingProfile(); + auto nonArrayRules = GetTextExpansionArray(nonArrayProfile); + nonArrayRules.Append(CreateTextExpansionJson( + TextExpansionId2, + L"valid", + CreateActivationJson({ VK_TAB }), + L"loaded", + true)); auto nonArrayRule = CreateTextExpansionJson( TextExpansionId1, L"brb", @@ -895,12 +996,13 @@ namespace RemappingUITests L"replacement", true); nonArrayRule.SetNamedValue(KeyboardManagerConstants::TextExpansionActivationKeysSettingName, json::value(L"32")); - GetTextExpansionArray(nonArrayProfile).Append(nonArrayRule); + nonArrayRules.Append(nonArrayRule); const auto nonArrayResult = configuration.LoadSettingsFromJson(nonArrayProfile); Assert::AreEqual(static_cast(MappingConfigurationLoadResult::Partial), static_cast(nonArrayResult)); Assert::AreEqual(static_cast(1), configuration.textExpansions.size()); - Assert::AreEqual(std::wstring(TextExpansionId3), configuration.textExpansions[0].id); + Assert::AreEqual(std::wstring(TextExpansionId2), configuration.textExpansions[0].id); + Assert::AreEqual(std::wstring(L"loaded"), configuration.textExpansions[0].replacementText); } TEST_METHOD (TextExpansionCrud_ShouldUseGuidPreserveOrderAndAllowDuplicateSourceAndActivation) diff --git a/src/modules/keyboardmanager/KeyboardManagerEngineLibrary/KeyboardManager.cpp b/src/modules/keyboardmanager/KeyboardManagerEngineLibrary/KeyboardManager.cpp index 0d5e112d18..a6703a1db9 100644 --- a/src/modules/keyboardmanager/KeyboardManagerEngineLibrary/KeyboardManager.cpp +++ b/src/modules/keyboardmanager/KeyboardManagerEngineLibrary/KeyboardManager.cpp @@ -181,12 +181,12 @@ void KeyboardManager::LoadSettings() std::this_thread::sleep_for(std::chrono::milliseconds(500)); // Retry only file-level/transient failures. A Partial result is deterministic - // invalid data and has already retained the last-known-good snapshot. + // invalid data whose valid entries have already been applied. loadResult = state.LoadSettingsWithResult(); } if (loadResult == MappingConfigurationLoadResult::Partial) { - Logger::error(L"Keyboard Manager settings contained invalid entries; retained the last-known-good runtime snapshot."); + Logger::error(L"Keyboard Manager settings contained invalid entries; skipped them and loaded the valid entries."); } try { diff --git a/src/modules/keyboardmanager/common/MappingConfiguration.cpp b/src/modules/keyboardmanager/common/MappingConfiguration.cpp index 46a17cf177..591ff627ec 100644 --- a/src/modules/keyboardmanager/common/MappingConfiguration.cpp +++ b/src/modules/keyboardmanager/common/MappingConfiguration.cpp @@ -776,7 +776,7 @@ bool MappingConfiguration::LoadShortcutRemaps(const json::JsonObject& jsonData, } // Load app specific shortcut remaps - result = result && LoadAppSpecificShortcutRemaps(remapShortcutsData); + result = LoadAppSpecificShortcutRemaps(remapShortcutsData) && result; } } catch (...) @@ -804,10 +804,6 @@ MappingConfigurationLoadResult MappingConfiguration::LoadSettingsFromJson(const result = candidate.LoadShortcutRemaps(configFile, KeyboardManagerConstants::RemapShortcutsToTextSettingName) && result; result = candidate.LoadSingleKeyToTextRemaps(configFile) && result; result = candidate.LoadTextExpansions(configFile) && result; - if (!result) - { - return MappingConfigurationLoadResult::Partial; - } singleKeyReMap = std::move(candidate.singleKeyReMap); scanMap = std::move(candidate.scanMap); @@ -818,7 +814,7 @@ MappingConfigurationLoadResult MappingConfiguration::LoadSettingsFromJson(const appSpecificShortcutReMap = std::move(candidate.appSpecificShortcutReMap); appSpecificShortcutReMapSortedKeys = std::move(candidate.appSpecificShortcutReMapSortedKeys); - return MappingConfigurationLoadResult::Success; + return result ? MappingConfigurationLoadResult::Success : MappingConfigurationLoadResult::Partial; } MappingConfigurationLoadResult MappingConfiguration::LoadSettingsFromFile( @@ -856,10 +852,7 @@ MappingConfigurationLoadResult MappingConfiguration::LoadSettingsFromFile( } const auto result = LoadSettingsFromJson(*configFile); - if (result == MappingConfigurationLoadResult::Success) - { - currentConfig = configurationName; - } + currentConfig = configurationName; return result; } diff --git a/src/modules/keyboardmanager/common/MappingConfiguration.h b/src/modules/keyboardmanager/common/MappingConfiguration.h index daa832cf2c..0fe1021cd2 100644 --- a/src/modules/keyboardmanager/common/MappingConfiguration.h +++ b/src/modules/keyboardmanager/common/MappingConfiguration.h @@ -29,6 +29,7 @@ using TextExpansionTable = std::vector; enum class MappingConfigurationLoadResult { Success = 0, + // The parsed profile was applied, but one or more invalid entries were skipped. Partial = 1, Failure = 2, }; @@ -55,10 +56,10 @@ public: // Load while distinguishing rejected entries from a file-level failure. MappingConfigurationLoadResult LoadSettingsWithResult(); - // Load an already parsed profile. Invalid entries are rejected and reported as Partial. + // Load an already parsed profile. Valid entries are applied, while rejected entries are reported as Partial. MappingConfigurationLoadResult LoadSettingsFromJson(const json::JsonObject& configFile); - // Load a named profile file. A profile that has not been created yet is a valid empty snapshot. + // Load a named profile file. A missing profile is a valid empty snapshot; a partial profile applies its valid entries. MappingConfigurationLoadResult LoadSettingsFromFile(const std::wstring& configurationName, const std::wstring& filePath); bool IsConfigurationNameResolved() const;