Add banner for optional migration to simplified sliding sync #3429
Conversation
- Add `MatrixClient.isNativeSlidingSyncSupported()` and `MatrixClient.isUsingNativeSlidingSync` to check whether the home server supports native sliding sync and we're already using it. - Add `NativeSlidingSyncMigrationBanner` composable to the `RoomList` screen when the home server supports native sliding sync but the current session is not using it. - Add an extra logout successful action to the logout flow, create `EnableNativeSlidingSyncUseCase` so it can be used there.
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3429 +/- ##
========================================
Coverage 82.65% 82.65%
========================================
Files 1705 1707 +2
Lines 40076 40115 +39
Branches 4881 4885 +4
========================================
+ Hits 33125 33158 +33
- Misses 5233 5236 +3
- Partials 1718 1721 +3 ☔ View full report in Codecov by Sentry. |
| @@ -176,6 +182,7 @@ class RoomListPresenter @Inject constructor( | |||
| derivedStateOf { | |||
| when { | |||
| currentSecurityBannerDismissed -> SecurityBannerState.None | |||
| needsSlidingSyncMigration -> SecurityBannerState.NeedsNativeSlidingSyncMigration | |||
There was a problem hiding this comment.
Are you sure that the NeedsNativeSlidingSyncMigration banner has higher priority than the SetUpRecovery and RecoveryKeyConfirmation banner?
It will fully hide the two latter, since if NeedsNativeSlidingSyncMigration is dismissed, currentSecurityBannerDismissed will be true and the other banner will never be displayed again (until the user logout and login again).
There was a problem hiding this comment.
Yes, you're right, this one should have the lowest priority.
| @@ -77,6 +77,8 @@ class FakeMatrixClient( | |||
| private val clearCacheLambda: () -> Unit = { lambdaError() }, | |||
| private val userIdServerNameLambda: () -> String = { lambdaError() }, | |||
| private val getUrlLambda: (String) -> Result<String> = { lambdaError() }, | |||
| var isNativeSlidingSyncSupportedLambda: suspend () -> Boolean = { true }, | |||
| var isUsingNativeSlidingSyncLambda: () -> Boolean = { true }, | |||
There was a problem hiding this comment.
I prefer to use private val for lambda like this. Also, they seem to be unused, maybe add a quick unit test to check the value of needsSlidingSyncMigration?
There was a problem hiding this comment.
I usually prefer using the public var because that way you can set up both the value on the constructor or later in the tests if you need to change it without having to create a new method just for setting it.
| ) { | ||
| operator fun invoke() { | ||
| appCoroutineScope.launch { | ||
| appPreferencesStore.setSimplifiedSlidingSyncEnabled(true) |
There was a problem hiding this comment.
There is maybe an issue if the user decides to login using another homeserver... I agree this is an edge case and this is temporary code, but with the current codebase, but we should maybe have a setting per server...
There was a problem hiding this comment.
Since this forces you to log out, I don't think most users will try it and then change their mind and log into another home server. Besides, changing this would required quite a few logic changes and maybe UI ones for the developer options toggle, and it seems like an overkill for something like this.
What we could do, maybe, is that when this is enabled, the 1st login attempt will be tried using a native sliding sync config, then if it fails with a not supported error the 2nd one will use the previous proxy one as a fallback. WDYT? Maybe I can add this in a separate PR.
There was a problem hiding this comment.
OK, let's not do anything then.
Also there is another (limited) risk of user seeing the banner and decide to logout from the settings. In this case, there will be no usage of the SSS and the banner will reappear.
jmartinesp
added
the
Run-Maestro
github-actions
bot
removed
the
Run-Maestro
…he encryption setup ones
|
|
I've just installed 0.5.3 And i logged out and in again. I'm missing info how to configure my server for simplified sliding sync. on the proxy i added Then i removed in my matrix-hosts nginx-config the part for the external sliding sync...
So i logged in with acitve external sliding sync, then i removed the entry for the external sliding sync in nginx.conf from my matrix-server and reloaded the nginx config. It seems, it should do, what this PR promises... but i can not use it. It there somewhere some docu how to configure matrix- and proxy-server and nginx correctly to use simplified sliding sync? Here is a short screencast to show, what i mean. |
|
I tried a lot to hit the button. And once i got it. So my configuration now is: |
Content
MatrixClient.isNativeSlidingSyncSupported()andMatrixClient.isUsingNativeSlidingSyncto check whether the home server supports native sliding sync and we're already using it.NativeSlidingSyncMigrationBannercomposable to theRoomListscreen when the home server supports native sliding sync but the current session is not using it.EnableNativeSlidingSyncUseCaseso it can be used there.Motivation and context
Closes #3427.
Screenshots / GIFs
In the PR.
Tests
Tested devices
Checklist