Bugfix/issue 1812 Android 13 Support #1826

JulianKast · 2022-08-25T19:10:52Z

Fixes #1812

This PR is [ready] for review.

Risk

This PR makes [minor] API changes.

Testing Plan

I have verified that I have not introduced new warnings in this PR (or explain why below)
I have run the unit tests with this PR
I have tested this PR against Core and verified behavior (if applicable, if not applicable, explain why below).
I have tested Android

Unit Tests

n/a

Core Tests

POST_NOTIFICATIONS TESTS

1 app API 33 on Android 13 device

Test 1:

App request POST_NOTIFICATIONS permission.
Accept permission
Connect to core

Results: Notifications appear and the app works as expected

Test 2:

App request POST_NOTIFICATIONS permission.
Click Don't Allow for permission.
Connect to core

Results: Notifications do not appear and the app works as expected

Test 3:

App request POST_NOTIFICATIONS permission.
Swipe away the request to allow notifications.
Connect to core

Results: Notifications do not appear and the app works as expected

Test 4:

App does not request POST_NOTIFICATIONS permission.
Connect to core

Results: Notifications do not appear and the app works as expected

1 app API 31 on Android 13 device

Test 5:

Connect to core
Accept Post Notification permission(Automatically sent by android)

Results: Notifications appear and the app works as expected

Test 6:

Connect to core
Click Don't Allow for Post Notification permission(Automatically sent by android)

Results: Notifications do not appear and the app works as expected

Test 7:

Connect to core
Swip away the request to allow notifications(Automatically sent by android).

Results: Notifications do not appear and the app works as expected

App 1 API 33, App 2 API 31 on Android 13 device

Test 8:

Accept notification permission for App 1.
Connects to core
Accept notification permissions for App 2 (Note Automatically sent by android you have to open app to view dialog to accept/deny permissions)

Results: Notifications appear and the app works as expected

Test 9:

Install in order App 2 then App 1
Accept notification permission for App 1
Connects to core
Deny permission for App 2

Results: Notifications appear for App 1 and RouterScervice but not for App 2. Apps work as expected

Test 10

Install App 1
Deny notification permission
Connect to core
Disconnects from core
Install App 2
Connects to core
Accepts notification permission

Expected Results: No notifications for App 1 and RouterService, App 2 has 1 notification. Apps work as expected.

Observed behavior: No Notifications at all until you disconnect and reconnect to core then App 2 has 1 notification. Outside of that sdl still works fine.

Core version / branch / commit hash / module tested against: Sync 3.0
HMI name / version / branch / commit hash / module tested against: Sync 3.0

Summary

This Pr makes updates to target API 33 as well as update hello_sdl_android to demonstrate how to request POST_NOTIFICATIONS permission added in Android 13.

There is one issue with test 10 where no notification appears when one should, but after a disconnect and reconnect it works itself out and the behavior of SDL is not altered outside of displayed notifications.

Notifications are still required, but they do not have to be displayed if they don't have the proper permissions. Another major downside to this is if you accidentally dismiss the BLUETOOTH_CONNECT permission and the POST_NOTIFICATIONS it does not give the user any sense of what they did wrong, but that should work itself out after the app gets closed and opened again.

Changelog

Bug Fixes

Demonstrate requesting POST_NOTIFICATIONS permission

CLA

I have signed the CLA

… app

codecov · 2022-08-25T19:20:20Z

Codecov Report

Merging #1826 (881bbc2) into develop (4f8e483) will increase coverage by 0.01%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             develop    #1826      +/-   ##
=============================================
+ Coverage      54.04%   54.05%   +0.01%     
- Complexity      5532     5535       +3     
=============================================
  Files            562      562              
  Lines          25821    25821              
  Branches        3398     3398              
=============================================
+ Hits           13954    13958       +4     
+ Misses         10596    10594       -2     
+ Partials        1271     1269       -2

Impacted Files	Coverage Δ
...rtdevicelink/streaming/video/SdlRemoteDisplay.java	`51.21% <0.00%> (-1.22%)`	⬇️
...ink/managers/screen/BaseTextAndGraphicManager.java	`64.58% <0.00%> (+0.41%)`	⬆️
...managers/screen/TextAndGraphicUpdateOperation.java	`71.70% <0.00%> (+0.54%)`	⬆️
...nk/managers/audio/AudioDecoderCompatOperation.java	`79.54% <0.00%> (+4.54%)`	⬆️

joeygrover · 2022-08-31T18:41:38Z

android/hello_sdl_android/src/main/java/com/sdl/hellosdlandroid/MainActivity.java

+            if (permissionsNeeded().length > 0) {
+                requestPermission(permissionsNeeded(), REQUEST_CODE);


Suggested change

if (permissionsNeeded().length > 0) {

requestPermission(permissionsNeeded(), REQUEST_CODE);

String[] permissionsNeeded = permissionsNeeded();

if (permissionsNeeded.length > 0) {

requestPermission(permissionsNeeded, REQUEST_CODE);

joeygrover · 2022-08-31T18:42:25Z

android/hello_sdl_android/src/main/java/com/sdl/hellosdlandroid/MainActivity.java

+                if (checkBTPermission()) {
+                    return;
+                }


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

Suggested change

if (checkBTPermission()) {

return;

}

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

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.

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.

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.

joeygrover · 2022-08-31T19:00:22Z

android/hello_sdl_android/src/main/java/com/sdl/hellosdlandroid/MainActivity.java

+        ActivityCompat.requestPermissions(this, permissions, REQUEST_CODE);
+    }
+
+    private String[] permissionsNeeded() {


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

Suggested change

private String[] permissionsNeeded() {

private @NonNull String[] permissionsNeeded() {

joeygrover · 2022-08-31T20:46:08Z

android/hello_sdl_android/src/main/java/com/sdl/hellosdlandroid/MainActivity.java

-                return;
+            String[] permissionsNeeded = permissionsNeeded();
+            if (permissionsNeeded.length > 0) {
+                requestPermission(permissionsNeeded(), REQUEST_CODE);


Suggested change

requestPermission(permissionsNeeded(), REQUEST_CODE);

requestPermission(permissionsNeeded, REQUEST_CODE);

joeygrover · 2022-08-31T20:56:06Z

android/hello_sdl_android/src/main/java/com/sdl/hellosdlandroid/MainActivity.java

+                if (checkBTPermission()) {
+                    return;
+                }


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.

joeygrover · 2022-08-31T20:57:34Z

android/hello_sdl_android/src/main/java/com/sdl/hellosdlandroid/MainActivity.java

+                if (checkBTPermission()) {
+                    return;
+                }


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.

JulianKast added 2 commits August 25, 2022 15:08

Update target and compile sdk to api 33 for sdl_android and hello sdl…

8dd082d

… app

Add POST_NOTIFICATION permission to hello_SDL

70754e7

JulianKast added 2 commits August 29, 2022 16:43

Add comments for denying permission

7ba0d67

Formatting fixes

399f373

joeygrover requested changes Aug 31, 2022

View reviewed changes

Review updates

bf7e660

joeygrover requested changes Aug 31, 2022

View reviewed changes

JulianKast added 2 commits September 4, 2022 12:44

Flip permission methods and rename them

cb1c48e

Check local array to see if we need bluetooth connect permission

881bbc2

joeygrover approved these changes Sep 6, 2022

View reviewed changes

joeygrover merged commit 77db656 into develop Sep 6, 2022

joeygrover deleted the bugfix/issue_1812_Android_13 branch September 6, 2022 20:33

joeygrover mentioned this pull request Sep 6, 2022

Android 13 issues #1812

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/issue 1812 Android 13 Support #1826

Bugfix/issue 1812 Android 13 Support #1826

JulianKast commented Aug 25, 2022 •

edited

Loading

codecov bot commented Aug 25, 2022 •

edited

Loading

joeygrover Aug 31, 2022

joeygrover Aug 31, 2022

JulianKast Aug 31, 2022 •

edited

Loading

joeygrover Aug 31, 2022

joeygrover Aug 31, 2022

JulianKast Sep 4, 2022

joeygrover Aug 31, 2022

joeygrover Aug 31, 2022

joeygrover Aug 31, 2022

joeygrover Aug 31, 2022

		if (permissionsNeeded().length > 0) {
		requestPermission(permissionsNeeded(), REQUEST_CODE);

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

	requestPermission(permissionsNeeded(), REQUEST_CODE);
	requestPermission(permissionsNeeded, REQUEST_CODE);

Bugfix/issue 1812 Android 13 Support #1826

Bugfix/issue 1812 Android 13 Support #1826

Conversation

JulianKast commented Aug 25, 2022 • edited Loading

Risk

Testing Plan

Unit Tests

Core Tests

Summary

Changelog

Bug Fixes

CLA

codecov bot commented Aug 25, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JulianKast Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JulianKast commented Aug 25, 2022 •

edited

Loading

codecov bot commented Aug 25, 2022 •

edited

Loading

JulianKast Aug 31, 2022 •

edited

Loading