Introduce Register API to take in assets by sharath2727 · Pull Request #2447 · microsoft/WindowsAppSDK

sharath2727 · 2022-04-28T06:51:56Z

Description: Allow developers to input DisplayName and Icon of their choice to be registered with AppNotificationManager so that these assets can show up when AppNotifications are posted or received.

Validation: Introduced unittests and validated them.

ghost · 2022-04-28T21:00:53Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ sharath2727 sign now
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

danielayala94 · 2022-04-29T17:28:54Z

+int main()
+{
+    auto manager = winrt::AppNotificationManager::Default();
+    manager.Register(L"AppNotifications", winrt::Windows::Foundation::Uri {L"<Path to Icon>"});


I think it's okay to set a mock path (C:\Users\<user>\AppData\MyApp\icon.png or the app folder which may be under ProgramFiles?).

Would like to see what everyone thinks of this before updating. Thanks.

I like more specific over too general, as long as there aren't too many options and that we list the most common options. Maybe we use :\Users<user>\AppData\MyApp\icon.png as proposed by Daniel and we also put a comment with an example path to a folder inside the program file folder.

Maybe we should show the functions to call in order to retrieve the path(s) to the folders where the app is installed.

Something like:

std::filesystem::path exePath{ GetPathToCurrentExe() }; auto iconPath{ exePath / "displayicon.png" };

Sample should show use of MRT or another resource loader indirection to fetch that display string, too.

brendanfly · 2022-05-02T21:01:45Z


+## Registering for App Notifications using assets
+
+For Unpackaged applications, the developer can opt to call into Register API with DisplayName and Icon.


"can Register using a custom DisplayName and Icon"?

hulumane · 2022-05-05T05:02:01Z

    }

-    void AppNotificationManager::Register()
+    void AppNotificationManager::RegisterHelper(hstring const& displayName, std::wstring const& iconFilePath)


Why is one argument hstring and the other wstring? We should be consistent

hulumane · 2022-05-05T05:05:45Z

+    {
+        THROW_HR_IF_MSG(E_ILLEGAL_METHOD_CALL, AppModel::Identity::IsPackagedProcess(), "Not applicable for packaged applications");
+
+        THROW_HR_IF(E_INVALIDARG, (displayName == winrt::hstring{}) || (iconUri == nullptr));


Same issue with iconUri. If it is null, don't check. Let it crash further along. You should instead check for the validity of the uri's content: eg. empty path, invalid path etc.

Atleast make sure that the uri is a valid file path and the file exists on disk.

hulumane · 2022-05-05T05:12:37Z

+        THROW_HR_IF(E_INVALIDARG, (displayName == winrt::hstring{}) || (iconUri == nullptr));
+
+        std::wstring iconFilePath{ iconUri.Path() };
+        iconFilePath = iconFilePath.substr(1, iconFilePath.size() - 1);


what is this substring for? Can you please explain this?

use RawUri()

hulumane · 2022-05-05T05:18:17Z

+        std::wstring iconFilePath{ iconUri.Path() };
+        iconFilePath = iconFilePath.substr(1, iconFilePath.size() - 1);
+        // Uri converts backward slashes to forward slashes
+        std::replace(iconFilePath.begin(), iconFilePath.end(), '/', '\\');


You are actually converting forward slash to back slash. So the comment is incorrect. https://newbedev.com/difference-between-forward-slash-and-backslash-in-file-path

hulumane · 2022-05-05T05:22:56Z

+        std::wstring iconFilePath{ iconUri.Path() };
+        iconFilePath = iconFilePath.substr(1, iconFilePath.size() - 1);
+        // Uri converts backward slashes to forward slashes
+        std::replace(iconFilePath.begin(), iconFilePath.end(), '/', '\\');


This conversion is extremely fragile. We should not have to do it. Have you checked why we were erroring out with forward slash in the path? We should confirm why we see this behavior before making this change.

hulumane · 2022-05-05T05:26:35Z


+    void AppNotificationManager::Register()
+    {
+        RegisterHelper(winrt::hstring{} /* displayName */, std::wstring{} /* iconFilePath */);


Why don't you retrieve the correct displayname and icon path here before calling into the helper? Passing in empty is a strange pattern.

Also Register would fail now because you call empty. Please don't use anti patterns and use valid arguments to pass into
APIs

hulumane · 2022-05-05T05:34:37Z

                    else
                    { 
-                        registeredClsid = RegisterComActivatorGuidAndAssets();
+                        registeredClsid = RegisterComActivatorGuidAndAssets(winrt::hstring{} /* displayName */, std::wstring{} /* iconUri */);


This is incorrect too. Don't pass empty arguments please. Instead pass in the overloaded arguments or the one from shortcut/process

Actually why do we need to re-register assets at all? Aren't we simply re-routing to another process? @pmpurifoy ? Can you explain this one please and why we need to re-register assets as part of this flow?

Actually what really needs to happen here is we need to fetch the registered Guid and nothing more. But I will wait for Paul to chip in here :)

std::wstring registeredGuid;
auto hr = GetActivatorGuid(registeredGuid);

Call GetActivatorGuid

loneursid · 2022-05-05T20:47:00Z

+    {
+        winrt::AppNotificationManager::Default().Register(L"AppNotificationApp", nullptr);
+
+        winrt::AppNotificationManager::Default().UnregisterAll();


A better pattern in this situation, would be to call UnregisterAll() from scope_exit. It would

make it instantly clear that UnregisterAll isn't under test here.

though it doesn't apply to this specific case as Unregister can't throw E_INVALIDARG, avoid a situation where the fuction under test does not throw as expected but the cleanup one does. The test would then pass although it should be failing.

This comment applies to all similar tests

loneursid · 2022-05-05T20:52:56Z

+}
+
+bool VerifyRegisterWithDisplayNameAndIcon_Unpackaged()
+{


It's a shame that the only validation we do in this test is that Register() didn't throw. I wish we could verify that calling register actually did something useful and valid, i.e.: besides running code that didn't crash / throw...

loneursid · 2022-05-05T21:02:19Z

+        }
+
+        TEST_METHOD(VerifyRegisterWithAssetsFail)
+        {


I assume that we run a single test for packaged just to confirm that register works for packaged, and that we rely on the more complete suite of unpackaged tests to actually verify Register()'s actual behavior.

It would be nice if, we could just run the suite twice: once for unpackaged and one for packaged. It seems the only difference is whether RunTestUnpackaged or RunTest is used to launch the test. It feels like dependency injection could help here.

Not suggesting you do it now, that's probably beyond the point of this PR but that could be a nice future refactor.

It's possible to detect at compile time if you're built with WindowsAppSDKSelfContained=true or false. At least, I think it is now in Foundation with recent changes I made to .props/.targets

Our test infrastructure does tend to build everything both ways though it was only running SelfContained=false. My PR in the Aggregator fixes that (going in soon).

Sorry limited network access at the moment. If you grip for SelfContained across build* you should find the C++ and C# symbols code can check for.
Though I suggest not messing with that until my changes go in as that has significant updates to the plumbing

DrusTheAxe · 2022-05-09T03:17:14Z

-    {
-        THROW_HR(HRESULT_FROM_WIN32(GetLastError()));
-    }
+    winrt::check_bool(DeleteFile(iconFilePath.c_str()) != FALSE);


NIT: DeleteFileW is preferable to DeleteFile (ensures no ambiguity to the reader)

Is winrt::check_bool() equivalent wil::RETURN_IF_WIN32_BOOL_FALSE()?

NOTE: It's weird bordering on perverse to explicitly check for == or != for values of TRUE or FALSE

DrusTheAxe · 2022-05-09T03:19:59Z

    }

-    void AppNotificationManager::Register()
+    void AppNotificationManager::RegisterHelper(hstring const& displayName, std::wstring const& iconFilePath)


Helper? 🤨

jonwis · 2022-05-10T04:22:00Z

        void Register();

+        // For Unpackaged apps only, the caller process will be registered as the COM server.
+        void Register(String displayName, Windows.Foundation.Uri iconUri);


What happens when a Packaged app calls this? Is that an error?

jonwis

See note about enhancing the samples.

sharath2727 · 2022-06-27T19:42:33Z

This PR will be closed and all the changes are now tracked on: #2596

ghost added the needs-triage label Apr 28, 2022

Introduce AppNotificationManager::Register API to take in assets

7a0a03a

sharath2727 force-pushed the user/vemancha/registeroverload branch from f429687 to 7a0a03a Compare April 28, 2022 06:55

loneursid added the area-Notifications Toast notification, badges, Live Tiles, push notifications label Apr 28, 2022

Update the spec to include the Register overload API

ace72d0

sharath2727 requested review from DrusTheAxe, danielayala94, hulumane, jonwis and robertzhou-wpn April 28, 2022 21:01

sharath2727 marked this pull request as ready for review April 28, 2022 21:36

sharath2727 changed the title ~~Introduce AppNotificationManager::Register API to take in assets~~ Introduce Register API to take in assets Apr 28, 2022

sharath2727 requested a review from pmpurifoy April 28, 2022 22:35

robertzhou-wpn reviewed Apr 28, 2022

View reviewed changes

Comment thread dev/Common/Common.vcxitems.filters

hulumane reviewed Apr 28, 2022

View reviewed changes

Comment thread dev/AppNotifications/AppNotificationManager.cpp Outdated

hulumane reviewed Apr 28, 2022

View reviewed changes

Comment thread dev/AppNotifications/AppNotificationManager.cpp Outdated

hulumane reviewed Apr 28, 2022

View reviewed changes

Comment thread dev/AppNotifications/AppNotificationManager.cpp Outdated

danielayala94 reviewed Apr 29, 2022

View reviewed changes

sharath2727 added 2 commits April 29, 2022 15:55

Address comments

f7196e5

Address minor comment

942062a

sharath2727 requested review from danielayala94, hulumane and robertzhou-wpn April 29, 2022 23:01

brendanfly reviewed May 2, 2022

View reviewed changes

danielayala94 reviewed May 3, 2022

View reviewed changes

Comment thread dev/AppNotifications/AppNotificationManager.cpp

Comment thread dev/AppNotifications/AppNotificationManager.cpp

Comment thread dev/AppNotifications/ShellLocalization.cpp

Comment thread dev/AppNotifications/AppNotificationManager.cpp

hulumane reviewed May 5, 2022

View reviewed changes

loneursid reviewed May 5, 2022

View reviewed changes

DrusTheAxe reviewed May 9, 2022

View reviewed changes

jonwis reviewed May 10, 2022

View reviewed changes

sharath2727 closed this Jun 27, 2022


		## Registering for App Notifications using assets

		For Unpackaged applications, the developer can opt to call into Register API with DisplayName and Icon.

Conversation

sharath2727 commented Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Apr 28, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loneursid May 5, 2022 • 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!

loneursid May 5, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharath2727 commented Apr 28, 2022 •

edited

Loading

loneursid May 5, 2022 •

edited

Loading

loneursid May 5, 2022 •

edited

Loading