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.

Sign up for GitHub

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

Jump to bottom

Migrate TaskDialogIndirect to CsWin32 #9900

Merged
merged 12 commits into from
Nov 28, 2023
Merged

Migrate TaskDialogIndirect to CsWin32 #9900

merged 12 commits into from
Nov 28, 2023

Conversation

elachlan
Copy link
Contributor

@elachlan elachlan commented Sep 13, 2023 •
edited
Loading

Related: #9825

We are waiting on some Win32MetaData changes to bubble into CsWin32 before I can complete this.

Currently blocked by microsoft/CsWin32#1020 which added BOOL.ToString clashing with our implementation but returning the hex value as a string instead of True/False.

Microsoft Reviewers: Open in CodeFlow

@ghost ghost assigned elachlan Sep 13, 2023
@ghost ghost added the draft draft PR label Sep 13, 2023
@elachlan
Copy link
Contributor Author

@JeremyKuhne any suggestions on the BOOL.ToString() clash? Should we rename our implementation, or should we fix it upstream in CsWin32?

@JeremyKuhne
Copy link
Member

@elachlan I'd just remove ours and have everything do ((bool)value).ToString().

@elachlan
Copy link
Contributor Author

elachlan commented Oct 9, 2023

@AArnott could you please bump the CsWin32 Win32Metadata SDK version?

@AArnott
Copy link
Contributor

AArnott commented Oct 9, 2023

@elachlan I have a local change where I started the work to update the metadata, but some breaking changes in the metadata made it more challenging than usual, so I set it aside. I'll pick it back up and see what I can do.

@AArnott
Copy link
Contributor

AArnott commented Oct 9, 2023

Metadata update coming: microsoft/CsWin32#1063

@elachlan
Copy link
Contributor Author

elachlan commented Oct 9, 2023

Thank you!

@elachlan elachlan force-pushed the CsWin32-TaskDialogIndirect branch from af12fdd to 6adaebc Compare October 23, 2023 23:35
@elachlan
Copy link
Contributor Author

@lonitra can you please have a look?

@elachlan elachlan marked this pull request as ready for review October 24, 2023 00:04
@elachlan elachlan requested a review from a team as a code owner October 24, 2023 00:04
@lonitra
Copy link
Member

lonitra commented Oct 24, 2023

I will take a look this week

@lonitra lonitra removed the draft draft PR label Oct 24, 2023
lonitra
lonitra reviewed Oct 24, 2023
View reviewed changes
Copy link
Member

@lonitra lonitra left a comment

Choose a reason for hiding this comment

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

Great work here! My main concern here is making the .Anonymous more readable

....Windows.Forms.Primitives/tests/UnitTests/Interop/ComCtl32/TASKDIALOGCONFIGIconUnionTests.cs Outdated Show resolved Hide resolved
src/System.Windows.Forms/src/System/Windows/Forms/TaskDialog.cs Outdated Show resolved Hide resolved
src/System.Windows.Forms/src/System/Windows/Forms/TaskDialog.cs Outdated
IntPtr wParam,
IntPtr lParam,
IntPtr lpRefData) =>
uint msg,
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we didn't keep this as TASKDIALOG_NOTIFICATIONS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To match the pfcallback signature of the TASKDIALOGCONFIG struct. Which changed when we switched to using the cswin32 generated struct. See line 1330.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might make sense to update win32metadata based on the doc.
https://learn.microsoft.com/en-us/windows/win32/api/commctrl/nc-commctrl-pftaskdialogcallback

Copy link
Member

Choose a reason for hiding this comment

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

Based on docs that does seem to make sense. Could you open an issue there and perhaps also add a comment here to update this when that issue is resolved and cswin32 has consumed the fix? I think we'd probably get an error anyways once we update cswin32 version that has that fix, but it doesn't hurt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue raised at: microsoft/win32metadata#1733

I am unsure if it will be accepted. But I think we are now allowing enums to group the constants and then adding an attribute to point to the enum for cswin32 to consume. This is instead of using the enum directly, to avoid issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, we managed to get the issue resolved and now I guess we wait for a cswin32 update?

Copy link
Member

@lonitra lonitra Nov 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Pinged for a cswin32 update to consume latest metadata. Will circle back to this to review changes soon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
Based on the commit history, we need to use v57 and cswin32 is using v56.

Copy link
Member

Choose a reason for hiding this comment

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

Oops 🤦‍♀️ v57 is not released yet so we would need to wait on that.. Once that is released we can follow steps in cswin32 readme to use newer metadata to avoid waiting for cswin32 to consume. Alternatively if we'd rather not wait we can leave as is and create tracking issue to bump and update this code when v57 is released.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tracking issue is good:
#10348

src/System.Windows.Forms.Primitives/src/Windows/Win32/UI/Controls/ToolInfoWrapper.cs Show resolved Hide resolved
src/System.Windows.Forms.Primitives/src/Windows/Win32/UI/Controls/ToolInfoWrapper.cs Outdated Show resolved Hide resolved
@lonitra lonitra added the waiting-author-feedback The team requires more information from the author label Oct 24, 2023
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Oct 25, 2023
@lonitra lonitra added the waiting-author-feedback The team requires more information from the author label Oct 27, 2023
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Oct 31, 2023
@elachlan elachlan force-pushed the CsWin32-TaskDialogIndirect branch from 6b7d44c to ebb5cbc Compare November 14, 2023 04:02
@elachlan elachlan requested a review from lonitra November 14, 2023 08:39
@lonitra lonitra added the waiting-author-feedback The team requires more information from the author label Nov 15, 2023
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Nov 15, 2023
@elachlan elachlan requested a review from lonitra November 16, 2023 01:02
@lonitra lonitra added the waiting-author-feedback The team requires more information from the author label Nov 17, 2023
@elachlan elachlan force-pushed the CsWin32-TaskDialogIndirect branch from 51a6d8e to 2627884 Compare November 21, 2023 00:59
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Nov 21, 2023
@elachlan elachlan mentioned this pull request Nov 21, 2023
Open
lonitra
lonitra previously approved these changes Nov 21, 2023
View reviewed changes
Copy link
Member

@lonitra lonitra left a comment

Choose a reason for hiding this comment

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

👍

@lonitra lonitra added the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Nov 21, 2023
Tanya-Solyanik
Tanya-Solyanik reviewed Nov 22, 2023
View reviewed changes
Copy link
Member

@Tanya-Solyanik Tanya-Solyanik left a comment

Choose a reason for hiding this comment

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

@elachlan - seem like https://github.com/dotnet/winforms/pull/9900/files#r1374857146 is not addressed, are we waiting on something to implement it?

src/System.Windows.Forms.Primitives/src/Windows/Win32/UI/Controls/ToolInfoWrapper.cs Outdated Show resolved Hide resolved
src/System.Windows.Forms/src/System/Windows/Forms/Dialogs/TaskDialog/TaskDialogButton.cs Outdated Show resolved Hide resolved
@Tanya-Solyanik Tanya-Solyanik removed the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Nov 22, 2023
Tanya-Solyanik
Tanya-Solyanik previously approved these changes Nov 28, 2023
View reviewed changes
@Tanya-Solyanik Tanya-Solyanik added the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Nov 28, 2023
@ghost ghost added the waiting-author-feedback The team requires more information from the author label Nov 28, 2023
@lonitra lonitra removed the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Nov 28, 2023
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Nov 28, 2023
@lonitra lonitra enabled auto-merge (squash) November 28, 2023 22:02
@lonitra lonitra merged commit 85c155e into dotnet:main Nov 28, 2023
8 of 9 checks passed
@ghost ghost added this to the 9.0 Preview1 milestone Nov 28, 2023
@elachlan elachlan deleted the CsWin32-TaskDialogIndirect branch November 28, 2023 23:03
@elachlan
Copy link
Contributor Author

Thank you for pushing through with the reviews!

@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lonitra lonitra lonitra approved these changes

@Tanya-Solyanik Tanya-Solyanik Tanya-Solyanik left review comments

Assignees

@elachlan elachlan

Labels
None yet
Projects
None yet
Milestone
9.0 Preview1
Development

Successfully merging this pull request may close these issues.
5 participants
@elachlan @JeremyKuhne @AArnott @lonitra @Tanya-Solyanik