Using invalidateChildInParent for API levels lower than 26. #31533
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
…rnally-calculated area instead. Because of this, clients are encouraged to just call invalidate().
| } | ||
|
|
||
| /** Used on Android O+, {@link invalidateChildInParent} used for previous versions. */ | ||
| @SuppressLint("NewApi") |
There was a problem hiding this comment.
why is this required? Do you set a lower targetSdkVersion?
There was a problem hiding this comment.
I think we should also consider the API levels lower than 26. :)
There was a problem hiding this comment.
Shouldn't this be a TargetApi attribute instead of suppress lint?
There was a problem hiding this comment.
Shouldn't this be a TargetApi attribute instead of suppress lint?
We override this method and call invalidate simply in it. I consider @SuppressLint is fine here.
There was a problem hiding this comment.
Yes, but the right way to express this to the linter is via @TargetApi and/or @RequiresApi - see https://stackoverflow.com/questions/40007365/requiresapi-vs-targetapi-android-annotations/45773141. SuppressLint works but it looks like we're ignoring a lint without explaining why. It's not a huge deal either way in this case, especially given the comment - but with those lints you don't need the comments since it's encoded in the attribute :)
There was a problem hiding this comment.
why is this required? Do you set a lower targetSdkVersion?
I think we should also consider the API levels lower than 26. :)
I misunderstood what @blasten meant earlier. Here @SuppressLint("NewApi") is not required indeed because onDescendantInvalidated is only called by the Android framework.
|
Needs a test. |
|
In |
@dnfield Thank you for your patient reply. |
|
@blasten can you provide a second review? |
dnfield
added
waiting for tree to go green
4 participants
Using
invalidateChildInParentfor API levels lower than 26./cc @blasten @dnfield
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.