-
Notifications
You must be signed in to change notification settings - Fork 253
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
Get back to Recovering syncing when we haven't sync for a while #3995
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3995 +/- ##
=======================================
Coverage 84.67% 84.68%
=======================================
Files 269 269
Lines 28753 28767 +14
=======================================
+ Hits 24348 24361 +13
- Misses 4405 4406 +1 ☔ View full report in Codecov by Sentry. |
4f08a1f
to
0b79ccb
Compare
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.
Thank you for working on this. I appreciate.
I'm a bit annoyed by the introduction of the new testing
feature: we are trying to remove then, and this PR adds a new one. I'm proposing a solution in my comment. Feel free to question my comment.
a1419c3
to
e3d754b
Compare
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.
Thanks! It's indeed much simpler now!
Can you rebase all your patches now? I think we should be good.
26c8a43
to
6087d41
Compare
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.
Last bit and we are good!
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.
We are close to something I really like, thanks! What's left on your side?
Mainly that last refactor introduced an error somewhere since it seems to break a lot of tests 😅 https://github.com/matrix-org/matrix-rust-sdk/actions/runs/11217272471/job/31178453216?pr=3995 I just tried a revert and tests are back to green (outside of coverage). I can't really figure out why this is happening so if you can have a look that would be neat. |
/// The current state of the `RoomListService`. | ||
current: SharedObservable<State>, | ||
|
||
last_sync_date: Instant, |
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 value is never updated. I'm working on it :-).
@@ -83,6 +127,8 @@ impl State { | |||
} | |||
}; | |||
|
|||
self.set(next_state.clone()); |
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 the error that makes test_sliding_sync_indicator
to fail, and possibly other tests. This method caculates the next state, but the state must be updated if and only if a sync has run. It's RoomListService::sync
that is responsible to update the state.
I've pushed tchapgouv#2, it's a patch on your branch. If you merge it, I think this patch should pass all the tests. Please, rebase all commits into a single one. |
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.
Now we are good! Thanks for having merged my patches and rebased everything!
Fixes #3935.
Untested in the apps for now, but integration test is included and working.
Signed-off-by: Mathieu Velten [email protected]