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

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

Closed
AraHaan opened this issue Mar 23, 2021 · 22 comments · Fixed by #4746
Closed

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

AraHaan opened this issue Mar 23, 2021 · 22 comments · Fixed by #4746
Assignees
Labels
api-approved (4) API was approved in API review, it can be implemented tenet-compatibility-OS Compatibility with OS features

Comments

@AraHaan
Copy link
Member

AraHaan commented Mar 23, 2021

Is your feature request related to a problem? Please describe.

Nope, I am filing this because I noticed that the documentations here: https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-messagebox
As such I noticed that DialogResults of 10, and 11 are possible.

Describe the solution you'd like and alternatives you've considered

Currently I have no other alternatives in mind other than adding the 2 members to DialogResult.
Also might need to add MB_CANCELTRYCONTINUE to the button options too for Cancel, Try Again, and Continue buttons (I can file a separate issue for this one).
It also seems to miss on the default buttons to also be able to make button 4 the default if they wanted to as well (I can file a separate issue for this one as well). Nowever I would logically only make it available in .NET 6+ if approved.

Will this feature affect UI controls?

I do not think it will affect them at all, it will instead allow one to handle if one pressed the continue button on a dialog or try again to do specific things for those specific results which could be seen as an enhancement.

API Changes:

namespace System.Windows.Forms
{
    public enum DialogResult
    {
-        OK = 1,
+        OK = (int)ID.OK,
        // docs stay the same.
-        Cancel = 2,
+        Cancel = (int)ID.CANCEL,
        // docs stay the same.
-        Abort = 3,
+        Abort = (int)ID.ABORT,
        // docs stay the same.
-        Retry = 4,
+        Retry = (int)ID.RETRY,
        // docs stay the same.
-        Ignore = 5,
+        Ignore = (int)ID.IGNORE,
        // docs stay the same.
-        Yes = 6,
+        Yes = (int)ID.YES,
        // docs stay the same.
-        No = 7,
+        No = (int)ID.NO,
+
+        /// <summary>
+        /// The dialog box return value is Try Again (usually sent from a button labeled Try Again).
+        /// </summary>
+        TryAgain = (int)ID.TRYAGAIN,
+
+        /// <summary>
+        /// The dialog box return value is Continue (usually sent from a button labeled Continue).
+        /// </summary>
+        Continue = (int)ID.CONTINUE,
    }
    public enum MessageBoxButtons
    {
+        /// <summary>
+        ///  Specifies that the message box contains Cancel, Try Again, and Continue buttons.
+        ///  This field is constant.
+        /// </summary>
+        CancelTryContinue = (int)MB.CANCELTRYCONTINUE,
    }
    public enum MessageBoxDefaultButton
    {
+        /// <summary>
+        ///  Specifies that the forth button on the message box should be the
+        ///  default button.
+        /// </summary>
+        Button4 = (int)MB.DEFBUTTON4,
    }
}

As for MessageBox class this code here:

DialogResult result;
try
{
result = Win32ToDialogResult(MessageBoxW(new HandleRef(owner, handle), text, caption, style));
}
finally
{
Application.EndModalMessageLoop();
ThemingScope.Deactivate(userCookie);
}
// Right after the dialog box is closed, Windows sends WM_SETFOCUS back to the previously active control
// but since we have disabled this thread main window the message is lost. So we have to send it again after
// we enable the main window.
User32.SendMessageW(new HandleRef(owner, handle), User32.WM.SETFOCUS);

can be replaced with:

      try
      {
          ID mbresult = MessageBoxW(new HandleRef(owner, handle), text, caption, style);
+         return mbresult switch
+         {
+             _ => (DialogResult)mbresult,
+         };
      }
      finally
      {
          Application.EndModalMessageLoop();
          ThemingScope.Deactivate(userCookie);
          // Right after the dialog box is closed, Windows sends WM_SETFOCUS back to the previously active control
          // but since we have disabled this thread main window the message is lost. So we have to send it again after
          // we enable the main window.
          User32.SendMessageW(new HandleRef(owner, handle), User32.WM.SETFOCUS);
      }

And this could be attempted on removal:

private static DialogResult Win32ToDialogResult(ID value)
{
switch (value)
{
case ID.OK:
return DialogResult.OK;
case ID.CANCEL:
return DialogResult.Cancel;
case ID.ABORT:
return DialogResult.Abort;
case ID.RETRY:
return DialogResult.Retry;
case ID.IGNORE:
return DialogResult.Ignore;
case ID.YES:
return DialogResult.Yes;
case ID.NO:
return DialogResult.No;
default:
return DialogResult.No;
}
}

Edit: Shown complete changes to the enums, The changes to the MessageBox class that would be needed for this is a WIP.

@AraHaan AraHaan added the api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation label Mar 23, 2021
@RussKie RussKie added the tenet-compatibility-OS Compatibility with OS features label Mar 23, 2021
@RussKie

This comment has been minimized.

@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Mar 23, 2021
@AraHaan
Copy link
Member Author

AraHaan commented Mar 23, 2021

Alright made changes, I hope you guys do not mind if I later use var on top of already using switch expressions with or patterns to simplify it's readability.

As for the rest I think it should be complete, let me know about ID.CLOSE => buttons is MessageBoxButtons.CancelTryContinue or MessageBoxButtons.OKCancel or MessageBoxButtons.RetryCancel or MessageBoxButtons.YesNoCancel ? DialogResult.Cancel : DialogResult.No, I thought it was a good idea to make it return DialogResult.Cancel whenever the buttons are told to contain a cancel button, DialogResult.No otherwise.

I guess that part could need documentation change so the user knows it as well too. I was thinking it's more logical to use DialogResult.Cancel when close button is pressed as technically I would consider it the same as pressing the cancel button, However I could change it to always use DialogResult.Cancel for close.

@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Mar 23, 2021
@RussKie RussKie added the waiting-review This item is waiting on review by one or more members of team label Mar 24, 2021
@dreddy-work dreddy-work added this to the 6.0 milestone Mar 25, 2021
@RussKie

This comment has been minimized.

@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 Mar 30, 2021
@AraHaan

This comment has been minimized.

@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Mar 30, 2021
@RussKie RussKie added api-ready-for-review (2) API is ready for formal API review; applied by the issue owner and removed api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation labels Mar 31, 2021
@RussKie
Copy link
Member

RussKie commented Mar 31, 2021

Thank you. I'll get the ball rolling to get an approval. In mean time if you can - open a draft PR with the change, it would aid the process.

@AraHaan
Copy link
Member Author

AraHaan commented Mar 31, 2021

@RussKie DialogResult.None does not seem to be used by the MessageBox class at all, is anything using that member of DialogResult or is it just a dummy member that goes unused entirely just for backwards compatibility with old code that used it?

@AraHaan
Copy link
Member Author

AraHaan commented Mar 31, 2021

opened #4746.

@ghost ghost added the 🚧 work in progress Work that is current in progress label Mar 31, 2021
@AraHaan
Copy link
Member Author

AraHaan commented Mar 31, 2021

It does not seem there are any tests for MessageBox at all except for a call I see in the TaskDialog test.

@weltkante
Copy link
Contributor

weltkante commented Mar 31, 2021

DialogResult.None does not seem to be used by the MessageBox class at all

Its used for modal forms as default value while the form is prepared/open, once you assign a value different from None it will autoclose with that result. (You can also use it for non-modal forms to communicate a result, but it won't have the autoclose behavior.)

@terrajobst terrajobst added blocking Used by the API Review Board api-approved (4) API was approved in API review, it can be implemented and removed api-ready-for-review (2) API is ready for formal API review; applied by the issue owner blocking Used by the API Review Board labels Apr 1, 2021
@terrajobst
Copy link
Member

  • Looks good as proposed
namespace System.Windows.Forms
{
    public partial enum DialogResult
    {
        // ...
        TryAgain,
        Continue
    }
    public partial enum MessageBoxButtons
    {
        // ...
        CancelTryContinue
    }
    public partial enum MessageBoxDefaultButton
    {
        // ...
        Button4
    }
}

@AraHaan
Copy link
Member Author

AraHaan commented Apr 1, 2021

@terrajobst so do I convert the pr from draft now for merging or does it still have some steps left that needs done before merge?
(Well other than the fact that MessageBox really does need a file for testing MessageBox specific calls and the different options people can provide into it).

@RussKie
Copy link
Member

RussKie commented Apr 2, 2021

The API proposal has been approved, and you now have a green light to make your PR non-draft :)

@AraHaan
Copy link
Member Author

AraHaan commented Apr 2, 2021

ok, converted it to a normal PR.

@AraHaan
Copy link
Member Author

AraHaan commented Apr 14, 2021

@terrajobst after some testing it seems setting the default button to Button4 crashes the application when trying to display a messagebox according to what @RussKie tested.

Not sure if it's just an issue with Windows however and that the crash might need to be fixed or not, it might have been an accidentally exposed field sdk headers but was never implemented.

As such the proposal might need to be edited to remove Button4, unless there is a reason why it somehow crashes applications that set it.

@terrajobst
Copy link
Member

That's odd. Seems hard to believe that we found a bug in the SDK though. @RussKie, do we have a repro?

@RussKie
Copy link
Member

RussKie commented Apr 15, 2021

@terrajobst no, false alarm. More details here #4746 (comment)

@AraHaan
Copy link
Member Author

AraHaan commented Apr 15, 2021

Ah ok, I thought it was a windows bug causing it to crash, good to know now what the 4th button was for.

@ghost ghost removed this from the 6.0 milestone Jun 14, 2021
@ghost ghost removed the 🚧 work in progress Work that is current in progress label Jun 14, 2021
@Zheng-Li01
Copy link
Member

@AraHaan @RussKie, verified the issue with .NET 6.0 6.0.100-preview.6.21322.9, the new function has been implemented for now, just a minor issue need to confirm that no similar Summary for new added buttons. see below screenshot.
image

@AraHaan
Copy link
Member Author

AraHaan commented Jun 23, 2021

How is that possible?

@RussKie
Copy link
Member

RussKie commented Jun 23, 2021

This means our intellisence bundles haven't been updated yet.

@AraHaan
Copy link
Member Author

AraHaan commented Jun 23, 2021

This means our intellisence bundles haven't been updated yet.

wait there is a separate bundle?

@RussKie
Copy link
Member

RussKie commented Jun 23, 2021

Yes, xml-docs flow through a number of other systems and teams before becoming intellisence and docs. :)

@dotnet dotnet 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
api-approved (4) API was approved in API review, it can be implemented tenet-compatibility-OS Compatibility with OS features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants