Fix incorrect pinyin comparision for polyphonic characters (e.g. 重启) & Add related UI for polyphonic parsing & Refactor PinyinAlphabet logic - #4544
Conversation
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}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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}
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
|
Nice workaround. Our 3rd party pinyin nuget is not good while dealing with polyphonic. |
Jack251970
left a comment
Jack251970
left a comment
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.
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.
|
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 |
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.
Jack251970
changed the title
Jack251970
changed the title
|
@VictoriousRaptor Sorry for any misleading in the title of this PR. I have not fixed pinyin highlight for polyphonic characters in this PR. So could you please merged the changes of #4607 into this PR if the changes are pretty minor? If you think there are plenty of changes to fix the polyphonic highlight, I think we can merge this PR first and then you can continue your fix in another PR. |
Changed the glyph icon in the SettingsCard header from "" to "" for improved UI consistency. No functional changes were made.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Flow.Launcher.Infrastructure/PinyinAlphabet.cs:86
- Every relevant settings change rebuilds both resources, so changing only the double-pinyin schema synchronously reparses and reallocates the entire 47,111-entry phrase dictionary while holding
_configurationLock. This runs from the settings property-change callback and can stall the UI while also blocking translations. Cache the immutable phrase data after its first load, or reload only the resource affected by the changed setting instead of recreating both tables on every revision.
var doublePinyinTable = LoadDoublePinyinTable(useDoublePinyin, doublePinyinSchema);
var (polyphonicPhraseOverrides, maxPolyphonicPhraseLength) =
LoadPolyphonicPhraseOverrides(shouldUsePinyin, usePolyphonicPhraseOverrides);
Flow.Launcher.Infrastructure/PinyinAlphabet.cs:320
- The override data uses spellings such as
Nü,Lü, andLüe, but the existing double-pinyin tables only accept the ToolGood-style keysNv,Lv, andLve. Assigning those values unchanged meansToDoublePinyinfalls back to the full syllable, so phrases such as女儿and略知一二cannot be matched with their configured double-pinyin forms (and full-pinyinnv/lvqueries also diverge from the library output). Normalize the override syllables to the samevconvention before storing them, and add an umlaut-case test.
resultList[start + i] = pinyin[i];
#4607 may have performance issue in some cases because it matches every potential polyphonic combinations. I thought it was some issue unrelated to polyphonic so assigned it to Copilot. |
|
I have an idea about the issue: instead of creating our patch for polyphonic characters, use another external library (like ib_matcher)for MATCHING Chinese queries rather than just converting Chinese to pinyin and use our fuzzy matcher to match. Another feasible solutions besides creating our own solution: fixing the dictionary of ToolGoods.Pinyin（may solve "重启" and “核查”). Any thoughts? @SparkUiX @Jack251970 |
|
I think the PR is good to go. But can we check the json have some redundant phrases in ToolGood's dictionary? |
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
Summary of changes
Improves Pinyin matching for Chinese polyphonic phrases (e.g., 重启) by applying phrase-level overrides and making
PinyinAlphabetconfig/cache thread-safe. Old behavior read 重启 as Zhong Qi; new behavior reads Chong Qi, works with Double Pinyin, and prevents stale results when settings change.PinyinAlphabetnow uses an immutable config snapshot with a lock and a revisioned cache; discards builds if settings change mid-translate; applies phrase overrides before Double Pinyin; reloads onShouldUsePinyin,UseDoublePinyin,DoublePinyinSchema, andUsePolyphonicPhraseOverrides.Resources/polyphonic_pinyin.jsonto correct common polyphonic words; addsUsePolyphonicPhraseOverrides(default on) with UI and localized strings; JSON loaders accept comments and fail safe; project copies the new resource at build; internal constructor allows injecting a pinyin source for tests; updated Settings card icon glyph.Release Note
Typing “chongqi” now finds the Restart command, and Pinyin results for many Chinese phrases are more accurate, even with Double Pinyin enabled.
Written for commit ea5d823. Summary will update on new commits.