Page MenuHomePhabricator

Build files set-up for Android 13
ClosedPublic

Authored by marcin on Mar 20 2023, 12:08 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 16, 5:51 AM
Unknown Object (File)
Fri, Dec 6, 9:49 PM
Unknown Object (File)
Fri, Dec 6, 6:10 PM
Unknown Object (File)
Thu, Dec 5, 8:27 PM
Unknown Object (File)
Thu, Dec 5, 8:27 PM
Unknown Object (File)
Thu, Dec 5, 7:34 AM
Unknown Object (File)
Nov 11 2024, 5:56 PM
Unknown Object (File)
Nov 10 2024, 12:35 AM
Subscribers

Details

Summary

This differential modifies Android build configuration files so that we can use Android 13 specific API on devices where it is
available. I was following instructions here: https://developer.android.com/develop/ui/views/notifications/notification-permission

Test Plan

This differential should be tested on both Android 12 and Android 13 devices. For credible results it is recommended to used physical device rather than emulator.

Test on Android 13:

  1. Fresh-install the app.
  2. Make sure that no notifications permission prompt appears neither when logged in nor when logged out.
  3. Make sure push notifs are disabled and not working unless it is directly changed in settings.

The reason for that is because since Android 13 notifications are not granted by default and unless app is configured to target Android 13 the system prompts for notifications permissions when it decides it is best to do so. If the app is configured to target Android 13, the system is not involved and it is the app responsibility to request those permissions. We have not implemented the code yet so permission are not granted until user directly grants them in settings app.

Test on Android 12:
We expected that older androids do not change their behaviour after applying this diffs as well as the entire stack. Push notifs are granted by default and should be working unless directly disabled in the settings app.

Both of those plans should be executed on dev and release builds.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Mar 20 2023, 12:10 PM
Harbormaster failed remote builds in B17424: Diff 23877!

(Ignore CI failure, that was my fault. Restarting)

This revision is now accepted and ready to land.Mar 20 2023, 1:29 PM
ashoat requested changes to this revision.Mar 20 2023, 3:08 PM

I'm not sure, but I think we should bump buildToolsVersion as well

native/android/build.gradle
5 ↗(On Diff #23877)

Typically, we bump this at the same time as bumping compileSdkVersion and targetSdkVersion. Eg. see here

22 ↗(On Diff #23877)

Just wondering, why was this change necessary? I did some research upstream in the React Native repository:

Upstream in React Native, starting from version 0.72 (we are on 0.70) the version here is removed entirely. I don't think we can do the same yet, since the commit notes:

I've also removed the explicit version in the template as now the AGP version is provided by RNGP. That means that the user will get the correct version they need.

But react-native-gradle-plugin is not used in version 0.70. The version of this was bumped a couple times before it was removed in React Native, but all of the changes happened in version 0.72.

It's probably okay to bump to 7.2.2, but it would be good to understand the reasoning here!

This revision now requires changes to proceed.Mar 20 2023, 3:08 PM
native/android/build.gradle
5 ↗(On Diff #23877)

I will make the change.

22 ↗(On Diff #23877)

Without this change everything worked but I was seeing a deprecation warning described here: https://stackoverflow.com/questions/69911807/we-recommend-using-a-newer-android-gradle-plugin-to-use-compilesdkpreview-sv2. After bumping AGP version to 7.2.2 the warning disappeared.

native/android/build.gradle
22 ↗(On Diff #23877)

Thanks!! It can be good to explain this stuff ahead of time, so we can reduce review churn

not very well versed in gradle, but please add me to cmake related stuff in the future :)

Please update native/expo-modules/aes-crypto/android/build.gradle before landing!

native/expo-modules/android-lifecycle/android/build.gradle
75

It looks like we'll need to update native/expo-modules/aes-crypto/android/build.gradle as well

This revision is now accepted and ready to land.Mar 22 2023, 11:39 AM

Important changes for media files permission and ability to launch MainActivity from SplachActivity

native/android/app/src/debug/AndroidManifest.xml
26 ↗(On Diff #24026)

Context for this change is more complex. When I first modified .gradle files to target android 13 every time I run react-native run-android I was receiving the following error:

Starting: Intent { cmp=app.comm.android/.MainActivity }
Error type 3
Error: Activity class {app.comm.android/app.comm.android.MainActivity} does not exist.

However the app was successfully installed and I was able to launch it by tapping icon. Therefore I postponed fixing this error since I wanted to get the diffs up as soon as possible. After some research I found this article: https://medium.com/androiddevelopers/making-sense-of-intent-filters-in-android-13-8f6656903dde
It states that since Android 13 even explicit intents are checked against intent filters. Therefore after our migration the intent that we use in SplashActivity to start MainActivity does not pass intent-filter data test. After discussion with @bartek I decided to add additional intent filter that is compliant with intent from SplashActivity.java we use to start MainActivity.

native/android/app/src/main/AndroidManifest.xml
12 ↗(On Diff #24026)
ashoat requested changes to this revision.Mar 23 2023, 6:59 PM

Can you please amend the Test Plan to include:

  1. Please test release builds (as well as debug builds) on both Android 12 and 13
  2. Please provide details on whether you used a simulator or an emulator
  3. Please mention how you tested the app (ie. not just that it built, but that it ran and you could use it)
native/android/app/src/debug/AndroidManifest.xml
26 ↗(On Diff #24026)

Thanks for explaining! I spent a long time researching this, trying to figure out if this change should really be in main/AndroidManifest.xml. Regardless of whether we place in main or debug, the Android Studio error that @bartek first mentioned in D6320 appears to be unavoidable:

Screenshot 2023-03-23 at 9.53.39 PM.png (708×1 px, 238 KB)

Some questions:

  1. We can silence that warning with tools:ignore="AppLinkUrlError". Should we do that?
  2. It's not clear how the SplashActivity -> MainActivity transition works in a release build, where MainActivity declares no intent-filter. Does the explicit intent always get rejected since there is no matching intent-filter? Or are explicit intents not checked against the intent-filters since there are none defined?
    • I tried to search for documentation on this, but couldn't find any. There's barely any documentation about the Android 13 change regarding intent filters!
    • If you can find some official documentation to explain what happens with an explicit intent when there are no intent-filters defined, then we can keep this change only in the debug manifest
    • Either way, the Test Plan should be amended to include testing a release build

Even if the release build works, I would prefer to move this to the main manifest file unless we find official documentation as described above. I'd rather not depend on "undefined behavior"... it seems safer to define a compatible intent filter than to rely on explicit intents working like this in the future.

native/android/app/src/main/AndroidManifest.xml
15 ↗(On Diff #24026)

This comment seems to imply that everything below it is required to maintain app compatiblity. Can you reorder so that READ_EXTERNAL_STORAGE (and the associated comment) are at the end of the list of uses-permission declarations, to avoid implying that eg. CAMERA is required to maintain app compatibility?

This revision now requires changes to proceed.Mar 23 2023, 6:59 PM
native/android/app/src/debug/AndroidManifest.xml
26 ↗(On Diff #24026)
  1. Regarding that silence - generally I thought it's weird that Android Studio marks this as error, while the documentation says:

To specify accepted intent data, an intent filter can declare zero or more <data> elements,

Then I discovered that changing action.VIEW to e.g. action.MAIN silences that warning. This implies that some actions require <data> to be set while others not. Maybe we should find an action type that better fits this case?


  1. Generally I'm leaning toward moving this to the main manifest. The only reason I didn't rush forcing it was that @marcin ensured me offline that he tested this on a release build.
native/android/app/src/debug/AndroidManifest.xml
26 ↗(On Diff #24026)
  1. In the article I linked it is stated that since Android 13 explicit intents are checked against intent-filters. Older android versions used intent-filters exclusively against implicit intents. Intent matching is more complex. Briefly there are the rules:

https://developer.android.com/guide/components/intents-filters#Resolution

  • If there are no intent filters the intent always matches. If there are many we need to just match one.
  • If intent filter declares no action then no intent matches filter. However if there are actions in the filter but not in the intent the intent matches the filter. Intent can't have different action than the one declared in the filter
  • Same for categories. But since CATEGORY_DEFAULT is always in intent then intent filter must have this category so that it can be matched.
  • If filter declares data then intent MUST contain data that matches data format declared in the filter. If intent has no data then we fail the test.

We can silence that warning with tools:ignore="AppLinkUrlError". Should we do that?

Actually after searching a bit I found a way to avoid warning without suppressing. We can actually define our own custom action for the intent and intent filter: https://developer.android.com/guide/components/intents-filters#Receiving. So I defined the following intent filter in main (worked in debug as well) manifest:

<intent-filter>
  <action android:name="app.comm.android.START_MAIN_ACTIVITY" />
  <category android:name="android.intent.category.DEFAULT" />
</intent-filter>

This solution also works without warning in Android Studio. Based on the matching rules I described above, the action field in an intent filter doesn't matter as long as it is there and intent does not carry any action on its own. The category field DEFAULT is a must.

I would proceed with this solution, however would first like to receive reviewers opinions. cc @ashoat , @bartek

native/android/app/src/debug/AndroidManifest.xml
26 ↗(On Diff #24026)

After reading the documentation and based on my own experiences, your solution looks totally legit to me.

If you tested:

  • running both debug and release builds
  • on both of these you tried:
    • running this from adb cli / react-native run-android
    • running this by clicking launcher icon

Then I'm OK with your approach. Just be sure to make this change in main AndroidManifest, not the debug one

Add intent-filter for MainActivity to be started from SplashActivity in debug build

Awesome!! Thanks for taking the time to revise this a bunch, I know it was more complicated than initially expected

This revision is now accepted and ready to land.Mar 24 2023, 12:28 PM
This revision was automatically updated to reflect the committed changes.