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

[WinRT] Use a queue to prevent multiple MessageDialogs from causing a crash #347

Merged
merged 2 commits into from Dec 16, 2016

Conversation

Projects
None yet
6 participants
@pauldipietro
Member

pauldipietro commented Sep 12, 2016

Description of Change

Calling DisplayAlert to show multiple dialogs would cause a crash. According to this StackOverflow question it appears that there cannot be multiple MessageDialogs. Per the answer there, the dialogs can instead be queued. This functionality could perhaps be discussed further, but it brings the behavior in line with Android and iOS and prevents this crash from occurring.

Bugs Fixed

https://bugzilla.xamarin.com/show_bug.cgi?id=43469

API Changes

Added:
public static class MessageDialogExtensions

  • contains public static async Task<IUICommand> ShowAsyncQueue(this MessageDialog dialog)

Behavioral Changes

The SO post does mention a reason or two that having multiple dialogs might not be desirable, but again, this behavior can be discussed further if need be.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
Show outdated Hide outdated Xamarin.Forms.Platform.WinRT/Platform.cs
while (_currentDialogShowRequest != null)
{
await _currentDialogShowRequest.Task;
}

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Sep 13, 2016

Member

That looks strange to me. As the signature is not async void the control won't return to the caller while this call is awaiting.

I'd rewrite this method to be public static Task<IUICommand> ShowAsyncQueue(this MessageDialog dialog) and fix this...

unless this is the desired behavior, obviously

@StephaneDelcroix

StephaneDelcroix Sep 13, 2016

Member

That looks strange to me. As the signature is not async void the control won't return to the caller while this call is awaiting.

I'd rewrite this method to be public static Task<IUICommand> ShowAsyncQueue(this MessageDialog dialog) and fix this...

unless this is the desired behavior, obviously

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Sep 13, 2016

Member

as the caller is async void, this is probably fine... for this time

@StephaneDelcroix

StephaneDelcroix Sep 13, 2016

Member

as the caller is async void, this is probably fine... for this time

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Sep 26, 2016

Member

Can you rebase please @pauldipietro

Member

rmarinho commented Sep 26, 2016

Can you rebase please @pauldipietro

@samhouts

This comment has been minimized.

Show comment
Hide comment
@samhouts

samhouts Oct 11, 2016

Member

Please address the failing unit tests, @pauldipietro. 😄

Member

samhouts commented Oct 11, 2016

Please address the failing unit tests, @pauldipietro. 😄

Show outdated Hide outdated Xamarin.Forms.Platform.WinRT/Platform.cs
public static async Task<IUICommand> ShowAsyncQueue(this MessageDialog dialog)
{
if (!Window.Current.Dispatcher.HasThreadAccess)

This comment has been minimized.

@hartez

hartez Dec 13, 2016

Member

Window.Current isn't a good way to make this check here - if the user tries to call DisplayAlert() from a non-UI thread, Window.Current could be null and they'll get a NullReferenceException.

Instead of if (!Window.Current.Dispatcher.HasThreadAccess) you could check if (Device.IsInvokeRequired), which doesn't have that issue.

@hartez

hartez Dec 13, 2016

Member

Window.Current isn't a good way to make this check here - if the user tries to call DisplayAlert() from a non-UI thread, Window.Current could be null and they'll get a NullReferenceException.

Instead of if (!Window.Current.Dispatcher.HasThreadAccess) you could check if (Device.IsInvokeRequired), which doesn't have that issue.

Show outdated Hide outdated Xamarin.Forms.Platform.WinRT/Platform.cs
{
if (!Window.Current.Dispatcher.HasThreadAccess)
{
throw new InvalidOperationException("This method can only be invoked from UI thread.");

This comment has been minimized.

@hartez

hartez Dec 13, 2016

Member

Instead of throwing an exception here, does it make sense to simply do the BeginInvoke() for the user?

@hartez

hartez Dec 13, 2016

Member

Instead of throwing an exception here, does it make sense to simply do the BeginInvoke() for the user?

@pauldipietro

This comment has been minimized.

Show comment
Hide comment
@pauldipietro

pauldipietro Dec 15, 2016

Member

@hartez I changed that behavior to rely on Device.IsInvokeRequired. Let me know if this is what you had in mind. 😃

Member

pauldipietro commented Dec 15, 2016

@hartez I changed that behavior to rely on Device.IsInvokeRequired. Let me know if this is what you had in mind. 😃

@hartez

hartez approved these changes Dec 15, 2016

@rmarinho rmarinho merged commit db4486d into master Dec 16, 2016

2 of 6 checks passed

Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Snapshot dependency …
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Snapshot depende…
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Snapshot dependen…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Snapshot dependen…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: Windows Debug : Tests passed: 3685, ignored: 10
Details

@rmarinho rmarinho deleted the fix-bugzilla43469 branch Dec 16, 2016

@hartez hartez referenced this pull request Apr 10, 2018

Closed

[UWP] Restore ShowAsyncQueue method for dialogs #2418

4 of 4 tasks complete

@samhouts samhouts added this to the 2.3.4 milestone Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment