-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix(battery_plus): Use context compat to set exported state #1811
fix(battery_plus): Use context compat to set exported state #1811
Conversation
I think you meant something like this: https://developer.android.com/about/versions/14/behavior-changes-14 |
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 your contribution. I am totally up for preparations to latest Android in advance. So looks good to me.
The only question is in the comment with DO NOT MERGE
text among your changes.
override fun onListen(arguments: Any?, events: EventSink) { | ||
chargingStateChangeReceiver = createChargingStateChangeReceiver(events) | ||
applicationContext?.registerReceiver(chargingStateChangeReceiver, IntentFilter(Intent.ACTION_BATTERY_CHANGED)) | ||
// DO NOT MERGE, this alternates states. reidbaker debug before review. |
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.
This is a forgotten comment? Because I see that the PR is marked as ready for review, but this comment is here.
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.
Yes, sorry I wanted to make sure not to merge/reivew until my local testing worked. Sorry, removed.
@@ -77,9 +81,16 @@ class BatteryPlusPlugin : MethodCallHandler, EventChannel.StreamHandler, Flutter | |||
} | |||
} | |||
|
|||
@SuppressLint("WrongConstant") // Error in ContextCompat for RECEIVER_NOT_EXPORTED |
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.
I filed a bug against androidx for this behavior https://b.corp.google.com/issues/282596027
FYI we saw flutter/flutter#127014 after merging our version of this. I checked out battery_plus and rebuilt the example app and make sure that pressing back does not crash. So I believe this is safe but for transparency I wanted to add this comment. |
Description
Most of the context is in the bug but short summary is that broadcast receivers will require and export state if apps are targeting android api 34. Unfortunately I cant find a public document or post saying this but I see this on internal facing documents.
There is an internal to google team using battery_plus who wishes to target api 34 and they reached out to me as the TL of flutter on android to help get their dependencies updated. You can see a similar issues and a pull request to fix our own url_launcher plugin below.
I think the other examples of registerReceiver are ok because they are protected by a version check and not called on api 34. If that is incorrect I can fix those in a different pr.
Related Issues
#1810
flutter/flutter#126460
flutter/packages#3973
Checklist
CHANGELOG.md
nor the plugin version inpubspec.yaml
files.flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?
!
in the title as explained in Conventional Commits).