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

Avoid exception when calling Activity.reportFullyDrawn() #103

Conversation

simonschiller
Copy link
Contributor

Proposed Changes

  • Avoid a bug on API 19 when calling Activity.reportFullyDrawn()

I'm not quite sure if this class is the right place for this workaround, the code could also live in other places, e.g.:

  • androidx.core.app.ComponentActivity
  • androidx.activity.ComponentActivity
  • androidx.appcompat.app.AppCompatActivity

Testing

Test: Manually tested. If your test harness is also executed on devices with API 19, I could write an automated test for this change if you want.

Issues Fixed

Fixes: 163239764

@dlam
Copy link
Member

dlam commented Dec 8, 2020

Just a heads up that we are in the middle of transitioning to androidx-main instead of androidx-master-dev, so you may want to base your PR on top of there instead as I'll be disabling PR mirroring to androidx-master-dev this week.

@dlam
Copy link
Member

dlam commented Dec 8, 2020

(But if this gets in in the next few days you won't run into any issues)

Copy link
Member

@ianhanniballake ianhanniballake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this code to androidx.activity.ComponentActivity and write a test for this (at the very least, our internal CI runs on API 19 devices and will help serve as a regression test that this method doesn't crash).

!= PackageManager.PERMISSION_GRANTED) {
return;
}
super.reportFullyDrawn();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this should be guarded with a Build.VERSION.SDK_INT >= 19 so that reportFullyDrawn() can be called on all API levels.

That means it may be more appropriate to re-arrange this method to be something more like:

if (Build.VERSION.SDK_INT >= 20) {
  super.reportFullyDrawn();
} else if (Build.VERSION.SDK_INT == 19 && ContextCompat...) {
  // Your current code and comment re: the specific behavior of API 19
}

// On KitKat (API 19) the Activity.reportFullyDrawn() method requires the
// UPDATE_DEVICE_STATS permission, otherwise it throws an exception. Instead of throwing,
// we fall back to a no-op call here.
if (Build.VERSION.SDK_INT == Build.VERSION_CODES.KITKAT && ContextCompat
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general convention in AndroidX code is to use the numbers themselves, rather than Build.VERSION_CODES values. Please use 19 here.

@simonschiller simonschiller changed the base branch from androidx-master-dev to androidx-main December 8, 2020 11:19
@dlam
Copy link
Member

dlam commented Dec 8, 2020

Sorry for the churn - do you mind actually basing your PR on top of androidx-master-dev, looks like it's still a couple days out before we fully switch

@simonschiller simonschiller changed the base branch from androidx-main to androidx-master-dev December 8, 2020 19:12
@simonschiller
Copy link
Contributor Author

Sure, no problem ✔️

@dlam
Copy link
Member

dlam commented Dec 8, 2020

jcenter 502'ing :|

@copybara-service copybara-service bot closed this in fa01cd0 Dec 8, 2020
@simonschiller simonschiller deleted the bugfix/report-fully-drawn branch January 22, 2021 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants