Skip to content

[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

Merged
merged 9 commits into from
Jun 19, 2025

Conversation

moooyo
Copy link
Contributor

@moooyo moooyo commented Jun 12, 2025

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

  • Closes: #xxx
  • Communication: I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected
  • 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
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Yu Leng added 2 commits June 12, 2025 14:16
@moooyo moooyo marked this pull request as ready for review June 12, 2025 06:57
@moooyo moooyo requested a review from Copilot June 12, 2025 06:57
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 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.

@yeelam-gordon yeelam-gordon added the Product-Command Palette Refers to the Command Palette utility label Jun 12, 2025
Copy link
Contributor

@yeelam-gordon yeelam-gordon left a 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.

@yeelam-gordon yeelam-gordon requested a review from lei9444 June 13, 2025 02:26

This comment has been minimized.

This comment has been minimized.

@moooyo moooyo added this to the PowerToys 0.92 milestone Jun 19, 2025
@moooyo moooyo merged commit 1020c5a into microsoft:main Jun 19, 2025
9 checks passed
@moooyo moooyo deleted the yuleng/aot/common branch June 19, 2025 05:45
yeelam-gordon pushed a commit that referenced this pull request Jun 20, 2025
)

<!-- 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product-Command Palette Refers to the Command Palette utility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants