Skip to content

feat(cmdpal): add pinyin support for Chinese input method #39354

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

frg2089
Copy link
Contributor

@frg2089 frg2089 commented May 11, 2025

Summary of the Pull Request

  • Add ToolGood.Words.Pinyin package to support pinyin conversion
  • Implement pinyin matching in StringMatcher class
  • Update project dependencies and Directory.Packages.props

PR Checklist

Detailed Description of the Pull Request / Additional comments

I've completed a rough implementation of pinyin support, but since I'm currently unsure where to add the toggle for pinyin support, this feature is enabled by default for now.

2025-05-11.230855.mp4

Validation Steps Performed

@htcfreek
Copy link
Collaborator

htcfreek commented May 11, 2025

@frg2089
You need to update Notice.md. Otherwise build will fail later.

What is the reason for using a different Pinyin package than we already use for PT Run (hyjiacan.pinyin4net)? I think we should avoid using different pinyin packages. What do you think @zadjii-msft?

@htcfreek
Copy link
Collaborator

I've completed a rough implementation of pinyin support, but since I'm currently unsure where to add the toggle for pinyin support, this feature is enabled by default for now.

@zadjii-msft
Can you help with this?

@frg2089
Copy link
Contributor Author

frg2089 commented May 11, 2025

What is the reason for using a different Pinyin package than we already use for PT Run (hyjiacan.pinyin4net)?

I just selected a recently updated package on nuget according to the last update time. The last update time of hyjiacan.pinyin4net was 2022/2/23.

Please let me know if you want me to continue using hyjiacan.pinyin4net.

@moooyo
Copy link
Contributor

moooyo commented May 12, 2025

one question, does this package support Native AOT? if not, we can't get it in.
@frg2089

@zadjii-msft zadjii-msft added the Product-Command Palette Refers to the Command Palette utility label May 13, 2025
@frg2089
Copy link
Contributor Author

frg2089 commented May 14, 2025

one question, does this package support Native AOT? if not, we can't get it in. @frg2089

I build it with <IsAotCompatible>true</IsAotCompatible> and they have not any warning.

@yeelam-gordon yeelam-gordon requested review from Copilot and moooyo May 16, 2025 06:50
@yeelam-gordon yeelam-gordon added this to the PowerToys 0.92 milestone May 16, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for matching Chinese text by converting inputs and targets to their pinyin representations before performing fuzzy matching.

  • Introduces the ToolGood.Words.Pinyin package as a dependency.
  • Extends StringMatcher.FuzzyMatch to generate and match pinyin variants.
  • Updates central package versions in Directory.Packages.props.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/modules/cmdpal/extensionsdk/Microsoft.CommandPalette.Extensions.Toolkit/StringMatcher.cs Implement pinyin generation and matching loop
src/modules/cmdpal/extensionsdk/Microsoft.CommandPalette.Extensions.Toolkit/Microsoft.CommandPalette.Extensions.Toolkit.csproj Add ToolGood.Words.Pinyin package reference
Directory.Packages.props Pin ToolGood.Words.Pinyin version centrally
Comments suppressed due to low confidence (1)

src/modules/cmdpal/extensionsdk/Microsoft.CommandPalette.Extensions.Toolkit/StringMatcher.cs:82

  • New pinyin matching behavior isn't covered by existing tests. Add unit tests for Chinese inputs and their pinyin variants to validate correctness.
queryList.Add(WordsHelper.GetPinyin(input));

List<string> stringToCompareList = [stringToCompare];

// TODO: Add switch for Chinese.
if (true)
Copy link
Preview

Copilot AI May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Avoid using if (true) as a placeholder. Replace with a real feature flag or user-setting check to toggle pinyin support.

Suggested change
if (true)
if (EnablePinyinSupport)

Copilot uses AI. Check for mistakes.

This comment has been minimized.

This comment has been minimized.

@yeelam-gordon yeelam-gordon added the Needs-Triage For issues raised to be triaged and prioritized by internal Microsoft teams label Jun 10, 2025
@yeelam-gordon
Copy link
Contributor

@cinnamon-msft, in this PR, there are multiple input need and see if you have any suggestion/objection:

  1. in run v1, there is an 3rd party library hyjiacan.pinyin4net (last update is 2022) for pinyun support. While it looks like this ToolGood.Words.Pinyin is faster and lightweight, as well as more up-to-date (2025) with wider usage .
    ToolGood.Words.Pinyin is Apache-2.0 license

  2. If there is no AOT problem, and let's say we add a new configuration in cmdpal to support pinyin matching, see if there is other concern.

This comment has been minimized.

@moooyo
Copy link
Contributor

moooyo commented Jun 11, 2025

It's easy to add a new setting to control this behaviour.

See this PR as sample: https://github.com/microsoft/PowerToys/pull/38923/files#diff-2550e476d6684aaba319126262868c92cceb2d9776d3d78348baf4167824b30c

@frg2089

Ok, I've verified this PR is AOT compatible.

@frg2089
Copy link
Contributor Author

frg2089 commented Jun 11, 2025

It's easy to add a new setting to control this behaviour.

See this PR as sample: https://github.com/microsoft/PowerToys/pull/38923/files#diff-2550e476d6684aaba319126262868c92cceb2d9776d3d78348baf4167824b30c

I have created settings for Pinyin support, but I don't know how to make StringMatcher use it.

This comment has been minimized.

This comment has been minimized.

@moooyo
Copy link
Contributor

moooyo commented Jun 12, 2025

Could you please merge main into your branch?

@frg2089

frg2089 and others added 6 commits June 13, 2025 00:21
- Add ToolGood.Words.Pinyin package to support pinyin conversion
- Implement pinyin matching in StringMatcher class
- Update project dependencies and Directory.Packages.props

Signed-off-by: 舰队的偶像-岛风酱! <frg2089@outlook.com>
- Added notice for the use of ToolGood.Words.Pinyin NuGet package
- Included MIT License information for ToolGood.Words.Pinyin
- Updated NOTICE.md to reflect the new dependency

Signed-off-by: 舰队的偶像-岛风酱! <frg2089@outlook.com>
…sions.Toolkit/StringMatcher.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Upgraded ToolGood.Words.Pinyin package from version 3.1.0.2 to 3.1.0.3

Signed-off-by: 舰队的偶像-岛风酱! <frg2089@outlook.com>
- Add EnableChinesePinYinSupport setting to SettingsModel
- Implement EnableChinesePinYinSupport property in SettingsViewModel
- Create UI toggle for EnableChinesePinYinSupport in GeneralPage.xaml
- Add localization strings for new Chinese PinYin support setting

Signed-off-by: 舰队的偶像-岛风酱! <frg2089@outlook.com>
- Rename "Enable Chinese PinYin Support" to "PinYin Support for Chinese"
- Update description from "Allows searching for Chinese characters using PinYin" to "Search Chinese contents with PinYin"

thanks @sovetskyfish

Signed-off-by: 舰队的偶像-岛风酱! <frg2089@outlook.com>
@frg2089 frg2089 force-pushed the feat/pinyin-next branch from 56696b4 to e6c6bb8 Compare June 13, 2025 00:21

This comment has been minimized.

@moooyo
Copy link
Contributor

moooyo commented Jun 13, 2025

Could you please fix spellcheck issue? see: https://github.com/microsoft/PowerToys/blob/main/.github/actions/spell-check/expect.txt

Or grant the permission to me and I can help to fix this issue.

- Add 'toolgood' to the list of expected words
- Maintain existing entries and order

Signed-off-by: 舰队的偶像-岛风酱! <frg2089@outlook.com>
@moooyo
Copy link
Contributor

moooyo commented Jun 14, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@moooyo
Copy link
Contributor

moooyo commented Jun 16, 2025

Pipeline failed.

Could you please add the pinyin package name to Notice.md?
see 7d257cf#diff-53337bf7ea91d782c573ac8653c18f95b7b23c38da9de08288639bcec9993406R1500 as example.

MissingFromNotice:

  • ToolGood.Words.Pinyin 3.1.0.3
    @frg2089

@moooyo
Copy link
Contributor

moooyo commented Jun 18, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@moooyo
Copy link
Contributor

moooyo commented Jun 18, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@htcfreek
Copy link
Collaborator

@moooyo
Is this up to date with main? If not merging main fixes ci.

@moooyo
Copy link
Contributor

moooyo commented Jun 18, 2025

@moooyo Is this up to date with main? If not merging main fixes ci.

ci passed

@moooyo
Copy link
Contributor

moooyo commented Jun 18, 2025

Oh...I missed.

Sure, it's hard to use settings in toolkit. We need to find a way. Maybe add a config parameter is a good idea?

@moooyo
Copy link
Contributor

moooyo commented Jun 18, 2025

Oh...I missed.

Sure, it's hard to use settings in toolkit. We need to find a way. Maybe add a config parameter is a good idea?

We need to design it carefully, because it was used in so many case.

Copy link
Contributor

@moooyo moooyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this PR it self, I like this idea and this implement.

But we still need to find a proper way to use it in StringMatcher.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Triage For issues raised to be triaged and prioritized by internal Microsoft teams Product-Command Palette Refers to the Command Palette utility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants