-
Notifications
You must be signed in to change notification settings - Fork 739
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
Align Autorageshake with web implementation #5639
Conversation
Unit Test Results114 files + 8 114 suites +8 1m 36s ⏱️ +3s Results for commit 363afd0. ± Comparison against base commit 1097436. This pull request removes 2 and adds 15 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll defer to @BillCarsonFr review for the content of the change.
Just one remark about the changelog (a PR can have multiple changelog files)
matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/LiveEventListener.kt
Show resolved
Hide resolved
session.getSyncStatusLive() | ||
.asFlow() | ||
.onEach { | ||
hasSynced = it !is SyncStatusService.Status.Progressing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not exaclty what we want no? Looks a bit strange. Also we are subscribing to the flow for the all session lifecycle no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"For the lifetime of the session, confirm we are not in an initial sync", Being outside an initial sync I thought would be reasonably equivalent to after an initial sync.
Web uses the sync token, so equivalent to hasAlreadySynced
. We could keep a reference to the active sessions and call hasAlreadySynced
synchronously in enabled
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is acceptable.
Maybe we should rename SyncStatusService.Status.Progressing
to SyncStatusService.Status.InitialSyncProgressing
for clarity.
It's historical, previously we did not track the incremental sync status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #5596