-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[AOT] move some LibraryImport define to common to avoid dup code #40006
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
Conversation
There was a problem hiding this 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 consolidates LibraryImport definitions into a common location and updates native interop calls across multiple modules to use the new common implementations.
- Removed duplicated native helper code from the Indexer project.
- Added project references to ManagedCsWin32 and updated interop calls accordingly.
- Refactored COM instantiation parameters and shell execution calls to align with the common libraries.
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Indexer/Native/NativeHelpers.cs | Removed duplicate native constants and helper definitions. |
src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Indexer/Microsoft.CmdPal.Ext.Indexer.csproj | Added ManagedCsWin32 project reference. |
src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Indexer/Indexer/Utils/QueryStringBuilder.cs | Updated COM instantiation call to use ManagedCsWin32 definitions and CLSCTX.ALL. |
src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Indexer/Indexer/Utils/NativeHelpers.cs | Introduced new common native helpers. |
src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Indexer/Indexer/SystemSearch/IGetRow.cs | Removed dependency on old native definitions. |
src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Indexer/Indexer/SearchQuery.cs | Updated COM instantiation and removed obsolete native usages. |
Other files (such as OpenWithCommand.cs, OpenPropertiesCommand.cs, ReparsePoint.cs, etc.) | Updated interop calls to reference ManagedCsWin32 instead of duplicated native implementations. |
PowerToys.sln | Added ManagedCsWin32 project to the solution and updated configuration mappings. |
Comments suppressed due to low confidence (1)
src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Indexer/Indexer/Utils/QueryStringBuilder.cs:35
- The change from a specific CLSCTX value (NativeHelpers.CLSCTXINPROCALL) to CLSCTX.ALL might alter COM instantiation behavior. Please verify that using the broader context 'CLSCTX.ALL' is intentional and has been validated.
var hr = Ole32.CoCreateInstance(ref Unsafe.AsRef(in CLSID.SearchManager), IntPtr.Zero, (uint)CLSCTX.ALL, ref Unsafe.AsRef(in IID.ISearchManager), out searchManagerPtr);
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See if need to discuss with my suggestion.
src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Indexer/Indexer/DataSourceManager.cs
Outdated
Show resolved
Hide resolved
src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Indexer/Indexer/Utils/NativeHelpers.cs
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Indexer/Indexer/Utils/QueryStringBuilder.cs
Outdated
Show resolved
Hide resolved
) <!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## Summary of the Pull Request To avoid dup code, try to move some LibraryImport define to common folder. This can make other modules/extensions easy to use. <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist - [x] **Closes:** #xxx - [x] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [x] **Tests:** Added/updated and all pass - [ ] **Localization:** All end-user-facing strings can be localized - [ ] **Dev docs:** Added/updated - [ ] **New binaries:** Added on the required places - [ ] [JSON for signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json) for new binaries - [ ] [WXS for installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs) for new binaries and localization folder - [ ] [YML for CI pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml) for new test projects - [ ] [YML for signed pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml) - [ ] **Documentation updated:** If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys) and link it here: #xxx <!-- Provide a more detailed description of the PR, other things fixed, or any additional comments/features here --> ## Detailed Description of the Pull Request / Additional comments <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## Validation Steps Performed --------- Co-authored-by: Yu Leng <yuleng@microsoft.com>
Summary of the Pull Request
To avoid dup code, try to move some LibraryImport define to common folder.
This can make other modules/extensions easy to use.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed