-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
chore(cordova/android): outline android lib #1571
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for the changes. It's exciting to be able to use Kotlin!
I do have a few concerns, since this breaks the build and I'm not sure how the new API level affects our app.
/cc @daniellacosse @jyyi1
@@ -1,5 +1,6 @@ | |||
// Top-level build file where you can add configuration options common to all sub-projects/modules. | |||
plugins { | |||
// This version number must be kept in sync with AGP_VERSION in cordova-android. | |||
id 'com.android.library' version '7.2.1' apply false | |||
id 'com.android.library' version '7.4.1' apply false |
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.
This may be breaking our Android build: https://github.com/Jigsaw-Code/outline-client/actions/runs/4133200855/jobs/7149446491
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.
@@ -1,6 +1,6 @@ | |||
#Fri Jan 06 14:17:15 EST 2023 | |||
distributionBase=GRADLE_USER_HOME | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-7.4-bin.zip | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-7.5-bin.zip |
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.
Or perhaps this is breaking the build?
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 updated the gradle version to 7.5. The plugin org.jetbrains.kotlin.android
should be set to 7.4.1.
Here is the reference: https://developer.android.com/studio/releases/gradle-plugin#updating-gradle
|
||
defaultConfig { | ||
minSdk 22 | ||
targetSdk 32 | ||
targetSdk 33 |
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 don't think we can accept this change without ensuring our app works well with API 33.
Do you need this change? I believe you can set a different target SDK on your app.
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.
Yes, I can set the SDK to 32.
I made this change is because since Android Studio Electric Eel ( 2022.1.1 Patch 1), the initial compileSdk and targetSdk are 33 while starting a new project. I want developers who want to use outline SDK no need to downgrade the SDK version after they create a new project while developing their VPN.
So we keep the SDK version to 32?
@@ -2,9 +2,13 @@ | |||
<manifest xmlns:android="http://schemas.android.com/apk/res/android"> | |||
|
|||
<uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" /> | |||
<uses-permission android:name="android.permission.POST_NOTIFICATIONS" /> |
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.
Why is this needed?
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.
Foreground Service, which is VpnTunnelService
in OutlineAndroidLib, needs this permission with targetSDK 33.
Here is the reference: https://developer.android.com/develop/ui/views/notifications/notification-permission
@@ -30,627 +30,634 @@ | |||
import android.net.VpnService; |
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.
Could you please give an overview of what's changing in this file?
Also, please restore the 2-space indentation, so we can see the changes and for consistency.
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.
Sorry, my bad.
Here is what I've changed,
private void tearDownActiveTunnel() {
stopVpnTunnel();
stopForeground();
// add this line, line 361
broadcastVpnConnectivityChange(TunnelStatus.DISCONNECTED);
tunnelConfig = null;
stopNetworkConnectivityMonitor();
tunnelStore.setTunnelStatus(TunnelStatus.DISCONNECTED);
}
0595a72
to
efc3606
Compare
efc3606
to
2c07215
Compare
I found that we're using references: https://cordova.apache.org/announcements/2022/07/12/cordova-android-release-11.0.0.html |
Modification for developing outline Android SDK by nthlink.