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.

Sign up for GitHub

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

Fix the logic of the room list banner state #3615

Merged
merged 5 commits into from
Oct 8, 2024
Merged

Fix the logic of the room list banner state #3615

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0b2ae98
Remove `deriveStateOf` preventing the banner state from updating
jmartinesp Oct 7, 2024
1b8b176
Fix tests
jmartinesp Oct 7, 2024
e88d74a
Merge remote-tracking branch 'origin/develop' into fix/jme/make-sure-…
jmartinesp Oct 7, 2024
434fbd4
Try improving implementation of the check
jmartinesp Oct 7, 2024
230abd3
No need for `syncState`
jmartinesp Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 32 additions & 18 deletions ...mlist/impl/src/main/kotlin/io/element/android/features/roomlist/impl/RoomListPresenter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@
import io.element.android.libraries.matrix.api.encryption.RecoveryState
import io.element.android.libraries.matrix.api.roomlist.RoomList
import io.element.android.libraries.matrix.api.sync.SyncService
import io.element.android.libraries.matrix.api.sync.SyncState
import io.element.android.libraries.matrix.api.timeline.ReceiptType
import io.element.android.libraries.preferences.api.store.SessionPreferencesStore
import io.element.android.libraries.push.api.notifications.NotificationCleaner
Expand Down Expand Up @@ -173,33 +172,48 @@
}

@Composable
private fun securityBannerState(
private fun rememberSecurityBannerState(
securityBannerDismissed: Boolean,
needsSlidingSyncMigration: Boolean,
Copy link
Member

Choose a reason for hiding this comment

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

Is it not because needsSlidingSyncMigration needs to be wrapped in a state like securityBannerDismissed, with rememberUpdatedState?

): State<SecurityBannerState> {
val currentSecurityBannerDismissed by rememberUpdatedState(securityBannerDismissed)
val currentNeedsSlidingSyncMigration by rememberUpdatedState(needsSlidingSyncMigration)
val recoveryState by encryptionService.recoveryStateStateFlow.collectAsState()
val syncState by syncService.syncState.collectAsState()
return remember {
derivedStateOf {
when {
currentSecurityBannerDismissed -> SecurityBannerState.None
syncState == SyncState.Running -> {
when (recoveryState) {
RecoveryState.DISABLED -> SecurityBannerState.SetUpRecovery
RecoveryState.INCOMPLETE -> SecurityBannerState.RecoveryKeyConfirmation
RecoveryState.UNKNOWN,
RecoveryState.WAITING_FOR_SYNC,
RecoveryState.ENABLED -> SecurityBannerState.None
}
}
needsSlidingSyncMigration -> SecurityBannerState.NeedsNativeSlidingSyncMigration
else -> SecurityBannerState.None
}
calculateBannerState(
securityBannerDismissed = currentSecurityBannerDismissed,
needsSlidingSyncMigration = currentNeedsSlidingSyncMigration,
recoveryState = recoveryState,
)
}
}
}

private fun calculateBannerState(
securityBannerDismissed: Boolean,
needsSlidingSyncMigration: Boolean,
recoveryState: RecoveryState,
): SecurityBannerState {
if (securityBannerDismissed) {
return SecurityBannerState.None
}

when (recoveryState) {
RecoveryState.DISABLED -> return SecurityBannerState.SetUpRecovery
RecoveryState.INCOMPLETE -> return SecurityBannerState.RecoveryKeyConfirmation
RecoveryState.UNKNOWN,
RecoveryState.WAITING_FOR_SYNC,
RecoveryState.ENABLED -> Unit
}

if (needsSlidingSyncMigration) {
return SecurityBannerState.NeedsNativeSlidingSyncMigration

Check warning on line 211 in features/roomlist/impl/src/main/kotlin/io/element/android/features/roomlist/impl/RoomListPresenter.kt

View check run for this annotation

Codecov / codecov/patch

features/roomlist/impl/src/main/kotlin/io/element/android/features/roomlist/impl/RoomListPresenter.kt#L211 

Added line #L211 was not covered by tests
}

return SecurityBannerState.None
}
Copy link
Member

@bmarty bmarty Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(For later) RoomListPresenter is quite a large file, so maybe this logic could be extracted and covered by a few unit tests?

Edit: we can see that a case is missing in the coverage, and unit test will also check the "priority" of the parameters:

image


@Composable
private fun roomListContentState(
securityBannerDismissed: Boolean,
Expand Down Expand Up @@ -228,7 +242,7 @@
showEmpty -> RoomListContentState.Empty
showSkeleton -> RoomListContentState.Skeleton(count = 16)
else -> {
val securityBannerState by securityBannerState(securityBannerDismissed, needsSlidingSyncMigration)
val securityBannerState by rememberSecurityBannerState(securityBannerDismissed, needsSlidingSyncMigration)
RoomListContentState.Rooms(
securityBannerState = securityBannerState,
fullScreenIntentPermissionsState = fullScreenIntentPermissionsPresenter.present(),
Expand Down
Loading