From 6e8e4bb39ceda4b08b3962cff58eb6f2de48e942 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 21 Jun 2023 11:37:00 +0200 Subject: [PATCH] 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": {},