Fixed stack corruption bug in InAppNotification in sample #3802

shweaver-MSFT · 2021-03-02T21:00:11Z

Fixes #3391

In the InAppNotifications sample page, there is the option to display a notification with or without a custom DataTemplate. Mixing the usage of these buttons in the sample app with a StackMode other than "Replace", would cause cause the notifications to show double close buttons, or be out of alignment.

PR Type

What kind of change does this PR introduce?

Bugfix

What is the current behavior?

Currently, the sample page for InAppNotifications shows the bug when interacting with the "... (with/without DataTemplate)" buttons. This is due to some odd wiring in the code behind. Though 3 notification objects were defined in XAML, only two were actually used. The third is configured to show the custom data template; But when it was time to show another notification it hot swaps templates with the custom notification, and attempts to show the custom content. Unfortunately this causes a bunch of issues because the need to display the close button and what content to show gets out of sync when we start to close the stack of notifications.

What is the new behavior?

In the update sample page, I've started using the third notification object. It maintains a separate stack because it is a separate notification, but so does the VS styled notification. The advise to developers going forward is to pick a model, not mix and match. Hot-swapping templates is not supported.

To support the flow of the sample, I added an option to pass an argument to the Dismiss function.

InAppNotification.Dismiss(bool dismissAll = false)

Passing true will dismiss all of the notifications in the stack. I read that there was a comment in the sample code-behind saying, "Do not dismiss all in production", but I disagree. Sometimes you really need to dismiss them all, such as during major app navigations (sign in/out). If a whole stack of notifications becomes suddenly irrelevant we should be able to dismiss them all. It's useful in the sample as well, for when we switch between the three notifications.

Lastly, in this process I uncovered a bug that causes the notification not to render when reusing the same DataTemplate. Here is the snippet from InAppNotification.cs

// From InAppNotifcation.UpdateContent(NotificationOptions notificationOptions)

    case DataTemplate dataTemplate:
        // Without this check, the dataTemplate will fail to render.
        // Why? Setting the ContentTemplate causes the control to re-evaluate it's Content value.
        // When we set the ContentTemplate to the same instance of itself, we aren't actually changing the value.
        // This means that the Content value won't be re-evaluated and stay null, causing the render to fail.
        if (_contentProvider.ContentTemplate != dataTemplate)
        {
            _contentProvider.ContentTemplate = dataTemplate;
            _contentProvider.Content = null;
        }

        break;

PR Checklist

Please check if your PR fulfills the following requirements:

Tested code with current supported SDKs
Pull Request has been submitted to the documentation repository instructions. Link:
Sample in sample app has been added / updated (for bug fixes / features)
- Icon has been created (if new sample) following the Thumbnail Style Guide and templates
New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
Tests for the changes have been added (for bug fixes / features) (if applicable)
Header has been added to all new source files (run build/UpdateHeaders.bat)
Contains NO breaking changes

Other information

Todo:

Document the new dismissAll flag.

ghost · 2021-03-02T21:00:15Z

Thanks shweaver-MSFT for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

Kyaa-dost

Thanks for the fix @shweaver-MSFT! Looks great 🚀

ghost · 2021-03-02T22:16:15Z

Hello @Kyaa-dost!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 6 hours 43 minutes. No worries though, I will be back when the time is right! 😉

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Kyaa-dost · 2021-03-02T22:16:44Z

@msftbot merge this PR once approved by @michael-hawker

ghost · 2021-03-02T22:16:48Z

Hello @Kyaa-dost!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @michael-hawker

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

michael-hawker · 2021-03-02T23:45:21Z

Microsoft.Toolkit.Uwp.UI.Controls.Core/InAppNotification/InAppNotification.cs

-            Opening?.Invoke(this, eventArgs);
-
-            if (eventArgs.Cancel)
+            await Dispatcher.RunAsync(Windows.UI.Core.CoreDispatcherPriority.Normal, () =>


Was this causing an issue somewhere else? Didn't see this called out in the PR notes.

In either case it needs to be DispatcherQueue now. 🙂 Dispatcher is no longer a thing we should use. We have the DispatcherQueue helpers we use in other controls too, but if InAppNotification doesn't have a reference already, this may get a bit messy.

Crap just noticed we have DispatcherTimer at the top too, it should probably be DispatcherQueueTimer... I'm assuming @azchohfi made these updates in the WinUI 3 branch, I'll check quick.

Nope, I'm seeing a lot of Dispatchers still in the code, thought we had fixed them all early on. I'll file an issue to track these later. Think ThemeListener is the best pattern we have currently, but not sure if we've tried that pattern on a Control yet.

Let me try removing them and see how it goes. I tend to add them during debugging to see if the XAML timer is causing any issues. They may not be necessary at all tbh

Gotcha! I've made the fixes for InAppNotification. Works well :) Thanks for the hint on ThemeListener, it was a good example.

…ows-toolkit/WindowsCommunityToolkit into shweaver/inappnotifications

michael-hawker

LGTM, thanks @shweaver-MSFT! 🎉🎉🎉

michael-hawker · 2021-03-03T22:34:21Z

Microsoft.Toolkit.Uwp.UI.Controls.Core/InAppNotification/InAppNotification.cs

@@ -34,7 +36,11 @@ public InAppNotification()
        {
            DefaultStyleKey = typeof(InAppNotification);

+            _dispatcherQueue = DispatcherQueue.GetForCurrentThread();


Good call, I suppose with a UI control (compared to something like ThemeListener) there'd be no reason to use a DispatcherQueue other than the one we get initially on the UI Thread here (as the UI Thread should hopefully be the one initializing the control...).

Right @azchohfi?

That's what I figured. Plus, XAML objects don't like non-empty constructors, and even declaring the default as null is not enough to appease the compiler. This made most sense to me :)

michael-hawker · 2021-03-03T22:49:04Z

@Kyaa-dost this one can be merged once #3796 is merged.

Kyaa-dost · 2021-03-03T22:56:25Z

@msftbot nevermind

ghost · 2021-03-03T22:56:30Z

Hello @Kyaa-dost!

Because you've told me to reset the custom auto-merge settings, I'll use the configured settings for this repository when I'm merging this pull request.

Kyaa-dost · 2021-03-03T23:48:26Z

@msftbot hold this for 1 hour

ghost · 2021-03-03T23:48:28Z

Hello @Kyaa-dost!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Thu, 04 Mar 2021 00:48:28 GMT, which is in 1 hour

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

Kyaa-dost · 2021-03-04T00:10:39Z

@msftbot merge this PR

ghost · 2021-03-04T00:10:41Z

Hello @Kyaa-dost!

I think you told me that you want to delay the approval for a certain amount of time, but I am not confident that I have understood you correctly.

Please try rephrasing your instruction to me.

Kyaa-dost · 2021-03-04T00:11:08Z

@msftbot nevermind

ghost · 2021-03-04T00:11:12Z

Hello @Kyaa-dost!

Because you've told me to reset the custom auto-merge settings, I'll use the configured settings for this repository when I'm merging this pull request.

shweaver-MSFT added 4 commits February 25, 2021 13:26

Adding dismissAll param to InAppNotifications Dismiss method

6f256a1

Restructuring use of custom notification

b66de95

Fixed bug in InAppNotification sample

d5d569c

Fixed the bug in InAppNotifications with datatemplate

5932530

ghost requested review from michael-hawker, azchohfi, Kyaa-dost and Rosuavio March 2, 2021 21:00

ghost added bug 🐛 An unexpected issue that highlights incorrect behavior controls 🎛️ labels Mar 2, 2021

Kyaa-dost added this to the 7.0 milestone Mar 2, 2021

Kyaa-dost approved these changes Mar 2, 2021

View reviewed changes

Kyaa-dost added the auto merge ⚡ label Mar 2, 2021

Merge branch 'master' into shweaver/inappnotifications

73be0aa

Kyaa-dost added the sample app 🖼 label Mar 2, 2021

michael-hawker reviewed Mar 2, 2021

View reviewed changes

shweaver-MSFT added 4 commits March 3, 2021 10:06

Merge branch 'master' into shweaver/inappnotifications

3a30842

Updated InAppNotification to use new DispatcherQueue

e6d29c8

Merge branch 'shweaver/inappnotifications' of https://github.com/wind…

267a2bd

…ows-toolkit/WindowsCommunityToolkit into shweaver/inappnotifications

Removed unnecessary async from InAppNotificaiton

5d1e885

michael-hawker approved these changes Mar 3, 2021

View reviewed changes

CommunityToolkit deleted a comment Mar 3, 2021

Merge branch 'master' into shweaver/inappnotifications

c04caaa

ghost merged commit a66da6e into master Mar 4, 2021

ghost deleted the shweaver/inappnotifications branch March 4, 2021 01:24

This pull request was closed.

Fixed stack corruption bug in InAppNotification in sample #3802

Fixed stack corruption bug in InAppNotification in sample #3802

Uh oh!

Conversation

shweaver-MSFT commented Mar 2, 2021

Fixes #3391

PR Type

What is the current behavior?

What is the new behavior?

PR Checklist

Other information

Uh oh!

ghost commented Mar 2, 2021

Uh oh!

Kyaa-dost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Mar 2, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Kyaa-dost commented Mar 2, 2021

Uh oh!

ghost commented Mar 2, 2021

Uh oh!

michael-hawker Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

michael-hawker Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

michael-hawker Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shweaver-MSFT Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

shweaver-MSFT Mar 3, 2021

Choose a reason for hiding this comment

Uh oh!

michael-hawker left a comment

Choose a reason for hiding this comment

Uh oh!

michael-hawker Mar 3, 2021

Choose a reason for hiding this comment

Uh oh!

shweaver-MSFT Mar 3, 2021

Choose a reason for hiding this comment

Uh oh!

michael-hawker commented Mar 3, 2021

Uh oh!

Kyaa-dost commented Mar 3, 2021

Uh oh!

ghost commented Mar 3, 2021

Uh oh!

Kyaa-dost commented Mar 3, 2021

Uh oh!

ghost commented Mar 3, 2021

Uh oh!

Kyaa-dost commented Mar 4, 2021

Uh oh!

ghost commented Mar 4, 2021

Uh oh!

Kyaa-dost commented Mar 4, 2021

Uh oh!

ghost commented Mar 4, 2021

Uh oh!

Uh oh!

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

michael-hawker Mar 2, 2021 •

edited

Loading