-
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
[flutter_local_notifications] Adds support for Android's getActiveNotifications #704
[flutter_local_notifications] Adds support for Android's getActiveNotifications #704
Conversation
Thanks for the PR. Would you be able to change the target branch to Regarding adding more info, do you know if it's possible to get the payload, as defined by the plugin, and include that as part of the info that is returned? |
I have just changed the target branch to I had tried to retrieve the payload, which would be great to have. But I didn't manage to, since the notification's |
There are some merge conflicts that need to be resolved though. Regarding the payload, I admittedly forgot about the intent part so don't know of another way at the moment. |
Oops I hadn't seen the merge conflicts.. I have just resolved them and pushed an updated version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed some changes to fix a merge issue that was my fault for the e2e test. Also made sure that flutter analyze
is run as there were some issues found that need to be fixed in addition to the areas I've commented on.
@@ -101,6 +105,8 @@ | |||
private static final String INVALID_BIG_PICTURE_ERROR_CODE = "INVALID_BIG_PICTURE"; | |||
private static final String INVALID_SOUND_ERROR_CODE = "INVALID_SOUND"; | |||
private static final String INVALID_LED_DETAILS_ERROR_CODE = "INVALID_LED_DETAILS"; | |||
private static final String GET_ACTIVE_NOTIFICATIONS_ERROR_CODE = "GET_ACTIVE_NOTIFICATIONS_ERROR_CODE"; | |||
private static final String GET_ACTIVE_NOTIFICATIONS_ERROR_MESSAGE = "Android version must be at least 23 to use getActiveNotifications"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
23 is the API level, 6.0 is the Android version. Could you update this error to refer to version number as that's what I've been using in the plugin as there will be developers who haven't worked on Android directly to be familiar with API levels
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, maybe we can discuss the exact wording here before I push changes?
I'm not sure I understood which number you prefer: the API level, or the Android version? I've searched for "Android" in your repository and the only occurence I could find was the README that says
**Android API 16+** (4.1+, the minimum version supported by Flutter).
so should I replace it with (version 1):
"Android API Level must be at least 23 to use getActiveNotifications"
or with (version 2)
"Android version must be at least 6.0 to use getActiveNotifications"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusing. You're right that the readme has both in there though for public API docs I've simplified it refer to Android version i.e. go with version 2
@@ -0,0 +1,22 @@ | |||
class ActiveNotification { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rename the file to match the name of the class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
final String title; | ||
final String body; | ||
|
||
toString() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be removed? Haven't implemented this for any other class in the plugin and removing it would allow users of the plugin to define their own implementation of toString
e.g. via extension methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
this.body, | ||
); | ||
|
||
final int id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing API docs for the property. Can refer to the existing public API docs to copy and paste them if it makes sense to do so. channelId
will need special mention that it is only returned on Android 8.0+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I added documentation comments to all the properties. Let me know if it's ok like that
…ds model documentation, renames file, updates error message
I just pushed deea969 , which also updates the README with a reference and usage example of the new feature, as you had mentioned in #690 (comment) |
Hmm the analyse task is failing is a weird way, maybe I'll need to remove it until I figure out what's going on |
By the way I didn't know someone else than me could add commits to my PRs. I've seen that your last commits have been added to my repo in the branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there, do you think you can add a unit test as well? There's a group of Android unit tests in the platform_flutter_local_notifications_test.dart
file? Once done I'll take a look at seeing if the analyse task can be fixed or if I need to remove it.
On a different note, are you part of the GDG ANZ Slack workspace? Tried to see search your name but couldn't find you in case you were there to see if I could ping you there to make it easier. Can understand if you don't want to join but if you're interested, this is the registration link http://bit.ly/join_gdgslack :)
if (activeNotifications.isNotEmpty) | ||
...activeNotifications | ||
.map( | ||
(ActiveNotification aN) => Column( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Convention being used in the existing codebase is to stick with a single letter for variables used in an anonymous function/lambda expression. Having said that, should be able to use the collection for
that is idiomatic to Dart and removes the need for spread, map and toList operations.
See this article for an example if you haven't come across it before https://medium.com/dartlang/announcing-dart-2-3-optimized-for-building-user-interfaces-e84919ca1dff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware of collection for
which I'll definitely use from now on! I've implemented it in this PR
final List<Map<dynamic, dynamic>> activeNotifications = | ||
await _channel.invokeListMethod('getActiveNotifications'); | ||
return activeNotifications | ||
.map((activeNotification) => ActiveNotification( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above on variable name. Also running flutter analyze
shows an error where the type hasn't been specified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added // ignore: always_specify_types
before that line as adding the type conflicts with another rule
/// The notification's id. | ||
final int id; | ||
|
||
/// The notification channel's id - Returned only on Android 8.0+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I've been putting things similar Returned only on Android 8.0+ in a separate paragraph as part of following these guidelines https://dart.dev/guides/language/effective-dart/documentation#do-start-doc-comments-with-a-single-sentence-summary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok sure
crossAxisAlignment: CrossAxisAlignment.start, | ||
children: <Widget>[ | ||
Text( | ||
'id: ${aN.id.toString()}\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toString()
is redundant when doing string interpolation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
children: <Widget>[ | ||
Text( | ||
'id: ${aN.id.toString()}\n' | ||
'channelId: "${aN.channelId}"\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest removing the double quotation marks as it might confuse users into thinking they are part of the value itself. There isn't a need to distinguish if a value is a string when it's present in the app if that was the reason why they were placed there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
} on PlatformException catch (error) { | ||
return Text( | ||
'Error calling "getActiveNotifications"\n' | ||
'code: "${error.code}"\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above on the double quotation marks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@@ -255,6 +256,19 @@ class AndroidFlutterLocalNotificationsPlugin | |||
Future<void> deleteNotificationChannel(String channelId) => | |||
_channel.invokeMethod('deleteNotificationChannel', channelId); | |||
|
|||
Future<List<ActiveNotification>> getActiveNotifications() async { | |||
final List<Map<dynamic, dynamic>> activeNotifications = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry missed this last time but does this continue to work if dynamic
was replaced with Object
? Been avoiding dynamic
where possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to cast to another type throws an Exception at runtime so this signature should be kept for now
Ok good to know about the PR updates you can make. |
Once the build passes, I'll merge this in. Will likely do some general cleanup separately before releasing this out. Thanks again :) |
…nterface] timezone, macOS support, various new features and fixes (#830) * initial working version of tzSchedule method on android and ios * add SR-310 ABP and use in plugin, add tests, update example and update iOS side * WIP on repeating scheduled notifications * updates to triggers * change alias used for timezone imports * fix test for tzSchedule * update example app dependencies and ios project * fix imports and remove ScheduledNotificationRepeatTrigger related code * deprecate schedule, bump Gradle plugin and update example app * add ability to specify repeat frequency for scheduled notification * update docs around grouping notifications * refactor ios code * update id used in example for weekly notification * rename tzSchedule to zonedSchedule and fix tests * add androidAllowWhileIdle to periodicallyShow * cleanup usages of dynamics for maps * update InitializationSettings and NotificationDetails to have named parameters * hide toMap and update API docs for classes with notification details * bumped e2e dependency and update code to work with it * hide toMap from Time class * update code using different linter ruleset * hide additional android examples when run on other platforms * update enum values to lower camel casing * update enum values in platform interface * have platform interface using rules from root analysis_options.yaml * updated podspec details * fix plugin version * remove macOS-related conditions * add assertions to IOSInitializationSettings * add support for macOS 10.14+, cleanup examples * update API docs for OS version specific properties * refactor macos plugin and put OS X version at class level * Revert "refactor macos plugin and put OS X version at class level" This reverts commit 59b1712. * bump plugin interface to 2.0.0-beta.1 * add support for older macOS versions using NSUserNotification APIs * update changelog to mention macOS support * add ability to specify how to interpret scheduled date on older iOS versions * refactor date validation and update API docs * revert example app back to use local timezone and scheduled duration * apply formatting to plugin methods * update documentation and code related to docs * more documentation updates and update Proguard rules * fix example app code for schedule 10 AM notification and rearrange cancel code * add info on more changes * update readme in usage of example app * bump AGP to 4.0 and use desugaring to access Java time APIs * add details on enabling desugaring and iOS presentation options * refactor string constants for method call arguments on macOS * fix error message on Android when sound raw cannot be found * refactor macOS method call logic * refactor iOS code * Revert "bump AGP to 4.0 and use desugaring to access Java time APIs" This reverts commit 40f0021. * fix logic in receiver to repeat zoned scheduled notifications * update readme to mention fix for invalid source resource error and update notes on how zonedSchedule works on Android * include macOS on note about restrictions on notification sounds * clean up changelog * ignore macOS example Podfile-related files * bump plugin_platform_interface dependency * update to use beta platform interface * parse date interpretation on iOS * add note to readme on methods that aren't implemented * update changelog about Dart SDK dependency being bumped * remove redundant ios classes * restore android timestamp fix to ensure consumers of older plugin versions will still have scheduled notifications shown * add tests for macOS * add ability to specify subtitle for an iOS notification * add todo for testing periodicallyShow * update testing section of readme and move it * fix how launch notification details are read on android * fix casing of changelog entry * fix issue where ThreeTenABP lib wasn't initialised * bump e2e dev dependency * fix initialisation of ThreeTen Android Backport library when notifications are being rescheduled * [flutter_local_notifications] Adds support for Android's getActiveNotifications (#704) * Adds support for Android's getActiveNotifications * Updates button label after new demo page layout * fix e2e test code * update CI script to include code analysis * Improves getActiveNotifications implementation: Adds README usage, adds model documentation, renames file, updates error message * Updates code style and documentation * Adds basic test * try no-pub option for fixing analysis task * analyze platform interface code from within the directory * add analysis task for plugin and update dependencies * revert cirrus script * amend API docs Co-authored-by: Michael Bui <25263378+MaikuB@users.noreply.github.com> * [flutter_local_notifications] cleanup code and API docs (#708) * fix and update API docs * cleanup Java code around checking Android versions * remove use of dyancmi * remove redundant conditional in example code * bump plugin and updated more docs * fix grammar in API docs * fix Android version mentioned in example app for getActiveNotifications * update wording in changelog * mention Android versions that getActiveNotifications is compatible with in changelog * fix grammar for error message related to calling getActiveNotifications * fix macOS code quality issues (#734) * [flutter_local_notifications] Android Full-Screen Notification (#730) * added full-screen support for android * addressed some PR comments * removed wakelock * added full-screen intent notification to example * fullScreen -> fullScreenIntent * android fixes * _showFullScreenNotification now uses zonedSchedule * doc fix * reverted unneeded format change in main.dart * dart analysis fixes * README - nit: use sentence casing instead of title casing * Update main.dart * Update main.dart * removed imports * format code * configure priority and importance Co-authored-by: Michael Bui <25263378+MaikuB@users.noreply.github.com> * bump version for 1.5.0-beta.7 release (#762) * [flutter_local_notifications] null checking mainActivity before .getIntent() (#763) * bump version (#765) * [flutter_local_notifications] Add Null-Safety to FullScreen Intent Check (#781) * Add Null-Safety to FullScreen Intent Check (#780) * use helper method to remove nested if condition Co-authored-by: Michael Bui <25263378+MaikuB@users.noreply.github.com> * [flutter_local_notifications] bump version to 1.5.0-beta.9 (#800) * bump version * fix changelog entry * [flutter_local_notifications] Added table of contents (#808) * 📝 Added table of contents * 📝 Updated TOC * remove kvm_script that isn't needed on cirrus anymore Co-authored-by: Michael Bui <25263378+MaikuB@users.noreply.github.com> * [flutter_local_notifications] bump compile SDK to 30 and ability to specify shortcut id (#820) * bump plugin to use compile SDK version 30 for Android * add ability to specify shortcut id * bump plugin version * [flutter_local_notifications] updates to static analysis and plugin versions (#829) * update example app and readme to match repo linter rules * updates to changelog and versions for stable release * remove development team info Co-authored-by: Vincent <vkammerer@gmail.com> Co-authored-by: Nadav Fima <nadavfima@gmail.com> Co-authored-by: Jonas Bornold <jonas@bornold.se> Co-authored-by: Sebastian Faust <sebastian.faust1997@gmail.com> Co-authored-by: Ascênio <Ascenio@users.noreply.github.com>
This PR follows up on the discussion initiated in #690.
@MaikuB I followed your last comment suggestion and a
PlatformException
is thrown when the Android API level is under 23.I'm a bit unsure which fields should be part of the
ActiveNotification
class: I have currently only implementedid
,channelId
,title
andbody
, but would be happy to add more if you think it is relevant. Indeed, since we have access to theNotification
instance on the Android's side, we could get more data.