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.

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

Memory usage improvements #3

Merged
merged 6 commits into from
Feb 7, 2022
Merged

Memory usage improvements #3

merged 6 commits into from
Feb 7, 2022

Conversation

sergiou87
Copy link
Member

This PR makes a few changes around memory usage. But first, some context:

  • Windows doesn't have a specific event when a notification is completely dismissed vs when it's hidden away from the user into the Action Center, so we're forced to keep around the click/event callbacks for as long as we can.
  • In my initial implementation, the native side would store these callbacks for every notification.

The changes made in this PR are:

  • Unifying all the events sent by the native side to only one callback that will receive both the event and the notificationID.
  • The JS side will manage the callbacks with the consumer app. The less we do on native side, the less we need to port to other platforms, if we ever do that 😄
  • On this JS side, we will use a LRU cache to keep the number of in-memory notifications/callbacks under control.

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

@@ -2,109 +2,29 @@
#include "DesktopNotificationsManager.h"
#include <iostream>

typedef ABI::Windows::Foundation::ITypedEventHandler<
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.h

{
m_appID = std::wstring(appID);
CoTaskMemFree(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.

This block comes from DesktopNotification.cpp

return nativeHistory;
}
return {};
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This was literally moved here from the top of the file, no changes.

@@ -193,3 +212,86 @@ bool DesktopNotificationsManager::closeNotification(ComPtr<DesktopNotification>
DN_LOG_ERROR("Notification " << d->getID() << " does not exist");
return false;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This whole 🟢 block was brought from DesktopNotification.cpp, and then I added some changes to obtain the notification ID and send it with the callback.


return E_NOINTERFACE;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This block comes from DesktopNotification.h

@Aminadad27

This comment was marked as spam.

Copy link
Member

@niik niik left a comment

Choose a reason for hiding this comment

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

Two suggestions, neither blocking. I understand maybe 10% of the cpp code in here but I cam away with a renewed appreciation of having you on my team 😄

lib/desktop-notifications.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
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.

sergiou87 and others added 2 commits February 7, 2022 11:04
Co-authored-by: Markus Olsson <j.markus.olsson@gmail.com>
@sergiou87 sergiou87 merged commit 722c276 into main Feb 7, 2022
@sergiou87 sergiou87 deleted the mem-usage-improvements branch February 7, 2022 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants