-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Update target and compile Android sdk versions to v29 #25488
Update target and compile Android sdk versions to v29 #25488
Conversation
} | ||
view.setGravity(Gravity.START); | ||
} else { | ||
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | ||
view.setJustificationMode(Layout.JUSTIFICATION_MODE_NONE); | ||
view.setJustificationMode(LineBreaker.JUSTIFICATION_MODE_NONE); |
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.
Looks like LineBreaker was introduced in Q. Should we do something like this?
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) {
// use LineBreaker.JUSTIFICATION_MODE...;
} else if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
// use LAYOUT.JUSTIFICATION_MODE...;
}
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.
Good question ;). I believe we don't need to do that. Constants are replaced during compilation and they haven't changed the values of the constants. They just moved them to a different file. Our compileSdkVersion is set to 29 so using just the constant for api 29 seems legit. Either case, I'll try to run this code on API 27 emulator to make sure I'm not wrong :).
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.
Hmm, I'm looking into this more. It seems that lint on CI complains in WPAndroid - it doesn't complain locally.
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 think lint is quite confused, when I wrap the call it fails with:
/Users/jirimalina/Code/Automattic/WordPress-Android-submodule/libs/gutenberg-mobile/gutenberg/packages/react-native-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecManager.java:375: Error: Unnecessary; SDK_INT is never < 21 [ObsoleteSdkInt]
if (Build.VERSION.SDK_INT >= VERSION_CODES.Q) {
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I think the original code would work just fine for the reasons stated above. However, I wrapped the code with the "build version" ifs. It's not nice but hopefully both local and CI lint will be happy :P.
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 think lint is quite confused
Nice job figuring out the magic incantation to make lint happy! 😄
It looks like we're missing an import for the @Require
annotation, which is causing the build failure. Also, what would you think about adding a comment explaining that we're jumping through these weird hoops because of (what looks like) a bug in lint? Otherwise, I feel like someone is eventually going to "fix" this and end up going down this same rabbithole once they start getting lint errors on their PR.
Alternatively, with a comment like that, I'd be fine just suppressing the relevant lint warning if you preferred to do that. I'm fine with either approach.
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 have filed an issue (wordpress-mobile/gutenberg-mobile#2653) for us to fix this "code change in gutenberg causes no problems until we try to run lint in WPAndroid" problem. Wanted to let you know because I figured you might have some thoughts on that given your recent experience. 😉
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.
Looks good and tested this out and everything is working well. I'll wait for your go-ahead to start the merge train @malinajirka (or feel free to ping someone else if I'm not around).
Congratulations on your first merged pull request, @malinajirka! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts: https://profiles.wordpress.org/me/profile/edit/ And if you don't have a WordPress.org account, you can create one on this page: https://login.wordpress.org/register Kudos! |
Do NOT merge this PR - we plan to merge it together with PRs from gutenberg-mobile and WPAndroid.
Description
This PR updates:
This change is required so we can update WPAndroid - draft PR.
How has this been tested?
I have tested the changes in WPAndroid - successful build should be enough.
Types of changes
Updates build setup for Android
Checklist: