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

[flutter_local_notifications] Add getActiveNotificationMessagingStyle #1541

Merged
merged 1 commit into from
May 3, 2022

Conversation

emersion
Copy link
Contributor

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?

@emersion emersion force-pushed the active-notification-style branch 2 times, most recently from d649ac3 to 61f8ccc Compare March 26, 2022 17:13
@MaikuB
Copy link
Owner

MaikuB commented Apr 3, 2022

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?

@emersion
Copy link
Contributor Author

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

This approach is already generic, ActiveNotification.styleInformation is not a MessagingStyleInformation, it's a StyleInformation.

From what I also saw, it seems like the other notification styles don't support this either

Right, that's a TODO. Not sure I'll find time to type up all of the glue code for all style TBH.

avoids potentially bloating the Dart class for the active notification with various platform-specific properties

ActiveNotification is already bloated: things like channelId and tag are Android-specific. In the first place getActiveNotifications only works on Android (and needs to be called on the AndroidFlutterLocalNotificationsPlugin). So I don't think it's a big issue.

@emersion emersion changed the title [flutter_local_notifications] Add ActiveNotification.style [flutter_local_notifications] Add ActiveNotification.styleInformation Apr 15, 2022
@MaikuB
Copy link
Owner

MaikuB commented Apr 18, 2022

This approach is already generic, ActiveNotification.styleInformation is not a MessagingStyleInformation, it's a StyleInformation.

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

Right, that's a TODO. Not sure I'll find time to type up all of the glue code for all style TBH.

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

ActiveNotification is already bloated: things like channelId and tag are Android-specific. In the first place getActiveNotifications only works on Android (and needs to be called on the AndroidFlutterLocalNotificationsPlugin). So I don't think it's a big issue.

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

@emersion
Copy link
Contributor Author

Hm indeed, Android is missing the APIs to introspect other styles.

@emersion emersion changed the title [flutter_local_notifications] Add ActiveNotification.styleInformation [flutter_local_notifications] Add getActiveMessagingStyle Apr 28, 2022
@emersion
Copy link
Contributor Author

I've updated the PR, was this what you had in mind?

@emersion emersion marked this pull request as ready for review April 28, 2022 09:08
@MaikuB
Copy link
Owner

MaikuB commented Apr 28, 2022

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

@emersion
Copy link
Contributor Author

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?

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?

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

Good point, fixed.

@MaikuB
Copy link
Owner

MaikuB commented Apr 28, 2022

Would you be fine adding support for URI icons and documenting that other icon types can't be fetched?

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

. Note the code there is in Kotlin but should be able to do the same thing via Java too

@emersion emersion changed the title [flutter_local_notifications] Add getActiveMessagingStyle [flutter_local_notifications] Add getActiveNotificationMessagingStyle Apr 29, 2022
@emersion
Copy link
Contributor Author

All right, I've applied the changes.

Copy link
Owner

@MaikuB MaikuB left a 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

@emersion emersion force-pushed the active-notification-style branch from e4306aa to e464087 Compare May 2, 2022 08:54
@emersion
Copy link
Contributor Author

emersion commented May 2, 2022

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.
@emersion emersion force-pushed the active-notification-style branch from e464087 to d8db25a Compare May 2, 2022 09:10
Copy link
Owner

@MaikuB MaikuB left a comment

Choose a reason for hiding this comment

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

Thanks so much!

@MaikuB MaikuB merged commit e0fa35a into MaikuB:master May 3, 2022
@emersion emersion deleted the active-notification-style branch May 3, 2022 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants