From ec5c1a3c701c08c5c8055edce51345756faea836 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 21 Jun 2023 09:17:10 +0200 Subject: [PATCH 1/9] feat(ui): Implement `RoomList::stop_sync`. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch implements `RoomList::stop_sync`. The goal is twofold: 1. It forces to stop the syncing, thus putting the state-machine into the `Terminated` state, which is semantically better than “stop polling the `sync`'s `Stream`”. 2. It literally forces to stop the syncing. It cancels pending futures, it cancels in-flight HTTP requests etc. It's a more robust way to stop the `RoomList` sync. --- crates/matrix-sdk-ui/src/room_list/mod.rs | 23 +++++- .../tests/integration/room_list.rs | 80 +++++++++++++++++++ 2 files changed, 100 insertions(+), 3 deletions(-) diff --git a/crates/matrix-sdk-ui/src/room_list/mod.rs b/crates/matrix-sdk-ui/src/room_list/mod.rs index 766c3c7a5..8d34254b6 100644 --- a/crates/matrix-sdk-ui/src/room_list/mod.rs +++ b/crates/matrix-sdk-ui/src/room_list/mod.rs @@ -147,9 +147,9 @@ impl RoomList { /// /// The `RoomList`' state machine is run by this method. /// - /// Stopping the [`Stream`] (i.e. stop polling it) and calling - /// [`Self::sync`] again will resume from the previous state of the state - /// machine. + /// Stopping the [`Stream`] (i.e. by calling [`Self::stop_sync`]), and + /// calling [`Self::sync`] again will resume from the previous state of + /// the state machine. pub fn sync(&self) -> impl Stream> + '_ { stream! { let sync = self.sliding_sync.sync(); @@ -193,6 +193,23 @@ impl RoomList { } } + /// Force to stop the sync of the room list started by [`Self::sync`]. + /// + /// It's better to call this method rather than stop polling the `Stream` + /// returned by [`Self::sync`] because it will literally force the cancel + /// and exit the sync-loop, i.e. it will cancel any in-flight HTTP requests, + /// cancel any pending futures etc. + /// + /// Ideally, one wants to consume the `Stream` returned by [`Self::sync`] + /// until it returns `None`, because of [`Self::stop_sync`], so that it + /// ensures the states are correctly placed. + /// + /// Stopping the sync of the room list via this method will put the + /// state-machine into the [`State::Terminated`] state. + pub fn stop_sync(&self) -> Result<(), Error> { + self.sliding_sync.stop_sync().map_err(Error::SlidingSync) + } + /// Get a subscriber to the state. pub fn state(&self) -> Subscriber { self.state.subscribe() diff --git a/crates/matrix-sdk-ui/tests/integration/room_list.rs b/crates/matrix-sdk-ui/tests/integration/room_list.rs index 4fc4347e2..ecfe22d88 100644 --- a/crates/matrix-sdk-ui/tests/integration/room_list.rs +++ b/crates/matrix-sdk-ui/tests/integration/room_list.rs @@ -900,6 +900,86 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { Ok(()) } +#[async_test] +async fn test_sync_can_be_stopped() -> Result<(), Error> { + let (server, room_list) = new_room_list().await?; + + // Let's stop the sync before actually syncing (we never know!). + // We get an error, obviously. + assert!(room_list.stop_sync().is_err()); + + let sync = room_list.sync(); + pin_mut!(sync); + + // First sync, everything is alright and up and running. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = Init => FirstRooms, + assert request >= { + "lists": { + ALL_ROOMS: { + "ranges": [[0, 19]], + }, + }, + }, + respond with = { + "pos": "0", + "lists": { + ALL_ROOMS: { + "count": 10, + "ops": [] + }, + }, + "rooms": {}, + }, + }; + + // Now let's stop the `sync`. + room_list.stop_sync()?; + + // `sync` doesn't provide more value. + assert!(sync.next().await.is_none()); + + // If a new sync is started, it will resume from a `Terminated` state. + let sync = room_list.sync(); + pin_mut!(sync); + + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = Terminated { .. } => AllRooms, + assert request >= { + "lists": { + ALL_ROOMS: { + "ranges": [[0, 9]], + }, + VISIBLE_ROOMS: { + "ranges": [[0, 19]], + }, + INVITES: { + "ranges": [[0, 99]], + }, + }, + }, + respond with = { + "pos": "1", + "lists": { + ALL_ROOMS: { + "count": 0, + }, + VISIBLE_ROOMS: { + "count": 0, + }, + INVITES: { + "count": 0, + }, + }, + "rooms": {}, + }, + }; + + Ok(()) +} + #[async_test] async fn test_entries_stream() -> Result<(), Error> { let (server, room_list) = new_room_list().await?; From a78ad89fdd6d9c76c3443498bad1451f901e651b Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 21 Jun 2023 09:22:50 +0200 Subject: [PATCH 2/9] feat(ffi): Implement `RoomList::stop_sync`. This patch implements `RoomList::stop_sync` on the FFI bindings. Technically, it's no more useful to store the `TaskHandle` of the `sync`, but it's still here, in case of. --- bindings/matrix-sdk-ffi/src/room_list.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/bindings/matrix-sdk-ffi/src/room_list.rs b/bindings/matrix-sdk-ffi/src/room_list.rs index be42837cd..f21fd3ae6 100644 --- a/bindings/matrix-sdk-ffi/src/room_list.rs +++ b/bindings/matrix-sdk-ffi/src/room_list.rs @@ -100,6 +100,10 @@ impl RoomList { }))) } + fn stop_sync(&self) -> Result<(), RoomListError> { + self.inner.stop_sync().map_err(Into::into) + } + fn state(&self, listener: Box) -> Arc { let state_stream = self.inner.state(); From f0bc6d6698d08d22f5ec1b3889c37cdc40ef1da2 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 21 Jun 2023 11:12:51 +0200 Subject: [PATCH 3/9] chore(ui): Rename `State::FirstRooms` and `State::AllRooms`. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch renames `State::FirstRooms` to `State::SettingUp`, and `State::Running` as requested by users. It hides the “inner Sliding Sync logic”, the Sliding Sync lists etc. --- crates/matrix-sdk-ui/src/room_list/state.rs | 27 ++++----- .../tests/integration/room_list.rs | 56 +++++++++---------- 2 files changed, 42 insertions(+), 41 deletions(-) diff --git a/crates/matrix-sdk-ui/src/room_list/state.rs b/crates/matrix-sdk-ui/src/room_list/state.rs index 10b79fdc5..d485907d9 100644 --- a/crates/matrix-sdk-ui/src/room_list/state.rs +++ b/crates/matrix-sdk-ui/src/room_list/state.rs @@ -21,11 +21,12 @@ pub enum State { /// That's the first initial state. Init, - /// At this state, the first rooms start to be synced. - FirstRooms, + /// At this state, the first rooms are starting to sync. + SettingUp, - /// At this state, all rooms start to be synced. - AllRooms, + /// At this state, all rooms are syncing, and the visible rooms + invites + /// lists exist. + Running, /// This state is the cruising speed, i.e. the “normal” state, where nothing /// fancy happens: all rooms are syncing, and life is great. @@ -43,9 +44,9 @@ impl State { use State::*; let (next_state, actions) = match self { - Init => (FirstRooms, Actions::none()), - FirstRooms => (AllRooms, Actions::first_rooms_are_loaded()), - AllRooms => (CarryOn, Actions::none()), + Init => (SettingUp, Actions::none()), + SettingUp => (Running, Actions::first_rooms_are_loaded()), + Running => (CarryOn, Actions::none()), CarryOn => (CarryOn, Actions::none()), // If the state was `Terminated`, the next state is calculated again, because it means // the sync has been restarted. In this case, let's jump back on the @@ -53,12 +54,12 @@ impl State { // scenario. Terminated { from: previous_state } => { match previous_state.as_ref() { - state @ Init | state @ FirstRooms => { + state @ Init | state @ SettingUp => { // Do nothing. (state.to_owned(), Actions::none()) } - state @ AllRooms | state @ CarryOn => { + state @ Running | state @ CarryOn => { // Refresh the lists. (state.to_owned(), Actions::refresh_lists()) } @@ -233,24 +234,24 @@ mod tests { // Next state. let state = state.next(sliding_sync).await?; - assert_eq!(state, State::FirstRooms); + assert_eq!(state, State::SettingUp); // Hypothetical termination. { let state = State::Terminated { from: Box::new(state.clone()) }.next(sliding_sync).await?; - assert_eq!(state, State::FirstRooms); + assert_eq!(state, State::SettingUp); } // Next state. let state = state.next(sliding_sync).await?; - assert_eq!(state, State::AllRooms); + assert_eq!(state, State::Running); // Hypothetical termination. { let state = State::Terminated { from: Box::new(state.clone()) }.next(sliding_sync).await?; - assert_eq!(state, State::AllRooms); + assert_eq!(state, State::Running); } // Next state. diff --git a/crates/matrix-sdk-ui/tests/integration/room_list.rs b/crates/matrix-sdk-ui/tests/integration/room_list.rs index ecfe22d88..d23a6beb2 100644 --- a/crates/matrix-sdk-ui/tests/integration/room_list.rs +++ b/crates/matrix-sdk-ui/tests/integration/room_list.rs @@ -201,7 +201,7 @@ macro_rules! assert_entries_stream { } #[async_test] -async fn test_sync_from_init_to_enjoy() -> Result<(), Error> { +async fn test_sync_all_states() -> Result<(), Error> { let (server, room_list) = new_room_list().await?; let (entries_loading_state, mut entries_loading_state_stream) = @@ -214,7 +214,7 @@ async fn test_sync_from_init_to_enjoy() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Init => FirstRooms, + states = Init => SettingUp, assert request >= { "lists": { ALL_ROOMS: { @@ -277,7 +277,7 @@ async fn test_sync_from_init_to_enjoy() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = FirstRooms => AllRooms, + states = SettingUp => Running, assert request >= { "lists": { ALL_ROOMS: { @@ -349,7 +349,7 @@ async fn test_sync_from_init_to_enjoy() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = AllRooms => CarryOn, + states = Running => CarryOn, assert request >= { "lists": { ALL_ROOMS: { @@ -491,7 +491,7 @@ async fn test_sync_resumes_from_previous_state() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Init => FirstRooms, + states = Init => SettingUp, assert request >= { "lists": { ALL_ROOMS: { @@ -519,7 +519,7 @@ async fn test_sync_resumes_from_previous_state() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = FirstRooms => AllRooms, + states = SettingUp => Running, assert request >= { "lists": { ALL_ROOMS: { @@ -561,7 +561,7 @@ async fn test_sync_resumes_from_previous_state() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = AllRooms => CarryOn, + states = Running => CarryOn, assert request >= { "lists": { ALL_ROOMS: { @@ -635,7 +635,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { // Do a regular sync from the `Terminated` state. sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Terminated { .. } => FirstRooms, + states = Terminated { .. } => SettingUp, assert request >= { "lists": { ALL_ROOMS: { @@ -655,15 +655,15 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { }, }; - // Simulate an error from the `FirstRooms` state. + // Simulate an error from the `SettingUp` state. sync_then_assert_request_and_fake_response! { [server, room_list, sync] sync matches Some(Err(_)), - states = FirstRooms => Terminated { .. }, + states = SettingUp => Terminated { .. }, assert request >= { "lists": { ALL_ROOMS: { - // In `FirstRooms`, the sync-mode has changed to growing, with + // In `SettingUp`, the sync-mode has changed to growing, with // its initial range. "ranges": [[0, 49]], }, @@ -696,11 +696,11 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { // Do a regular sync from the `Terminated` state. sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Terminated { .. } => AllRooms, + states = Terminated { .. } => Running, assert request >= { "lists": { ALL_ROOMS: { - // In `AllRooms`, the sync-mode is still growing, but the range + // In `Running`, the sync-mode is still growing, but the range // hasn't been modified due to previous error. "ranges": [[0, 49]], }, @@ -728,15 +728,15 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { }, }; - // Simulate an error from the `AllRooms` state. + // Simulate an error from the `Running` state. sync_then_assert_request_and_fake_response! { [server, room_list, sync] sync matches Some(Err(_)), - states = AllRooms => Terminated { .. }, + states = Running => Terminated { .. }, assert request >= { "lists": { ALL_ROOMS: { - // In `AllRooms`, the sync-mode is still growing, and the range + // In `Running`, the sync-mode is still growing, and the range // has made progress. "ranges": [[0, 99]], }, @@ -914,7 +914,7 @@ async fn test_sync_can_be_stopped() -> Result<(), Error> { // First sync, everything is alright and up and running. sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Init => FirstRooms, + states = Init => SettingUp, assert request >= { "lists": { ALL_ROOMS: { @@ -946,7 +946,7 @@ async fn test_sync_can_be_stopped() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Terminated { .. } => AllRooms, + states = Terminated { .. } => Running, assert request >= { "lists": { ALL_ROOMS: { @@ -992,7 +992,7 @@ async fn test_entries_stream() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Init => FirstRooms, + states = Init => SettingUp, assert request >= { "lists": { ALL_ROOMS: { @@ -1050,7 +1050,7 @@ async fn test_entries_stream() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = FirstRooms => AllRooms, + states = SettingUp => Running, assert request >= { "lists": { ALL_ROOMS: { @@ -1125,7 +1125,7 @@ async fn test_entries_stream_with_updated_filter() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Init => FirstRooms, + states = Init => SettingUp, assert request >= { "lists": { ALL_ROOMS: { @@ -1179,7 +1179,7 @@ async fn test_entries_stream_with_updated_filter() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = FirstRooms => AllRooms, + states = SettingUp => Running, assert request >= { "lists": { ALL_ROOMS: { @@ -1266,7 +1266,7 @@ async fn test_invites_stream() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Init => FirstRooms, + states = Init => SettingUp, assert request >= { "lists": { ALL_ROOMS: { @@ -1292,7 +1292,7 @@ async fn test_invites_stream() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = FirstRooms => AllRooms, + states = SettingUp => Running, assert request >= { "lists": { ALL_ROOMS: { @@ -1352,7 +1352,7 @@ async fn test_invites_stream() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = AllRooms => CarryOn, + states = Running => CarryOn, assert request >= { "lists": { ALL_ROOMS: { @@ -1908,7 +1908,7 @@ async fn test_input_viewport() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Init => FirstRooms, + states = Init => SettingUp, assert request >= { "lists": { ALL_ROOMS: { @@ -1925,7 +1925,7 @@ async fn test_input_viewport() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = FirstRooms => AllRooms, + states = SettingUp => Running, assert request >= { "lists": { ALL_ROOMS: { @@ -1952,7 +1952,7 @@ async fn test_input_viewport() -> Result<(), Error> { // The `timeline_limit` is not repeated because it's sticky. sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = AllRooms => CarryOn, + states = Running => CarryOn, assert request >= { "lists": { ALL_ROOMS: { From 7073e6f9e39d454cfac067310c2f42be43a483ce Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 21 Jun 2023 11:14:29 +0200 Subject: [PATCH 4/9] feat(ui): Remove the `State::CarryOn` state. This patch removes the `State::CarryOn`. It turns out that `Running` and `CarryOn` are doing exactly the same thing. So one of them is useless. --- crates/matrix-sdk-ui/src/room_list/state.rs | 26 ++++--------------- .../tests/integration/room_list.rs | 24 ++++++++--------- 2 files changed, 17 insertions(+), 33 deletions(-) diff --git a/crates/matrix-sdk-ui/src/room_list/state.rs b/crates/matrix-sdk-ui/src/room_list/state.rs index d485907d9..64bbf6981 100644 --- a/crates/matrix-sdk-ui/src/room_list/state.rs +++ b/crates/matrix-sdk-ui/src/room_list/state.rs @@ -28,10 +28,6 @@ pub enum State { /// lists exist. Running, - /// This state is the cruising speed, i.e. the “normal” state, where nothing - /// fancy happens: all rooms are syncing, and life is great. - CarryOn, - /// At this state, the sync has been stopped (because it was requested, or /// because it has errored too many times previously). Terminated { from: Box }, @@ -46,8 +42,7 @@ impl State { let (next_state, actions) = match self { Init => (SettingUp, Actions::none()), SettingUp => (Running, Actions::first_rooms_are_loaded()), - Running => (CarryOn, Actions::none()), - CarryOn => (CarryOn, Actions::none()), + Running => (Running, Actions::none()), // If the state was `Terminated`, the next state is calculated again, because it means // the sync has been restarted. In this case, let's jump back on the // previous state that led to the termination. No action is required in this @@ -59,9 +54,9 @@ impl State { (state.to_owned(), Actions::none()) } - state @ Running | state @ CarryOn => { + Running => { // Refresh the lists. - (state.to_owned(), Actions::refresh_lists()) + (Running, Actions::refresh_lists()) } Terminated { .. } => { @@ -256,24 +251,13 @@ mod tests { // Next state. let state = state.next(sliding_sync).await?; - assert_eq!(state, State::CarryOn); + assert_eq!(state, State::Running); // Hypothetical termination. { let state = State::Terminated { from: Box::new(state.clone()) }.next(sliding_sync).await?; - assert_eq!(state, State::CarryOn); - } - - // Next state. - let state = state.next(sliding_sync).await?; - assert_eq!(state, State::CarryOn); - - // Hypothetical termination. - { - let state = - State::Terminated { from: Box::new(state.clone()) }.next(sliding_sync).await?; - assert_eq!(state, State::CarryOn); + assert_eq!(state, State::Running); } Ok(()) diff --git a/crates/matrix-sdk-ui/tests/integration/room_list.rs b/crates/matrix-sdk-ui/tests/integration/room_list.rs index d23a6beb2..b96441e8a 100644 --- a/crates/matrix-sdk-ui/tests/integration/room_list.rs +++ b/crates/matrix-sdk-ui/tests/integration/room_list.rs @@ -349,7 +349,7 @@ async fn test_sync_all_states() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Running => CarryOn, + states = Running => Running, assert request >= { "lists": { ALL_ROOMS: { @@ -391,7 +391,7 @@ async fn test_sync_all_states() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = CarryOn => CarryOn, + states = Running => Running, assert request >= { "lists": { ALL_ROOMS: { @@ -433,7 +433,7 @@ async fn test_sync_all_states() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = CarryOn => CarryOn, + states = Running => Running, assert request >= { "lists": { ALL_ROOMS: { @@ -561,7 +561,7 @@ async fn test_sync_resumes_from_previous_state() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Running => CarryOn, + states = Running => Running, assert request >= { "lists": { ALL_ROOMS: { @@ -766,7 +766,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { // Do a regular sync from the `Terminated` state. sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Terminated { .. } => CarryOn, + states = Terminated { .. } => Running, assert request >= { "lists": { ALL_ROOMS: { @@ -797,11 +797,11 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { }, }; - // Do a regular sync from the `CarryOn` state to update the `ALL_ROOMS` list + // Do a regular sync from the `Running` state to update the `ALL_ROOMS` list // again. sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = CarryOn => CarryOn, + states = Running => Running, assert request >= { "lists": { ALL_ROOMS: { @@ -829,11 +829,11 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { }, }; - // Simulate an error from the `CarryOn` state. + // Simulate an error from the `Running` state. sync_then_assert_request_and_fake_response! { [server, room_list, sync] sync matches Some(Err(_)), - states = CarryOn => Terminated { .. }, + states = Running => Terminated { .. }, assert request >= { "lists": { ALL_ROOMS: { @@ -867,7 +867,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { // Do a regular sync from the `Terminated` state. sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Terminated { .. } => CarryOn, + states = Terminated { .. } => Running, assert request >= { "lists": { ALL_ROOMS: { @@ -1352,7 +1352,7 @@ async fn test_invites_stream() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Running => CarryOn, + states = Running => Running, assert request >= { "lists": { ALL_ROOMS: { @@ -1952,7 +1952,7 @@ async fn test_input_viewport() -> Result<(), Error> { // The `timeline_limit` is not repeated because it's sticky. sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Running => CarryOn, + states = Running => Running, assert request >= { "lists": { ALL_ROOMS: { From 6e8e4bb39ceda4b08b3962cff58eb6f2de48e942 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 21 Jun 2023 11:37:00 +0200 Subject: [PATCH 5/9] feat(ui): Differentiate between error and termination in `room_list::State`. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch introduces `State::Error`, which replaces `State::Termianted`. And a “new” `State::Terminated` state is added. They do the same, but their semantics is different. `Error` is when an error happened, and it's fine to restart the sync again. `Terminated` is when a termination has been reached (either because SS sync-loop has ended naturally, or because `RoomList::stop_sync` has been called), in this case, it may not be fine to restart the sync automatically for example. --- crates/matrix-sdk-ui/src/room_list/mod.rs | 2 +- crates/matrix-sdk-ui/src/room_list/state.rs | 47 ++- .../tests/integration/room_list.rs | 269 ++++++++++++++---- 3 files changed, 252 insertions(+), 66 deletions(-) diff --git a/crates/matrix-sdk-ui/src/room_list/mod.rs b/crates/matrix-sdk-ui/src/room_list/mod.rs index 8d34254b6..c9eccb547 100644 --- a/crates/matrix-sdk-ui/src/room_list/mod.rs +++ b/crates/matrix-sdk-ui/src/room_list/mod.rs @@ -174,7 +174,7 @@ impl RoomList { } Some(Err(error)) => { - let next_state = State::Terminated { from: Box::new(self.state.get()) }; + let next_state = State::Error { from: Box::new(self.state.get()) }; self.state.set(next_state); yield Err(Error::SlidingSync(error)); diff --git a/crates/matrix-sdk-ui/src/room_list/state.rs b/crates/matrix-sdk-ui/src/room_list/state.rs index 64bbf6981..cb24e2a18 100644 --- a/crates/matrix-sdk-ui/src/room_list/state.rs +++ b/crates/matrix-sdk-ui/src/room_list/state.rs @@ -28,8 +28,10 @@ pub enum State { /// lists exist. Running, - /// At this state, the sync has been stopped (because it was requested, or - /// because it has errored too many times previously). + /// At this state, the sync has been stopped because an error happened. + Error { from: Box }, + + /// At this state, the sync has been stopped because it was requested. Terminated { from: Box }, } @@ -43,11 +45,11 @@ impl State { Init => (SettingUp, Actions::none()), SettingUp => (Running, Actions::first_rooms_are_loaded()), Running => (Running, Actions::none()), - // If the state was `Terminated`, the next state is calculated again, because it means - // the sync has been restarted. In this case, let's jump back on the - // previous state that led to the termination. No action is required in this - // scenario. - Terminated { from: previous_state } => { + // If the state was `Error` or `Terminated`, the next state is calculated again, because + // it means the sync has been restarted. In this case, let's jump back on + // the previous state that led to the termination. No action is required in + // this scenario. + Error { from: previous_state } | Terminated { from: previous_state } => { match previous_state.as_ref() { state @ Init | state @ SettingUp => { // Do nothing. @@ -59,9 +61,10 @@ impl State { (Running, Actions::refresh_lists()) } - Terminated { .. } => { - // Having `Terminated { from: Terminated { … } }` is not allowed. - unreachable!("It's impossible to reach `Terminated` from `Terminated`"); + Error { .. } | Terminated { .. } => { + // Having `Error { from: Error { .. } }` or `Terminated { from: Terminated { + // … } }` is not allowed. + unreachable!("It's impossible to reach `Error` from `Error`, or `Terminated` from `Terminated`"); } } } @@ -220,6 +223,12 @@ mod tests { // First state. let state = State::Init; + // Hypothetical error. + { + let state = State::Error { from: Box::new(state.clone()) }.next(sliding_sync).await?; + assert_eq!(state, State::Init); + } + // Hypothetical termination. { let state = @@ -231,6 +240,12 @@ mod tests { let state = state.next(sliding_sync).await?; assert_eq!(state, State::SettingUp); + // Hypothetical error. + { + let state = State::Error { from: Box::new(state.clone()) }.next(sliding_sync).await?; + assert_eq!(state, State::SettingUp); + } + // Hypothetical termination. { let state = @@ -242,6 +257,12 @@ mod tests { let state = state.next(sliding_sync).await?; assert_eq!(state, State::Running); + // Hypothetical error. + { + let state = State::Error { from: Box::new(state.clone()) }.next(sliding_sync).await?; + assert_eq!(state, State::Running); + } + // Hypothetical termination. { let state = @@ -253,6 +274,12 @@ mod tests { let state = state.next(sliding_sync).await?; assert_eq!(state, State::Running); + // Hypothetical error. + { + let state = State::Error { from: Box::new(state.clone()) }.next(sliding_sync).await?; + assert_eq!(state, State::Running); + } + // Hypothetical termination. { let state = diff --git a/crates/matrix-sdk-ui/tests/integration/room_list.rs b/crates/matrix-sdk-ui/tests/integration/room_list.rs index b96441e8a..95d543c33 100644 --- a/crates/matrix-sdk-ui/tests/integration/room_list.rs +++ b/crates/matrix-sdk-ui/tests/integration/room_list.rs @@ -600,7 +600,7 @@ async fn test_sync_resumes_from_previous_state() -> Result<(), Error> { } #[async_test] -async fn test_sync_resumes_from_terminated() -> Result<(), Error> { +async fn test_sync_resumes_from_error() -> Result<(), Error> { let (server, room_list) = new_room_list().await?; let sync = room_list.sync(); @@ -610,7 +610,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] sync matches Some(Err(_)), - states = Init => Terminated { .. }, + states = Init => Error { .. }, assert request >= { "lists": { ALL_ROOMS: { @@ -632,10 +632,10 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { let sync = room_list.sync(); pin_mut!(sync); - // Do a regular sync from the `Terminated` state. + // Do a regular sync from the `Error` state. sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Terminated { .. } => SettingUp, + states = Error { .. } => SettingUp, assert request >= { "lists": { ALL_ROOMS: { @@ -659,7 +659,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] sync matches Some(Err(_)), - states = SettingUp => Terminated { .. }, + states = SettingUp => Error { .. }, assert request >= { "lists": { ALL_ROOMS: { @@ -693,10 +693,10 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { // Update the viewport, just to be sure it's not reset later. room_list.apply_input(Input::Viewport(vec![5..=10])).await?; - // Do a regular sync from the `Terminated` state. + // Do a regular sync from the `Error` state. sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Terminated { .. } => Running, + states = Error { .. } => Running, assert request >= { "lists": { ALL_ROOMS: { @@ -732,7 +732,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] sync matches Some(Err(_)), - states = Running => Terminated { .. }, + states = Running => Error { .. }, assert request >= { "lists": { ALL_ROOMS: { @@ -763,10 +763,10 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { let sync = room_list.sync(); pin_mut!(sync); - // Do a regular sync from the `Terminated` state. + // Do a regular sync from the `Error` state. sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Terminated { .. } => Running, + states = Error { .. } => Running, assert request >= { "lists": { ALL_ROOMS: { @@ -833,7 +833,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { sync_then_assert_request_and_fake_response! { [server, room_list, sync] sync matches Some(Err(_)), - states = Running => Terminated { .. }, + states = Running => Error { .. }, assert request >= { "lists": { ALL_ROOMS: { @@ -864,10 +864,10 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { let sync = room_list.sync(); pin_mut!(sync); - // Do a regular sync from the `Terminated` state. + // Do a regular sync from the `Error` state. sync_then_assert_request_and_fake_response! { [server, room_list, sync] - states = Terminated { .. } => Running, + states = Error { .. } => Running, assert request >= { "lists": { ALL_ROOMS: { @@ -901,7 +901,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { } #[async_test] -async fn test_sync_can_be_stopped() -> Result<(), Error> { +async fn test_sync_resumes_from_terminated() -> Result<(), Error> { let (server, room_list) = new_room_list().await?; // Let's stop the sync before actually syncing (we never know!). @@ -911,66 +911,225 @@ async fn test_sync_can_be_stopped() -> Result<(), Error> { let sync = room_list.sync(); pin_mut!(sync); - // First sync, everything is alright and up and running. + // Do a first sync. sync_then_assert_request_and_fake_response! { [server, room_list, sync] states = Init => SettingUp, assert request >= { "lists": { ALL_ROOMS: { + // The default range, in selective sync-mode. "ranges": [[0, 19]], }, }, }, - respond with = { - "pos": "0", - "lists": { - ALL_ROOMS: { - "count": 10, - "ops": [] - }, - }, - "rooms": {}, - }, - }; - - // Now let's stop the `sync`. - room_list.stop_sync()?; - - // `sync` doesn't provide more value. - assert!(sync.next().await.is_none()); - - // If a new sync is started, it will resume from a `Terminated` state. - let sync = room_list.sync(); - pin_mut!(sync); - - sync_then_assert_request_and_fake_response! { - [server, room_list, sync] - states = Terminated { .. } => Running, - assert request >= { - "lists": { - ALL_ROOMS: { - "ranges": [[0, 9]], - }, - VISIBLE_ROOMS: { - "ranges": [[0, 19]], - }, - INVITES: { - "ranges": [[0, 99]], - }, - }, - }, respond with = { "pos": "1", "lists": { ALL_ROOMS: { - "count": 0, + "count": 110, + }, + }, + "rooms": {}, + }, + }; + + // Stop the sync. + room_list.stop_sync()?; + assert!(sync.next().await.is_none()); + + // Start a new sync. + let sync = room_list.sync(); + pin_mut!(sync); + + // Do a regular sync from the `Terminated` state. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = Terminated { .. } => Running, + assert request >= { + "lists": { + ALL_ROOMS: { + // In `SettingUp`, the sync-mode has changed to growing, with + // its initial range. + "ranges": [[0, 49]], }, VISIBLE_ROOMS: { - "count": 0, + // Hello new list. + "ranges": [[0, 19]], + }, + INVITES: { + // Hello new list. + "ranges": [[0, 99]], + }, + }, + }, + respond with = { + "pos": "2", + "lists": { + ALL_ROOMS: { + "count": 110, + }, + }, + "rooms": {}, + }, + }; + + // Stop the sync. + room_list.stop_sync()?; + assert!(sync.next().await.is_none()); + + // Start a new sync. + let sync = room_list.sync(); + pin_mut!(sync); + + // Update the viewport, just to be sure it's not reset later. + room_list.apply_input(Input::Viewport(vec![5..=10])).await?; + + // Do a regular sync from the `Terminated` state. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = Terminated { .. } => Running, + assert request >= { + "lists": { + ALL_ROOMS: { + // In `Running`, the sync-mode is still growing, but the range + // hasn't been modified due to previous termination. + "ranges": [[0, 49]], + }, + VISIBLE_ROOMS: { + // We have set a viewport, which reflects here. + "ranges": [[5, 10]], + }, + INVITES: { + // The range hasn't been modified due to previous termination. + "ranges": [[0, 99]], + }, + }, + }, + respond with = { + "pos": "2", + "lists": { + ALL_ROOMS: { + "count": 110, + }, + INVITES: { + "count": 3, + } + }, + "rooms": {}, + }, + }; + + // Stop the sync. + room_list.stop_sync()?; + assert!(sync.next().await.is_none()); + + // Start a new sync. + let sync = room_list.sync(); + pin_mut!(sync); + + // Do a regular sync from the `Terminated` state. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = Terminated { .. } => Running, + assert request >= { + "lists": { + ALL_ROOMS: { + // In `Running`, the sync-mode is still growing, but the range + // hasn't been modified due to the previous termination. + "ranges": [[0, 49]], + }, + VISIBLE_ROOMS: { + // Despites the termination, the range is kept. + "ranges": [[5, 10]], + }, + INVITES: { + // Despites the error, the range has made progress. + "ranges": [[0, 2]], + }, + }, + }, + respond with = { + "pos": "3", + "lists": { + ALL_ROOMS: { + "count": 110, }, INVITES: { "count": 0, + } + }, + "rooms": {}, + }, + }; + + // Do a regular sync from the `Running` state to update the `ALL_ROOMS` list + // again. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = Running => Running, + assert request >= { + "lists": { + ALL_ROOMS: { + // No termination. The range is making progress. + "ranges": [[0, 99]], + }, + VISIBLE_ROOMS: { + // No termination. The range is still here. + "ranges": [[5, 10]], + }, + INVITES: { + // The range is making progress. + "ranges": [[0, 0]], + }, + }, + }, + respond with = { + "pos": "4", + "lists": { + ALL_ROOMS: { + "count": 110, + }, + }, + "rooms": {}, + }, + }; + + // Stop the sync. + room_list.stop_sync()?; + assert!(sync.next().await.is_none()); + + // Start a new sync. + let sync = room_list.sync(); + pin_mut!(sync); + + // Do a regular sync from the `Terminated` state. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = Terminated { .. } => Running, + assert request >= { + "lists": { + ALL_ROOMS: { + // A termination was received at the previous sync iteration. + // The list is still in growing sync-mode, but its range has + // been reset. + "ranges": [[0, 49]], + }, + VISIBLE_ROOMS: { + // The range is still here. + "ranges": [[5, 10]], + }, + INVITES: { + // The range is kept as it was. + "ranges": [[0, 0]], + }, + }, + }, + respond with = { + "pos": "5", + "lists": { + ALL_ROOMS: { + "count": 110, }, }, "rooms": {}, From fe0b4584039b8d5282a034022933e99096e98829 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 21 Jun 2023 11:53:50 +0200 Subject: [PATCH 6/9] chore(ffi): Update according to previous commits. --- bindings/matrix-sdk-ffi/src/room_list.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/room_list.rs b/bindings/matrix-sdk-ffi/src/room_list.rs index f21fd3ae6..0715a399d 100644 --- a/bindings/matrix-sdk-ffi/src/room_list.rs +++ b/bindings/matrix-sdk-ffi/src/room_list.rs @@ -199,9 +199,9 @@ pub struct RoomListEntriesLoadingStateResult { #[derive(uniffi::Enum)] pub enum RoomListState { Init, - FirstRooms, - AllRooms, - CarryOn, + SettingUp, + Running, + Error, Terminated, } @@ -211,9 +211,9 @@ impl From for RoomListState { match value { Init => Self::Init, - FirstRooms => Self::FirstRooms, - AllRooms => Self::AllRooms, - CarryOn => Self::CarryOn, + SettingUp => Self::SettingUp, + Running => Self::Running, + Error { .. } => Self::Error, Terminated { .. } => Self::Terminated, } } From 54fc0f38ea0b8b492353f40ace0459464fd69d55 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 21 Jun 2023 11:54:51 +0200 Subject: [PATCH 7/9] feat(ui): `RoomList::sync` no longer returns a `TaskHandle`. Because there is `RoomList::stop_sync` now, which is better than dealing with the `TaskHandle`. --- bindings/matrix-sdk-ffi/src/room_list.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/room_list.rs b/bindings/matrix-sdk-ffi/src/room_list.rs index 0715a399d..13d0be762 100644 --- a/bindings/matrix-sdk-ffi/src/room_list.rs +++ b/bindings/matrix-sdk-ffi/src/room_list.rs @@ -87,17 +87,17 @@ pub struct RoomList { #[uniffi::export] impl RoomList { - fn sync(&self) -> Arc { + fn sync(&self) { let this = self.inner.clone(); - Arc::new(TaskHandle::new(RUNTIME.spawn(async move { + RUNTIME.spawn(async move { let sync_stream = this.sync(); pin_mut!(sync_stream); while sync_stream.next().await.is_some() { // keep going! } - }))) + }); } fn stop_sync(&self) -> Result<(), RoomListError> { From 4ec917ca485137409057345a4a828662aa1b402e Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 21 Jun 2023 11:58:44 +0200 Subject: [PATCH 8/9] feat(ffi): Implement `RoomList::is_syncing`. --- bindings/matrix-sdk-ffi/src/room_list.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/bindings/matrix-sdk-ffi/src/room_list.rs b/bindings/matrix-sdk-ffi/src/room_list.rs index 13d0be762..2cd832420 100644 --- a/bindings/matrix-sdk-ffi/src/room_list.rs +++ b/bindings/matrix-sdk-ffi/src/room_list.rs @@ -104,6 +104,12 @@ impl RoomList { self.inner.stop_sync().map_err(Into::into) } + fn is_syncing(&self) -> bool { + use matrix_sdk_ui::room_list::State; + + matches!(self.inner.state().get(), State::SettingUp | State::Running) + } + fn state(&self, listener: Box) -> Arc { let state_stream = self.inner.state(); From f9452959a0c0157eb3ba5fb735974e966c97fa98 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 21 Jun 2023 13:52:14 +0200 Subject: [PATCH 9/9] doc(ui): Fix a typo. --- crates/matrix-sdk-ui/src/room_list/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/matrix-sdk-ui/src/room_list/mod.rs b/crates/matrix-sdk-ui/src/room_list/mod.rs index c9eccb547..d5da453bd 100644 --- a/crates/matrix-sdk-ui/src/room_list/mod.rs +++ b/crates/matrix-sdk-ui/src/room_list/mod.rs @@ -196,8 +196,8 @@ impl RoomList { /// Force to stop the sync of the room list started by [`Self::sync`]. /// /// It's better to call this method rather than stop polling the `Stream` - /// returned by [`Self::sync`] because it will literally force the cancel - /// and exit the sync-loop, i.e. it will cancel any in-flight HTTP requests, + /// returned by [`Self::sync`] because it will force the cancellation and + /// exit the sync-loop, i.e. it will cancel any in-flight HTTP requests, /// cancel any pending futures etc. /// /// Ideally, one wants to consume the `Stream` returned by [`Self::sync`]