feat: full support for persistent HTML5 notifications #34671
Conversation
|
💖 Thanks for opening this pull request! 💖 We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix. Examples of commit messages with semantic prefixes:
Things that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
|
Hello Electron team can You please share more details about problems with build I have fully compiled local repo and do not understand fail from build report |
YuriL180821
changed the title
|
@YuriL180821 you would need to scroll up a number of lines to see the error: ../../electron/shell/browser/notifications/win/windows_toast_notification.cc(1291,51): error: comparison of integers of different signs: 'int' and 'ULONG' (aka 'unsigned long') [-Werror,-Wsign-compare]
<< std::wstring((&item - data + 1 == dataCount) ? L"" : L",");
~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~
1 error generated. |
codebytere
changed the title
codebytere
changed the title
There was a problem hiding this comment.
As far as I know for the COM server to receive events from Action Center, there must be a start menu shortcut of the app with AppUserModelID and ToastActivatorCLSID properties assigned to it, can you write the requirements in the docs?
Also this PR is really long and complicated, it would take several turns of reviews and a long time to get it merged.
| @@ -48,5 +48,18 @@ | |||
| NotificationDelegate* delegate) { | |||
| return new CocoaNotification(delegate, this); | |||
| } | |||
| /* | |||
There was a problem hiding this comment.
Can you remove the commented code?
There was a problem hiding this comment.
Hello @zcbenz regarding to this:
As far as I know for the COM server to receive events from Action Center, there must be a start menu shortcut of the app with AppUserModelID and ToastActivatorCLSID properties assigned to it, can you write the requirements in the docs? - You are sharing well known and widely used variant from Microsoft with usage of Shortcut and static INotificationActivationCallback . Unfortunately this variant in is not good solution because bonds us into only one activator existing into the system
In general my words above looks next
//definition of the Notification activator class DECLSPEC_UUID(COM_GUID) // Static activator with PREDEFINED UUID NotificationActivator WrlSealed : public RuntimeClass < RuntimeClassFlags<ClassicCom>, INotificationActivationCallback > WrlFinal
`....
//Somewhere into the code of InstallShortcut(...) function
//Shortcut installation stage
PROPVARIANT propVar;
propVar.vt = VT_LPWSTR;
propVar.pwszVal = const_cast<PWSTR>(AppId.c_str()); // for _In_ scenarios, we don't need a copy
hr = propertyStore->SetValue(PKEY_AppUserModel_ID, propVar);
if (SUCCEEDED(hr))
{
propVar.vt = VT_CLSID;
propVar.puuid = const_cast<CLSID*>(&/*activatorID/*/__uuidof(NotificationActivator));
hr = propertyStore->SetValue(PKEY_AppUserModel_ToastActivatorCLSID, propVar);
if (SUCCEEDED(hr))
{`
Mentioned COM_GUID should be defined only before linking, after compilation App cannot change this Activator. As result possibility to have numerous apps with different activators(this is necessary for Cold-start) is absent. As result it is not possible to have few Teams/Skypes e.t.c without recompilation their backend with purpose to have different GUID. Just imagine to have as much as we want Skypes / Teams or any other messengers which work with own isolated Activators. Variant into this pull request does not use Shortcut at all and uses dynamic COM server creation. With this approach developer is able to define UUID for COM server into runtime(!)
this is part of test JS code which uses such possibilities
const { app } = require('electron'); app.notificationsComServerCLSID = '{00000001-0002-0003-0405-060708090001}';//any valid value
So even with same package developer can create as much different JS instances as he wants and all instances will have their own COM server without any collisions with usage of mentioned by You static Custom activator. And for this we should not have numerous shortcuts and make numerous recompilations of backend with purpose to setup new unique COM_GUID.
Dynamic COM server - is unlimited future for notifications and messengers on Win
| if (notification) | ||
| notification->NotificationDisplayed(); | ||
| - (void)didDeliverNotification:(NSUserNotification*)notif { | ||
| NSLog(@"didDeliverNotification"); |
There was a problem hiding this comment.
Can you remove logging code like this?
There was a problem hiding this comment.
Sure, code base in my request will be cleaned, this is temporary issue
| void CocoaNotification::NotificationDisplayed() { | ||
| if (delegate()) | ||
| delegate()->NotificationDisplayed(); | ||
|
|
||
| this->LogAction("displayed"); | ||
| } | ||
|
|
||
| void CocoaNotification::NotificationClicked() { | ||
| // SAP-14036 upgrade for persistent notifications support |
There was a problem hiding this comment.
There are lots of SAP-xxxx prefix in the comments, can you remove them?
There was a problem hiding this comment.
Yes it was internal markup for issues in Jira of my company, will do adaptation of comments
| std::wstring key_path = | ||
| LR"(SOFTWARE\Classes\CLSID\)" + clsid + LR"(\LocalServer32)"; | ||
| std::wstring launchStr = L"\"" + app_path_ + L"\" " + clsid; | ||
| if (!SetRegistryKeyValue(HKEY_CURRENT_USER, key_path, L"", launchStr)) |
There was a problem hiding this comment.
This is going to live in registry for forever, we should document this behavior and probably provide a way to clean up it.
There was a problem hiding this comment.
This is extremely necessary thing, be sure with usage of static COM approach mentioned by You we also have changes in registry. To understand what is going on in registry in mentioned by You lines of code please look on screenshot attached
|
@RaisinTen thank You a lot. But why does I do not have such |
|
This is a huge PR that will take some time to review, but a quick procedural question for @codebytere / @YuriL180821 just from reading this Conversation tab on GitHub: This has commits that include the text |
|
@YuriL180821 All our CI run with the In order to get the rest of our CI kicked off can you please sign in to https://app.circleci.com and then push another commit to this branch. I'd like to say as well Thank You this PR looks awesome, however as @zcbenz mentioned this PR is massive and will likely take a while and a few lengthy reviews before we're happy with where we're landing. |
YuriL180821
force-pushed
the
html5-notifications
branch
2 times, most recently
from
b02b0c7
to
c113667
Compare
docs/api/app.md
Outdated
| Returns: | ||
|
|
||
| * `event` Event | ||
| * `appUserModelId` string - contains App module ID |
There was a problem hiding this comment.
Will this ever not be the current apps appUserModelId, it seems like a superfluous event parameters.
There was a problem hiding this comment.
Hello @MarshallOfSound this is good remark, but I did forwarding parameters for this event according to list of those into INotificationActivationCallback::Activate https://docs.microsoft.com/en-us/windows/win32/api/notificationactivationcallback/nf-notificationactivationcallback-inotificationactivationcallback-activate
(It should to say that in this implementation I'm using it too but instead of static approach with Shortcut this activator creates dynamically into runtime not into compile time)
HRESULT Activate( [in] LPCWSTR appUserModelId, [in] LPCWSTR invokedArgs, [in] const NOTIFICATION_USER_INPUT_DATA *data, [in] ULONG count );
appUserModelId is present. In my vision this parameter potentially can be used and to distinguish notifications between different AppId in bounds of one process, it is hard to imagine this case for me now but I have feeling that this possibility can be useful (so on given stage of codebase this parameter can be marked as for future use).
I would to say - it is better to have more than less.
There was a problem hiding this comment.
I would to say - it is better to have more than less.
I understand your reasoning but I think the better thing to do would be to expose this instead as a details object with keys for data, args and then in the future if there is a use case for it exposing appUserModelId. Currently having this prop exposed means this event can only ever be used on windows
There was a problem hiding this comment.
Hello @MarshallOfSound this event is cross platform and works fine on Mac but for Mac appUserModelId holds notification identifier [notif.identifier UTF8String], that is also meaningful. Maybe it has sense to rename this parameter into identifier - in general appUserModelId plays this role.
docs/api/app.md
Outdated
| * `appUserModelId` string - contains App module ID | ||
| * `invokedArgs` string - contains arguments(in case of any) for toast button or data property in case of non-persistent notification | ||
| * `dataCount` integer - contains amount of token pairs present inputData string (not null, default value 0 - which means abcense of inputData) | ||
| * `inputData` string - valid JSON-string which contains result of User input into toast Reply fields. Empty string in case of absence of User input (i.e for case dataCount == 0) |
There was a problem hiding this comment.
Can we JSON.parse this before handing this off to the app?
Can you provide an example of what this might look like for my own reference or maybe in the docs as a commented out object so that folks can more easily visualize this object
There was a problem hiding this comment.
event is called into very critical part of App i.e in bounds of call INotificationActivationCallback::Activate I've tried to create as light as possible implementation to avoid any reduce of performance. I would propose to consider case with few hundreds notifications forwarded into module(let's assume some corner case for renotify usage), packing into JSON with third parties will take a lot of time.
Hope I understand right Your question about JSON parsing.
There was a problem hiding this comment.
The JSON parsed in //base is incredibly performant, given the array will either be empty [] or it will contain data and the user will want to run JSON.parse on it themselves I don't think the difference in performance will be noticeable / impactful in any way. We should just make the nicer API and expose the parsed structure
There was a problem hiding this comment.
I think that some misunderstanding is present
We have next
HRESULT STDMETHODCALLTYPE
Activate(_In_ LPCWSTR appUserModelId,
_In_ LPCWSTR invokedArgs,
_In_reads_(dataCount) const NOTIFICATION_USER_INPUT_DATA* data,
ULONG dataCount) override {
// packing user input structs into stream
std::wstringstream stm;
// feat: refine notification-activation for cold start
if (dataCount) {
stm << L"["; // json array open brace
std::for_each(data, data + dataCount,
[&](const NOTIFICATION_USER_INPUT_DATA& item) {
stm << item
// avoid problem with last delimeter
<< std::wstring((static_cast<ULONG>(&item - data +
1) == dataCount)
? L""
: L",");
});
stm << L"]"; // json array close brace
}
auto* app = api::App::Get();
if (app)
app->Emit("notification-activation", std::wstring(appUserModelId),
std::wstring(invokedArgs), dataCount, stm.str());
return S_OK;
}
To exclude consideration of the code base we have next scheme:
if (dataCount) -> inputData is JSON
if (dataCount == 0) this JSON array is empty
On the app module side when we have dataCount we can immediatly provide prediction should we use JSON parser or not. JSON is necessary to integrate data for pressed button and in that case JSON is extremely comfortable form of serialization.
We do not want to parse nothing data into C++ we are packing it for top level JS app module - that is idea
There was a problem hiding this comment.
Regarding to Your remarks about on first look not professional JSON forming by hands : we know format of data, we have only limited set of data variants I do not see necessity of usage some code from base instead of overridden ostream operator
namespace std {
wostream& operator<<(wostream& out, const NOTIFICATION_USER_INPUT_DATA& data) {
out << L"{"; // open brace for key-value pair
out << L"\"" << data.Key << L"\"";
out << L":"; // json delimeter (i.e colon) between key value
out << L"\"" << data.Value << L"\"";
out << L"}"; // close brace for key-value pair
return out;
}
} // namespace std
Please consider this code as extremely high level of code optimization with purpose to have maximal performance for huge sets of Notifications.
docs/api/app.md
Outdated
| * `event` Event | ||
| * `appUserModelId` string - contains App module ID | ||
| * `invokedArgs` string - contains arguments(in case of any) for toast button or data property in case of non-persistent notification | ||
| * `dataCount` integer - contains amount of token pairs present inputData string (not null, default value 0 - which means abcense of inputData) |
There was a problem hiding this comment.
Given inputData is an object with keys and values do we need dataCount? Like isn't this just identical to Object.keys(inputData).length
There was a problem hiding this comment.
Please consider my remark on the post above #34671 (comment)
if (dataCount) -> inputData is JSON
if (dataCount == 0) this JSON array is empty
If into few words: We do not want call length for empty string on top level module, we always have counter such as dataCount - in my vision it is much safe for usage into JS.
Please take into according that this code was tested during about a year by lot of QA and JS developers, al conceptual solutions are justified even if these solutions looks strange on the first look.
There was a problem hiding this comment.
We do not want call length for empty string on top level module
This is a zero cost operation on an empty object and will be optimized by V8 as such.
@YuriL180821 To be clear, all of my questions come from a place of API design and the fact that we as maintainers will have to support this code moving forward. IMO dataCount should not be here, it is a side-effect of cpp implementation not something that should be exposed to app developers.
all conceptual solutions are justified even if these solutions looks strange on the first look.
This is for us to determine during this review, what worked for y'all (in I assume your fork of Electron) may not work for all apps, or generically for Electron. That's the purpose of this review 😄 The goal is find comfortable middle ground between the code we want to see / support and the implementation you have right now.
There was a problem hiding this comment.
Hello @MarshallOfSound I'm fully agree with purpose of review but from my side I'm trying to prove advisability of all conceptual decisions present in current implementation. Also I'm trying to avoid some earlier optimization which can be cause of dramatic lose of functionality.
Is it possible to ask You share new high level design for inputData and dataCount in pseudo code which You are see?
docs/api/app.md
Outdated
|
|
||
| A `string` which contains fully formed CLSID for notifications COM server. | ||
| This is necessary only for apps with persistent notifications support. | ||
| The current default value for this property is `empty` (not `nil`). It means that by the default no persistent notifications support is exist. |
There was a problem hiding this comment.
Does this mean the default value it the literal string empty?
There was a problem hiding this comment.
This property is binded with ElectronBrowserClient::notifications_com_server_clsid_ in case if it is empty it means the we do not want persistent notifications. Think it is obviously that in case of assigning nullptr to std::string we will receive UB potentially with app crash. That is why valid variants of usage that property are bellow:
app.notificationsComServerCLSID = '{00000001-0002-0003-0405-060708090001}';
app.notificationsComServerCLSID;
//just do not call
Beware: variant app.notificationsComServerCLSID = nil; is restricted and not allowed
docs/api/app.md
Outdated
|
|
||
| ### `app.notificationsComServerCLSID` _Windows_ | ||
|
|
||
| A `string` which contains fully formed CLSID for notifications COM server. |
There was a problem hiding this comment.
Can we provide a link for where how to generate a CLSID, something like https://www.guidgenerator.com/online-guid-generator.aspx with Uppercase + Brackets + Hyphens checked is a reasonable way to do it.
Or just documenting it's an upper case uuid with curly braces
There was a problem hiding this comment.
In general case we need to have valid UUID string for WinAPI CLSIDFromString call.
See more details here
https://docs.microsoft.com/en-us/windows/win32/api/combaseapi/nf-combaseapi-clsidfromstring
Code which works with UUID is here
windows_toast_notification.cc:226
if (!notificationsCOMServerCLSID.empty() && SUCCEEDED( CLSIDFromString(notificationsCOMServerCLSID.c_str(), &clsid))) { // feat: Support COM activation registration at runtime DWORD registration{}; // Register callback if (FAILED(CoRegisterClassObject(
| // Code bellow is diagnostics and will be removed after XPC will be fully | ||
| // tested | ||
| [[[delegate_ connection] remoteObjectProxy] | ||
| upperCaseString:notification.title | ||
| withReply:^(NSString* aString) { | ||
| // We have received a response. Update our text field, but do it | ||
| // on the main thread. | ||
| NSLog(@"Result string was: %@", aString); | ||
| }]; |
| NSString* bundleIdentifier = [[NSBundle mainBundle] bundleIdentifier]; | ||
| bundleIdentifier = | ||
| [bundleIdentifier stringByAppendingString:@".AlertService"]; | ||
| NSLog(@"bundleIdentifier : %@", bundleIdentifier); | ||
| NSXPCConnection* connectionToService_ = | ||
| [[[NSXPCConnection alloc] initWithServiceName:bundleIdentifier] retain]; | ||
| connectionToService_.remoteObjectInterface = [NSXPCInterface | ||
| interfaceWithProtocol:@protocol(NSAlertDeliveryXPCProtocol)]; | ||
| connectionToService_.exportedInterface = [NSXPCInterface | ||
| interfaceWithProtocol:@protocol(NSAlertResponceXPCProtocol)]; | ||
| connectionToService_.exportedObject = self; | ||
| [connectionToService_ resume]; | ||
| NSLog(@"connectionToService_ : %@", connectionToService_); | ||
| NSLog(@"connectionToService_.remoteObjectProxy : %@", | ||
| connectionToService_.remoteObjectProxy); | ||
| NSLog(@"connectionToService_.exportedObject : %@", | ||
| connectionToService_.exportedObject); | ||
| connection_.reset(connectionToService_); |
| const char* key = [_replyKey UTF8String]; | ||
| stm << L"["; // json array open brace | ||
| stm << L"{"; // open brace for key-value pair | ||
| stm << L"\"" << (key ? key : "") << L"\""; | ||
| stm << L":"; // json delimeter (i.e colon) between key value | ||
| stm << L"\"" << reply << L"\""; | ||
| stm << L"}"; // close brace for key-value pair | ||
| stm << L"]"; // json array close brace |
There was a problem hiding this comment.
Same here, avoid manual JSON creation, prefer direct object passing or at least safer JSON creation
There was a problem hiding this comment.
Please consider my explanations above #34671 (comment)
Things with performance are playing significant role in case of hundred's or thousands notifications in few seconds
There was a problem hiding this comment.
As a middle ground, we might consider using Chromium's JSON writer given that it's much more thoroughly tested and used and thus safer cc @MarshallOfSound
There was a problem hiding this comment.
Things with performance are playing significant role in case of hundred's or thousands notifications in few seconds
I can't imagine a valid usecase for making thousands of notifications in a few seconds. But even if there was the cost of constructing objects / using a real JSON writer wouldn't be impactful enough to cause perf issues IMO
There was a problem hiding this comment.
Hello @MarshallOfSound regarding to Your words using a real JSON writer - to be honest I do not prefer +1 dependency in case when it can be done into more simple way (the motto of current implementation is : better simpler than clever)
I can't imagine a valid usecase for making thousands of notifications in a few seconds - this scenario can be real especially in case of usage renotify. I would propose to imagine notification which present for specific trading chart behavior. Modern stock exchanges can be too volatile regarding to market data feed. What if notification is used as signal to trader?
| didDeliverNotification:(NSUserNotification*)notification { | ||
| [self didDeliverNotification:notification]; | ||
| // Clear pending notification | ||
| presenter_->CloseNotificationWithId("![DELETE]"); |
There was a problem hiding this comment.
What is the significance of ![DELETE] here?
There was a problem hiding this comment.
Just internal token which widely used in current implementation to mark notification ID which is not exist or not reachable. In current implementation call of the CloseNotificationWithId("![DELETE]") it is used for potential clearing notification in in_pending_deletion_ state. In it's turn this strange on the first look solution is used to implement support renotify property. Potentially we can move hardcoded literal into some file with general definitions - for this variant You have my full agreement.
PS: Also I've used such hardcoded literal and into body of
PlatformNotificationService::GetDisplayedNotifications(
DisplayedNotificationsCallback callback)
|
|
||
| auto notification = presenter->CreateNotification(delegate, notification_id); | ||
| if (notification) { | ||
| content::WebContents* web_ctx = |
There was a problem hiding this comment.
This seems very insecure, we're getting (as far as I can tell and commented above) a completely random process ID, getting a webcontents and then asking "does this webcontents have permission to show notifications" rather than actually checking for the origin / service_worker_scope?
There was a problem hiding this comment.
Please take into according that non-persistent notifications are rather difficult as persistent
//non-persistent
void PlatformNotificationService::DisplayNotification(
content::RenderFrameHost* render_frame_host,
//pesistent
void PlatformNotificationService::DisplayPersistentNotification(
const std::string& notification_id,
const GURL& service_worker_scope,
const GURL& origin
If into few words: We does not have content::RenderFrameHost render_frame_host*, for persistent notifications, only one way to find necessary content::WebContents is approach given into DisplayPersistentNotification
Be sure - this method browser_client_->GetRenderFrameProcessID() returns only necessary process (I would propose to follow all call stack to understand this idea).
I would ask sorry, but I did not get Your idea with origin, based on my investigations this parameter is useless in our case and does not help.(It is only my vision). So I made implementation which stable based on my investigations and understanding for how it works with mojom and upper on call stack
ADD: Also I can say more - this thing content::RenderFrameHost render_frame_host, (I saw some patch to add this pointer) can be replaced on approach with finding RenderFrameProcessID and than necessary WebContents, i.e we can exclude adding content::RenderFrameHost render_frame_host, at all and have native not patched implementation.
There was a problem hiding this comment.
The association here is to the wrong RenderFrameHost. The correct thing to do here is to refactor our approach to creating PlatformNotificationService.
Currently the PlatformNotificationService class is a singleton, we should instead create one PlatformNotificationService per BrowserContext inside ElectronBrowserContext::GetPlatformNotificationService. This allows the notification service to store a reference to the browser context that constructed it. Then we use that to dispatch permission requests and make the breaking change to not provide webContents with those WebNotificationAllowed checks (this is already allowed by the API, just pass nullptr instead of WebContents*.
Then we can remove all this proc_id code and logic that tries to infer BrowserContext from rfh or from proc_id
There was a problem hiding this comment.
Hello @MarshallOfSound in my vision passing nullptr will lead to call
void ElectronBrowserClient::WebNotificationAllowed(
content::WebContents* web_contents,
base::OnceCallback<void(bool, bool)> callback) {
if (!web_contents) {
std::move(callback).Run(false, false);
return;
}
that in it's turn has predefined usage of false. On given moment I do not agree with such variant, in my vision it will broke existing functionality.
Refactoring for this critical part of code should be done only after full testing of current HTML5 module to be sure that we do not lose anything after changes. By these words I want to prevent potential quick refactoring for solution concept which can be cause of lose functionality. The most important do not lose full HTML5 support.
sometimes earlier optimization - is evil.
|
Hello @MarshallOfSound and Electron community, I would propose to use this JS module 2022-06-22-00-25-02.mp4Usage(Win) : Also app folder can be used as valid app folder mentioned into tutorial for Electron distribution |
YuriL180821
force-pushed
the
html5-notifications
branch
from
789aa30
to
d4daf29
Compare
docs/api/app.md
Outdated
| * `dataCount` integer - contains amount of token pairs present inputData string (not null, default value 0 - which means abcense of inputData) | ||
| * `inputData` string - valid JSON-string which contains result of User input into toast Reply fields. Empty string in case of absence of User input (i.e for case dataCount == 0) | ||
|
|
||
| Emitted after each completed user choise on notification. |
There was a problem hiding this comment.
| Emitted after each completed user choise on notification. | |
| Emitted after each completed user choice on notification. |
There was a problem hiding this comment.
Thank You for pointing this typo mistake(unfortunately I'm not native English speaker).
I will perform final review when be ready for push.
docs/api/app.md
Outdated
| This is necessary only for apps with persistent notifications support. | ||
| The current default value for this property is `empty` (not `nil`). It means that by the default no persistent notifications support is exist. | ||
|
|
||
| Changing for property value should be provided earlier than `ready` event triggered. Preferably changing for `app.notificationsComServerCLSID` is need to be provided into `will-finish-launching` event handler. |
There was a problem hiding this comment.
As someone not too familiar with persistent notifications, it's unclear how these added APIs can be used together to add support for them.
We have a page in the docs for notifications which might be a good place to add an entry for persistent notifications.
https://www.electronjs.org/docs/latest/tutorial/notifications
https://github.com/electron/electron/blob/main/docs/tutorial/notifications.md
Maybe adding a brief explainer on what, why, and how to use persistent notifications on supported platforms would be helpful.
There was a problem hiding this comment.
Hello @samuelmaddock if briefly Electron does not have support of persistent notifications at all. It means that there is no possibility to create toast with reply field or permanent toast(known as Alert) especially for MacOS, as I promised about month ago here is short promo with possibilities of newly API on MacOS.
During tests I've used same app entry point as into my previous demonstration
https://github.com/electron/electron/files/8952901/app.zip
I think it is obviously that possibility of replies, buttons, handling clicks on Alert extremely exceeds that what we have now into electron. Without such API JS dev just not able to create real Alert and interact with it. And more - current Electron realisation does not allow to have notifications with type Banner and Alert at the same time(I mean that Electron does not have double registration into notifications center as has Chrome)
Unfortunately since my last update I see new big stash from Community regarding to non-persistent notifications which I have to adapt to have chance for push my changes. I will try to make mentioned changes ASAP but have my own tasks which are out of this topic.
Screen.Recording.2022-09-17.at.18.43.02.mp4
YuriL180821
force-pushed
the
html5-notifications
branch
from
4e483b6
to
f62aab7
Compare
Description of Change
1.Full support persistent notifications
2. Fix for the #13041
3. Full support HTML5 notifications functionality according to spec https://developer.mozilla.org/en-US/docs/Web/API/Notifications_API including support for newly added properties:
Checklist
npm testpassesRelease Notes
Notes: This pull request introduced previously absent functionality for persistent notifications and brings support for such properties as