[flutter_local_notifications] Add custom notification view support with text replacement and actions - #2760
[flutter_local_notifications] Add custom notification view support with text replacement and actions#2760Rishyash wants to merge 5 commits into
Conversation
- Added CustomViewMapping class to map view IDs to text/actions - Added CustomNotificationView class for custom layouts - Support for customContentView, customBigContentView, and customHeadsUpContentView - Dynamic text replacement in XML layouts from Flutter - Button/view click action handling - Comprehensive unit and integration tests - Backward compatible with existing API
Rishyash
changed the title
- Added 3 simple XML layouts for custom views - Added example methods in main.dart: * _showCustomContentViewNotification() - basic custom view * _showCustomBigContentViewNotification() - collapsed + expanded views * _showCustomHeadsUpNotification() - heads-up notification - Added UI buttons to trigger each example
- Changed from PendingIntent.getActivity() to PendingIntent.getBroadcast() - Now uses ActionBroadcastReceiver for custom view actions - Notifications will auto-dismiss when action buttons are clicked - Consistent with regular notification action behavior
…cked" This reverts commit 8a20cfa.
|
Any update for review? |
There was a problem hiding this comment.
Apologies for the delay and thanks for the PR. I was conscious of this affecting the release I wanted to get out with web support and have had to deal with some personal matters.
I have reviewed this and left some feedback on this. Let me know if you have questions on the feedback I left. A few of the key callouts
- PR is missing a key addition that provides a mapping to NotificationCompat.DecoratedCustomViewStyle and calling setStyle() accordingly. This is why the additions in the example app doesn't fully show the notification content. Making use of the style is mentioned in official Android docs
- Seeing if support for actions through background isolate is possible and can be added in same PR
- Looking at how the errors/exceptions have been handled
| @@ -0,0 +1,7 @@ | |||
| distributionBase=GRADLE_USER_HOME | |||
There was a problem hiding this comment.
Can you remove the gradle/wrapper folder that includes this file? Plugins don't need this checked in eg. check the repo for first party plugins as a reference
| */ | ||
| private static int getLayoutResourceId(Context context, String layoutName) { | ||
| if (layoutName == null || layoutName.isEmpty()) { | ||
| Log.e(TAG, "Custom layout name is null or empty"); |
There was a problem hiding this comment.
This is a general comment and question for similar scenarios where an error is logged or an exception has been swallowed: what's the reason to only log this? These look like erroneous scenarios and if so, it would be better to throw an exception (e.g. make use of the PluginException class) so that plugin consumers can be notified to handle scenarios like this
| if (mapping.actionId != null) { | ||
| try { | ||
| Intent intent = getLaunchIntent(context); | ||
| intent.setAction(SELECT_NOTIFICATION); |
There was a problem hiding this comment.
SELECT_NOTIFICATION is for when a user taps on a notification. However, this portion of the code is for actions so this code and other additions you have should be adjusted to use SELECT_FOREGROUND_NOTIFICATION_ACTION instead.
On a related note, have you checked to see if having actions triggered callbacks via a background isolate is possible? Conscious on if adding support for it later will result in breaking changes or backwards compatibility issues that it may be better to add support for them from the get go
| NotificationDetails notificationDetails, | ||
| CustomNotificationView customView) { | ||
|
|
||
| if (customView == null || customView.layoutName == null) { |
There was a problem hiding this comment.
This method looks quite long overall. Are you able to refactor it in a logical matter so that it calls other methods?
| Context context, | ||
| NotificationDetails notificationDetails, | ||
| NotificationCompat.Builder builder) { | ||
| // Skip setting style if custom views are present |
There was a problem hiding this comment.
I would be inclined let it clash if it comes to it if it comes to it. This way it aligns with how the native APIs work and aligns with how the rest of plugin is designed to do so as well. In other words, this section could be removed.
More importantly, there is also a key part missing in this PR where the Flutter side should allow the consumer of this plugin to specify a subclass of StyleInformation that when processed on the Android, maps to NotificationCompat.DecoratedCustomViewStyle and will call builder.setStyle() accordingly. Without this, the notification content can appear cropped . It already appears cropped with the additions made in the example app so those should also be updated to provide a more complete working example once support for NotificationCompat.DecoratedCustomViewStyle is added
| intent.putExtra(NOTIFICATION_ID, notificationDetails.id); | ||
| intent.putExtra(ACTION_ID, mapping.actionId); | ||
| intent.putExtra(PAYLOAD, notificationDetails.payload); | ||
| intent.putExtra(CANCEL_NOTIFICATION, true); |
There was a problem hiding this comment.
Rather than assuming the action should cancel the notification, this could be an option left to developers to decide on. As reference, you can see how Android notification actions in the plugin provide a boolean property that allows developers decide if they want the action to cancel the notification. Once this is added in, it would be better to encapsulate the action id and if the action cancels the notification through another class e.g. CustomViewActionMapping
3 participants
Uh oh!
There was an error while loading. Please reload this page.