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

Android12 #4433

Merged
merged 27 commits into from
Nov 16, 2021
Merged

Android12 #4433

merged 27 commits into from
Nov 16, 2021

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented Nov 9, 2021

Ok, sanity test is passing (a problem at the end, will run it again), but see 0ecf1a4 to be able to run it.

The PR can be reviewed and CI should be happy now.

Notification are not working on the emulator, I'm testing F-Droid version. Will check on GPlay version, but weird. Quick reply from notification has to be tested because the Pending Intent should be MUTABLE in this case (but now it's IMMUTABLE), see https://developer.android.com/guide/components/intents-filters#CreateImmutablePendingIntents . I wanted to test it before applying the fix.

Edit: This is probably a pb with the emulator (or with my test account?).
The notification troubleshoot is working fine, the PUSH test is received, but take about 20s to come to the app...
Push tested OK on real device with Android 10.
We should merge the PR and see what happen during the next release.

Edit 2: room notification are now working fine thanks to dddcbfb

Fixes #4262

@github-actions
Copy link

github-actions bot commented Nov 9, 2021

Unit Test Results

  66 files  +  4    66 suites  +4   55s ⏱️ +3s
135 tests +17  135 ✔️ +17  0 💤 ±0  0 ±0 
418 runs  +68  418 ✔️ +68  0 💤 ±0  0 ±0 

Results for commit dddcbfb. ± Comparison against base commit a758225.

♻️ This comment has been updated with latest results.

@bmarty bmarty marked this pull request as ready for review November 10, 2021 18:39
bmarty and others added 24 commits November 15, 2021 12:24
…et when targeting Android 12

Do it for services coming from dependencies
Bumps work-runtime-ktx from 2.6.0 to 2.7.0.

---
updated-dependencies:
- dependency-name: androidx.work:work-runtime-ktx
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps lifecycle-livedata-ktx from 2.3.1 to 2.4.0.

---
updated-dependencies:
- dependency-name: androidx.lifecycle:lifecycle-livedata-ktx
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps core-ktx from 1.6.0 to 1.7.0.

---
updated-dependencies:
- dependency-name: androidx.core:core-ktx
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps browser from 1.3.0 to 1.4.0.

---
updated-dependencies:
- dependency-name: androidx.browser:browser
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
java.lang.SecurityException: To use the sampling rate of 0 microseconds, app needs to declare the normal permission HIGH_SAMPLING_RATE_SENSORS.
And make the code more efficient, since we call getColumnIndexOrNull only once and not on each cursor iteration
Ensure that column index is not -1
LiveData value assignment nullability mismatch
I guess we accept only images coming from the keyboard.
@bmarty bmarty force-pushed the feature/bma/android12 branch from 0ecf1a4 to fb8b720 Compare November 15, 2021 11:26
@@ -417,6 +420,22 @@
android:name="android.support.FILE_PROVIDER_PATHS"
android:resource="@xml/sdk_provider_paths" />
</provider>

<!-- Temporary fix for Android 12. android:exported has to be explicitly set when targeting Android 12
Do it for services coming from dependencies - BEGIN -->
Copy link
Contributor

Choose a reason for hiding this comment

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

💯 hopefully the libraries will update their targets soonish~ (especially androidx)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Jitsi lib has not been upgrading since a while, and it requires some manual steps. See https://github.com/vector-im/element-android/blob/main/docs/jitsi.md if you are curious (I know you are :) )

while (cursor.moveToNext()) {
val mimeType = cursor.getString(cursor.getColumnIndex(ContactsContract.Data.MIMETYPE))
val contactData = cursor.getString(cursor.getColumnIndex(ContactsContract.Data.DATA1))
)?.use inner@{ innerCursor ->
Copy link
Contributor

@ouchadam ouchadam Nov 16, 2021

Choose a reason for hiding this comment

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

this is my first time seeing inner@{, what does it do?

Copy link
Member Author

Choose a reason for hiding this comment

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

"inner" is just a label, it could be anything else. It allows to use return@inner below (lines 77 and 78). Using classical return@use is not possible there since we are in 2 nested use blocks, so is ambiguous.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah thanks for explaining

@@ -57,7 +56,7 @@ class AppStateHandler @Inject constructor(
private val sessionDataSource: ActiveSessionDataSource,
private val uiStateRepository: UiStateRepository,
private val activeSessionHolder: ActiveSessionHolder
) : LifecycleObserver {
) : DefaultLifecycleObserver {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice clean up! might speed up the lifecycle KAPT processing a tiny bit as well 🤞

@@ -46,7 +46,7 @@ class RageShake @Inject constructor(private val activity: FragmentActivity,

shakeDetector = ShakeDetector(this).apply {
setSensitivity(vectorPreferences.getRageshakeSensitivity())
start(sensorManager)
start(sensorManager, SensorManager.SENSOR_DELAY_GAME)
Copy link
Contributor

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'll have any issues but wanted to highlight this means we've changed fromSENSOR_DELAY_FASTEST to SENSOR_DELAY_GAME

I wonder if SENSOR_DELAY_UI could be more suited? haven't tested, assuming from the docs...

/** get sensor data as fast as possible */
public static final int SENSOR_DELAY_FASTEST = 0;
/** rate suitable for games */
public static final int SENSOR_DELAY_GAME = 1;
/** rate suitable for the user interface  */
public static final int SENSOR_DELAY_UI = **2;**

Copy link
Member Author

Choose a reason for hiding this comment

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

TBH I haven't checked SENSOR_DELAY_UI. The change was necessary because when using SENSOR_DELAY_FASTEST we need to add a permission in the manifest.
Rageshake still works with SENSOR_DELAY_GAME, I did not investigate more.

Copy link
Contributor

@ouchadam ouchadam left a comment

Choose a reason for hiding this comment

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

awesome stuff! 💯 thanks for updating the blocked dependencies as well

some small comments but no blockers

@bmarty bmarty merged commit be3aafe into develop Nov 16, 2021
@bmarty bmarty deleted the feature/bma/android12 branch November 16, 2021 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Android 12 (API 31)
2 participants