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
Details
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:
- Fresh-install the app.
- Make sure that no notifications permission prompt appears neither when logged in nor when logged out.
- 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
I'm not sure, but I think we should bump buildToolsVersion as well
native/android/build.gradle | ||
---|---|---|
5 | Typically, we bump this at the same time as bumping compileSdkVersion and targetSdkVersion. Eg. see here | |
22 | 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:
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! |
native/android/build.gradle | ||
---|---|---|
5 | I will make the change. | |
22 | 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 | 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 ↗ | (On Diff #23938) | It looks like we'll need to update native/expo-modules/aes-crypto/android/build.gradle as well |
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 |
native/android/app/src/main/AndroidManifest.xml | ||
12 ↗ | (On Diff #24026) | Context for this change is here: https://medium.com/tech-takeaways/migrating-my-app-to-android-13-f5ad0649d23d |
Can you please amend the Test Plan to include:
- Please test release builds (as well as debug builds) on both Android 12 and 13
- Please provide details on whether you used a simulator or an emulator
- 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: Some questions:
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? |
native/android/app/src/debug/AndroidManifest.xml | ||
---|---|---|
26 ↗ | (On Diff #24026) |
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?
|
native/android/app/src/debug/AndroidManifest.xml | ||
---|---|---|
26 ↗ | (On Diff #24026) |
https://developer.android.com/guide/components/intents-filters#Resolution
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:
Then I'm OK with your approach. Just be sure to make this change in main AndroidManifest, not the debug one |
Awesome!! Thanks for taking the time to revise this a bunch, I know it was more complicated than initially expected