CommunityToolkit / WindowsCommunityToolkit

The Windows Community Toolkit is a collection of helpers, extensions, and custom controls. It simplifies and demonstrates common developer tasks building .NET apps with UWP and the Windows App SDK / WinUI 3 for Windows 10 and Windows 11. The toolkit is part of the .NET Foundation.
https://docs.microsoft.com/windows/communitytoolkit/
Other
5.88k stars 1.38k forks source link

ToastContentBuilder.Show do not display expected content on Winui3/Unpackaged/.Net6 application #4734

Open fmaeseele opened 2 years ago

fmaeseele commented 2 years ago

Describe the bug

Using this code:

  new ToastContentBuilder()
        .AddText("My Title")        
        .Show();

Show a notification with text as expected: "My Title"

If I try to use an asset image like this:

  new ToastContentBuilder()
        .AddText("My Title")        
        .AddHeroImage("ms-appx:///Assets/logo.png")
        .Show();

Do not display a Toast Notification with "My Title" text and image, but instead a classic Windows Notification witch the name of my application and its icon, and the text "New notification". Please see following french screenshot: ToastNotificationNotWorking

Any idea why ????

Regression

No response

Reproducible in sample app?

Steps to reproduce

In a basic WinUI3 application, place the following code on a button click:

  new ToastContentBuilder()
        .AddText("My Title")        
        .AddHeroImage("ms-appx:///Assets/logo.png")
        .Show();

Create a folder "Assets" in project folder, and place an image "logo.png" into it.

Run the application and click the button.

Expected behavior

Should display the notification with My Title and Image (logo.png)

Screenshots

No response

Windows Build Number

Other Windows Build number

No response

App minimum and target SDK version

Other SDK version

No response

Visual Studio Version

2022

Visual Studio Build Number

No response

Device form factor

Desktop

Nuget packages

No response

Additional context

No response

Help us help you

Yes, but only if others can assist.

ghost commented 2 years ago

Hello fmaeseele, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

michael-hawker commented 2 years ago

FYI @vaheeshta

@fmaeseele you can also try the nightly packages https://aka.ms/wct/wiki/previewpackages? I believe the packages are actually agnostic, so the Uwp one should work from here I think still for WinUI 3? https://dev.azure.com/dotnet/CommunityToolkit/_artifacts/feed/CommunityToolkit-MainLatest/NuGet/Microsoft.Toolkit.Uwp.Notifications/versions/8.0.0-build.40

vaheeshta commented 2 years ago

You're seeing "New notification" because this is our default state in Window 11 if there is a bug in your notification content code. The issue seems to be with your .AddHeroImage("ms-appx:///Assets/logo.png"). Can you try with another image? Such as .AddHeroImage(new Uri("https://picsum.photos/364/180?image=1043")) from https://docs.microsoft.com/en-us/windows/apps/design/shell/tiles-and-notifications/adaptive-interactive-toasts?tabs=builder-syntax#hero-image? @fmaeseele

fmaeseele commented 2 years ago

Hi, As described in the title of the bug and this is important, this issue is only hapening because this is an Unpackaged application. Meaning there is no appx manifest, this is a pure Winui3 desktop application. So, according to the documentation, http images are not supported in this mode. It turns out that testing the .AddHeroImage(new Uri("http://xxxxx")) did not display the image but displayed correctly the rest of the toast content. In my case, it seems that "ms-appx" scheme is not supported as expected. (I didn't try the ms-file scheme)

Regarding at the toast notification code, I was curious about how my application icon is being displayed: In fact, the icon is temporary saved as a PNG image in folder: C:\Users\octo\AppData\Local\ToastNotificationManagerCompat\Apps\5175DD47-849F-40E3-448C-3A3C42BBE6AC\Icon.png Then the folder and its content is destroyed when closing the application by calling : ToastNotificationManagerCompat.Uninstall(); A workaround for Win32 application (Unpackaged), could be to save images on the fly like it is done for the icon.

fmaeseele commented 2 years ago

@michael-hawker I dit try the nightly package with the same result. In fact, 8.0 branch Notification code is pretty much the same as current 7.x branch, without a "fix for pre 7.01" version.

fmaeseele commented 2 years ago

I also tried from Microsoft documentation:

new ToastContentBuilder()
                .AddToastInput(new ToastSelectionBox("time")
                {
                    DefaultSelectionBoxItemId = "lunch",
                    Items =
                        {
                            new ToastSelectionBoxItem("breakfast", "Breakfast"),
                            new ToastSelectionBoxItem("lunch", "Lunch"),
                            new ToastSelectionBoxItem("dinner", "Dinner")
                        }
                }).Show();

It doesn't work at all: same notification with "New notification" instead of displaying the combobox. In my opinion, using toast notifications from Non Packaged application has serious issues.

vaheeshta commented 2 years ago

@fmaeseele The team has been working to fix those issues along with other pain points. Recently we released the latest notifications APIs via the Windows App SDK with consistent support for unpackaged and packaged applications. I'd recommend trying it out! Release notes, step by step guidance, sample code.

You'll notice there is no ToastContentBuilder equivalent yet in the Windows App SDK and guidance. However, you're welcome to follow the guidance on how to build the toast content using XML instead OR use ToastContentBuilder but don't call Show(), and instead call GetXML() to then plug the XML into AppNotificationManager.Default.Show(toast);. Yes, it does feel clunky, which is why the team is bringing the benefits of the ToastContentBuilder to the Windows App SDK. Still in progress but you're welcome to take a look at the spec.

If you have any questions regarding the Windows App SDK, please feel free to use https://github.com/microsoft/WindowsAppSDK/discussions.

fmaeseele commented 2 years ago

Thank you vaheeshta . I'll take some time to give a try with the notification api from Windows App SDK and I'll update this thread with the results. But let's say I can successfuly send a Toast Notification with image, is there gona be a fix for the WindowsCommunityToolkit.Notifications package, or will it be deprecated ?

vaheeshta commented 2 years ago

Thanks, looking forward to the feedback! And that would be a question for @michael-hawker

michael-hawker commented 2 years ago

@vaheeshta once you implement a migration of the ToastContentBuilder in the Windows App SDK, we'll depcreate and eventually remove the notification package here, as per our regular policy when things transition to the SDK.

If that'll be a while, we can also discuss with your team, replacing the core of the notifications package to be built on top of the new Windows App SDK bits with the remaining content builder bits (assuming the WindowsAppSDK API would work for UWP apps as well still).

Maybe we should setup a sync soon on our upcoming plans?

vaheeshta commented 2 years ago

Let me ping you internally on this @michael-hawker!

fmaeseele commented 2 years ago

Hi All,

After digging into the sample code., it appears that if you want to display an image in the toast, you have to reference the image with its full path on the disk, and not with the "ms-appx:///" scheme like it is mentioned into the documentation.

So after all, it seems to me that the current package is not buggy at all. If I use the following code, everything is working fine:

new ToastContentBuilder()
        .AddText("My Title")        
        .AddInlineImage(GetFullPathToAssets("logo.png"))
        .Show();

assuming the application is a Non Packaged application, and logo.png build property is set to "Content".

In the end, it's more a documentation issue for non packaged application not explaining this difference.

@vaheeshta I also tested using the native notification api, and I found a small bug:

My application exe file is "com.mycompany.myapplicationname.exe" and unfortunately the toast header is displaying "com" instead of "myapplicationname"

May be there is a way to set it correctly but everything else is working fine too.

The main difference between using the CommunityToolkit Notification package and using the native api, is that when you quit your application and unregister, the first clear all the history, and not the second.

vaheeshta commented 2 years ago

Thanks for the update! Adding @drewbatgit for the documentation issue. @fmaeseele, can you please clarify if the documentation that you are referring to is the "Image size restrictions" section of the "Toast content" page? https://docs.microsoft.com/en-us/windows/apps/design/shell/tiles-and-notifications/adaptive-interactive-toasts?tabs=builder-syntax#image-size-restrictions

Adding @robertzhou-wpn to track the Windows App SDK issue with "My application exe file is "com.mycompany.myapplicationname.exe" and unfortunately the toast header is displaying "com" instead of "myapplicationname"

robertzhou-wpn commented 2 years ago

Add @sharath2727 for the issue "My application exe file is "com.mycompany.myapplicationname.exe" and unfortunately the toast header is displaying "com" instead of "myapplicationname".

fmaeseele commented 2 years ago

@vaheeshta Sorry I didn't take time to give you the links for the documentation about the image uris schemes. Will do it very soon. Regarding the workaround for the DisplayName, thank you for the documentation about the upcoming Windows App Sdk, but is this version considered reliable enought ? As its name is 'experimental'... I'm a bit afraid of switching to it.

fmaeseele commented 2 years ago

@vaheeshta, I tried to use the Windows App SDK 1.2 experimental, and the following line is throwing InvalidCastException!

var notificationManager = AppNotificationManager.Default;

_System.InvalidCastException HResult=0x80004002 Message=Specified cast is not valid. Source=WinRT.Runtime StackTrace: at WinRT.IObjectReference.As[T](Guid iid) at Microsoft.Windows.AppNotifications.AppNotificationManager.Make_objRef_globalMicrosoft_Windows_AppNotifications_IAppNotificationManagerStatics() at Microsoft.Windows.AppNotifications.AppNotificationManager.getDefault()

I tried both using .Net6 and .Net7 preview. Any idea ?

vaheeshta commented 2 years ago

Thank you for sharing, adding @robertzhou-wpn and @sharath2727 to take a look!

robertzhou-wpn commented 2 years ago

@vaheeshta, I tried to use the Windows App SDK 1.2 experimental, and the following line is throwing InvalidCastException!

var notificationManager = AppNotificationManager.Default;

_System.InvalidCastException HResult=0x80004002 Message=Specified cast is not valid. Source=WinRT.Runtime StackTrace: at WinRT.IObjectReference.As[T](Guid iid) at Microsoft.Windows.AppNotifications.AppNotificationManager.Make_objRef_globalMicrosoft_Windows_AppNotifications_IAppNotificationManagerStatics() at Microsoft.Windows.AppNotifications.AppNotificationManager.getDefault()

I tried both using .Net6 and .Net7 preview. Any idea ?

Add @loneursid. Eric, have you seen similar issues in c# samples before? Any ideas here?

loneursid commented 2 years ago

I don't remember having seen this specific error but it seems to indicate the app has encountered an issue connecting to the COM server. Initializing the AppNotificationManager can be tricky and things need to happen in a specific order early in the app lifecycle so, I would recommend using the sample as a reference.

I have downloaded the C# app notification sample, updated it to using the WindowsAppSDK v1.2 Experimetal, downloaded and installed the runtime for my os (x64) from: https://docs.microsoft.com/en-us/windows/apps/windows-app-sdk/downloads and verified that everything is working as expected.

fmaeseele commented 2 years ago

Hi @loneursid I installed the new Windows App SDK 1.2 Preview1, instead of the experimental version, and everything is working fine. Thanks to the new builder api, I could remove the dependency on the Windows Community Notification package.

I still have the issue of the notification title which is "Com" instead of "SmartHome" with my application "com.mynamspace.SmartHome.exe"

The workaround suggested (using register with provided title) is not really an option, as I have to provide also the Uri of my application Icon. But the uri must be a file as my application is unpackaged. So I would have to save the icon in a temporary folder to reference it. So if this issue could be fixed in a later release, that would be perfect.

At this point I think the original issue can now be closed as it is clear from the documentation that images schemes "ms-appx or ms-appdata" are only supported for packaged application.

Thank you to all of you and for your hard work.

robertzhou-wpn commented 2 years ago

@fmaeseele. Thanks for your feedback. We will create a work item on our side to track the icon and name issue.

sharath2727 commented 2 years ago

Hi @fmaeseele,

Thanks for pointing out the displayName issue here.

Let me start off with some context: https://github.com/microsoft/WindowsAppSDK/blob/main/dev/AppNotifications/AppNotificationUtility.cpp - As you are calling Register() API with no assets(displayname and iconuri) we are trying to follow the best approach by trying to glean the assets from possible places by calling GetAssets API in the above link for unpackaged apps. We try to glean the assets from a couple of places:

  1. We look for a shortcut for your application, if not,
  2. We try to extract the process name and display that as your displayname.

I see that from what you are posting it looks like the latter is happening. If the processname would have been "Smarthome.exe" you would have seen the displayname as "Smarthome". But since you have a com.mynamespace.SmartHome.exe it is trying to get the first word before the first "." which is "com" here and it is displaying that. It is as per design. For further reference please look for the API: GetDisplayNameBasedOnProcessName in the above link.

I would like to understand your concern about not using the overloaded register API which takes in displayname and iconuri as assets which I guess would suit your needs. This would help us guide you in the right direction. Thanks.