Skip to content

[DialogResult] Add TryAgain and Continue (result 10, and 11) respectively.#4746

Merged
RussKie merged 13 commits intodotnet:mainfrom
AraHaan:patch-1
Jun 14, 2021
Merged

[DialogResult] Add TryAgain and Continue (result 10, and 11) respectively.#4746
RussKie merged 13 commits intodotnet:mainfrom
AraHaan:patch-1

Conversation

@AraHaan
Copy link
Member

@AraHaan AraHaan commented Mar 31, 2021

Resolves #4712.

Proposed changes

  • Adding TryAgain and Continue to DiagloResult.
  • Adding CancelTryContinue to MessageBoxButtons.
  • Adding Button4 to MessageBoxDefaultButton.
  • Simplifying some code to ShowCore in MessageBox class to remove the need for the Win32ToDialogResult method.

Customer Impact

  • Allows users to show MessageBox's with the Cancel, Try again, and continue buttons.
  • Allows them to be able to check if TryAgain or Continue was pressed specifically on a MessageBox.

Regression?

  • Yes / No
    No

Risk

  • With this, the current plan is for .NET 6 and above to get this new API, because of that the risk should be minimal.

Screenshots

Not sure if this is considered a UI change. Left it in case it is.

Before

After

Test methodology

  • Manual Testing with the added tests.

Accessibility testing

Not sure if this is considered a UI change. Left it in case it is.

Test environment(s)

.NET SDK (reflecting any global.json):
 Version:   6.0.100-preview.2.21155.3
 Commit:    1a9103db2d

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.21343
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\6.0.100-preview.2.21155.3\

Host (useful for support):
  Version: 6.0.0-preview.2.21154.6
  Commit:  3eaf1f316b

.NET SDKs installed:
  5.0.200-preview.21077.7 [C:\Program Files\dotnet\sdk]
  6.0.100-preview.2.21155.3 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.All 2.1.23 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.2.8 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.23 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.2.8 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.0.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.1.9 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.0-preview.2.21154.6 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.23 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.2.8 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.0.3 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.1.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.0-preview.2.21154.6 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.0.3 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 3.1.9 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.2 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.0-preview.2.21154.2 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

To install additional .NET runtimes or SDKs:
  https://aka.ms/dotnet-download
Microsoft Reviewers: Open in CodeFlow

@ghost ghost assigned AraHaan Mar 31, 2021
@RussKie RussKie added the 🚫 * NO-MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 31, 2021
@codecov
Copy link

codecov bot commented Mar 31, 2021

Codecov Report

Merging #4746 (0423ba6) into main (129d44a) will decrease coverage by 0.01812%.
The diff coverage is n/a.

❗ Current head 0423ba6 differs from pull request most recent head 296dca1. Consider uploading reports for the commit 296dca1 to get more accurate results

@@                 Coverage Diff                 @@
##                main       #4746         +/-   ##
===================================================
- Coverage   97.97301%   97.95489%   -0.01812%     
===================================================
  Files            543         548          +5     
  Lines         264728      266049       +1321     
  Branches        5007        5112        +105     
===================================================
+ Hits          259362      260608       +1246     
- Misses          4486        4547         +61     
- Partials         880         894         +14     
Flag Coverage Δ
Debug 97.95489% <ø> (-0.01812%) ⬇️
test 97.95489% <ø> (-0.01812%) ⬇️

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

@RussKie RussKie added 📖 documentation: breaking please open a breaking change issue https://github.com/dotnet/docs/issues/new?assignees=gewarren and removed 📖 documentation: breaking please open a breaking change issue https://github.com/dotnet/docs/issues/new?assignees=gewarren labels Mar 31, 2021
@AraHaan AraHaan marked this pull request as ready for review April 2, 2021 02:28
@AraHaan AraHaan requested a review from a team as a code owner April 2, 2021 02:28
@RussKie RussKie removed the 🚫 * NO-MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 5, 2021
@RussKie
Copy link
Contributor

RussKie commented Apr 5, 2021

I don't believe we can add any unit tests here, but you can add a test app that exposes and features the new functionality to WinformsControlsTest project. Perhaps in a way similar to how we test "dialogs" or "task dialog" 🤔

@RussKie RussKie added the waiting-author-feedback The team requires more information from the author label Apr 5, 2021
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Apr 6, 2021
@RussKie RussKie added the waiting-author-feedback The team requires more information from the author label Apr 6, 2021
@RussKie
Copy link
Contributor

RussKie commented Apr 6, 2021

I'm converting this to draft until we get tests.

@RussKie RussKie marked this pull request as draft April 6, 2021 05:43
@AraHaan

This comment has been minimized.

@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Apr 10, 2021
@AraHaan

This comment has been minimized.

@AraHaan AraHaan marked this pull request as ready for review April 11, 2021 01:20
@AraHaan

This comment has been minimized.

@RussKie RussKie added the waiting-review This item is waiting on review by one or more members of team label Apr 12, 2021
@RussKie
Copy link
Contributor

RussKie commented Apr 14, 2021

I pushed an alternative version to how we can test the MessageBox functionality in a more robust way. It isn't exhaustive, but should give you an idea.

With that I discovered what looks like a bug in EnumConverter an by-design yet unexpected behaviour, so ignore it:
image

I also struggle to see how MessageBoxDefaultButton.Button4 can be used, as there doesn't seem a way to set 4 buttons.

@RussKie RussKie added waiting-author-feedback The team requires more information from the author and removed waiting-review This item is waiting on review by one or more members of team labels Apr 14, 2021
@AraHaan

This comment has been minimized.

@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Apr 14, 2021
@RussKie
Copy link
Contributor

RussKie commented Apr 14, 2021

No, it doesn't look so. If you specify only 3 buttons but set the default button to 4, the app will crash.

@RussKie RussKie added the waiting-author-feedback The team requires more information from the author label Apr 14, 2021
@AraHaan

This comment has been minimized.

@AraHaan
Copy link
Member Author

AraHaan commented Jun 12, 2021

I rebased this on top of the current tip of main and removed the code parts that are in #4809 from this PR.

@AraHaan
Copy link
Member Author

AraHaan commented Jun 14, 2021

@RussKie think the blocking label can be removed now?

@RussKie RussKie removed the ⛔ blocked Blocked by external dependencies label Jun 14, 2021
@RussKie
Copy link
Contributor

RussKie commented Jun 14, 2021

Yep :)

@AraHaan AraHaan marked this pull request as ready for review June 14, 2021 21:14
@AraHaan
Copy link
Member Author

AraHaan commented Jun 14, 2021

Sweet this looks 👌 to merge with all checks passing.

@RussKie RussKie merged commit e59784d into dotnet:main Jun 14, 2021
@ghost ghost added this to the 6.0 Preview6 milestone Jun 14, 2021
@RussKie
Copy link
Contributor

RussKie commented Jun 14, 2021

Thank you

@AraHaan AraHaan deleted the patch-1 branch June 14, 2021 21:53
@ghost ghost locked as resolved and limited conversation to collaborators Jan 28, 2022
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.

[DialogResult] Add TryAgain and Continue (result 10, and 11) respectively.

2 participants