microsoft / microsoft-ui-xaml

WinUI: a modern UI framework with a rich set of controls and styles to build dynamic and high-performing Windows applications.
MIT License
6.37k stars 683 forks source link

[Preview 4] MessageDialog is no longer functional #4167

Open dotMorten opened 3 years ago

dotMorten commented 3 years ago

Describe the bug Showing a message dialog no longer works in a Win32 app using Preview 4

Steps to reproduce the bug Run the following code, for instance in a button click event:

            var dlg = new MessageDialog("Test");
            try
            {
                await dlg.ShowAsync();
            }
            catch (System.Exception ex)
            {
                Debug.WriteLine(ex.Message);
            }

See the following exception thrown in the catch:

Invalid window handle. (0x80070578)

Expected behavior Dialog displayed

Version Info

NuGet package version: [Microsoft.WinUI 3.0.0-preview4.210210.4]

Windows app type: UWP Win32
Yes
Windows 10 version Saw the problem?
Insider Build (xxxxx)
October 2020 Update (19042)
May 2020 Update (19041)
November 2019 Update (18363)
May 2019 Update (18362)
October 2018 Update (17763)
April 2018 Update (17134)
Fall Creators Update (16299)
Creators Update (15063)
Device form factor Saw the problem?
Desktop
Xbox
Surface Hub
IoT

Additional context Workaround:

        [ComImport]
        [InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
        [Guid("3E68D4BD-7135-4D10-8018-9FB6D9F33FA1")]
        internal interface IInitializeWithWindow
        {
            void Initialize(IntPtr hwnd);
        }

        [ComImport]
        [InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
        [Guid("EECDBF0E-BAE9-4CB6-A68E-9598E1CB57BB")]
        internal interface IWindowNative
        {
            IntPtr WindowHandle { get; }
        }

// ...
            var dlg = new MessageDialog("Test");
            IInitializeWithWindow withWindow = dlg.As<IInitializeWithWindow>();
            var handle = this.As<IWindowNative>().WindowHandle;
            try
            {
                withWindow.Initialize(handle);
                await dlg.ShowAsync();
            }
            catch (System.Exception ex)
            {
                Debug.WriteLine(ex.Message);
            }

Alternative workaround without requiring knowledge of the current window:

        public static IAsyncOperation<IUICommand> ShowDialogAsync(string content, string title = null)
        {
            var dlg = new MessageDialog(content, title ?? "");
            var handle = GetActiveWindow();
            if (handle == IntPtr.Zero)
                throw new InvalidOperationException();
            dlg.As<IInitializeWithWindow>().Initialize(handle);
            return dlg.ShowAsync();
        }

        [ComImport]
        [InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
        [Guid("3E68D4BD-7135-4D10-8018-9FB6D9F33FA1")]
        internal interface IInitializeWithWindow
        {
            void Initialize(IntPtr hwnd);
        }

        [DllImport("user32.dll")]
        private static extern IntPtr GetActiveWindow();
dotMorten commented 3 years ago

It should be added that ContentDialog exhibit similar brokenness, however the above workaround doesn't work, as it couldn't be cast to IInitializeWithWindow.

huoyaoyuan commented 3 years ago

FYI: The IInitializeWithWindow workaround still works for FolderPicker.

nickrandolph commented 3 years ago

Was this supposed to have been fixed? or is this still a known issue in ProjectReunion 0.8.1?

dotMorten commented 3 years ago

@nickrandolph I hit it too the other day with 0.8.1. It's overly complicated to do a quick message box to a user. You can't use the simple message dialog, so you have to use the content dialog, and also remember to set the XamlRoot.

Example: https://github.com/dotMorten/UnifiClient/blob/9be1a1ebf48ce207734170da32a1f700316ffd5b/src/UnifiClientApp/UnifiClientApp/MainWindow.xaml.cs#L73-L80

I really wish they'd make a workflow as common as a message box a lot easier.

nickrandolph commented 3 years ago

MessageDialog with the hack worked for me. Wonder if there are edge cases where it doesn't work.

sylveon commented 3 years ago

That's not really a hack, it's the proper way to use MessageDialog in the absence of a CoreWindow. For ContentDialog, if you declare it in XAML the XamlRoot is automatically set so you can just call ShowAsync from code-behind.

dotMorten commented 3 years ago

That's not really a hack, it's the proper way

potato potahto. That's merely an excuse for a poorly designed / hard to use api. Calling it a hack because of how using this undiscoverable API feels like, seems fair.

sylveon commented 3 years ago

Sure, there's a point to be made for the API to be made more discoverable, I opened a docs issue about it: https://github.com/MicrosoftDocs/winrt-api/issues/2047

There's helpers in CsWinRT (WinRT.Interop.InitializeWithWindow.Initialize and WinRT.Interop.WindowNative.GetWindowHandle, which are documented here) that avoids needing to manually import COM interfaces, so the code can be written as the following, making it much less hard to use.

public static IAsyncOperation<IUICommand> ShowDialogAsync(string content, string title = null)
{
    var dlg = new MessageDialog(content, title ?? "");
    var handle = GetActiveWindow();
    if (handle == IntPtr.Zero)
        throw new InvalidOperationException();
    InitializeWithWindow.Initialize(dlg, handle);
    return dlg.ShowAsync();
}

// or
var dlg = new MessageDialog("Test");
InitializeWithWindow.Initialize(dlg, WindowNative.GetWindowHandle(this));
await dlg.ShowAsync();
dotMorten commented 3 years ago

Yup I'm aware of the new helper, but it doesn't really address the problem. If the dialog took the xamlroot as a parameter it would at least be a bit more obvious, perhaps combined with hiding the empty constructor from intellisense (since it needs to stay there for xaml support).

And speaking of that helper: It isn't even clear what type it takes as a parameter (just any object), nor is there any intellisense to tell me anything: image image

When you don't hook up the window handle or root (because how are you supposed to know you need to?), you get really really poor error messages, not help messages (it's not invalid, it's just not set):

image

At least you can get a bit further with some extension methods, but we shouldn't have to do this: image

All of this ultimately leads to an extremely frustrating and off-putting developer experience. I feel a bit like beating a dead horse, since I've been making this same point over and over. It's one thing that WinUI "works" and can do what it needs to, but that doesn't help, if the developer experience is so frustrating we give up before we really get into using it.

sylveon commented 3 years ago

Remember that MessageDialog is an old Windows 8 era system API. So at this point it's effectively frozen and most likely won't be seeing any new changes, especially considering it's deprecated. There are other issues to the XamlRoot constructor idea:

dotMorten commented 3 years ago

Remember that MessageDialog is an old Windows 8 era system API

Again just an excuse. WinUI needs a message dialog that fits into the WinUI model of things. We shouldn't have to be doing these crazy hacks to make basic things work. But we're currently left with no choice.

sylveon commented 3 years ago

Then wouldn't the better idea be to suggest a modern WinUI API for this, or improvements to ContentDialog that makes it easier to use, rather than complain about an old API.

JaiganeshKumaran commented 1 year ago

@dotMorten What's the problem with this?

ContentDialog dialog = new()
{
    Title = "your title",
    Content = "your message",
    PrimaryButtonText = "OK",
    SecondaryButtonText = "Cancel",
    XamlRoot = this.XamlRoot
};

var result = await dialog.ShowAsync();
sylveon commented 1 year ago

Nice necro

mikernet commented 1 year ago

@JaiganeshKumaran

What's the problem with this?

1) You need to jump through annoying hoops that may or may not play nicely with your MVVM library dialog flow mechanism to show a simple message dialog when you're already showing a content dialog, because you can't stack content dialogs on top of each other, so you end up with convoluted code to hide the current content dialog, then show the message dialog, and then re-show the content dialog. If any code is awaiting the original dialog ShowAsync() then that becomes a big problem. 2) If you are building a cross-platform app with Uno+WinUI, MessageDialog gets translated into the native message dialog functionality on each platform, which is desirable in a lot of situations.

ranjeshj commented 1 year ago

Ideally, we should make ContentDialog support this model in WinUI3 desktop, multiple islands scenarios.