Skip to content

Notifications assets full support - #2295

Merged
Daniel Ayala (danielayala94) merged 12 commits into
mainfrom
notifications-assets-full-support
Apr 1, 2022
Merged

Daniel Ayala (danielayala94) merged 12 commits into
mainfrom
notifications-assets-full-support

Conversation

@danielayala94

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.h
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
// subKey: \Software\Classes\AppUserModelId\{AppGUID}
std::wstring subKey{ c_appIdentifierPath + appId };

THROW_IF_WIN32_ERROR(RegCreateKeyEx(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does WIL have registry functions?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe it does. But that would be very nice to have :)

Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
@riverar

Rafael Rivera (riverar) commented Mar 19, 2022 •

Copy link
Copy Markdown
Contributor

I can't attach comments to unchanged lines so am putting it here instead.


GUID newNotificationGuid;
THROW_IF_FAILED(CoCreateGuid(&newNotificationGuid));
wil::unique_cotaskmem_string newNotificationGuidString;
THROW_IF_FAILED(StringFromCLSID(newNotificationGuid, &newNotificationGuidString));
RegisterValue(hKey, L"NotificationGUID", reinterpret_cast<const BYTE*>(newNotificationGuidString.get()), REG_SZ, wcslen(newNotificationGuidString.get()) * sizeof(wchar_t));
return newNotificationGuidString.get();

This chunk of code generates a GUID with a known length then measures the length again, which isn't efficient. Instead you can perhaps do one of a few things (consult your nearest FTE for final guidance):

  • Switch out the StringFromCLSID for StringFromGUID2, which returns the length
  • Switch out the wcslen and use a WIL guid length constant (i.e. wil::guid_string_length)
  • ...

displayName = wil::make_unique_string<wil::unique_cotaskmem_string>(SetDisplayNameBasedOnProcessName().c_str());

RegisterValue(hKey, L"DisplayName", reinterpret_cast<const BYTE*>(displayName.get()), REG_EXPAND_SZ, wcslen(displayName.get()) * sizeof(wchar_t));
RegisterValue(hKey, L"IconUri", reinterpret_cast<const BYTE*>(iconFilePath.get()), REG_EXPAND_SZ, wcslen(iconFilePath.get()) * sizeof(wchar_t));
RegisterValue(hKey, L"CustomActivator", reinterpret_cast<const BYTE*>(clsid.c_str()), REG_SZ, clsid.size() * sizeof(wchar_t));

SetDisplayNameBasedOnProcessName returns a std::wstring here (which tracks string length!), then we pull out a raw pointer, then measure the string length again. Maybe rejigger things a bit here?

Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.h Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.h Outdated
Comment thread dev/AppNotifications/ShellLocalization.cpp Outdated
Comment thread dev/AppNotifications/ShellLocalization.cpp
Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/ShellLocalization.cpp Outdated
Comment thread dev/AppNotifications/ShellLocalization.cpp Outdated

SetDisplayNameAndIcon();
// Not mandatory, but it's highly recommended to specify AppUserModelId
THROW_IF_FAILED(SetCurrentProcessExplicitAppUserModelID(L"TestAppId"));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vaheeshta Mehrshahi (@vaheeshta) This is needed if we want apps to maintain their toast states even after a process name update later in the future. For example, if unpackaged app foo.exe has a name change to bar.exe in the future, this ensures that the apps toasts remain in the actioncentre. We should document.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also is needed if apps change their path from c:\foo\name.exe to d:\bar\name.exe.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I'll document this once I have the unpackaged sample code.

Comment thread dev/AppNotifications/ShellLocalization.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationManager.cpp Outdated
Comment thread dev/AppNotifications/ShellLocalization.cpp
Comment thread dev/AppNotifications/ShellLocalization.cpp Outdated
Comment thread dev/AppNotifications/ShellLocalization.cpp
Comment thread dev/AppNotifications/ShellLocalization.cpp Outdated
Comment thread dev/AppNotifications/ShellLocalization.cpp Outdated
Comment thread dev/AppNotifications/ShellLocalization.cpp Outdated
Comment thread dev/AppNotifications/ShellLocalization.cpp Outdated
Comment thread dev/AppNotifications/ShellLocalization.cpp Outdated
Comment thread dev/AppNotifications/ShellLocalization.h Outdated
Comment thread dev/AppNotifications/ShellLocalization.h Outdated
@danielayala94

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Comment thread dev/AppNotifications/AppNotificationUtility.cpp Outdated
Comment thread dev/AppNotifications/AppNotificationUtility.cpp
@danielayala94

Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@danielayala94
Daniel Ayala (danielayala94) dismissed stale reviews from Howard Kapustein (DrusTheAxe) and Jon Wiswall (jonwis) April 1, 2022 23:23

I have addressed all comments. Thank you for reviewing this PR! 😊

@danielayala94
Daniel Ayala (danielayala94) deleted the notifications-assets-full-support branch April 1, 2022 23:24

// Store the converted bitmap as ppToRenderBitmapSource
winrt::com_ptr<IWICBitmapSource> wicBitmapSourceConverted;
THROW_IF_FAILED(wicFormatConverter->QueryInterface(_uuidof(IWICBitmapSource), wicBitmapSourceConverted.put_void()));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(No change required)

I think this can be return wicFormatConverter.as<IWICBitmapSource>() ?


THROW_IF_FAILED(wicFormatConverter->Initialize(wicBitmapSource.get(), guidPixelFormatSource, bitmapDitherType, nullptr, 0.f, WICBitmapPaletteTypeCustom));

// Store the converted bitmap as ppToRenderBitmapSource

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead comment, please remove

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

Labels

area-Notifications Toast notification, badges, Live Tiles, push notifications needs-triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants