SecureBackup: update matrix sdk module. #1653
Conversation
| @@ -58,7 +58,7 @@ class DefaultLogoutPreferencePresenter @Inject constructor(private val matrixCli | |||
|
|
|||
| private fun CoroutineScope.logout(logoutAction: MutableState<Async<String?>>) = launch { | |||
| suspend { | |||
| matrixClient.logout() | |||
| matrixClient.logout(false /* TODO */) | |||
There was a problem hiding this comment.
Will be updated in a later PR.
|
Kudos, SonarCloud Quality Gate passed!
|
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1653 +/- ##
===========================================
- Coverage 63.06% 63.04% -0.03%
===========================================
Files 1215 1219 +4
Lines 31345 31392 +47
Branches 6447 6450 +3
===========================================
+ Hits 19768 19790 +22
- Misses 8604 8629 +25
Partials 2973 2973
☔ View full report in Codecov by Sentry. |
| import kotlinx.coroutines.flow.StateFlow | ||
|
|
||
| interface EncryptionService { | ||
| val backupStateStateFlow: StateFlow<BackupState> |
There was a problem hiding this comment.
Just style:
StateState :/
unless we have a very strict naming scheme we want to stick to I'd suggest we could drop the StateFlow suffix and just use Flow even though it doesn't match the actual type name.
There was a problem hiding this comment.
Yes, I agree it's not ideal... I will change it in a coming PR.
| val backupUploadStateStateFlow: StateFlow<BackupUploadState> | ||
| val enableRecoveryProgressStateFlow: StateFlow<EnableRecoveryProgress> | ||
|
|
||
| suspend fun enableBackups(): Result<Unit> |
There was a problem hiding this comment.
Is there any way to disable them (I mean... there's a enableBackups API but not a diableBackups one... is it possible to disable the backup?)?
There was a problem hiding this comment.
Yes, there is one, but the name from the SDK is disableRecovery, do not ask me why. I will document the API later.
We have to decide if we stick to the SDK name or if we change it to make is clearer.
|
|
||
| suspend fun enableBackups(): Result<Unit> | ||
|
|
||
| suspend fun isLastDevice(): Result<Boolean> |
There was a problem hiding this comment.
Missing context: last what? Maybe add docstring
I mean.. last device that logged in? Last device created? Last device used?
There was a problem hiding this comment.
This is the last Session, so last device that logged in. Will document the API.
| } | ||
| ) | ||
| // enableRecovery returns the encryption key, but we read it from the state flow | ||
| .let { } |
There was a problem hiding this comment.
Why .let{} when Unit was enough ?
There was a problem hiding this comment.
Unit is working, but quality check does not like it.
There was a problem hiding this comment.
Why doesn't it? It's the most correct thing to do in this case kotlin-wise.
There was a problem hiding this comment.
Yeah, I agree. Detekt is reporting this:
~/workspaces/element-x-android/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/encryption/RustEncryptionService.kt:84:13: A single Unit expression is unnecessary and can safely be removed. [OptionalUnit]
There are several similar reports in the detekt project https://github.com/detekt/detekt, but all the issues seems to be close.
In the mean time, using .let { } is the solution, and it's already used in the codebase.
There was a problem hiding this comment.
Seems like a detekt bug
|
|
||
| override suspend fun enableBackups(): Result<Unit> = withContext(dispatchers.io) { | ||
| runCatching { | ||
| service.enableBackups() |
There was a problem hiding this comment.
Just a question, why none of these ffi methods are suspend? Didn't we just get rid of all *blocking versions of many APIs?
SDK part of #1648.