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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Sep 23, 2020
Merged

Update target and compile Android sdk versions to v29 #25488

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1334c24
Update target and compile Android sdk versions to v29
malinajirka Sep 21, 2020
f29f99b
Fix justification mode lint errors/warnings
malinajirka Sep 22, 2020
ca2c3dd
Merge branch 'master' into update/android-target-and-compile-sdk-vers…
malinajirka Sep 22, 2020
baf1822
Add missing import and a comment
malinajirka Sep 23, 2020
1a8730e
Fix missing import
malinajirka Sep 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/react-native-aztec/android/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,11 @@ List<String> dirs = [
'template'] // boilerplate code that is generated by the sample template process

android {
compileSdkVersion 28
compileSdkVersion 29

defaultConfig {
minSdkVersion 21
targetSdkVersion 28
targetSdkVersion 29
}

compileOptions {
Expand Down
5 changes: 3 additions & 2 deletions ...-aztec/android/src/main/java/org/wordpress/mobile/ReactNativeAztec/ReactAztecManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import android.graphics.Color;
import android.graphics.Typeface;
import android.graphics.text.LineBreaker;
import android.os.Build;
import androidx.annotation.Nullable;
import androidx.core.graphics.ColorUtils;
Expand Down Expand Up @@ -370,12 +371,12 @@ private static int parseNumericFontWeight(String fontWeightString) {
public void setTextAlign(ReactAztecText view, @Nullable String textAlign) {
if ("justify".equals(textAlign)) {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
view.setJustificationMode(Layout.JUSTIFICATION_MODE_INTER_WORD);
view.setJustificationMode(LineBreaker.JUSTIFICATION_MODE_INTER_WORD);
}
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);
Copy link
Contributor

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...;
}

Copy link
Contributor Author

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 :).

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@mchowning mchowning Sep 22, 2020 •
edited
Loading

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.

Copy link
Contributor

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. 😉

}

if (textAlign == null || "auto".equals(textAlign)) {
Expand Down
4 changes: 2 additions & 2 deletions packages/react-native-bridge/android/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,11 @@ def hermesPath = hermesOriginalPath;
def buildAssetsFolder = 'build/assets'

android {
compileSdkVersion 28
compileSdkVersion 29

defaultConfig {
minSdkVersion 21
targetSdkVersion 28
targetSdkVersion 29
versionCode 1
versionName "1.0"
}
Expand Down
6 changes: 3 additions & 3 deletions packages/react-native-editor/android/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ buildscript {
ext {
gradlePluginVersion = '3.4.2'
kotlinVersion = '1.3.61'
buildToolsVersion = "28.0.3"
buildToolsVersion = "29.0.2"
minSdkVersion = 21
compileSdkVersion = 28
targetSdkVersion = 28
compileSdkVersion = 29
targetSdkVersion = 29
supportLibVersion = '28.0.0'
wordpressUtilsVersion = '1.22'
}
Expand Down