sliding sync: Rejigger the range API (#1955)

* feat: introduce SlidingSyncSelectiveModeBuilder
* feat: get rid of `CannotModifyRanges` \o/
* chore: rustfmt
* chore: remove scope in test
* chore: move request generator update to its own mod, gets rid of `set_ranges`
* chore: add comments on the request generator methods
* chore: sink one range_end definition into the match arm that matters
* ffi: remove unused `add_range` method
* test: update incorrect test expectation
* chore: make clippy happy
* address first review comments
* review: don't reuse the ranges field for two things
* chore: use a builder pattern for sliding sync selective mode
* address review comments + CI

Signed-off-by: Benjamin Bouvier <public@benj.me>

Author: bnjbvr

bindings/matrix-sdk-ffi/src/sliding_sync.rs

@@ -3,18 +3,21 @@ use std::sync::{Arc, RwLock};
 use anyhow::Context;
 use eyeball_im::VectorDiff;
 use futures_util::{future::join4, pin_mut, StreamExt};
-use matrix_sdk::ruma::{
-    api::client::sync::sync_events::{
-        v4::RoomSubscription as RumaRoomSubscription,
-        UnreadNotificationsCount as RumaUnreadNotificationsCount,
-    },
-    assign, IdParseError, OwnedRoomId, RoomId,
-};
 pub use matrix_sdk::{
     ruma::api::client::sync::sync_events::v4::SyncRequestListFilters, Client as MatrixClient,
     LoopCtrl, RoomListEntry as MatrixRoomEntry, SlidingSyncBuilder as MatrixSlidingSyncBuilder,
     SlidingSyncMode, SlidingSyncState,
 };
+use matrix_sdk::{
+    ruma::{
+        api::client::sync::sync_events::{
+            v4::RoomSubscription as RumaRoomSubscription,
+            UnreadNotificationsCount as RumaUnreadNotificationsCount,
+        },
+        assign, IdParseError, OwnedRoomId, RoomId,
+    },
+    sliding_sync::SlidingSyncSelectiveModeBuilder as MatrixSlidingSyncSelectiveModeBuilder,
+};
 use matrix_sdk_ui::timeline::SlidingSyncRoomExt;
 use tokio::task::JoinHandle;
 use tracing::{debug, error, warn};
@@ -104,9 +107,6 @@ pub enum SlidingSyncError {
     /// initialized. It happens when a response is handled before a request has
     /// been sent. It usually happens when testing.
     RequestGeneratorHasNotBeenInitialized { msg: String },
-    /// Someone has tried to modify a sliding sync list's ranges, but the
-    /// selected sync mode doesn't allow that.
-    CannotModifyRanges { msg: String },
     /// Ranges have a `start` bound greater than `end`.
     InvalidRange {
         /// Start bound.
@@ -129,7 +129,6 @@ impl From<matrix_sdk::sliding_sync::Error> for SlidingSyncError {
             E::RequestGeneratorHasNotBeenInitialized(msg) => {
                 Self::RequestGeneratorHasNotBeenInitialized { msg }
             }
-            E::CannotModifyRanges(msg) => Self::CannotModifyRanges { msg },
             E::InvalidRange { start, end } => Self::InvalidRange { start, end },
             E::InternalChannelIsBroken => Self::InternalChannelIsBroken,
             error => Self::Unknown { error: error.to_string() },
@@ -410,6 +409,25 @@ pub trait SlidingSyncListStateObserver: Sync + Send {
     fn did_receive_update(&self, new_state: SlidingSyncState);
 }
 
+#[derive(Clone, uniffi::Object)]
+pub struct SlidingSyncSelectiveModeBuilder {
+    inner: MatrixSlidingSyncSelectiveModeBuilder,
+}
+
+#[uniffi::export]
+impl SlidingSyncSelectiveModeBuilder {
+    #[uniffi::constructor]
+    pub fn new() -> Arc<Self> {
+        Arc::new(Self { inner: SlidingSyncMode::new_selective() })
+    }
+
+    pub fn add_range(self: Arc<Self>, start: u32, end_inclusive: u32) -> Arc<Self> {
+        let mut builder = unwrap_or_clone_arc(self);
+        builder.inner = builder.inner.add_range(start..=end_inclusive);
+        Arc::new(builder)
+    }
+}
+
 #[derive(Clone, uniffi::Object)]
 pub struct SlidingSyncListBuilder {
     inner: matrix_sdk::SlidingSyncListBuilder,
@@ -458,9 +476,13 @@ impl SlidingSyncListBuilder {
         Arc::new(Self { inner: matrix_sdk::SlidingSyncList::builder(name) })
     }
 
-    pub fn sync_mode_selective(self: Arc<Self>) -> Arc<Self> {
+    pub fn sync_mode_selective(
+        self: Arc<Self>,
+        selective_mode_builder: Arc<SlidingSyncSelectiveModeBuilder>,
+    ) -> Arc<Self> {
         let mut builder = unwrap_or_clone_arc(self);
-        builder.inner = builder.inner.sync_mode(SlidingSyncMode::new_selective());
+        let selective_mode_builder = unwrap_or_clone_arc(selective_mode_builder);
+        builder.inner = builder.inner.sync_mode(selective_mode_builder.inner);
         Arc::new(builder)
     }
 
@@ -526,12 +548,6 @@ impl SlidingSyncListBuilder {
         Arc::new(builder)
     }
 
-    pub fn add_range(self: Arc<Self>, from: u32, to_included: u32) -> Arc<Self> {
-        let mut builder = unwrap_or_clone_arc(self);
-        builder.inner = builder.inner.add_range(from..=to_included);
-        Arc::new(builder)
-    }
-
     pub fn once_built(self: Arc<Self>, callback: Box<dyn SlidingSyncListOnceBuilt>) -> Arc<Self> {
         let mut builder = unwrap_or_clone_arc(self);
 
@@ -613,27 +629,6 @@ impl SlidingSyncList {
         self.inner.room_list()
     }
 
-    /// Reset the ranges to a particular set
-    ///
-    /// Remember to cancel the existing stream and fetch a new one as this will
-    /// only be applied on the next request.
-    pub fn set_range(&self, start: u32, end: u32) -> Result<(), SlidingSyncError> {
-        self.inner.set_range(start..=end).map_err(Into::into)
-    }
-
-    /// Set the ranges to fetch
-    ///
-    /// Remember to cancel the existing stream and fetch a new one as this will
-    /// only be applied on the next request.
-    pub fn add_range(&self, start: u32, end: u32) -> Result<(), SlidingSyncError> {
-        self.inner.add_range(start..=end).map_err(Into::into)
-    }
-
-    /// Reset the ranges
-    pub fn reset_ranges(&self) -> Result<(), SlidingSyncError> {
-        self.inner.reset_ranges().map_err(Into::into)
-    }
-
     /// Total of rooms matching the filter
     pub fn current_room_count(&self) -> Option<u32> {
         self.inner.maximum_number_of_rooms()

crates/matrix-sdk-ui/tests/integration/timeline/sliding_sync.rs

@@ -219,8 +219,7 @@ impl Match for SlidingSyncMatcher {
 #[async_test]
 async fn test_timeline_basic() -> Result<()> {
     let (server, sliding_sync) = new_sliding_sync(vec![SlidingSyncList::builder("foo")
-        .sync_mode(SlidingSyncMode::Selective)
-        .add_range(0..=10)])
+        .sync_mode(SlidingSyncMode::new_selective().add_range(0..=10))])
     .await?;
 
     let stream = sliding_sync.sync();
@@ -266,8 +265,7 @@ async fn test_timeline_basic() -> Result<()> {
 #[async_test]
 async fn test_timeline_duplicated_events() -> Result<()> {
     let (server, sliding_sync) = new_sliding_sync(vec![SlidingSyncList::builder("foo")
-        .sync_mode(SlidingSyncMode::Selective)
-        .add_range(0..=10)])
+        .sync_mode(SlidingSyncMode::new_selective().add_range(0..=10))])
     .await?;
 
     let stream = sliding_sync.sync();

crates/matrix-sdk/src/sliding_sync/README.md

@@ -72,12 +72,11 @@ are **inclusive**) like so:
 use ruma::{assign, api::client::sync::sync_events::v4};
 
 let list_builder = SlidingSyncList::builder("main_list")
-    .sync_mode(SlidingSyncMode::Selective)
+    .sync_mode(SlidingSyncMode::new_selective().add_range(0..=9))
     .filters(Some(assign!(
         v4::SyncRequestListFilters::default(), { is_dm: Some(true)}
     )))
-    .sort(vec!["by_recency".to_owned()])
-    .add_range(0u32..=9);
+    .sort(vec!["by_recency".to_owned()]);
 ```
 
 Please refer to the [specification][MSC], the [Ruma types][ruma-types],
@@ -424,8 +423,7 @@ let full_sync_list = SlidingSyncList::builder(&full_sync_list_name)
      ]); // only want to know if the room is encrypted
 
 let active_list = SlidingSyncList::builder(&active_list_name) // the active window
-    .sync_mode(SlidingSyncMode::Selective)  // sync up the specific range only
-    .add_range(0u32..=9) // only the top 10 items
+    .sync_mode(SlidingSyncMode::new_selective().add_range(0..=9))  // sync up the specific range only, first 10 items
     .sort(vec!["by_recency".to_owned()]) // last active
     .timeline_limit(5u32) // add the last 5 timeline items for room preview and faster timeline loading
     .required_state(vec![ // we want to know immediately:

crates/matrix-sdk/src/sliding_sync/error.rs

@@ -19,11 +19,6 @@ pub enum Error {
     #[error("The sliding sync list `{0}` is handling a response, but its request generator has not been initialized")]
     RequestGeneratorHasNotBeenInitialized(String),
 
-    /// Someone has tried to modify a sliding sync list's ranges, but only the
-    /// `Selective` sync mode does allow that.
-    #[error("The chosen sync mode for the list `{0}` doesn't allow to modify the ranges; only `Selective` allows that")]
-    CannotModifyRanges(String),
-
     /// Ranges have a `start` bound greater than `end`.
     #[error("Ranges have invalid bounds: `{start}..{end}`")]
     InvalidRange {

crates/matrix-sdk/src/sliding_sync/list/builder.rs

@@ -4,7 +4,6 @@ use std::{
     collections::BTreeMap,
     convert::identity,
     fmt,
-    ops::RangeInclusive,
     sync::{Arc, RwLock as StdRwLock},
 };
 
@@ -45,7 +44,6 @@ pub struct SlidingSyncListBuilder {
     filters: Option<v4::SyncRequestListFilters>,
     timeline_limit: Option<Bound>,
     name: String,
-    ranges: Vec<RangeInclusive<Bound>>,
 
     /// Should this list be cached and reloaded from the cache?
     cache_policy: SlidingSyncListCachePolicy,
@@ -68,7 +66,6 @@ impl fmt::Debug for SlidingSyncListBuilder {
             .field("filters", &self.filters)
             .field("timeline_limit", &self.timeline_limit)
             .field("name", &self.name)
-            .field("ranges", &self.ranges)
             .finish_non_exhaustive()
     }
 }
@@ -85,7 +82,6 @@ impl SlidingSyncListBuilder {
             filters: None,
             timeline_limit: None,
             name: name.into(),
-            ranges: Vec::new(),
             reloaded_cached_data: None,
             cache_policy: SlidingSyncListCachePolicy::Disabled,
             once_built: Arc::new(Box::new(identity)),
@@ -106,8 +102,8 @@ impl SlidingSyncListBuilder {
     }
 
     /// Which SlidingSyncMode to start this list under.
-    pub fn sync_mode(mut self, value: SlidingSyncMode) -> Self {
-        self.sync_mode = value;
+    pub fn sync_mode(mut self, value: impl Into<SlidingSyncMode>) -> Self {
+        self.sync_mode = value.into();
         self
     }
 
@@ -142,12 +138,6 @@ impl SlidingSyncListBuilder {
         self
     }
 
-    /// Set the ranges to fetch.
-    pub fn add_range(mut self, range: RangeInclusive<Bound>) -> Self {
-        self.ranges.push(range);
-        self
-    }
-
     /// Marks this list as sync'd from the cache, and attempts to reload it from
     /// storage.
     ///
@@ -184,13 +174,11 @@ impl SlidingSyncListBuilder {
         let list = SlidingSyncList {
             inner: Arc::new(SlidingSyncListInner {
                 // From the builder
-                sync_mode: StdRwLock::new(self.sync_mode.clone()),
                 sort: self.sort,
                 required_state: self.required_state,
                 filters: self.filters,
                 timeline_limit: StdRwLock::new(self.timeline_limit),
                 name: self.name,
-                ranges: StdRwLock::new(self.ranges),
                 cache_policy: self.cache_policy,
 
                 // Computed from the builder.