Migrate alerter to "User Notifications" framework #114
Conversation
| @@ -42,7 +42,7 @@ packages: | |||
| name: async | |||
| url: "https://pub.dartlang.org" | |||
| source: hosted | |||
| version: "2.8.2" | |||
| version: "2.8.1" | |||
There was a problem hiding this comment.
Were these supposed to go back a version?
There was a problem hiding this comment.
Good catch! I am not sure why my flutter decided to roll those versions back...
| if(options[@"closeLabel"]){ | ||
| userNotification.otherButtonTitle = options[@"closeLabel"]; | ||
| if (options[@"groupID"] && [self notificationWithGroupIdExists:options[@"groupID"]]) { | ||
| NSLog(@"Sending notification with the same groupID as existing ones is not supported."); |
There was a problem hiding this comment.
Can you say more about this switch from removing old notifications with a matching GroupID to exiting ? Is it just not implemented or is there another issue as well that makes it unsupported?
There was a problem hiding this comment.
We still support removing old notifications with a matching GroupID, implemented at line 154-157. What we don't support here was sending a notification with the same groupID as an existing one.
In the old alerter, when sending a notification with the same groupID as an existing one, the existing one will be removed before the new one get displayed. However, for us it will make monitoring notification removal subject to race conditions (concurrent editing of an object tracking notifications and removals). A careful design with some locks might eliminate these race conditions, but currently we don't actually need this feature.
Note that if we don't use a tracking object to track notifications and removals, but instead periodically check whether the corresponding notification is still present, as what is done in the old alerter, then the above race conditions could be eliminated. However, that will potentially cause new race conditions since it is unknown whether the notification action callback will be called before the notification disappearance being detected.
There was a problem hiding this comment.
I just realized I could maybe combine the tracking object with periodically checking whether the corresponding notification is still present. So when the notification disappears, the detector will know whether it was caused by removal.
There was a problem hiding this comment.
The case I was thinking of was two triggers within a Data Collector that overlapped, for example a Schedule and an Event Trigger. We would want to ensure that the latest one wins. I think. So, would that be the case for removal of an existing for the same Group ID?
There was a problem hiding this comment.
During my trying (to enable sending notifications with the same groupID), I sometimes encounter an issue: when I consecutively and quickly send multiple notifications with the same groupID and quickly click the notification of the last one, the alerter will hang... So for now let's just keep it as is.
There was a problem hiding this comment.
Is this ready for checkin? Or are you doing other work on it still?
There was a problem hiding this comment.
Sorry for the delayed updates. It is not ready yet. I was still working on the bug we discovered this morning.
|
Thanks for the update.
…Sent from my iPhone
On Dec 14, 2021, at 10:51 PM, rundong08 ***@***.***> wrote:
@rundong08 commented on this pull request.
In third_party/alerter/alerter/AppDelegate.m:
> - userNotification.hasActionButton = 1 ;
- }
- }else{
- userNotification.actionButtonTitle = options[@"actions"];
- }
- }else if (options[@"reply"]) {
- [userNotification ***@***.*** forKey:@"_showsButtons"];
- userNotification.hasReplyButton = 1;
- userNotification.responsePlaceholder = options[@"reply"];
- }
-
- // Close button
- if(options[@"closeLabel"]){
- userNotification.otherButtonTitle = options[@"closeLabel"];
+ if (options[@"groupID"] && [self notificationWithGroupIdExists:options[@"groupID"]]) {
+ NSLog(@"Sending notification with the same groupID as existing ones is not supported.");
Sorry for the delayed updates. It is not ready yet. I was still working on the bug we discovered this morning.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
|
Now the code are there, just need to add proper copyright notice and license text before merging |
Fixes #113
Known issues: