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

Bugfix/issue 1812 Android 13 Support #1826

Merged
merged 7 commits into from
Sep 6, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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 android/hello_sdl_android/build.gradle
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
apply plugin: 'com.android.application'

android {
compileSdkVersion 31
compileSdkVersion 33
defaultConfig {
applicationId "com.sdl.hellosdlandroid"
minSdkVersion 16
targetSdkVersion 31
targetSdkVersion 33
versionCode 1
versionName "1.0"
testInstrumentationRunner 'androidx.test.runner.AndroidJUnitRunner'
Expand Down
2 changes: 2 additions & 0 deletions android/hello_sdl_android/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
<uses-permission android:name="android.permission.BLUETOOTH" />
<uses-permission android:name="android.permission.BLUETOOTH_CONNECT"
tools:targetApi="31"/>
<uses-permission android:name="android.permission.POST_NOTIFICATIONS"
tools:targetApi="33"/>
<uses-permission android:name="android.permission.INTERNET" />
<!-- Required to check if WiFi is enabled -->
<uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" />
Expand Down
Original file line number Diff line number Diff line change
@@ -1,18 +1,17 @@
package com.sdl.hellosdlandroid;

import android.Manifest;
import android.content.Intent;
import android.content.pm.PackageManager;
import android.os.Build;
import android.os.Bundle;
import android.view.Menu;
import android.view.MenuItem;

import androidx.annotation.NonNull;
import androidx.appcompat.app.AppCompatActivity;
import androidx.core.app.ActivityCompat;
import androidx.core.content.ContextCompat;

import static android.Manifest.permission.BLUETOOTH_CONNECT;
import java.util.ArrayList;

public class MainActivity extends AppCompatActivity {

Expand All @@ -23,12 +22,14 @@ protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
setContentView(R.layout.activity_main);


if (BuildConfig.TRANSPORT.equals("MULTI") || BuildConfig.TRANSPORT.equals("MULTI_HB")) {
if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.S && !checkPermission()) {
requestPermission();
return;
if (permissionsNeeded().length > 0) {
requestPermission(permissionsNeeded(), REQUEST_CODE);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (permissionsNeeded().length > 0) {
requestPermission(permissionsNeeded(), REQUEST_CODE);
String[] permissionsNeeded = permissionsNeeded();
if (permissionsNeeded.length > 0) {
requestPermission(permissionsNeeded, REQUEST_CODE);

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
requestPermission(permissionsNeeded(), REQUEST_CODE);
requestPermission(permissionsNeeded, REQUEST_CODE);

if (checkBTPermission()) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this is still needed? Seems like we could get rid of it.

Suggested change
if (checkBTPermission()) {
return;
}

Copy link
Contributor Author

@JulianKast JulianKast Aug 31, 2022

Choose a reason for hiding this comment

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

This is needed because we need to not call SdlReceiver.queryForConnectedService(this); below only if we need the BLUETOOTH_CONNECT permission.

We could change it below at line 35 to

            if (!checkBTPermission()) {
                //If we are connected to a module we want to start our SdlService
                SdlReceiver.queryForConnectedService(this);
            }

Copy link
Member

Choose a reason for hiding this comment

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

With having an array with strings that contain our permission needs already it seems like we could check those with something like:

for (String permission : permissionsNeeded) {
    if(Manifest.permission.BLUETOOTH_CONNECT.equals(permission)){
        return;
    }
}

I just don't know the efficiency of checking for the permission again, and it seems redundant. At the very least I would say your suggestions should be used as it's bit more concise.

Copy link
Member

Choose a reason for hiding this comment

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

Another option would be to flip the methods for checking permission into returning if the permission is granted instead of currently returning if it is not granted, because that in itself is a big confusing as well.

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 agree that it was confusing especially with the naming of those methods, I flipped them and renamed to hasPNPermission and hasBTPermission. I wanted to not flip them and name them needPNPermision and needBTPermission since its checking the API level and then the permission, But need really isn't a proper java naming convention that I could find, so I added some JavDocs to them.

}

//If we are connected to a module we want to start our SdlService
SdlReceiver.queryForConnectedService(this);
} else if (BuildConfig.TRANSPORT.equals("TCP")){
Expand All @@ -37,24 +38,53 @@ protected void onCreate(Bundle savedInstanceState) {
}
}

private boolean checkPermission() {
return PackageManager.PERMISSION_GRANTED == ContextCompat.checkSelfPermission(getApplicationContext(), BLUETOOTH_CONNECT);
private boolean checkBTPermission() {
return Build.VERSION.SDK_INT >= Build.VERSION_CODES.S && !checkPermission(Manifest.permission.BLUETOOTH_CONNECT);
}

private boolean checkPNPermission() {
return Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU && !checkPermission(Manifest.permission.POST_NOTIFICATIONS);
}

private void requestPermission() {
ActivityCompat.requestPermissions(this, new String[]{BLUETOOTH_CONNECT}, REQUEST_CODE);
private boolean checkPermission(String permission) {
return PackageManager.PERMISSION_GRANTED == ContextCompat.checkSelfPermission(getApplicationContext(), permission);
}

private void requestPermission(String[] permissions, int REQUEST_CODE) {
ActivityCompat.requestPermissions(this, permissions, REQUEST_CODE);
}

private String[] permissionsNeeded() {
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a big deal, but still a good idea.

Suggested change
private String[] permissionsNeeded() {
private @NonNull String[] permissionsNeeded() {

ArrayList<String> result = new ArrayList<>();
if (checkBTPermission()) {
result.add(Manifest.permission.BLUETOOTH_CONNECT);
}
if (checkPNPermission()) {
result.add(Manifest.permission.POST_NOTIFICATIONS);
}
return (result.toArray(new String[result.size()]));
}

@Override
public void onRequestPermissionsResult(int requestCode, @NonNull String[] permissions, @NonNull int[] grantResults) {
switch (requestCode) {
case REQUEST_CODE:
if (grantResults.length > 0) {

boolean btConnectGranted = grantResults[0] == PackageManager.PERMISSION_GRANTED;

if (btConnectGranted) {
SdlReceiver.queryForConnectedService(this);
for (int i = 0; i < grantResults.length; i++) {
if (permissions[i].equals(Manifest.permission.BLUETOOTH_CONNECT)) {
boolean btConnectGranted =
grantResults[i] == PackageManager.PERMISSION_GRANTED;
if (btConnectGranted) {
SdlReceiver.queryForConnectedService(this);
}
} else if (permissions[i].equals(Manifest.permission.POST_NOTIFICATIONS)) {
boolean postNotificationGranted =
grantResults[i] == PackageManager.PERMISSION_GRANTED;
if (!postNotificationGranted) {
// User denied permission, Notifications for SDL will not appear
// on Android 13 devices.
}
}
}
}
break;
Expand Down
4 changes: 2 additions & 2 deletions android/sdl_android/build.gradle
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
apply plugin: 'com.android.library'

android {
compileSdkVersion 31
compileSdkVersion 33
defaultConfig {
minSdkVersion 16
targetSdkVersion 31
targetSdkVersion 33
versionCode 23
versionName new File(projectDir.path, ('/../../VERSION')).text.trim()
buildConfigField "String", "VERSION_NAME", '\"' + versionName + '\"'
Expand Down