8000 Adding more MessageBox buttons and result values by bstordrup · Pull Request #10720 · dotnet/wpf · GitHub
[go: up one dir, main page]
Skip to content

Adding more MessageBox buttons and result values #10720

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
Merged
merged 16 commits into from
Apr 17, 2025
Merged

Adding more MessageBox buttons and result values #10720

merged 16 commits into from
Apr 17, 2025

Conversation

bstordrup
Copy link
Contributor
@bstordrup bstordrup commented Apr 7, 2025 •
edited by dotnet-policy-service bot
Loading

Fixes #9613

Description

The purpose with the pull request is to streamline the MessageBox.Show overloads between WinForms and WPF thus making it more intuitive and easier to migrate a WinForms applicatio to WPF.
Also, since (on Windows), both WinForms and WPF call into the same underlying Windows API functions, there is in my opinion only little reason not to streamline them.

Customer Impact

The customer impact of not taking this fix is that a migration from WinForms to WPF becomes a bit more complex when it comes to using MessageBox.Show with more options than just Yes/No or OK/Cancel. With the extension of both MessageBoxButton and MessageBoxResult enumerations, a more generous set of options is available for the application after showing MessageBox.Show (...) method.

Regression

This is no regression.

Testing

I tested the extensions of MessageBoxButton and MessageBoxResult enumerations with a sample Wpf application targeting a locally build version of Wpf. It simply had a set of Button and TextBlock controls in MainWindow, and each of these buttons uses the MessageBoxButton enumeration value as indicated by the button title. The result of MessageBox.Show is then assigned to the corresponding TextBlock control.

Risk

The risk of taking this fix is minimal (if not even none). The WinForms repository already contains these extensions, and they are building upon an existing Windows API.

Microsoft Reviewers: Open in CodeFlow

@Copilot Copilot AI review requested due to automatic review settings April 7, 2025 14:17
@bstordrup bstordrup requested review from a team as code owners April 7, 2025 14:17
Copilot
Copilot AI reviewed Apr 7, 2025
View reviewed changes
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.

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

@dotnet-policy-service dotnet-policy-service bot added PR metadata: Label to tag PRs, to facilitate with triage Community Contribution A label for all community Contributions labels Apr 7, 2025
@miloush
Copy link
Contributor
miloush commented Apr 7, 2025 •
edited
Loading

Why is there so much whitespace changes involved? This should be trivial to review but these changes make it unnecessarily complicated.

@bstordrup
Copy link
Contributor Author

Why is there so much whitespace changes involved? This should be trivial to review but these changes make it unnecessarily complicated.

It might be some settings in Visual Studio that causes this. I will see if I can revert the white space changes.

@miloush
Copy link
Contributor
miloush commented Apr 7, 2025

Or at least have them in a separate commit if intentional. I cannot easily tell whether src/Microsoft.DotNet.Wpf/cycle-breakers/PresentationFramework/PresentationFramework.cs contains any changes or not.

@codecov Codecov
Copy link
codecov bot commented Apr 7, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 76.31579% with 18 lines in your changes missing coverage. Please review.

Project coverage is 13.16493%. Comparing base (b878966) to head (ca0bcd5).
Report is 25 commits behind head on main.

Additional details and impacted files 
@@                 Coverage Diff                 @@
##                main      #10720         +/-   ##
===================================================
+ Coverage   11.23215%   13.16493%   +1.93277%     
===================================================
  Files           3314        3317          +3     
  Lines         665180      665389        +209     
  Branches       74668       74672          +4     
===================================================
+ Hits           74714       87598      +12884     
+ Misses        589167      575312      -13855     
- Partials        1299        2479       +1180
Flag Coverage Δ
Debug 13.16493% <76.31579%> (+1.93277%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@h3xds1nz
Copy link
Member
h3xds1nz commented Apr 7, 2025

Learning WPF project development by pressing auto format the hard way. 😀

@bstordrup
Copy link
Contributor Author

Or at least have them in a separate commit if intentional. I cannot easily tell whether src/Microsoft.DotNet.Wpf/cycle-breakers/PresentationFramework/PresentationFramework.cs contains any changes or not.

I fixed many of the white space changes, but it seems I have a setting that trims white spaces on line endings. That account for most of the changes remaining.

@bstordrup
Copy link
Contributor Author

@miloush, @h3xds1nz, the only white space changes left now are white spaces trimmed from line ends (except for removing a double space in original code).

Is there more for me to do? There is a comment from the Code-Cov, but not sure how much can be actually tested in this.

@h3xds1nz
Copy link
Member
h3xds1nz commented Apr 7, 2025

@bstordrup Having tests would be a nice addition indeed!

As for the whitespaces, yeah funny enough there's always a whitespace before the \r\n, removing it is what the final state is supposed to be anyways, I guess that's fine at this state (for me), the whitespaces there would be removed sooner or later via format tool anyways.

8000
@bstordrup bstordrup requested review from h3xds1nz and Copilot April 8, 2025 09:27
Copilot
Copilot AI reviewed Apr 8, 2025
View reviewed changes
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.

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • src/Microsoft.DotNet.Wpf/tests/UnitTests/PresentationFramework.Tests/PresentationFramework.Tests.csproj: Language not supported
Comments suppressed due to low confidence (1)

src/Microsoft.DotNet.Wpf/cycle-breakers/PresentationFramework/PresentationFramework.cs:982

  • Verify that the enum ordering and assigned values in the cycle-breakers file are consistent with the primary definitions to avoid potential mapping inconsistencies.
AbortRetryIgnore = 2,

@bstordrup
Copy link
Contributor Author

@h3xds1nz, Added tests for exceptional paths of MessageBox.ShowCore method. But please note the comment on the commented out test at the bottom of the MessageBox.Tests.cs file.

bstordrup added 2 commits April 8, 2025 16:04
@bstordrup
Reorganized tests
34cf438 
Using WpfTheory and MemberData for similar tests
@bstordrup
Changed the problematic test after advice.
e91ad50
@bstordrup bstordrup requested review from h3xds1nz and Copilot April 8, 2025 14:23
Copilot
Copilot AI reviewed Apr 8, 2025
View reviewed changes
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.

Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • src/Microsoft.DotNet.Wpf/tests/UnitTests/PresentationFramework.Tests/PresentationFramework.Tests.csproj: Language not supported
Comments suppressed due to low confidence (2)

src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/MessageBox.cs:89

  • The default case in Win32ToMessageBoxResult returns MessageBoxResult.No. Consider returning MessageBoxResult.None to clearly indicate an unmapped or unexpected Win32 result.
default:

src/Microsoft.DotNet.Wpf/cycle-breakers/PresentationFramework/PresentationFramework.cs:1013

  • [nitpick] The ordering of MessageBoxResult enum members differs from the primary reference file. For improved readability and maintainability, consider ordering these members consistently with the main implementation.
Cancel = 2,

9E88
@bstordrup bstordrup requested review from h3xds1nz and Copilot April 10, 2025 07:14
Copilot
Copilot AI reviewed Apr 10, 2025
View reviewed changes
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.

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • src/Microsoft.DotNet.Wpf/tests/UnitTests/PresentationFramework.Tests/PresentationFramework.Tests.csproj: Language not supported

@bstordrup bstordrup requested a review from h3xds1nz April 11, 2025 12:06
h3xds1nz
h3xds1nz approved these changes Apr 16, 2025
View reviewed changes
Copy link
Member
@h3xds1nz h3xds1nz left a comment

Choose a reason for hiding this comment

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

@bstordrup I got no further comments on this by the way, LGTM.

The whitespace changes are unfortunate but basically correct, it replaces \u0020\r\n sequence with \r\n, there's no reason why there should be a space on each line before CRLF, so imho no reason to painfully revert it.

@siagupta0202 siagupta0202 merged commit 15cc5c1 into dotnet:main Apr 17, 2025
8 checks passed
@siagupta0202
Copy link
Contributor

@bstordrup Thank you for taking up this API proposal, seeing it through to completion, and also contributing the accompanying tests 😊

P.S. @h3xds1nz Thank you for your detailed and thorough review of the proposal!

@siagupta0202 siagupta0202 mentioned this pull request Apr 17, 2025
Closed
@bstordrup
Copy link
Contributor Author

@siagupta0202 my pleasure 😊

@h3xds1nz
Copy link
Member

@siagupta0202 happy to :)

@miloush
Copy link
Contributor
miloush commented May 14, 2025 •
edited
Loading

Who is in charge of XML documentation for these?

@dipeshmsft
Copy link
Member

@miloush, we are waiting of this PR now : dotnet/dotnet-api-docs#11295 (review)

Once this is done, I will make the updates.

@dipeshmsft dipeshmsft mentioned this pull request May 23, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@h3xds1nz h3xds1nz h3xds1nz approved these changes

@siagupta0202 siagupta0202 siagupta0202 approved these changes

@dipeshmsft dipeshmsft Awaiting requested review from dipeshmsft

@singhashish-wpf singhashish-wpf Awaiting requested review from singhashish-wpf

@Kuldeep-MS Kuldeep-MS Awaiting requested review from Kuldeep-MS

Assignees

@bstordrup bstordrup

@siagupta0202 siagupta0202

Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Make additional MessageBoxButton and MessagBoxResult values available
5 participants
@bstordrup @miloush @h3xds1nz @siagupta0202 @dipeshmsft
0