From 4d45b02e916df490b506ab33f49f85fa14d9b7dd Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Mon, 7 Oct 2024 15:31:42 +0200 Subject: [PATCH] fix(ui): Consider `timeline_limit` in sliding sync as non-sticky. This patch changes the behaviour of `timeline_limit` in sliding sync requests. It previously was sticky, but since it's now mandatory with MSC4186, it's preferable it to be non-sticky, otherwise in some scenarios it might default to 0 (its default value). How? If the server doesn't reply with our `txn_id` (because it doesn't support sticky parameters or because it misses a `txn_id`), the next request will be built with a default `timeline_limit` value, which is zero, and won't get updated to the `timeline_limit` value from `SlidingSyncListStickyParameters`. This is not good. Instead, we must consider `timeline_limit` as non-sticky, and moves it from `SlidingSyncListStickyParameters` to `SlidingSyncListInner`. This is what this patch does. --- .../tests/integration/room_list_service.rs | 66 +++++++++---------- .../src/sliding_sync/list/builder.rs | 2 +- .../matrix-sdk/src/sliding_sync/list/mod.rs | 18 ++--- .../src/sliding_sync/list/sticky.rs | 18 +---- 4 files changed, 45 insertions(+), 59 deletions(-) diff --git a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs index 12c2bd036..674cbddbd 100644 --- a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs +++ b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs @@ -376,7 +376,7 @@ async fn test_sync_all_states() -> Result<(), Error> { "lists": { ALL_ROOMS: { "ranges": [[0, 99]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -401,7 +401,7 @@ async fn test_sync_all_states() -> Result<(), Error> { "lists": { ALL_ROOMS: { "ranges": [[0, 199]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -426,7 +426,7 @@ async fn test_sync_all_states() -> Result<(), Error> { "lists": { ALL_ROOMS: { "ranges": [[0, 299]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -451,7 +451,7 @@ async fn test_sync_all_states() -> Result<(), Error> { "lists": { ALL_ROOMS: { "ranges": [[0, 399]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -515,7 +515,7 @@ async fn test_sync_resumes_from_previous_state() -> Result<(), Error> { "lists": { ALL_ROOMS: { "ranges": [[0, 9]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -543,7 +543,7 @@ async fn test_sync_resumes_from_previous_state() -> Result<(), Error> { "lists": { ALL_ROOMS: { "ranges": [[0, 9]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -627,7 +627,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> { ALL_ROOMS: { // The sync-mode has changed to growing, with its initial range. "ranges": [[0, 99]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -652,7 +652,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> { ALL_ROOMS: { // Due to previous error, the sync-mode is back to selective, with its initial range. "ranges": [[0, 19]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -675,7 +675,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> { ALL_ROOMS: { // Sync-mode is now growing. "ranges": [[0, 99]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -699,7 +699,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> { ALL_ROOMS: { // The sync-mode is still growing, and the range has made progress. "ranges": [[0, 199]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -724,7 +724,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> { ALL_ROOMS: { // Due to previous error, the sync-mode is back to selective, with its initial range. "ranges": [[0, 19]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -747,7 +747,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> { ALL_ROOMS: { // The sync-mode is now growing. "ranges": [[0, 99]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -770,7 +770,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> { ALL_ROOMS: { // No error. The range is making progress. "ranges": [[0, 199]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -795,7 +795,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> { // Range is making progress and is even reaching the maximum // number of rooms. "ranges": [[0, 209]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -820,7 +820,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> { ALL_ROOMS: { // Due to previous error, the sync-mode is back to selective, with its initial range. "ranges": [[0, 19]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -843,7 +843,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> { ALL_ROOMS: { // Sync-mode is now growing. "ranges": [[0, 99]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -913,7 +913,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { ALL_ROOMS: { // The sync-mode is still selective, with its initial range. "ranges": [[0, 19]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -937,7 +937,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { ALL_ROOMS: { // The sync-mode is now growing, with its initial range. "ranges": [[0, 99]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -969,7 +969,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { ALL_ROOMS: { // The sync-mode is back to selective. "ranges": [[0, 19]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -993,7 +993,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { ALL_ROOMS: { // Sync-mode is growing, with its initial range. "ranges": [[0, 99]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -1017,7 +1017,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { ALL_ROOMS: { // Range is making progress, and has reached its maximum. "ranges": [[0, 149]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -1089,7 +1089,7 @@ async fn test_loading_states() -> Result<(), Error> { "lists": { ALL_ROOMS: { "ranges": [[0, 9]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -1120,7 +1120,7 @@ async fn test_loading_states() -> Result<(), Error> { "lists": { ALL_ROOMS: { "ranges": [[0, 11]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -1271,7 +1271,7 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> { "lists": { ALL_ROOMS: { "ranges": [[0, 9]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -1381,7 +1381,7 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> { "lists": { ALL_ROOMS: { "ranges": [[0, 9]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -1548,7 +1548,7 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> { "lists": { ALL_ROOMS: { "ranges": [[0, 9]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -1718,7 +1718,7 @@ async fn test_room_sorting() -> Result<(), Error> { "lists": { ALL_ROOMS: { "ranges": [[0, 4]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -1804,7 +1804,7 @@ async fn test_room_sorting() -> Result<(), Error> { "lists": { ALL_ROOMS: { "ranges": [[0, 4]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -1879,7 +1879,7 @@ async fn test_room_sorting() -> Result<(), Error> { "lists": { ALL_ROOMS: { "ranges": [[0, 5]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, @@ -2099,7 +2099,7 @@ async fn test_room_subscription() -> Result<(), Error> { "lists": { ALL_ROOMS: { "ranges": [[0, 2]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, "room_subscriptions": { @@ -2143,7 +2143,7 @@ async fn test_room_subscription() -> Result<(), Error> { "lists": { ALL_ROOMS: { "ranges": [[0, 2]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, "room_subscriptions": { @@ -2190,7 +2190,7 @@ async fn test_room_subscription() -> Result<(), Error> { "lists": { ALL_ROOMS: { "ranges": [[0, 2]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, // NO `room_subscriptions`! @@ -2257,7 +2257,7 @@ async fn test_room_unread_notifications() -> Result<(), Error> { "lists": { ALL_ROOMS: { "ranges": [[0, 0]], - "timeline_limit": 0, + "timeline_limit": 1, }, }, }, diff --git a/crates/matrix-sdk/src/sliding_sync/list/builder.rs b/crates/matrix-sdk/src/sliding_sync/list/builder.rs index fe4b2d163..312acbe93 100644 --- a/crates/matrix-sdk/src/sliding_sync/list/builder.rs +++ b/crates/matrix-sdk/src/sliding_sync/list/builder.rs @@ -177,9 +177,9 @@ impl SlidingSyncListBuilder { self.required_state, self.include_heroes, self.filters, - self.timeline_limit, ), )), + timeline_limit: StdRwLock::new(self.timeline_limit), name: self.name, cache_policy: self.cache_policy, diff --git a/crates/matrix-sdk/src/sliding_sync/list/mod.rs b/crates/matrix-sdk/src/sliding_sync/list/mod.rs index c0e07a1d8..a3b3ec3b0 100644 --- a/crates/matrix-sdk/src/sliding_sync/list/mod.rs +++ b/crates/matrix-sdk/src/sliding_sync/list/mod.rs @@ -133,12 +133,12 @@ impl SlidingSyncList { /// Get the timeline limit. pub fn timeline_limit(&self) -> Bound { - self.inner.sticky.read().unwrap().data().timeline_limit() + *self.inner.timeline_limit.read().unwrap() } /// Set timeline limit. pub fn set_timeline_limit(&self, timeline: Bound) { - self.inner.sticky.write().unwrap().data_mut().set_timeline_limit(timeline); + *self.inner.timeline_limit.write().unwrap() = timeline; } /// Get the maximum number of rooms. See [`Self::maximum_number_of_rooms`] @@ -233,6 +233,9 @@ pub(super) struct SlidingSyncListInner { /// knows). sticky: StdRwLock>, + /// The maximum number of timeline events to query for. + timeline_limit: StdRwLock, + /// The total number of rooms that is possible to interact with for the /// given list. /// @@ -307,12 +310,11 @@ impl SlidingSyncListInner { /// request generator. #[instrument(skip(self), fields(name = self.name))] fn request(&self, ranges: Ranges, txn_id: &mut LazyTransactionId) -> http::request::List { - use ruma::UInt; - - let ranges = - ranges.into_iter().map(|r| (UInt::from(*r.start()), UInt::from(*r.end()))).collect(); + let ranges = ranges.into_iter().map(|r| ((*r.start()).into(), (*r.end()).into())).collect(); let mut request = assign!(http::request::List::default(), { ranges }); + request.room_details.timeline_limit = (*self.timeline_limit.read().unwrap()).into(); + { let mut sticky = self.sticky.write().unwrap(); sticky.maybe_apply(&mut request, txn_id); @@ -594,10 +596,10 @@ mod tests { .timeline_limit(7) .build(sender); - assert_eq!(list.inner.sticky.read().unwrap().data().timeline_limit(), 7); + assert_eq!(list.timeline_limit(), 7); list.set_timeline_limit(42); - assert_eq!(list.inner.sticky.read().unwrap().data().timeline_limit(), 42); + assert_eq!(list.timeline_limit(), 42); } macro_rules! assert_ranges { diff --git a/crates/matrix-sdk/src/sliding_sync/list/sticky.rs b/crates/matrix-sdk/src/sliding_sync/list/sticky.rs index b0cb0bcdd..f5b12798b 100644 --- a/crates/matrix-sdk/src/sliding_sync/list/sticky.rs +++ b/crates/matrix-sdk/src/sliding_sync/list/sticky.rs @@ -1,7 +1,6 @@ use matrix_sdk_base::sliding_sync::http; use ruma::events::StateEventType; -use super::Bound; use crate::sliding_sync::sticky_parameters::StickyData; /// The set of `SlidingSyncList` request parameters that are *sticky*, as @@ -17,9 +16,6 @@ pub(super) struct SlidingSyncListStickyParameters { /// Any filters to apply to the query. filters: Option, - - /// The maximum number of timeline events to query for. - timeline_limit: Bound, } impl SlidingSyncListStickyParameters { @@ -27,21 +23,10 @@ impl SlidingSyncListStickyParameters { required_state: Vec<(StateEventType, String)>, include_heroes: Option, filters: Option, - timeline_limit: Bound, ) -> Self { // Consider that each list will have at least one parameter set, so invalidate // it by default. - Self { required_state, include_heroes, filters, timeline_limit } - } -} - -impl SlidingSyncListStickyParameters { - pub(super) fn timeline_limit(&self) -> Bound { - self.timeline_limit - } - - pub(super) fn set_timeline_limit(&mut self, timeline: Bound) { - self.timeline_limit = timeline; + Self { required_state, include_heroes, filters } } } @@ -50,7 +35,6 @@ impl StickyData for SlidingSyncListStickyParameters { fn apply(&self, request: &mut Self::Request) { request.room_details.required_state = self.required_state.to_vec(); - request.room_details.timeline_limit = self.timeline_limit.into(); request.include_heroes = self.include_heroes; request.filters = self.filters.clone(); }