Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Memory usage improvements #3

Merged
merged 6 commits into from
Feb 7, 2022
Merged

Memory usage improvements #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
920915f
Start refactor to improve memory usage
sergiou87 Feb 1, 2022
f946bef
Start invoking callbacks!
sergiou87 Feb 1, 2022
1b8d757
Invoke callbacks for non-click events too
sergiou87 Feb 3, 2022
1532381
Use LRU cache to store callbacks
sergiou87 Feb 3, 2022
63225ad
Increase number of cached notifications
sergiou87 Feb 7, 2022
2ad4e30
Use `quick-lru` instead of `lru-cache`
sergiou87 Feb 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 31 additions & 25 deletions lib/desktop-notifications.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,40 +1,29 @@
import QuickLRU from 'quick-lru'
import { v4 as uuidv4 } from 'uuid'

// Windows-only for now
const nativeModule =
process.platform === 'win32'
? require('../../build/Release/desktop-notifications.node')
: null

export function initializeNotifications(toastActivatorClsid: string) {
if (!nativeModule) {
// this code is a no-op when the module is missing
return
}

nativeModule.initializeNotifications(toastActivatorClsid)
nativeModule?.initializeNotifications(
toastActivatorClsid,
onNotificationEvent
)
}

export function terminateNotifications() {
if (!nativeModule) {
// this code is a no-op when the module is missing
return
}

nativeModule.terminateNotifications()
nativeModule?.terminateNotifications()
}

function showNotification(
id: string,
title: string,
body: string,
callback: (event: string) => void
body: string
): number | null {
if (!nativeModule) {
// this code is a no-op when the module is missing
return null
}

return nativeModule.showNotification(id, title, body, callback)
return nativeModule?.showNotification(id, title, body) ?? null
}

function closeNotification(id: string) {
Expand All @@ -46,6 +35,25 @@ function closeNotification(id: string) {
return nativeModule.closeNotification(id)
}

type DesktopNotificationEvent = 'click'

const shownNotifications = new QuickLRU<string, DesktopNotification>({
maxSize: 200,
})

function onNotificationEvent(event: DesktopNotificationEvent, id: string) {
const notification = shownNotifications.get(id)
if (notification === undefined) {
// TODO: handle notifications that are not in the cache
return
}

if (event === 'click') {
Copy link
Member

Choose a reason for hiding this comment

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

So we can't even be sure that the notification is dismissed if it's clicked? Can it still live on in the notification center?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can, yeah! Before this PR, I had some SEGFAULT crashes trying to remove the notification and its callback from memory right after the click event. Then I forgot to do that. I'll tackle that in a different PR along with other minor improvements I want to do.

notification.onclick?.()
}
// TODO: handle other events?
}

export class DesktopNotification {
public onclick?: () => void
private readonly id: string
Expand All @@ -55,14 +63,12 @@ export class DesktopNotification {
}

public show() {
showNotification(this.id, this.title, this.body, (event: string) => {
if (event === 'click') {
this.onclick?.()
}
})
shownNotifications.set(this.id, this)
showNotification(this.id, this.title, this.body)
}

public close() {
shownNotifications.delete(this.id)
closeNotification(this.id)
}
}
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
"@types/uuid": "^8.3.4",
"benchmark": "^2.1.4",
"jest": "^26.4.2",
"node-abi": "^2.21.0",
"prebuild": "^11.0.2",
"prettier": "^2.0.5",
"ts-node": "^9.0.0",
Expand All @@ -48,6 +47,7 @@
"dependencies": {
"node-addon-api": "^4.3.0",
"prebuild-install": "^7.0.1",
"quick-lru": "^3.0.0",
"uuid": "^8.3.2"
},
"binary": {
Expand Down
141 changes: 39 additions & 102 deletions src/DesktopNotification.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,96 +9,23 @@
#include <assert.h>
#include <wrl\wrappers\corewrappers.h>

const std::wstring kLaunchAttribute = L"launch";
const std::wstring kActionAttribute = L"action";
const std::wstring kNotificationIDAttribute = L"notificationId";

using namespace ABI::Windows::UI::Notifications;
using namespace Windows::Foundation;
using namespace Wrappers;

DesktopNotification::DesktopNotification(const std::wstring &id,
const std::wstring &title, const std::wstring &body,
const Napi::Function &callback)
: m_ref(0),
m_title(title),
const std::wstring &appID,
const std::wstring &title,
const std::wstring &body)
: m_title(title),
m_body(body),
m_callback(Napi::ThreadSafeFunction::New(callback.Env(), callback, "Notification Callback", 0, 1)),
m_appID(appID),
m_id(id)
{
PWSTR appID;
Copy link
Member Author

Choose a reason for hiding this comment

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

The deleted code here has been moved to DesktopNotificationsManager.cpp

HRESULT hr = GetCurrentProcessExplicitAppUserModelID(&appID);
if (!SUCCEEDED(hr))
{
DN_LOG_ERROR(L"Couldn't retrieve AUMID");
return;
}
else
{
m_appID = std::wstring(appID);
CoTaskMemFree(appID);
}
}

DesktopNotification::~DesktopNotification()
{
m_callback.Release();
}

// DesktopToastActivatedEventHandler
IFACEMETHODIMP DesktopNotification::Invoke(_In_ IToastNotification * /*sender*/,
_In_ IInspectable *args)
{
IToastActivatedEventArgs *buttonReply = nullptr;
args->QueryInterface(&buttonReply);
if (buttonReply == nullptr)
{
DN_LOG_ERROR(L"args is not a IToastActivatedEventArgs");
return S_OK;
}

invokeJSCallback("click");

return S_OK;
}

// DesktopToastDismissedEventHandler
IFACEMETHODIMP DesktopNotification::Invoke(_In_ IToastNotification * /* sender */,
_In_ IToastDismissedEventArgs *e)
{
ToastDismissalReason tdr;
HRESULT hr = e->get_Reason(&tdr);
if (SUCCEEDED(hr))
{
switch (tdr)
{
case ToastDismissalReason_ApplicationHidden:
invokeJSCallback("hidden");
break;
case ToastDismissalReason_UserCanceled:
invokeJSCallback("dismissed");
break;
case ToastDismissalReason_TimedOut:
invokeJSCallback("timedout");
break;
}
}
return S_OK;
}

// DesktopToastFailedEventHandler
IFACEMETHODIMP DesktopNotification::Invoke(_In_ IToastNotification * /* sender */,
_In_ IToastFailedEventArgs * /* e */)
{
DN_LOG_ERROR(L"The toast encountered an error.");
invokeJSCallback("error");
return S_OK;
}

void DesktopNotification::invokeJSCallback(std::string eventName)
{
auto cb = [eventName](Napi::Env env, Napi::Function jsCallback)
{
jsCallback.Call({Napi::String::New(env, eventName)});
};

m_callback.BlockingCall(cb);
}

// Set the values of each of the text nodes
Expand All @@ -116,29 +43,18 @@ HRESULT DesktopNotification::setTextValues()
return setNodeValueString(HStringReference(m_body.c_str()).Get(), textNode.Get());
}

HRESULT DesktopNotification::startListeningEvents()
HRESULT DesktopNotification::startListeningEvents(DesktopNotificationsManager *desktopNotificationsManager)
{
ComPtr<IToastNotification> toast = m_notification;

// Register the event handlers
DN_RETURN_ON_ERROR(toast->add_Activated(this, &m_activatedToken));
DN_RETURN_ON_ERROR(toast->add_Dismissed(this, &m_dismissedToken));
DN_RETURN_ON_ERROR(toast->add_Failed(this, &m_failedToken));
DN_RETURN_ON_ERROR(toast->add_Activated(desktopNotificationsManager, &m_activatedToken));
DN_RETURN_ON_ERROR(toast->add_Dismissed(desktopNotificationsManager, &m_dismissedToken));
DN_RETURN_ON_ERROR(toast->add_Failed(desktopNotificationsManager, &m_failedToken));

return S_OK;
}

void DesktopNotification::stopListeningEvents()
{
if (m_notification)
{
ComPtr<IToastNotification> toast = m_notification;
toast->remove_Activated(m_activatedToken);
toast->remove_Dismissed(m_dismissedToken);
toast->remove_Failed(m_failedToken);
}
}

HRESULT DesktopNotification::setNodeValueString(const HSTRING &inputString, IXmlNode *node)
{
ComPtr<IXmlText> inputText;
Expand Down Expand Up @@ -201,14 +117,15 @@ std::wstring DesktopNotification::formatAction(
const std::vector<std::pair<std::wstring, std::wstring>> &extraData = {}) const
{
std::vector<std::pair<std::wstring, std::wstring>> data = {
{L"action", action},
{L"notificationId", m_id}};
{kActionAttribute, action},
{kNotificationIDAttribute, m_id}};
data.insert(data.end(), extraData.cbegin(), extraData.cend());
return Utils::formatData(data);
}

// Create and display the toast
HRESULT DesktopNotification::createToast(ComPtr<IToastNotificationManagerStatics> toastManager)
HRESULT DesktopNotification::createToast(ComPtr<IToastNotificationManagerStatics> toastManager,
DesktopNotificationsManager *desktopNotificationsManager)
{
DN_RETURN_ON_ERROR(toastManager->GetTemplateContent(
ToastTemplateType_ToastImageAndText02, &m_toastXml));
Expand All @@ -222,7 +139,7 @@ HRESULT DesktopNotification::createToast(ComPtr<IToastNotificationManagerStatics
DN_RETURN_ON_ERROR(root->get_Attributes(&rootAttributes));

const auto data = formatAction(L"clicked");
DN_RETURN_ON_ERROR(addAttribute(L"launch", rootAttributes.Get(), data));
DN_RETURN_ON_ERROR(addAttribute(kLaunchAttribute, rootAttributes.Get(), data));
DN_RETURN_ON_ERROR(setTextValues());

// printXML();
Expand Down Expand Up @@ -252,7 +169,7 @@ HRESULT DesktopNotification::createToast(ComPtr<IToastNotificationManagerStatics
switch (setting)
{
case NotificationSetting_Enabled:
DN_RETURN_ON_ERROR(startListeningEvents());
DN_RETURN_ON_ERROR(startListeningEvents(desktopNotificationsManager));
break;
case NotificationSetting_DisabledForApplication:
error = L"DisabledForApplication";
Expand All @@ -277,3 +194,23 @@ HRESULT DesktopNotification::createToast(ComPtr<IToastNotificationManagerStatics
}
return m_notifier->Show(m_notification.Get());
}

std::string DesktopNotification::getNotificationIDFromToast(IToastNotification *toast)
{
IXmlDocument *xmlDoc = nullptr;
toast->get_Content(&xmlDoc);
if (xmlDoc == nullptr)
{
DN_LOG_ERROR(L"Could not get xml document from toast");
return "";
}

// Get "launch" attribute and split it to obtain the notification ID
HSTRING launchArgs;
IXmlElement *rootElement = nullptr;
xmlDoc->get_DocumentElement(&rootElement);
rootElement->GetAttribute(HStringReference(kLaunchAttribute.c_str()).Get(), &launchArgs);
std::wstring launchData = WindowsGetStringRawBuffer(launchArgs, nullptr);
const auto launchDataMap = Utils::splitData(launchData);
return Utils::wideCharToUTF8(launchDataMap.at(kNotificationIDAttribute));
}
Loading