Fix pinyin match for Chinese restart command & Fix incorrect pinyin highlight for polyphonic characters - #4544
Open
SparkUiX wants to merge 9 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes pinyin matching for the built-in Chinese restart command phrase 重启 by introducing a small phrase-level override so that queries like chongqi correctly match 重启 (and 重启 Flow Launcher) while preserving existing behavior for unrelated polyphonic words like 重庆.
Changes:
- Added a polyphonic phrase override map in
PinyinAlphabetand applied it during pinyin cache construction. - Refactored
PinyinAlphabetto supportSettingsinjection (improves testability). - Added unit tests covering the
重启override and fuzzy matching forchongqi.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Flow.Launcher.Infrastructure/PinyinAlphabet.cs | Adds a targeted override for 重启 pinyin output and enables Settings injection for deterministic behavior in tests. |
| Flow.Launcher.Test/PinyinAlphabetTest.cs | Adds unit tests validating translation and fuzzy-match behavior for 重启/chongqi, plus a regression check for 重庆. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
📝 WalkthroughWalkthrough
ChangesPolyphonic Phrase Pinyin Override
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Settings
participant PinyinAlphabet
participant PinyinProvider
Settings->>PinyinAlphabet: Change phrase-override setting
PinyinAlphabet->>PinyinAlphabet: Reload snapshot and invalidate cache
PinyinAlphabet->>PinyinProvider: Generate character Pinyin
PinyinProvider-->>PinyinAlphabet: Return Pinyin tokens
PinyinAlphabet->>PinyinAlphabet: Apply phrase overrides
PinyinAlphabet->>PinyinAlphabet: Convert using double-Pinyin configuration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Flow.Launcher.Infrastructure/PinyinAlphabet.cs (1)
185-188: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDefensively validate override span writes.
The override loop assumes phrase length and pinyin token count always align. A future bad entry can throw
IndexOutOfRangeExceptionat Line 187. Add a guard before writing the span to keep translation resilient.Suggested hardening
while (index >= 0) { + if (pinyin.Length != phrase.Length || index + pinyin.Length > resultList.Length) + { + index = content.IndexOf(phrase, index + phrase.Length, StringComparison.Ordinal); + continue; + } + for (var i = 0; i < pinyin.Length; i++) { resultList[index + i] = pinyin[i]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Flow.Launcher.Infrastructure/PinyinAlphabet.cs` around lines 185 - 188, The loop at line 185-188 that writes pinyin characters to resultList lacks bounds validation and assumes the phrase length always matches the pinyin token count. Add a guard condition before the for loop that iterates over pinyin to validate that index plus pinyin.Length does not exceed resultList.Length, preventing potential IndexOutOfRangeException when future bad entries are encountered. If the bounds check fails, handle it gracefully by skipping the write or logging a warning to keep the translation resilient.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Flow.Launcher.Infrastructure/PinyinAlphabet.cs`:
- Around line 31-33: The PinyinAlphabet constructor does not validate that the
injected settings parameter is null before assigning it to the _settings field.
Add a null guard at the beginning of the PinyinAlphabet constructor that throws
an ArgumentNullException if the settings parameter is null, ensuring the error
fails fast instead of allowing a NullReferenceException to occur later when
_settings is dereferenced.
---
Nitpick comments:
In `@Flow.Launcher.Infrastructure/PinyinAlphabet.cs`:
- Around line 185-188: The loop at line 185-188 that writes pinyin characters to
resultList lacks bounds validation and assumes the phrase length always matches
the pinyin token count. Add a guard condition before the for loop that iterates
over pinyin to validate that index plus pinyin.Length does not exceed
resultList.Length, preventing potential IndexOutOfRangeException when future bad
entries are encountered. If the bounds check fails, handle it gracefully by
skipping the write or logging a warning to keep the translation resilient.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19d08c6a-e4e2-492c-adb9-1621554a762a
📒 Files selected for processing (2)
Flow.Launcher.Infrastructure/PinyinAlphabet.csFlow.Launcher.Test/PinyinAlphabetTest.cs
Contributor
|
Nice workaround. Our 3rd party pinyin nuget is not good while dealing with polyphonic. |
Jack251970
requested changes
Jun 29, 2026
Jack251970
left a comment
Jack251970
left a comment
Member
There was a problem hiding this comment.
LGTM! Could you please add more pairs in PolyphonicPhraseOverrides? Currently, we just have one pair in it.
Here is some tools for your reference: https://chatgpt.com/share/6a428475-1ba4-83ea-b48c-ef9ab02c3859.
Jack251970
marked this pull request as draft
August 7, 2026 08:36
Significantly increased the polyphonic pinyin dictionary by adding many new Chinese words, idioms, place names, and technical terms with accurate pinyin mappings. No existing entries were modified or removed; all changes are additive and improve pinyin conversion coverage.
Jack251970
changed the title
- Load polyphonic phrase data from `polyphonic_pinyin.json` - Add `UsePolyphonicPhraseOverrides` setting with UI and localization - Apply phrase-level overrides in `PinyinAlphabet` with cache invalidation - Add tests for override enabled, disabled, and toggled at runtime - Update project file to deploy new resource file
The _usePolyphonicPhraseOverrides field in Settings.cs now defaults to true instead of false, making UsePolyphonicPhraseOverrides enabled by default.
Added three unit tests in PinyinAlphabetTest.cs to verify: - Polyphonic phrase overrides take precedence over double pinyin. - Double pinyin enabled after caching respects polyphonic overrides. - Fuzzy matching uses override pronunciation with both features enabled.
Jack251970
marked this pull request as ready for review
August 7, 2026 09:11
Contributor
|
This PR has 238,359 reviewable changed lines after ignored/generated files are excluded, above cubic's default 50,000-changed-line automatic review limit. Most of the diff comes from:
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Flow.Launcher.Infrastructure/PinyinAlphabet.cs`:
- Around line 36-59: Make cache invalidation atomic with cache construction in
the settings-change handler and the cache flow around BuildCacheFromContent:
synchronize replacement of pronunciation tables and maxPolyphonicPhraseLength
with cache lookup and publication, ensuring a worker cannot publish results
built from stale settings after _pinyinCache.Clear(). Alternatively, add a
configuration revision to each cache entry and reject results built against an
older revision.
In `@Flow.Launcher/Flow.Launcher.csproj`:
- Around line 181-183: Add a None item for Resources\polyphonic_pinyin.json in
Flow.Launcher.Test.csproj with CopyToOutputDirectory set to PreserveNewest,
matching the existing resource staging pattern so PinyinAlphabet can load the
test override from AppContext.BaseDirectory.
In `@Flow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml`:
- Around line 563-578: Update the SettingsPaneGeneralViewModel.ShouldUsePinyin
setter to raise PropertyChanged after changing the setting, so the Visibility
binding on the UsePolyphonicPhraseOverrides SettingsCard refreshes immediately
when Pinyin is toggled.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cea32d0f-5ea9-44e1-afe5-01cc860e4a61
📒 Files selected for processing (8)
Flow.Launcher.Infrastructure/PinyinAlphabet.csFlow.Launcher.Infrastructure/UserSettings/Settings.csFlow.Launcher.Test/PinyinAlphabetTest.csFlow.Launcher/Flow.Launcher.csprojFlow.Launcher/Languages/en.xamlFlow.Launcher/Resources/polyphonic_pinyin.jsonFlow.Launcher/SettingPages/ViewModels/SettingsPaneGeneralViewModel.csFlow.Launcher/SettingPages/Views/SettingsPaneGeneral.xaml
Refactored PinyinAlphabet to encapsulate configuration in an immutable PinyinConfiguration record, protected by a lock. Updated pinyin cache to use content and config revision as key, preventing stale translations after settings changes. Rewrote Translate to handle concurrent config changes and avoid publishing outdated results. Made table/override loaders static and return values. Added unit test to ensure stale results are not published if config changes during translation. Improves correctness and thread safety during config updates.
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.
Summary
重启重启and重启 Flow Launcherto match queries likechongqiScope
This does not try to solve every Chinese polyphonic character case. It fixes the common built-in command phrase from #3955 while keeping the behavior for
重庆asChong Qing.Tests
dotnet test Flow.Launcher.Test\Flow.Launcher.Test.csproj --filter "FullyQualifiedName~PinyinAlphabetTest"dotnet test Flow.Launcher.Test\Flow.Launcher.Test.csproj --no-restore --filter "FullyQualifiedName~PinyinAlphabetTest|FullyQualifiedName~FuzzyMatcherTest"Fixes #3955
Summary by cubic
Fixes Pinyin matching for the Chinese Restart command with phrase-level polyphonic overrides enabled by default, and improves accuracy across many phrases. Adds thread-safe config and cache management to prevent stale translations, while working with Double Pinyin.
Summary of changes
PinyinAlphabetnow uses an immutable config snapshot with a lock and revisioned cache; discards stale translations if settings change duringTranslate; applies phrase overrides before Double Pinyin; settings (ShouldUsePinyin,UseDoublePinyin,DoublePinyinSchema,UsePolyphonicPhraseOverrides) trigger reload; JSON loaders accept comments with safe fallback.Resources/polyphonic_pinyin.json(significantly expanded); settingUsePolyphonicPhraseOverrides(default true) with UI anden.xamlstrings; resource copied inFlow.Launcher.csproj; internal ctor overload to inject pinyin source for tests.PinyinAlphabetTestcovers override on/off; “重启” vs “重庆”; fuzzy match for “chongqi”; runtime toggles and cache invalidation; override precedence over Double Pinyin; Double Pinyin after caching; fuzzy matching with both features on; prevents publishing stale results if settings change mid-translation.Release Note
Typing “chongqi” now finds the Restart command, and Pinyin results for many Chinese phrases are more accurate, including with Double Pinyin.
Written for commit f15de62. Summary will update on new commits.