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

WIP: [flutter_local_notifications ] Notification actions for iOS, macOS, Android #880

Merged
merged 67 commits into from
Jan 3, 2022

Conversation

ened
Copy link
Contributor

@ened ened commented Oct 29, 2020

This PR addresses #17 - and includes a very basic implementation for iOS & Android.

A click on the notification will launch (if not already running) a separate Flutter engine, which initializes a callback dispatcher.
The dispatcher function then sets up platform communication for other plugins and starts listening to a event channel for existing or additional notification click events.

Screenshots

Android:

Simple actions:

screen

Text input actions to collect user entered text:

screen

iOS:

Simulator Screen Shot - iPod touch (7th generation) - 2020-11-01 at 09 23 40

macOS:

Simple actions:

Screen Shot 2021-12-29 at 20 50 59

Text input actions to collect user entered text:

Screen Shot 2022-01-02 at 22 28 54

The PR is in early stage to gather feedback on the basic use case by others.

Feedback requested

Please support this PR by testing it in your own App and comment with missing features / use cases.

To use it, add the following to your pubspec.yaml file:

dependency_overrides:
  flutter_local_notifications:
    git:
      url: https://github.com/ened/flutter_local_notifications.git
      path: flutter_local_notifications
      ref: 17-notification-actions
  flutter_local_notifications_platform_interface:
    git:
      url: https://github.com/ened/flutter_local_notifications.git
      path: flutter_local_notifications_platform_interface
      ref: 17-notification-actions

The documentation is still very incomplete, therefore please refer to the example project of this branch.

Open tasks

@ened
Copy link
Contributor Author

ened commented Oct 29, 2020

@MaikuB what code style does this project follow? Any particular setting I could enable/import in Android Studio?

@ghenry
Copy link

ghenry commented Oct 29, 2020 via email

@MaikuB
Copy link
Owner

MaikuB commented Oct 31, 2020

@ened I've done very little of pure native Android development so don't know how style settings could be configured so admittedly there isn't a particular style used at the moment. Only thing I try to do is avoid hungarian notification, naming constants with the k prefix and try to use early returns where possible.

FYI that there were other PRs before. The most comprehensive one was probably #338 as it included iOS support.

There were also issues with background execution that I mentioned in #17 and reported on the Flutter repo. From memory, I saw discussions on the main Flutter repo that looking at background execution was on the horizon.

Note: I say pure native Android development as my previous experience doing Android development was using Xamarin

@ened
Copy link
Contributor Author

ened commented Oct 31, 2020

@MaikuB ok - thx for the link to the other PR & I will extract what is needed. The background execution support is included in Flutter, see flutter_workmanager or flutter_uploader plugins as a reference. I think we can get this sorted, but it will be a bit foreign to some developers, because the callback function runs in a separate thread. On the other hand, if you are using firebase_messaging and the onBackgroundMessage handler there, you'll feel right at home.

For the code formatting, I normally enable the "google-java-formatter" plugin in Android studio and let it do it's magic on ALL java files. That's what the Flutter team is using as well.

I can not include formatting changes in this PR though, so would you be willing to tackle this or should I submit a separate PR to reformat all of the Android java code using that plugin? Obviously it would include some notes in the README.

@MaikuB
Copy link
Owner

MaikuB commented Oct 31, 2020

@ened I know it's supported but I ran into what seemed like a rather severe issue when I tried to implement it in this plugin and within the code of one of the Flutter team member's own samples that I reported in flutter/flutter#23904. Note that I accidentally deleted the fork of the geofencing sample but it was acknowledged as an issue

With regards to formatting, happy to take guidance on this and a PR on this. I have had to format Java code for PRs I've submitted to the Flutter and Firebase plugins that I would assume use the same formatter but used the script they had created for it as I wasn't aware that there's an Android Studio plugin for it

@ghenry
Copy link

ghenry commented Oct 31, 2020

I think we can get this sorted, but it will be a bit foreign to some developers, because the callback function runs in a separate thread. On the other hand, if you are using firebase_messaging and the onBackgroundMessage handler there, you'll feel right at home.

Sorry to ask something unrelated to this thread, but the above caught my eye.

I'm stuck on background messaging and thought it is exactly something to do with threading and my .Application class and .MainActivity class. Do you have a link to something I can read more about on what you've just highlighted?

Thanks,
Gavin.

@ened ened changed the title WIP: [flutter_local_notifications ] Notification actions on Android WIP: [flutter_local_notifications ] Notification actions for iOS / Android Nov 1, 2020
@ened
Copy link
Contributor Author

ened commented Nov 1, 2020

The PR has been now been updated with a iOS implementation - from basic tests in emulator it works the same way as Android, but does require a a bit more preparation work by the user of this plugin:

  1. AppDelegate needs to register the plugin registrant callback
  2. iOS defines actions per notification category, so the developer needs to configure them during the initialize call. Internally, the categories need to be configured before the permissions are requested; which is reflected in the code.

I've updated the example project with this - but documentation is still a bit sparse; this is probably the biggest task now.

It would be great if the community could support testing this and submit feedback. Will update the PR description with details.

Also, we should define how much of the APIs available is to be implemented:

https://developer.android.com/training/notify-user/build-notification#Actions
https://developer.apple.com/documentation/usernotifications/declaring_your_actionable_notification_types

@MaikuB For the code formatting, unfortunately google/google-java-format#536 is blocking a proper solution. Will follow this up when there are news.

@ghenry I'm not sure what the problem is exactly. The example project in this fork uses the same mechanism, so I'd encourage you to test it locally. It's also a lot smaller than the firebase plugin, so should be easier to navigate around. :)

@ghenry
Copy link

ghenry commented Nov 1, 2020 via email

@ened
Copy link
Contributor Author

ened commented Nov 1, 2020

Thanks. After reading all the firebase code last night and callkeep code, I think my issue is something being run in one isolate (background message handler) and callkeep running in another isolate/activity with no access to each other and callbacks in callkeep acting on empty states as the states are in the other thread. Still researching. I may just bring the lot to the foreground :) Looking forward to playing with this code with GetX and xmpp_stone Great work! Gavin.

Transporting messages across isolates could be done in two ways:

  1. You could try registering a named ReceivePort using IsolateNameServer.
  2. You could use a plugin with a static field to keep/publish state messages between the isolates. Having it done this way would allow you to customize cache behavior when one of the isolates disappeared. This will require both isolates to have access to plugins though.

@ghenry
Copy link

ghenry commented Nov 1, 2020 via email

@Barry0501
Copy link

Woa, awesome! This is exactly what I need in this plugin

@yaakov-lightricks
Copy link

Hi, this is great! Exactly what I miss in this great plugin for long time!
If help is needed I can try help.

@postacik
Copy link

postacik commented Nov 8, 2020

Glad to see this coming finally. I'll test and report...

@postacik
Copy link

postacik commented Nov 9, 2020

I need help for using this feature.

The expected behavior of clicking an action button is similar to clicking the notification body except for an additional parameter which holds the clicked button id.

However, in your solution, a function in a separate isolate is called and even the notification is not dismissed when the action button is clicked. (I tested it on Android)

Is it not possible to trigger the onSelectNotification event when an action button is clicked?

Or is it possible to wake the application up in the backgroundHandler ?

@ened
Copy link
Contributor Author

ened commented Nov 9, 2020

Following answer is for the current implementation:

However, in your solution, a function in a separate isolate is called and even the notification is not dismissed when the action button is clicked. (I tested it on Android)

It's a good question whether the notification action should be dismissed automatically. I tend to agree it should be. Will look at implementing the code.

Is it not possible to trigger the onSelectNotification event when an action button is clicked?

Precisely because it's in a separate isolate, the onSelectNotification event may not be called.
Consider that the actions can be simple buttons like in the demo, or in the future perhaps input / message fields (like for message apps). When the notification arrives, the App may be terminated & the only part of the App handling the notification action is the background handler.

The handler has full app functionality available (plugins, etc), just not the App itself.

This is a design decision in the Android system itself, as evidenced by how notification actions are handled (in a "BroadcastReceiver" which has a separate lifecycle).

Or is it possible to wake the application up in the backgroundHandler ?

Based on above, you could use plugins like android_intent to start the main activity back.

There may be other issues by starting the activity this way, e.g. it will probably need to get a flag called "start new task", which means the App starts from 0 and you'd see the notification in a different place then onSelectNotification.

@ghost
Copy link

ghost commented Nov 10, 2020

The feature I've been waiting for so long, Is there any prediction about when it will be merged?

@ened
Copy link
Contributor Author

ened commented Nov 10, 2020

@enesoren it needs more testing.. I can resolve issues but am currently very time-constrained rgd. testing.

@MadhaviMadineni
Copy link

@ened Hi, this is great. any predictions, as to when will this get merged.

@ened
Copy link
Contributor Author

ened commented Dec 3, 2020

@MadhaviMadineni the PR needs community testing, please see the instructions on the top.

While at it, the plugin should probably also support arbitrary notification actions like text input (so that you can build a chat message app); will look at that next.

@ghost
Copy link

ghost commented Dec 3, 2020

Hi, I need help to test it. I get the following error when I make the change in pubspec:

Could not find a file named "pubspec.yaml" in https://github.com/ened/flutter_local_notifications.git 6954ace.
Running "flutter pub get" in app...
pub get failed (1; Could not find a file named "pubspec.yaml" in https://github.com/ened/flutter_local_notifications.git
6954ace.)

How can I solve this problem?

@ened
Copy link
Contributor Author

ened commented Dec 3, 2020

@enesoren ah - I have written the wrong instructions on the top, please use this instead:

dependency_overrides:
  flutter_local_notifications:
    git:
      url: https://github.com/ened/flutter_local_notifications.git
      path: flutter_local_notifications
      ref: 17-notification-actions
  flutter_local_notifications_platform_interface:
    git:
      url: https://github.com/ened/flutter_local_notifications.git
      path: flutter_local_notifications_platform_interface
      ref: 17-notification-actions

@ghost
Copy link

ghost commented Dec 3, 2020

@ened Yep, it works. I can test it now, thank you :)

@ened
Copy link
Contributor Author

ened commented Dec 31, 2021

Thanks @ened. Been a while since I used iOS. Ran into a weird problem though, the actions were appearing for me before and from the notification centre, I could swipe and select "View" to see the actions. However, after some point, I no longer see the actions. See below to see what I mean

Screen.Recording.2021-12-29.at.4.34.02.pm.mov

Any idea what might be the cause of the issue?

No idea yet - this seems to work well at the moment. I have not explored

A few thoughts upon looking at the PR and seeing what's there. Let me know what you think

  • Some applications will want actions to trigger the app to navigate to a specific page/route when

    • when the action launches the app when it's in a terminated state. From what I could see, it doesn't seem like it's possible. On iOS, I believe it should've been possible via UNNotificationActionOptionForeground. I suspect this isn't working as expected as if you check the numeric value in the documentation page I linked, it's 1 << 2 and that ends up with 4. From what I could tell, the mapping logic within the PR takes the numeric index value of the enum and adds one to it so it ends with a value of 3 instead. I suspect IOSNotificationCategoryOption may have a similar issue given bitwise operations are involved as opposed to being a straightforward integer value. Putting that issue aside, is there a way to determine which action (id) launched the app? This would possible involve changes to the logic related to getNotificationAppLaunchDetails()

Fixed!

  • Would it be possible to show how an action could trigger another plugin like the url_launcher one mentioned in the updates in the readme? I don't know what would actually be involved myself. This could be done by updating the example app

Have added a basic version for this.

  • On iOS, tapping on an action dismisses a notification by default. Although it's not the default behaviour on Android, I believe most apps would also want this behaviour on Android. Is there a way to support this? It looks like the example app tried to demonstrate this by calling the cancel() method of the plugin . This could cause an issue for applications dealing with scheduled/recurring notifications that have actions as calling cancel() would also unschedule it so to speak. Perhaps this calls for having a boolean autoCancel property on the AndroidNotificationAction that defaults to true?

OK seems reasonable - will add to the todo list.

  • Was the plan to add macOS support to this PR too? Asking as the PR introduces a number of classes/enum with the IOS prefix. iOS and macOS share the same notification APIs though so following this convention could lead to some duplication where there'd be macOS classes and enums. Perhaps replacing IOS with Apple would be better in cases like this? I may need to see if this can be done throughout other places in the existing code of the plugin first

Yes I was hoping to get macOS support in as well, at least in the basic fashion. There are some limitations outlined in the code now.

@MaikuB
Copy link
Owner

MaikuB commented Dec 31, 2021

Fixed!

Thanks I'll see if it'll cooperate again on iOS for me to try this out. Is there a way for an action to trigger the app to be launched on Android though? I'd imagine that a launch intent should be used instead but then wonder how that would play out with callbacks. Not sure if that means a separate callback is required as the current one for notification actions would be for "background" actions

@ened
Copy link
Contributor Author

ened commented Jan 2, 2022

Hi everyone subscribed. Please give this PR a try and support by reviewing code, writing tests and/or documentation. I think we are pretty close to a releasable version but I need your support. My time will be quite limited and I prefer not to keep this PR open for another year. ;-)
Thank you.

@MaikuB
Copy link
Owner

MaikuB commented Jan 3, 2022

@ened one option I can think of is to merge this in and do a pre-release. Would allow for changes to be consumed and tested more easily and reduce the need for you to deal with merge conflicts. This would also be a good basis for the plugin to have more refactoring done to create the Darwin classes for iOS and macOS. Thanks for your efforts on this

To everyone else, I know there's a lot of subscribers on this but can I get a better indication of how many having been using @ened's fork so far? There are comments with questions or those describe issue but I wonder how many of you have successfully been using it and what feedback you have. If you could perhaps upvote on this message as indicator and optionally reply with feedback then that would be much appreciated

@ened
Copy link
Contributor Author

ened commented Jan 3, 2022

@ened one option I can think of is to merge this in and do a pre-release. Would allow for changes to be consumed and tested more easily and reduce the need for you to deal with merge conflicts. This would also be a good basis for the plugin to have more refactoring done to create the Darwin classes for iOS and macOS. Thanks for your efforts on this

Yes that sounds Good. You can mark the feature as experimental somewhere in the public API. And the next major version of the plug-in could remove the flag for example.

@MaikuB
Copy link
Owner

MaikuB commented Jan 3, 2022

I'll do that then :)

@MaikuB MaikuB merged commit 8e1ad8e into MaikuB:master Jan 3, 2022
@AdamBridges
Copy link

@ened one option I can think of is to merge this in and do a pre-release. Would allow for changes to be consumed and tested more easily and reduce the need for you to deal with merge conflicts. This would also be a good basis for the plugin to have more refactoring done to create the Darwin classes for iOS and macOS. Thanks for your efforts on this

To everyone else, I know there's a lot of subscribers on this but can I get a better indication of how many having been using @ened's fork so far? There are comments with questions or those describe issue but I wonder how many of you have successfully been using it and what feedback you have. If you could perhaps upvote on this message as indicator and optionally reply with feedback then that would be much appreciated

I was using ened's fork in September 2021 but it was having compatibility issues with other plugins as this fork's dependencies were outdated then and assumed this wasn't being worked on anymore, so I copied ened's fork and updated things on my own and have been using it ever since. Anyway, I suspect it's all fixed now so I'll try this current version going forward.

@MaikuB
Copy link
Owner

MaikuB commented Jan 4, 2022

@AdamBridges thanks for the input. What kind of use cases did you have in your app and was the actions and the way they were handled in this PR sufficient to cover them?

@MaikuB
Copy link
Owner

MaikuB commented Jan 4, 2022

Is there a way to prevent the Android device to kill the isolate when the user taps the action button? I mean, would it be a
foreground service that requires a notification to give a feedback to the user? Or we don't need to do nothing because
Flutter Engine handle this by itself?

I am talking about the Required notifications section of this link

@lucasribolli late response but I suspect you might be confusing some things. An isolate is a concept specific to Flutter and a background isolate is spun when the user taps on action. That's not related to foreground services that is mentioned in the link. Furtherrmore, the plugin already supports foreground services. Clone the repository and run the example app on Android to take a look

@AdamBridges
Copy link

@AdamBridges thanks for the input. What kind of use cases did you have in your app and was the actions and the way they were handled in this PR sufficient to cover them?

The use case included 3 actions that would either (1) store data to local/remote databases without opening the app; (2) open the app to a specific page; or (3) would reschedule the notification to fire again in 1-3 hours without opening the app. All of these actions work on Android except (2) and I had to resort to using an Android Intent package.

@MaikuB
Copy link
Owner

MaikuB commented Jan 5, 2022

@AdamBridges how did you handle (2) on iOS and application lifecycle i.e. navigating to a page when it app is running or terminated?

@AdamBridges
Copy link

@MaikuB I had issues with getting it to play nice on iOS so I used another package to handle notifications on iOS: The other package is capable of launching the app from an action and then there's logic in place to navigate the user from there. Messy, but it worked.

I'll have an opportunity to test the notifications on iOS and Android with only this package in a couple weeks from now if you want to ask me about it again then. Otherwise, I'll try to remember to post my experiences then.

@lucasribolli
Copy link
Contributor

To everyone else, I know there's a lot of subscribers on this but can I get a better indication of how many having been using @ened's fork so far? There are comments with questions or those describe issue but I wonder how many of you have successfully been using it and what feedback you have. If you could perhaps upvote on this message as indicator and optionally reply with feedback then that would be much appreciated

I have been used this fork for some months and it seems to be good. But I have one issue to report that I cannot identify the cause. In iOS it seems not possible to use the own FlutterLocalNotificationsPlugin to schedule notifications inside the background isolate. I am trying to schedule new notifications using the same methods used in the main isolate, with initialize etc. The requisite is, when user press the action button, a notification are scheduled.
It works well on Android, no issues.

@ened
Copy link
Contributor Author

ened commented Jan 28, 2022

@AdamBridges @lucasribolli really appreciate you took the time and wrote some feedback. We can look into getting the component improved in the future.

@MaikuB let's lock this conversation and add a new label/tag (perhaps issue template) for the issue tracker so that future bug reports can be found quickly.

@ened ened deleted the 17-notification-actions branch January 28, 2022 14:14
@MaikuB MaikuB added the notification actions Relates to functionality associated notification actions label Jan 29, 2022
@MaikuB
Copy link
Owner

MaikuB commented Jan 29, 2022

Yep makes sense. I've created a "notification actions" labels and will lock this. If you have some feedback or issue, relating to the functionality that's currently in the pre-release, please file a new issue with the label

Repository owner locked and limited conversation to collaborators Jan 29, 2022
if (newCategories.count > 0) {
UNUserNotificationCenter *center =
[UNUserNotificationCenter currentNotificationCenter];
[center getNotificationCategoriesWithCompletionHandler:^(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ened I figured out what was the cause behind why notification actions stopped appearing for me on iOS and narrowed it to here. removing the logic to query the existing categories and then appending to that list allowed me to see the notification actions again. was there a reason for querying this? from my own tests and what i've seen in docs, should be able to just call [center setNotificationCategories:newCategories];

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
notification actions Relates to functionality associated notification actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.