[various] Add support for notification dismissed callbacks - #2790
Conversation
There was a problem hiding this comment.
Thanks for the PR. I tested this out and it works great. I had the following questions
- There are a couple of inconsistencies with how Android and iOS behave. Android always triggers the callback via background isolate whilst on iOS, it depends on how the notification was configured. If I tweaked the example app a bit, the callback can be on the background isolate on iOS instead. Would it be possible/feasible to make use of a parameter common across Android and iOS that gives developers the option on which isolate the prefer the dismissal to fire on? This could be made opt in and replace
emitDismissEvent. I'm thinking something like an enum and is treated as a "preference" in that when specifying a background isolate is to be used, for platforms that don't support it (e.g. macOS), it'll fallback to the main isolate - If the above is possible, can you update the example app to show the additional use cases? If it's not possible then is there a way to update the example app so that there's a scenario that shows iOS firing the callback on a background isolate?
- To help avoid confusion, I would suggest the print entry used on background isolate and main isolate have the text updated around which isolate was used
Edit: the iOS error isn't due to this PR. I'll see if I can fix this up
|
Thanks for the review! I've added a dismissIsolate preference (NotificationDismissedIsolate.main / .background) to both AndroidNotificationDetails and DarwinNotificationDetails. Each value maps to a single isolate: main fires on the main isolate (only while the app is running) and background on the background isolate (works even when the app has been terminated). macOS has no background isolate, so it always reports on the main isolate. I did consider falling back from main to the background isolate when the app is terminated, but left it out: it makes the same configuration behave differently depending on app state, and it doesn't fit how the plugin already picks the isolate statically (as with notification actions). So the only fallback is the macOS one you described. The example now has two buttons (main / background), and the print states which isolate the dismissal fired on. I've verified the full main/background × running/terminated matrix on Android, iOS and macOS. |
|
Thanks for making the changes
Yeah I was really referring to macOS when I called this out so all good. I'll get back to you once I have given this a spin but one thing that came to mind is that since your changes mean specifying a preference is possible, I'm questioning if it would be better to make the default behaviour be that it notifies on main isolate without having to opt in. What's your thoughts on that? My thinking is it may allow some apps with existing scheduled notifications to handle dismissals in some form and potentially less effort for plugin consumers to get this setup. The latter assumes that most will want to do something when the app is running though I don't know how realistic that is. Another thing to note is I addressed the iOS integration test issue as part of #2793. Can you update your fork/branch? |
Resolves MaikuB#74. Adds an opt-in `emitDismissEvent` flag to `AndroidNotificationDetails` and maps iOS/macOS dismissals (for categories configured with `customDismissAction`) to a new `NotificationResponseType.notificationDismissed`. The dismissal is delivered through the existing notification response callbacks, reusing the background isolate pipeline already used for notification actions.
…missal callback Adds a `dismissIsolate` preference (`NotificationDismissedIsolate`) to the platform-specific details, replacing the boolean flag, so apps can choose whether a dismissal fires on the main or a background isolate. Where a platform can't honour the choice it degrades: macOS has no background isolate so it always reports on the main isolate, and the main isolate only fires while the app is running.
3dbe8f2 to
cf8d389
Compare
|
I've rebased the branch onto the latest master, so it now includes your iOS integration fix (#2793). On the default question: my vote is to keep it opt-in. iOS/macOS only deliver a dismissal when the category has customDismissAction, so it can't be on by default there without opting in anyway; defaulting only Android to on would bring back the Android/iOS inconsistency the preference was meant to remove. Happy to revisit if you feel differently after a spin. |
Resolves #74.
Summary
NotificationResponseType.notificationDismissed. No new callback is introduced.dismissIsolatepreference (NotificationDismissedIsolate.main/.background) onAndroidNotificationDetails.backgroundregisters a delete intent that broadcasts to the existingActionBroadcastReceiver, so dismissals go through the same background-isolate pipeline as notification actions and work even when the app has been terminated (the concern that blocked [flutter_local_notifications] Added callback for dismissing a notification #722);maindelivers to the running app instead.DarwinNotificationDetails, for categories configured with thecustomDismissActionoption. macOS has no background isolate ([macOS] Launch isolate with an entrypoint from another library? flutter/flutter#65222), so it always reports on the main isolate.Notes
timeoutAfter), so it is slightly broader than a user swipe. Tapping a notification or cancelling it programmatically does not emit it.Test plan
cd flutter_local_notifications && flutter test(added tests covering the preference being sent over the channel, its default, and a dismissed response decoding to the new type).