-
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] Add getActiveNotificationMessagingStyle #1541
Conversation
d649ac3
to
61f8ccc
Compare
As this is about returning style information, unless it can be made more generic so it applies to all the other styles then I'd say it'd be something you'll need to keep in your fork. From what I also saw, it seems like the other notification styles don't support this either. For this to be included in the plugin, I would think this should be done via a new Android-specific API call within the Android-specific implementation of the plugin https://pub.dev/documentation/flutter_local_notifications/latest/flutter_local_notifications/AndroidFlutterLocalNotificationsPlugin-class.html This would mirror how there's a separate API call to get the messaging style information though I know this in case it's slightly less efficient as the Android API to do so requires passing a notification object but from the Dart API, it accepts a notification id and/or tag that will results in an Android API to get the active notification and then do another Android API call to extract the messaging style information. Whilst less efficient, I would think should be ok as retrieving the messaging style information would then be something consumers of the plugin opt into as opposed to be being forcefully done for each active notification. Also avoids potentially bloating the Dart class for the active notification with various platform-specific properties. Thoughts on this? |
This approach is already generic,
Right, that's a TODO. Not sure I'll find time to type up all of the glue code for all style TBH.
|
I was referring to doing the wiring up so it works for all the other styles. I know this was a draft so could have been why you didn't get to it yet. Sounds like you have also misunderstanding given the following response too
What I was saying is that from what I saw in the native APIs is that it looks like you wouldn't even be able to do the "glue code" for all styles as it looks like it's not possible to do a similar thing for every other style. In other words, this only works for messaging style so I would say it's another reason why it makes more sense that returning such info is done via another API call
I know what you mean though however had to weigh up if it was worth separating those into its own class given the number of platform-specific info being returned. The other difference is those two properties you mentioned are more core information in helping to identify a notification though, at least with tag |
Hm indeed, Android is missing the APIs to introspect other styles. |
61f8ccc
to
05b9af4
Compare
I've updated the PR, was this what you had in mind? |
05b9af4
to
ff0b2a7
Compare
Yep it is though I noticed a todo comment on returning the icon. Was that so you could check if this is what I had in mind before going further? Another thing to add is an optional tag parameter to the Dart method as a notification can be uniquely identified via id only or using a combination of id and tag |
ff0b2a7
to
dc4fe59
Compare
I'm not sure how we can handle icons. Android only supports fetching URI icon data (and maybe resource icon data, not sure). Other icon types can't be fetched. Would you be fine adding support for URI icons and documenting that other icon types can't be fetched?
Good point, fixed. |
Yep this is all based on what is supported by the native APIs so there's not much choice there and even though using the APIs directly would run into similar problems. With resource icon data. It looks a resource id can be returned, in which case this can be converted to a Uri similar to what's in the example app at Line 26 in 83c975b
|
flutter_local_notifications/lib/src/platform_flutter_local_notifications.dart
Outdated
Show resolved
Hide resolved
dc4fe59
to
9d75855
Compare
9d75855
to
44f0de9
Compare
All right, I've applied the changes. |
44f0de9
to
e4306aa
Compare
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.
Thanks for making more changes on this. I noticed this actually isn't working whilst testing so left comments around areas that I believe are the cause of the problem. With this in mind, I'd propose updating the example app with another a scenario using the messaging style that calls this new API. This example would just use the supported icon types (drawable and content uri) and helps facilitate testing in addition to showing how it could be used
...d/src/main/java/com/dexterous/flutterlocalnotifications/FlutterLocalNotificationsPlugin.java
Outdated
Show resolved
Hide resolved
...d/src/main/java/com/dexterous/flutterlocalnotifications/FlutterLocalNotificationsPlugin.java
Outdated
Show resolved
Hide resolved
e4306aa
to
e464087
Compare
Fixed the bugs, and added an example. |
This allows fetching an active notification's MessagingStyleInformation. A typical use-case is appending a new Message to a MessagingStyle when receiving a push message.
e464087
to
d8db25a
Compare
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.
Thanks so much!
This allows fetching an active notification's StyleInformation.
A typical use-case is appending a new Message to a MessagingStyle
when receiving a push message.
ATM this is pretty incomplete and only covers my exact use-case. Would you be interested in such a feature?