-
Notifications
You must be signed in to change notification settings - Fork 176
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
Feature/fga/start sync on push #3260
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
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, although I have my doubts about startSyncIfNeeded()
and its stop
counterpart.
if (!appForegroundStateService.isInForeground.value) { | ||
val syncService = client.syncService() | ||
syncService.startSyncIfNeeded() | ||
room.waitsUntilEventIsKnown(eventId = notifiableEvent.eventId, timeout = 10.seconds) |
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.
When I was doing similar tests what I did here was just wait until the syncState
was Running
, which I believe is enough to mean the first sync has been received. But we can keep this to make it more explicit, it will probably even work better.
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.
No waiting for running is not enough, the data usually comes after several sync iterations
} | ||
|
||
private suspend fun SyncService.startSyncIfNeeded() { | ||
if (syncCounter.getAndIncrement() == 0) { |
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.
Wouldn't it be better to just check the syncService
state? What if some other part of the app started a sync on its own?
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.
The counter is just to avoid cancelling if there are other pushes received in parallel.
If the sync is already started from somewhere else, this won't have any effect.
If the app is backgrounded again it will cancel the sync anyway.
So, in the end, this is just best effort.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3260 +/- ##
===========================================
+ Coverage 76.16% 76.18% +0.02%
===========================================
Files 1648 1650 +2
Lines 38825 38848 +23
Branches 7532 7536 +4
===========================================
+ Hits 29570 29596 +26
+ Misses 5353 5347 -6
- Partials 3902 3905 +3 ☔ View full report in Codecov by Sentry. |
Content
#3257 wasn't enough, so try to sync and waits for the event in the timeline if the app is in background.
Motivation and context
Improve on top of #3257
Screenshots / GIFs
Tests
Tested devices
Checklist