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 support for Android Tiramisu' POST_NOTIFICATIONS permission #1658

Merged
merged 19 commits into from
Aug 17, 2022
Merged

[flutter_local_notifications] Add support for Android Tiramisu' POST_NOTIFICATIONS permission #1658

merged 19 commits into from
Aug 17, 2022

Conversation

bartekpacia
Copy link
Contributor

Implements two new methods on AndroidFlutterLocalNotificationsPlugin, requestPermission and isPermissionGranted, to let developers work with the new notification runtime permission added in Android 13 Tiramisu (API level 33)

Fixes #1597

@bartekpacia bartekpacia changed the title [flutter_local_notifications] Add support for Android Tiramisu' POST_NOTIFICATION permission [flutter_local_notifications] Add support for Android Tiramisu' POST_NOTIFICATIONS permission Jul 30, 2022
@bartekpacia
Copy link
Contributor Author

bartekpacia commented Jul 30, 2022

Locally Android integration tests pass when I use Flutter v3.0.5, but fail on Flutter v2.10.5.

@bartekpacia
Copy link
Contributor Author

What's missing is returning a boolean result from requestPermission(). I'll implement this as well in this PR :)

@MaikuB
Copy link
Owner

MaikuB commented Jul 31, 2022

Thanks for the PR, was going to comment that it looks like that was missing when it comes to dealing with the listener.

Also the the Java code from the isPermissionGranted method remains after you had removed the code on the Flutter/Dart side.

I'll take a closer look once the changes are done

@MaikuB
Copy link
Owner

MaikuB commented Jul 31, 2022

Also just remembered about flutter/flutter#12561 that can impact testing. A workaround is mentioned there

@bartekpacia
Copy link
Contributor Author

bartekpacia commented Aug 1, 2022

@MaikuB Now AndroidFlutterLocalNotificationsPlugin.requestPermission() returns a Future<bool?> :)

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 the changes. I've left some comments on this, hope they make sense and happy to discuss further if required

@bartekpacia
Copy link
Contributor Author

bartekpacia commented Aug 4, 2022

@MaikuB Thanks the for review, I'll make the changes soon :)

@MaikuB
Copy link
Owner

MaikuB commented Aug 17, 2022

Thanks for making the change. I pushed a grammar fix for the error message you added in so I can try to merge sooner. One thing that this made me think about though is the ability to use the existing initialize() method to also request permissions too. This is something that's possible for iOS and macOS. I can try to look at that after this PR is merged

@MaikuB MaikuB merged commit f13b9b9 into MaikuB:master Aug 17, 2022
@bartekpacia bartekpacia deleted the feature/android_notification_permission branch August 17, 2022 15:52
@bartekpacia
Copy link
Contributor Author

Thank you for your help with getting this merged @MaikuB :)

Regarding your idea with requesting permissions on initialize(), you have the example app in mind, not the plugin itself, right?

@MaikuB
Copy link
Owner

MaikuB commented Aug 17, 2022

No I'm referring to the plugin itself

@stephanie-finch
Copy link

Thanks both for working on this! 👏🏻

Weighing in from the product side for requesting on initialize(), it's often important to prompt the user for notif permissions at a very specific step in the new user flow to optimize conversion for accepting. So I'd wanna make sure the plugin doesn't automatically prompt users for permission in a way that isn't within our control.

@MaikuB
Copy link
Owner

MaikuB commented Aug 18, 2022

If you look at how iOS/macOS works then there is control over that. This should be clearer if you look at the example app where it decides not to have permissions requested when calling initialize

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.

Support for Android 13 Tiramisu (API 33)
3 participants