-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Implement support for notification actions #338
Conversation
Won't get to look at this properly yet but wanted to bring a couple of this to your attention
|
c73f21b
to
260a48e
Compare
Thanks. Both should be fixed now. |
@MaikuB I've read a few of the issue threads and understand your stance on waiting for generic support for actions. I haven't been able to thoroughly read through the code on this commit properly, but is this something that has the potential for being merged in to main, or should I (and others like me) fork this branch as a best alternative for the time being? |
@TannerYoung it may be potentially merged in but needs to be looked at in more detail so I would suggest you fork and use it. It's been a while so unless I've forgotten, the only thing that was of more concern to me was more around being able to make sure the logic tied to the action buttons can be run even if the app isn't running. Based on the code I've seen so far, this is make use of the APIs for headless execution so should be fine on that front. Will need to test it out though and see what scenarios might be missing. It would be great if you could assist in testing out the changes and provide feedback. I've had other things that required attention lately and there's a long weekend coming up (note: this may gave me some time to look at this but in the middle of something else atm that I'd like to finish if possible). Your patience and help would be greatly appreciated |
"Being able to make sure the logic tied to the action buttons can be run even if the app isn't running" <-- yes, this is definitely the hard part! As you can probably already infer, the approach I took is that client code can react immediately to actions if it the app is running (can be in the background); but if it's not, they get queued up until next start. This is good enough for my use case (imagine the action being something like "mark this thing as done"). I don't know of any Flutter interface that would allow the client code to execute arbitrary logic in response to actions even when the app is down, without significantly complicating the interface. |
I had a suspicion because I saw that it didn't because headless execution generally only works with top-level functions but then I saw that there was code to makes use of the related APIs. Having taken a look at that now (e.g. in the receiver), looks like those APIs are used to make sure the actions are queued up so they're executed the next time |
Yes, all correct. I think it's a good tradeoff because:
|
Found a minor regression: AndroidNotificationDetails now requires that the 'icon' field be set for due to FlutterLocalNotificationsPlugin.java:159 where it now calls getDrawableResourceId(context, notificationDetails.icon). It should instead check if there is an icon field, and if not use the defaultIcon. |
Co-authored-by: Jakub Trzebiatowski <[email protected]>
@TannerYoung thanks for spotting! Should be fixed now, please try again. (FWIW arguably it wasn't a regression, it only triggered if you used actions.) |
Alright, I think I'm a bit lost as to what's supposed to be happening now. I've set up a It's possible that this is because I am running the plugin from a separate isolate (a non ui isolate). Thoughts on how to diagnose / fix this? |
@noinskit thanks for clarifying. In that case I'm going to wait for a generic solution that allows for logic to be executed whilst the app isn't running. Whilst what's in the PR may have some use cases, I don't think it covers enough to merge it in. When headless execution gets refined more that a generic solution can be worked on, it potentially requires coming with a way to migrate code (particularly for scheduled notifications) that makes use of the changes in this PR across. |
@TannerYoung this message ( |
@MaikuB I understand, this code can live in a fork in the meantime. I'm a bit sad that this won't be supported in the main plugin for what seems to be a long time, though. Is there actually any commitment at all from the flutter team to implement what you need (ideally with an ETA)? |
There are issues on the main repo around this that I have raised. Cant say much around the commitment side though there have been replies on the issues. The community should vote if they feel it's important as the team does look those. The meta-bug collecting some of this is tracked in flutter/flutter#32164 |
Thanks for the pointer to android manifest! I've got two more bugs:
|
Closing this PR since it won't be merged in. This may affect seeing updates being added so you may need to monitor the fork |
@TannerYoung (1) - sorry for that. It was a simple bug and should now be fixed. Please try again. (You need to get it from https://github.com/noinskit/flutter_local_notifications/tree/actions as this PR doesn't appear to be getting updates anymore.) As for (2) - I don't know what this could be right away. I'll dig into that. |
@TannerYoung about (2)... am I correct to assume that it's also about Android? I've got one suspicion - it might be about the Firebase plugin not liking to be initialized multiple times.
Could you please try changing the latter to be more specific:
? |
Yeah, working with Android, and thanks for your help I'm rather new to the space.
With that above I get the slightly worrying log error:
But the multiple notifications work! Thanks for your help :D |
I'm not sure I fully understand the above solution - if you have the time I'd very much appreciate an explanation of why that worked. |
@TannerYoung I see, so the firebase plugin is also registering dependencies. Could you please try something similar to (warning: not tested):
I hope it makes sense - this way we wouldn't change the callback passed to FlutterFirebaseMessagingService, but we'd narrow down what FlutterLocalNotificationsPlugin does for plugin registration. |
Oh, maybe I wasn't clear - the changes in your MyApplication.java that I'm asking for is following a hypothesis about this problem you described about how clicking on actions is breaking firebase_messaging with error about "detached view". It's still only a hypothesis, it might be wrong. The problem with multiple notifications was totally unrelated and it was a simple fix in the (forked) plugin code. |
Great - got it working and thanks for the explanation. If I encounter more issues I'll raise them on the fork repo. Thanks a ton for your work here. |
Great, I'm happy that my guess on firebase was correct! Feel free to use the fork and report issues, and let's see how the situation evolves from there. |
Hi. I am trying to use your plugin.
Then in AndroidManifest.xml: The notification is showing correctly, but when I click on an action, it throw me a fatal error:
Can anyone help me with that? |
Probably open a new issue on the fork repo https://github.com/noinskit/flutter_local_notifications.git But what mine looks like: pubspect.yaml
AndroidManifest.xml:
MyApplication.java
|
Thanks. I didn't have the MyApplication.java part. |
Thanks @TannerYoung for responding faster than I saw this! :) In the future, you're welcome to file issues directly under this fork. |
Hi. Is there a way to have one action button as "background action" (dismiss the notification and do some background action ...if I understand it right, after the next start of the app) and the second button to open the app on specific screen? Thanks. |
Co-authored-by: Jakub Trzebiatowski [email protected]