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

fix(battery_plus): Use context compat to set exported state #1811

Merged

Conversation

reidbaker
Copy link
Contributor

@reidbaker reidbaker commented May 12, 2023

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

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I titled the PR using Conventional Commits.
  • I did not modify the CHANGELOG.md nor the plugin version in pubspec.yaml files.
  • All existing and new tests are passing.
  • The analyzer (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?

  • Yes, this is a breaking change (please indicate that with a ! in the title as explained in Conventional Commits).
  • No, this is not a breaking change.

@reidbaker reidbaker marked this pull request as draft May 12, 2023 19:49
@reidbaker reidbaker marked this pull request as ready for review May 12, 2023 21:28
@vbuberen
Copy link
Collaborator

Unfortunately I cant find a public document or post saying this but I see this on internal facing documents.

I think you meant something like this: https://developer.android.com/about/versions/14/behavior-changes-14

Copy link
Collaborator

@vbuberen vbuberen 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 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.
Copy link
Collaborator

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.

Copy link
Contributor Author

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
Copy link
Contributor Author

@reidbaker reidbaker May 15, 2023

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

@vbuberen vbuberen merged commit b901615 into fluttercommunity:main May 15, 2023
@reidbaker
Copy link
Contributor Author

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.

@reidbaker reidbaker deleted the i1810-compat-context-for-broadcast branch May 22, 2023 14:59
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