[flutter_local_notifications] Adds support for Android's getActiveNotifications #704

vkammerer · 2020-07-09T07:59:54Z

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 implemented id, channelId, title and body, but would be happy to add more if you think it is relevant. Indeed, since we have access to the Notification instance on the Android's side, we could get more data.

MaikuB · 2020-07-09T08:12:14Z

Thanks for the PR. Would you be able to change the target branch to timezone_macos_support instead of master so I can put this feature into the current beta? Will go through the PR more when it's done.

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?

vkammerer · 2020-07-09T08:34:33Z

I have just changed the target branch to timezone_macos_support, and the diff is identical.

I had tried to retrieve the payload, which would be great to have. But I didn't manage to, since the notification's PendingIntent doesn't allow access to its Intent which contains the payload data.. Can you think of another way to retrieve it?

MaikuB · 2020-07-09T08:50:13Z

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.

vkammerer · 2020-07-09T09:31:29Z

Oops I hadn't seen the merge conflicts.. I have just resolved them and pushed an updated version.

MaikuB

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.

MaikuB · 2020-07-11T00:38:26Z

...d/src/main/java/com/dexterous/flutterlocalnotifications/FlutterLocalNotificationsPlugin.java

@@ -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";


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

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"

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

MaikuB · 2020-07-11T00:39:54Z

flutter_local_notifications/lib/src/platform_specifics/android/status_bar_notification.dart

@@ -0,0 +1,22 @@
+class ActiveNotification {


Can you rename the file to match the name of the class?

MaikuB · 2020-07-11T00:43:39Z

flutter_local_notifications/lib/src/platform_specifics/android/status_bar_notification.dart

+  final String title;
+  final String body;
+
+  toString() {


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?

MaikuB · 2020-07-11T00:45:11Z

flutter_local_notifications/lib/src/platform_specifics/android/status_bar_notification.dart

+    this.body,
+  );
+
+  final int id;


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+

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

vkammerer · 2020-07-11T03:08:58Z

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)

MaikuB · 2020-07-11T03:14:09Z

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

vkammerer · 2020-07-11T03:26:05Z

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 android_get_active_notifications.
Which git command did you use to do that?

MaikuB

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 :)

MaikuB · 2020-07-11T03:28:55Z

flutter_local_notifications/example/lib/main.dart

+          if (activeNotifications.isNotEmpty)
+            ...activeNotifications
+                .map(
+                  (ActiveNotification aN) => Column(


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

I wasn't aware of collection for which I'll definitely use from now on! I've implemented it in this PR

MaikuB · 2020-07-11T03:30:22Z

flutter_local_notifications/lib/src/platform_flutter_local_notifications.dart

+    final List<Map<dynamic, dynamic>> activeNotifications =
+        await _channel.invokeListMethod('getActiveNotifications');
+    return activeNotifications
+        .map((activeNotification) => ActiveNotification(


Same comment as above on variable name. Also running flutter analyze shows an error where the type hasn't been specified

I've added // ignore: always_specify_types before that line as adding the type conflicts with another rule

MaikuB · 2020-07-11T03:34:43Z

flutter_local_notifications/lib/src/platform_specifics/android/active_notification.dart

+  /// The notification's id.
+  final int id;
+
+  /// The notification channel's id - Returned only on Android 8.0+


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

MaikuB · 2020-07-11T03:46:32Z

flutter_local_notifications/example/lib/main.dart

+                    crossAxisAlignment: CrossAxisAlignment.start,
+                    children: <Widget>[
+                      Text(
+                        'id: ${aN.id.toString()}\n'


toString() is redundant when doing string interpolation

MaikuB · 2020-07-11T03:48:45Z

flutter_local_notifications/example/lib/main.dart

+                    children: <Widget>[
+                      Text(
+                        'id: ${aN.id.toString()}\n'
+                        'channelId: "${aN.channelId}"\n'


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

MaikuB · 2020-07-11T03:49:09Z

flutter_local_notifications/example/lib/main.dart

+    } on PlatformException catch (error) {
+      return Text(
+        'Error calling "getActiveNotifications"\n'
+        'code: "${error.code}"\n'


See comment above on the double quotation marks

MaikuB · 2020-07-11T03:51:14Z

flutter_local_notifications/lib/src/platform_flutter_local_notifications.dart

@@ -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 =


Sorry missed this last time but does this continue to work if dynamic was replaced with Object? Been avoiding dynamic where possible

Trying to cast to another type throws an Exception at runtime so this signature should be kept for now

MaikuB · 2020-07-11T04:01:36Z

Regarding pushing changes to the PR, no special git commands are needed for that. When you create a PR, there's a checkbox on if you allow edits to be done by maintainers. If that's enabled, I can use the GitHub client to download your fork/branch

vkammerer · 2020-07-11T10:47:56Z

Ok good to know about the PR updates you can make.
I've added a basic test that looks like the others in the list, and ran them and they all pass.

MaikuB · 2020-07-12T01:16:05Z

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>

vkammerer changed the base branch from master to time_zone_macos_support July 9, 2020 08:25

vkammerer added 2 commits July 9, 2020 21:21

Adds support for Android's getActiveNotifications

11f570d

Updates button label after new demo page layout

beff17b

vkammerer mentioned this pull request Jul 9, 2020

Interested in a PR to support getActiveNotifications? #690

Closed

MaikuB added 2 commits July 11, 2020 10:16

fix e2e test code

21b63c1

update CI script to include code analysis

308fd37

MaikuB requested changes Jul 11, 2020

View reviewed changes

Improves getActiveNotifications implementation: Adds README usage, ad…

deea969

…ds model documentation, renames file, updates error message

MaikuB requested changes Jul 11, 2020

View reviewed changes

vkammerer added 2 commits July 11, 2020 17:09

Updates code style and documentation

a157195

Adds basic test

c5ef4d1

MaikuB added 5 commits July 12, 2020 10:45

try no-pub option for fixing analysis task

81fd558

analyze platform interface code from within the directory

d749eb2

add analysis task for plugin and update dependencies

070919e

revert cirrus script

7b1ac77

amend API docs

eb8c9f3

MaikuB approved these changes Jul 12, 2020

View reviewed changes

MaikuB merged commit 7553b74 into MaikuB:time_zone_macos_support Jul 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flutter_local_notifications] Adds support for Android's getActiveNotifications #704

[flutter_local_notifications] Adds support for Android's getActiveNotifications #704

[flutter_local_notifications] Adds support for Android's getActiveNotifications #704

[flutter_local_notifications] Adds support for Android's getActiveNotifications #704

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment