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(device_info_plus): Ensure use of Activity Context to obtain WindowManager #2688

Merged
merged 5 commits into from
Mar 14, 2024

Conversation

miquelbeltran
Copy link
Member

@miquelbeltran miquelbeltran commented Mar 13, 2024

Description

  • Ensure that the WindowManager is obtained from the Activity Context instead of Application Context
  • Use ActivityAware to obtain Activity binding in DeviceInfoPlusPlugin class.
  • Set a null MethodHandler when the activity is detached (doing the same as in the onDetachedFromEngine method).
  • Add the Strict Mode configuration to example project Android code to ensure the Activity Context use is correct.
  • Tested on Pixel 5.

Related Issues

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.

@miquelbeltran miquelbeltran marked this pull request as ready for review March 13, 2024 20: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.

Nice one

@miquelbeltran miquelbeltran merged commit 13248d9 into main Mar 14, 2024
19 of 20 checks passed
@miquelbeltran miquelbeltran deleted the miquelbeltran/2687 branch March 14, 2024 07:58
@ChristianEdwardPadilla
Copy link
Contributor

Hey folks thanks for the quick response on #2687 but based on some feedback from Flutter apps built within Google there has been some pushback on this fix. I'll file a new issue but here is some of the raw feedback:

The DeviceInfo plugin should not have a 
dependency on the activity. This change introduces two problems:

1. The activity could change, but the plugin stores the first read of data in memory so 
 it can't change, possibly resulting in incorrect display data. The activity's display 
 data is not guaranteed to match the device's display data.
2. The plugin should be able to be called even if the FlutterEngine isn't attached to an 
 activity. Right now, at best the plugin returns empty data and at worst the app crashes 
 or freezes if somebody were to try this.

If we need to exempt the StrictMode violations to get the original version submitted, we 
can do that and figure out a resolution to the issue later. I would much prefer that to 
the current implementation.

@ChristianEdwardPadilla
Copy link
Contributor

Hmm since this hasn't been incorporated into a release yet, maybe it doesn't make sense to file a new issue. So, I'll just keep the conversation here for now.

Here are my thoughts:

A. Does it even make sense to offer display metrics as part ofAndroidDeviceInfo? With the advent of foldables and other multi-display devices it seems Android has moved away from offering "device screen size" APIs (for instance, getDefaultDisplay is deprecated) in favor of an Activity-dependent APIs. If it doesn't make sense for the Android OS itself to offer such a device-wide API then how can device_info_plus offer such an API?

B. device_info_plus, being a plugin, can be aware of the activity lifecycle but should not be dependent on it for successful calls from Dart.

@miquelbeltran
Copy link
Member Author

miquelbeltran commented Mar 19, 2024

Hi Christian,

I am more in favor of getting rid of Display Metrics, indeed.

What I'll do is to revert this PR (partially) and re-open the original ticket.

Then, we will have to make a breaking change, removing the Display Metrics part.

edit: Actually, I will just do it in one single PR

@math1man
Copy link

Thanks folks! I second everything Christian mentioned, it doesn't make sense to include the display metrics info in the DeviceInfo plugin on Android.

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.

[Bug]: device_info_plus violates Android StrictMode's IncorrectContextUseViolation
4 participants