[flutter_local_notifications] Add support to pass bubble activity + intent to notification #2484
Conversation
|
Interesting! Perhaps you can make another Dart class for the bubble details? That way they stay bundled together and you can just use Then be sure to properly handle the error case in your Java code. And of course, please be sure to document the Dart parts, any extra Android-specific setup, and add a test case if possible, and let us know when you're finished. I'd be happy to test on my end as well. |
There was a problem hiding this comment.
In case you weren't waiting for my review, I also left comments some of which echo what @Levi-Lesches mentioned. Are you also able to update example app to add a scenario that demonstrates it usage?
...d/src/main/java/com/dexterous/flutterlocalnotifications/FlutterLocalNotificationsPlugin.java
Outdated
Show resolved
Hide resolved
| builder.setBubbleMetadata(bubbleData); | ||
| } | ||
| } catch (ClassNotFoundException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
is there a reason for swallowing the exception? can this be surfaced as an exception on the Flutter with appropriate details for users of the plugin to handle?
There was a problem hiding this comment.
I'm not sure the best way to do this, I would appreciate some guidance here
There was a problem hiding this comment.
When catching theClassNotFoundException, you can throw a RuntimeException that wraps around the ClassNotFoundException. My understanding is Flutter will handle catching this and on the Dart side, a PlatformException will be thrown so that developers can handle accordingly. Are you able to reproduce the scenarios where the ClassNotFoundException will occur? If so, then once you make the changes then you'll be able to confirm this
flutter_local_notifications/lib/src/platform_specifics/android/notification_details.dart
Outdated
Show resolved
Hide resolved
|
@Airyzz I received notifications that you've been updating your fork and wanted to ping to see if there were comments left on the PR to see if you look into. If not then I'll unfortunately may have to close this as it's been a while since this has been opened. I'm also conscious of making attempts to update this directly as you hadn't created a separate branch on your fork |
|
hey, I'll work on implementing your feedback soon, i sort of forgot about this PR 😅 |
|
I saw you pushed some changes now but I'm not sure if you're done so let know when it's ready for a review. I spotted some things I could comment on already but thought better to hold off until I know you're done For future reference, if you intend to submit a PR to another repo like this one, one thing I was taught was to create branch on the fork and use that branch to hold the changes that you intend to contribute. This way the default branch (e.g. main/master) on your fork is solely reserved for keeping it up to date with the original repo you forked from. It makes it easier to maintain a fork as well. |
|
Sorry, it's not ready yet, i'll convert this to a draft for now until its ready |
|
No worries. A general note I can mention is that I've put some information in the contribution guide. One of the main things, I wanted to shout out on is around API docs that can be applied to the PR |
|
No problem, i'll go in and document everything once I'm finished with the rest |
|
Apart from your previous comment about handling exceptions, I think this is ready to look at |
|
Ok I'll use the PR comments to explain further around that. Note I've not gotten to test the changes in the PR yet |
| @@ -432,6 +432,50 @@ protected static Notification createNotification( | |||
| setProgress(notificationDetails, builder); | |||
| setCategory(notificationDetails, builder); | |||
| setTimeoutAfter(notificationDetails, builder); | |||
|
|
|||
| if (notificationDetails.bubbleActivity != null) { | |||
There was a problem hiding this comment.
Sorry I missed this before but can you refactor out all the changes to a separate method
| @@ -0,0 +1,29 @@ | |||
| /// Defines metadata to be set for the notification's chat bubble | |||
There was a problem hiding this comment.
This is a general comment on all the API docs
- There are casing issues with the API docs and lack of punctuation marks
- Look to follow these guidelines around documentation that were mentioned in the contribution guide. You can reference the existing API docs to see examples. A couple of issues that stick to me are violations of this and I also try to have the docs follow this where possible.
This allows for use of Android conversation bubbles api