[Quick Accent] Move language data to PowerAccent.Common library, refactor#47211
Conversation
… and Settings. Moved language/character mapping data and enums (Language, LetterKey) from PowerAccent.Core to new PowerAccent.Common project. Introduced CharacterMappings class for shared mapping logic. Updated Core to use a thin adapter for WinRT/managed enum bridging. Removed old Languages.cs. Updated all references in Core and Settings to use shared types. Added unit tests to ensure enum sync. Updated solution and project files to include new shared and test projects.
…arian. Correct issue for Welsh double quotes.
…order for languages and language groups, and additional unit tests.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
|
@snickler Just FYI, I've added new project file with an explicit It also made me wonder if the version-related information from Common.Dotnet.CSWinRT.props could/should be moved to a separate file for this sort of thing, i.e. the first PropertyGroup. Maybe Common.Dotnet.Versions.props or Common.Dotnet.WindowsTargets.props or something? |
This comment has been minimized.
This comment has been minimized.
Hmmm yeah maybe we move the |
There was a problem hiding this comment.
Pull request overview
This PR extracts Quick Accent language/character mapping data into a new PowerAccent.Common library so both the Quick Accent module and the Settings UI consume the same canonical dataset, with explicit ordering and new unit tests to prevent regressions.
Changes:
- Introduces
PowerAccent.CommoncontainingLanguage/LetterKey/LanguageInfoand the centralizedCharacterMappingsregistry + ordering/lookup APIs. - Updates Settings UI and
PowerAccent.Coreto consumeCharacterMappingsinstead of the removedLanguages.cs. - Adds
PowerAccent.Common.UnitTeststo validate enum sync, data completeness, and ordering invariants.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/settings-ui/Settings.UI/ViewModels/PowerAccentViewModel.cs | Builds Settings language/group models from PowerAccent.Common.CharacterMappings instead of a hardcoded list. |
| src/settings-ui/Settings.UI/PowerToys.Settings.csproj | References the new PowerAccent.Common project from Settings UI. |
| src/modules/poweraccent/PowerAccent.Core/Services/SettingsService.cs | Switches selected language parsing/storage to PowerAccent.Common.Language. |
| src/modules/poweraccent/PowerAccent.Core/PowerAccent.cs | Uses the new CharacterMappings.GetCharacters API for popup character retrieval. |
| src/modules/poweraccent/PowerAccent.Core/PowerAccent.Core.csproj | References the new PowerAccent.Common project from Core. |
| src/modules/poweraccent/PowerAccent.Core/Languages.cs | Removes the old monolithic mapping implementation. |
| src/modules/poweraccent/PowerAccent.Core/CharacterMappings.cs | Adds a thin adapter to bridge WinRT LetterKey to managed PowerAccent.Common.LetterKey. |
| src/modules/poweraccent/PowerAccent.Common/PowerAccent.Common.csproj | New shared library project for mappings and related types. |
| src/modules/poweraccent/PowerAccent.Common/Language.cs | New shared Language enum. |
| src/modules/poweraccent/PowerAccent.Common/LetterKey.cs | New managed LetterKey enum mirroring the WinRT IDL values. |
| src/modules/poweraccent/PowerAccent.Common/LanguageGroup.cs | Defines language group categories shared by UI and core. |
| src/modules/poweraccent/PowerAccent.Common/LanguageInfo.cs | Defines the per-language metadata record used by the registry. |
| src/modules/poweraccent/PowerAccent.Common/CharacterMappings.cs | New canonical mapping registry, explicit ordering, lookup dictionary, and character collection API. |
| src/modules/poweraccent/PowerAccent.Common.UnitTests/PowerAccent.Common.UnitTests.csproj | New unit test project validating mapping invariants and enum sync. |
| src/modules/poweraccent/PowerAccent.Common.UnitTests/LetterKeyTests.cs | Tests that managed LetterKey matches WinRT LetterKey names/values. |
| src/modules/poweraccent/PowerAccent.Common.UnitTests/CharacterMappingsTests.cs | Tests completeness, ordering, deduping, caching behavior, and invariants. |
| PowerToys.slnx | Adds the new Common library and unit test project to the solution. |
| public static readonly IReadOnlyList<LanguageInfo> All = | ||
| [ | ||
| new(Language.SPECIAL, "Special", LanguageGroup.Special, new Dictionary<LetterKey, string[]> | ||
| { | ||
| [LetterKey.VK_0] = ["₀", "⁰", "°", "↉", "₎", "⁾"], |
There was a problem hiding this comment.
CharacterMappings.All is exposed publicly but is backed by mutable Dictionary<LetterKey, string[]> instances (and the string[] values are mutable as well). Because LanguageInfo.Characters is an interface type, consumers can still downcast and mutate the underlying dictionaries/arrays, which could lead to hard-to-debug runtime behavior and cache inconsistencies. Consider exposing immutable/read-only collections (e.g., ReadOnlyDictionary/ImmutableDictionary and immutable character collections) or otherwise preventing mutation of the shared mapping data.
There was a problem hiding this comment.
Acknowledged — the outer Characters property is already typed as IReadOnlyDictionary<LetterKey, string[]>, which prevents adding/removing entries through the interface. The remaining mutability is in the inner string[] values. Converting these to ReadOnlyMemory<string> or ImmutableArray<string> would require changing the LanguageInfo record signature and all consumers — escalating this as a design trade-off for the maintainer to decide.
- Remove unsupported UserDefined group from _groupResourceKeys - Add .Where() guards for groups without resource keys to prevent KeyNotFoundException when GroupDisplayOrder includes future groups Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
… 10, and add the .csproj to the exclusion list for the pipeline verification step, as it does not target WinRT.
This comment has been minimized.
This comment has been minimized.
|
please merge main and resolve the conflict. |
|
@moooyo I've merged main and resolved the conflicts. Thanks. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@daverayment still failed. src\common\interop\interop-tests\bin\x64\Release\Microsoft.Interop.Tests.dll(0,0): Error run failed: Tests failed: 'C:\a_work\1\s\src\common\interop\interop-tests\bin\x64\Release\TestResults\Microsoft.Interop.Tests_net10.0-windows10.0.26100.0_x64.log' [net10.0-windows10.0.26100.0|x64] |
|
@moooyo These PowerAccent changes have no effect on IPC messaging, which is what the Interop test is exercising. The test has this note and ad-hoc 100ms delay, which indicates it may have failed because of a resource-constrained agent or other timing issue: // Test can be flaky as the pipes are still being set up and we end up receiving no message. Wait for a bit to avoid that.
Thread.Sleep(100);Could you try the run again, please? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
need a owner approval |
…ytonic resource (#48054) ## Summary Fixes the **"Build PowerToys main project"** failure on every PR/CI build off current `main` (e.g. Dart build [`147597426`](https://microsoft.visualstudio.com/Dart/_build/results?buildId=147597426)): ``` WINAPPSDKGENERATEPROJECTPRIFILE: Error PRI175: Processing Resources failed with error: Duplicate Entry. WINAPPSDKGENERATEPROJECTPRIFILE: Error PRI277: Conflicting values for resource 'Resources/QuickAccent_SelectedLanguage_Greek_Polytonic' ``` ## Root cause `src/settings-ui/Settings.UI/Strings/en-us/Resources.resw` contained two `<data name="QuickAccent_SelectedLanguage_Greek_Polytonic">` entries: - Line 3597 — original, added by #47021 (*"[PowerAccent] adding greek polytonic"*), placed in the alphabetized QuickAccent block. - Line 6175 — duplicate, appended at the file tail by #47211 (*"[Quick Accent] Move language data to PowerAccent.Common library, refactor"*). The two entries are byte-identical (same value `Greek Polytonic`, same `xml:space="preserve"`). The WinAppSDK PRI generator (`MakePri`) rejects duplicates, so every build off current `main` fails before reaching the compile step. ## Fix Remove the tail duplicate (3-line block right before `</root>`). Keeps the original entry in its alphabetized location. ## Validation - `git diff` shows exactly 3 deleted lines. - `[xml](Get-Content … -Raw)` round-trip succeeds — file is still well-formed XML. - `Select-String` count of `QuickAccent_SelectedLanguage_Greek_Polytonic` in the file is now **1** (was 2). - Only `en-us` has this key (other localized `.resw` files are populated later via Touchdown), so no other file needs touching. ## Note Discovered while investigating a CI failure unrelated to my other PR #48050 (signing of `WinUI3Apps\YamlDotNet.dll`). Both fixes are needed for the 0.100 release pipeline; this PR unblocks PR builds off `main`, and #48050 unblocks the signed release pipeline once both are merged to `main` and the next `main → stable` sync happens. Co-authored-by: Copilot <Copilot@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary of the Pull Request
Language data for Quick Accent was previously defined in PowerAccent.Core/Languages.cs, internal to the Quick Accent application itself. The Settings application had no access to the list and had to maintain a parallel, manually-synchronised list of the language names and groups, and there was no single place to look up the complete set of languages.
This PR resolves this and extracts the language data into a new PowerAccent.Common project with no external or UI dependencies, making it the single source of truth for both the character popup and Settings UI.
The implicit and non-obvious ordering of the old Languages.cs has been replaced with explicit ordering of the language groups and languages list.
A new unit test project for the application has also been added.
PR Checklist
Detailed Description of the Pull Request / Additional comments
The following changes and additions have been made:
New project PowerAccent.Common contains:
Language(for spoken languages),Special(for sets like Currency and International Phonetic Alphabet), orUserDefined(reserved for future work)All- the canonical registry of everyLanguageInfoentryDisplayOrder- the explicit within-group ordering for languages, for the default popup ordering, i.e. when the user has not got frequency-based ordering enabled. This is a culturally-neutral alphabetical ordering by our pseudo-ISO language IDs, and close to the previous order, minimising any impact to users' muscle memoryGroupDisplayOrder- the explicit ordering of language groups for both the popup and the Settings UILanguageLookup- new O(1)LanguagetoLanguageInfodictionaryGetCharacters(LetterKey, Language[])- a deduplicating character collector and sorterA significant improvement over the old Languages.cs is that language ordering is now explicit and in one place. Previously, popup ordering was an implicit side-effect of a large Union chain across all language mappings, with the enum, the language mapping declarations and the union all carrying different orderings. This was a source of confusion and made adding new languages fragile.
The original Languages.cs has been deleted. There's now a thin
CharacterMappingsadapter that casts theLetterKeyto the managed equivalent by numeric value. This is another effort to decouple the Settings UI and Quick Accent and only share the required elements.In Settings, the
PowerAccentViewModelclass has been updated. It now derives the language list directly fromCharacterMappings.All; theInitializeLanguages()call handles localisation, sorting and grouping in a single place usingGroupDisplayOrder.A new unit test project covers
CharacterMappings, theLetterKeyIDL-to-managed code bridge and general language declaration checks. The application is now much more robust against changes which alter the declared order, introduce empty/null elements, and so on. It is now impossible to add a new language and forget a constituent part (the enum entry, its place in the display order, the character mappings themselves) without a test failing.Validation Steps Performed
See new unit test project for comprehensive tests.
Manually confirmed: