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

[flutter_local_notifications] AndroidFlutterLocalNotificationsPlugin Consume selected notification #2212

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

KirillBorodin
Copy link

Sets an extra to Android intent which indicates that selected
notification has been consumed.

The feature is a fix of the issue: #1926

@giridat
Copy link

giridat commented Jan 20, 2024

LGTM BUMP!

@MaikuB
Copy link
Owner

MaikuB commented Jan 22, 2024

Not sure if you sure this but a similar had already been raised at #2150 though your one is conveys what it does more clearly. With that in mind, I believe what I wrote #2150 (comment) applies here too. Let me know what you think about changing how your PR works to match what I mentioned there

@bitsydarel
Copy link

Not sure if you sure this but a similar had already been raised at #2150 though your one is conveys what it does more clearly. With that in mind, I believe what I wrote #2150 (comment) applies here too. Let me know what you think about changing how your PR works to match what I mentioned there

Hi @MaikuB , I believe your comment may not directly apply to this PR. We have designed the functionality to be opt-in, requiring an explicit call to consume to actually consume the notification.

If this method does not meet your expectations, we propose an alternative: instead of omitting the notification when getNotificationAppLaunchDetails is called, we plan to still return it but include a consumed flag within the notification details returned by getNotificationAppLaunchDetails. This allows the caller to decide whether to redirect the user to the screen indicated by the notification or to ignore it, recognizing that it has been marked as consumed. This modification aims to maintain consistent behavior across Android, iOS, and macOS platforms.

I understand that a similar concern was raised in issue #2150. However, this PR seeks to more clearly define the implementation. Reflecting on your insights shared in #2150 (comment), could you please provide your feedback on adjusting this PR to reflect those suggestions?

@MaikuB
Copy link
Owner

MaikuB commented Feb 22, 2024

@bitsydarel it applies to this PR as well as they both try to solve the same problem. What I'm referring to would also allow for an opt-in and the "cause" of this issue is developers have had assumptions before upon calling the getNotificationAppLaunchDetails(), a subsequent call to getNotificationAppLaunchDetails() wouldn't result in the same notification being returned with the assumption that the plugin has consumed the notification. Part of this might be because the FCM plugin does this.

As there have been these expectations before, what I'm proposing is something similar to what you're saying but it's not based on updating the response and it also retains the opt-in behaviour. If you look at what I wrote in #2150 (comment), I had described the proposal already. This was about updating the getNotificationAppLaunchDetails() to take an additional argument so that the plugin it would return and consume the notification in one go. This would be up to developers to opt-in and reduces the burden on developers using the API as there only be one API call to make compared the current approach in the PR that involves two separate API calls. If an app opted into this behaviour then wouldn't also be the need to check a consumed flag in the response as calling getNotificationAppLaunchDetails() again could just return null. Hope that is clearer now

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.

4 participants