Skip to content
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

Migrate Shell32 enums to Cswin32 #9909

Merged
merged 7 commits into from
Sep 20, 2023
Merged

Conversation

elachlan
Copy link
Contributor

@elachlan elachlan commented Sep 14, 2023

Migrates shell32 custom implemented enums to use ones generated from cswin32.

Microsoft Reviewers: Open in CodeFlow

@elachlan elachlan requested a review from a team as a code owner September 14, 2023 02:48
@ghost ghost assigned elachlan Sep 14, 2023
@elachlan
Copy link
Contributor Author

CC: @lonitra

@@ -8,6 +8,6 @@ internal static partial class Interop
internal static partial class Shell32
{
[DllImport(Libraries.Shell32, CharSet = CharSet.Unicode, ExactSpelling = true)]
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to change this PInvoke over? The defines should come with that, I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTIFYICONDATAW has different versions for different architectures (x86/x64). I wasn't sure how to support that so I left it alone.

Copy link
Member

Choose a reason for hiding this comment

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

@elachlan Sorry that I missed your response. The right thing to do is move the struct to the same namespace that it would live under CsWin32 and put a reference to the active CsWin32 issue. microsoft/CsWin32#882

Copy link
Member

Choose a reason for hiding this comment

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

Same for the PInvoke itself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@elachlan
Copy link
Contributor Author

@JeremyKuhne anything else I need to address?

Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@JeremyKuhne JeremyKuhne merged commit c19d76f into dotnet:main Sep 20, 2023
@elachlan elachlan deleted the cswin32-shell32 branch September 20, 2023 19:40
@ghost ghost locked as resolved and limited conversation to collaborators Oct 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants