diff --git a/crates/matrix-sdk/src/event_cache/linked_chunk/as_vector.rs b/crates/matrix-sdk/src/event_cache/linked_chunk/as_vector.rs index 4a39e5d5a74..f392f8a233b 100644 --- a/crates/matrix-sdk/src/event_cache/linked_chunk/as_vector.rs +++ b/crates/matrix-sdk/src/event_cache/linked_chunk/as_vector.rs @@ -452,7 +452,10 @@ mod tests { use imbl::{vector, Vector}; - use super::{super::LinkedChunk, VectorDiff}; + use super::{ + super::{EmptyChunk, LinkedChunk}, + VectorDiff, + }; fn apply_and_assert_eq( accumulator: &mut Vector, @@ -614,7 +617,10 @@ mod tests { ); let removed_item = linked_chunk - .remove_item_at(linked_chunk.item_position(|item| *item == 'c').unwrap()) + .remove_item_at( + linked_chunk.item_position(|item| *item == 'c').unwrap(), + EmptyChunk::Remove, + ) .unwrap(); assert_eq!(removed_item, 'c'); assert_items_eq!( @@ -634,7 +640,10 @@ mod tests { apply_and_assert_eq(&mut accumulator, as_vector.take(), &[VectorDiff::Remove { index: 7 }]); let removed_item = linked_chunk - .remove_item_at(linked_chunk.item_position(|item| *item == 'z').unwrap()) + .remove_item_at( + linked_chunk.item_position(|item| *item == 'z').unwrap(), + EmptyChunk::Remove, + ) .unwrap(); assert_eq!(removed_item, 'z'); assert_items_eq!( @@ -773,7 +782,7 @@ mod tests { continue; }; - linked_chunk.remove_item_at(position).expect("Failed to remove an item"); + linked_chunk.remove_item_at(position, EmptyChunk::Remove).expect("Failed to remove an item"); } } } diff --git a/crates/matrix-sdk/src/event_cache/linked_chunk/mod.rs b/crates/matrix-sdk/src/event_cache/linked_chunk/mod.rs index 99fd02fe5bc..c3c69a77719 100644 --- a/crates/matrix-sdk/src/event_cache/linked_chunk/mod.rs +++ b/crates/matrix-sdk/src/event_cache/linked_chunk/mod.rs @@ -408,9 +408,18 @@ impl LinkedChunk { /// Remove item at a specified position in the [`LinkedChunk`]. /// - /// Because the `position` can be invalid, this method returns a - /// `Result`. - pub fn remove_item_at(&mut self, position: Position) -> Result { + /// `position` must point to a valid item, otherwise the method returns + /// `Err`. + /// + /// The chunk containing the item represented by `position` may be empty + /// once the item has been removed. In this case, the chunk can be removed + /// if `empty_chunk` contains [`EmptyChunk::Remove`], otherwise the chunk is + /// kept if `empty_chunk` contains [`EmptyChunk::Keep`]. + pub fn remove_item_at( + &mut self, + position: Position, + empty_chunk: EmptyChunk, + ) -> Result { let chunk_identifier = position.chunk_identifier(); let item_index = position.index(); @@ -446,9 +455,9 @@ impl LinkedChunk { } }; - // If the `chunk` can be unlinked, and if the `chunk` is not the first one, we - // can remove it. - if can_unlink_chunk && chunk.is_first_chunk().not() { + // If removing empty chunk is desired, and if the `chunk` can be unlinked, and + // if the `chunk` is not the first one, we can remove it. + if empty_chunk.remove() && can_unlink_chunk && chunk.is_first_chunk().not() { // Unlink `chunk`. chunk.unlink(&mut self.updates); @@ -1336,6 +1345,21 @@ where } } +/// A type representing what to do when the system has to handle an empty chunk. +pub(crate) enum EmptyChunk { + /// Keep the empty chunk. + Keep, + + /// Remove the empty chunk. + Remove, +} + +impl EmptyChunk { + fn remove(&self) -> bool { + matches!(self, Self::Remove) + } +} + #[cfg(test)] mod tests { use std::ops::Not; @@ -1343,8 +1367,8 @@ mod tests { use assert_matches::assert_matches; use super::{ - Chunk, ChunkContent, ChunkIdentifier, ChunkIdentifierGenerator, Error, LinkedChunk, - Position, + Chunk, ChunkContent, ChunkIdentifier, ChunkIdentifierGenerator, EmptyChunk, Error, + LinkedChunk, Position, }; #[test] @@ -1950,21 +1974,21 @@ mod tests { // that. The chunk is removed. { let position_of_f = linked_chunk.item_position(|item| *item == 'f').unwrap(); - let removed_item = linked_chunk.remove_item_at(position_of_f)?; + let removed_item = linked_chunk.remove_item_at(position_of_f, EmptyChunk::Remove)?; assert_eq!(removed_item, 'f'); assert_items_eq!(linked_chunk, ['a', 'b', 'c'] ['d', 'e'] ['g', 'h', 'i'] ['j', 'k']); assert_eq!(linked_chunk.len(), 10); let position_of_e = linked_chunk.item_position(|item| *item == 'e').unwrap(); - let removed_item = linked_chunk.remove_item_at(position_of_e)?; + let removed_item = linked_chunk.remove_item_at(position_of_e, EmptyChunk::Remove)?; assert_eq!(removed_item, 'e'); assert_items_eq!(linked_chunk, ['a', 'b', 'c'] ['d'] ['g', 'h', 'i'] ['j', 'k']); assert_eq!(linked_chunk.len(), 9); let position_of_d = linked_chunk.item_position(|item| *item == 'd').unwrap(); - let removed_item = linked_chunk.remove_item_at(position_of_d)?; + let removed_item = linked_chunk.remove_item_at(position_of_d, EmptyChunk::Remove)?; assert_eq!(removed_item, 'd'); assert_items_eq!(linked_chunk, ['a', 'b', 'c'] ['g', 'h', 'i'] ['j', 'k']); @@ -1985,19 +2009,19 @@ mod tests { // that. The chunk is NOT removed because it's the first chunk. { let first_position = linked_chunk.item_position(|item| *item == 'a').unwrap(); - let removed_item = linked_chunk.remove_item_at(first_position)?; + let removed_item = linked_chunk.remove_item_at(first_position, EmptyChunk::Remove)?; assert_eq!(removed_item, 'a'); assert_items_eq!(linked_chunk, ['b', 'c'] ['g', 'h', 'i'] ['j', 'k']); assert_eq!(linked_chunk.len(), 7); - let removed_item = linked_chunk.remove_item_at(first_position)?; + let removed_item = linked_chunk.remove_item_at(first_position, EmptyChunk::Remove)?; assert_eq!(removed_item, 'b'); assert_items_eq!(linked_chunk, ['c'] ['g', 'h', 'i'] ['j', 'k']); assert_eq!(linked_chunk.len(), 6); - let removed_item = linked_chunk.remove_item_at(first_position)?; + let removed_item = linked_chunk.remove_item_at(first_position, EmptyChunk::Remove)?; assert_eq!(removed_item, 'c'); assert_items_eq!(linked_chunk, [] ['g', 'h', 'i'] ['j', 'k']); @@ -2017,19 +2041,19 @@ mod tests { // that. The chunk is removed. { let first_position = linked_chunk.item_position(|item| *item == 'g').unwrap(); - let removed_item = linked_chunk.remove_item_at(first_position)?; + let removed_item = linked_chunk.remove_item_at(first_position, EmptyChunk::Remove)?; assert_eq!(removed_item, 'g'); assert_items_eq!(linked_chunk, [] ['h', 'i'] ['j', 'k']); assert_eq!(linked_chunk.len(), 4); - let removed_item = linked_chunk.remove_item_at(first_position)?; + let removed_item = linked_chunk.remove_item_at(first_position, EmptyChunk::Remove)?; assert_eq!(removed_item, 'h'); assert_items_eq!(linked_chunk, [] ['i'] ['j', 'k']); assert_eq!(linked_chunk.len(), 3); - let removed_item = linked_chunk.remove_item_at(first_position)?; + let removed_item = linked_chunk.remove_item_at(first_position, EmptyChunk::Remove)?; assert_eq!(removed_item, 'i'); assert_items_eq!(linked_chunk, [] ['j', 'k']); @@ -2050,7 +2074,7 @@ mod tests { // The chunk is removed. { let position_of_k = linked_chunk.item_position(|item| *item == 'k').unwrap(); - let removed_item = linked_chunk.remove_item_at(position_of_k)?; + let removed_item = linked_chunk.remove_item_at(position_of_k, EmptyChunk::Remove)?; assert_eq!(removed_item, 'k'); #[rustfmt::skip] @@ -2058,7 +2082,7 @@ mod tests { assert_eq!(linked_chunk.len(), 1); let position_of_j = linked_chunk.item_position(|item| *item == 'j').unwrap(); - let removed_item = linked_chunk.remove_item_at(position_of_j)?; + let removed_item = linked_chunk.remove_item_at(position_of_j, EmptyChunk::Remove)?; assert_eq!(removed_item, 'j'); assert_items_eq!(linked_chunk, []); @@ -2092,27 +2116,27 @@ mod tests { let _ = linked_chunk.updates().unwrap().take(); let position_of_c = linked_chunk.item_position(|item| *item == 'c').unwrap(); - let removed_item = linked_chunk.remove_item_at(position_of_c)?; + let removed_item = linked_chunk.remove_item_at(position_of_c, EmptyChunk::Remove)?; assert_eq!(removed_item, 'c'); assert_items_eq!(linked_chunk, ['a', 'b'] [-] ['d']); assert_eq!(linked_chunk.len(), 3); let position_of_d = linked_chunk.item_position(|item| *item == 'd').unwrap(); - let removed_item = linked_chunk.remove_item_at(position_of_d)?; + let removed_item = linked_chunk.remove_item_at(position_of_d, EmptyChunk::Remove)?; assert_eq!(removed_item, 'd'); assert_items_eq!(linked_chunk, ['a', 'b'] [-]); assert_eq!(linked_chunk.len(), 2); let first_position = linked_chunk.item_position(|item| *item == 'a').unwrap(); - let removed_item = linked_chunk.remove_item_at(first_position)?; + let removed_item = linked_chunk.remove_item_at(first_position, EmptyChunk::Remove)?; assert_eq!(removed_item, 'a'); assert_items_eq!(linked_chunk, ['b'] [-]); assert_eq!(linked_chunk.len(), 1); - let removed_item = linked_chunk.remove_item_at(first_position)?; + let removed_item = linked_chunk.remove_item_at(first_position, EmptyChunk::Remove)?; assert_eq!(removed_item, 'b'); assert_items_eq!(linked_chunk, [] [-]); @@ -2134,6 +2158,110 @@ mod tests { Ok(()) } + #[test] + fn test_remove_item_at_and_keep_empty_chunks() -> Result<(), Error> { + use super::Update::*; + + let mut linked_chunk = LinkedChunk::<3, char, ()>::new_with_update_history(); + linked_chunk.push_items_back(['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h']); + assert_items_eq!(linked_chunk, ['a', 'b', 'c'] ['d', 'e', 'f'] ['g', 'h']); + assert_eq!(linked_chunk.len(), 8); + + // Ignore previous updates. + let _ = linked_chunk.updates().unwrap().take(); + + // Remove all items from the same chunk. The chunk is empty after that. The + // chunk is NOT removed because we asked to keep it. + { + let position = linked_chunk.item_position(|item| *item == 'd').unwrap(); + let removed_item = linked_chunk.remove_item_at(position, EmptyChunk::Keep)?; + + assert_eq!(removed_item, 'd'); + assert_items_eq!(linked_chunk, ['a', 'b', 'c'] ['e', 'f'] ['g', 'h']); + assert_eq!(linked_chunk.len(), 7); + + let removed_item = linked_chunk.remove_item_at(position, EmptyChunk::Keep)?; + + assert_eq!(removed_item, 'e'); + assert_items_eq!(linked_chunk, ['a', 'b', 'c'] ['f'] ['g', 'h']); + assert_eq!(linked_chunk.len(), 6); + + let removed_item = linked_chunk.remove_item_at(position, EmptyChunk::Keep)?; + + assert_eq!(removed_item, 'f'); + assert_items_eq!(linked_chunk, ['a', 'b', 'c'] [] ['g', 'h']); + assert_eq!(linked_chunk.len(), 5); + + assert_eq!( + linked_chunk.updates().unwrap().take(), + &[ + RemoveItem { at: Position(ChunkIdentifier(1), 0) }, + RemoveItem { at: Position(ChunkIdentifier(1), 0) }, + RemoveItem { at: Position(ChunkIdentifier(1), 0) }, + ] + ); + } + + // Remove all items from the same chunk. The chunk is empty after that. The + // chunk is NOT removed because we asked to keep it. + { + let position = linked_chunk.item_position(|item| *item == 'g').unwrap(); + let removed_item = linked_chunk.remove_item_at(position, EmptyChunk::Keep)?; + + assert_eq!(removed_item, 'g'); + assert_items_eq!(linked_chunk, ['a', 'b', 'c'] [] ['h']); + assert_eq!(linked_chunk.len(), 4); + + let removed_item = linked_chunk.remove_item_at(position, EmptyChunk::Keep)?; + + assert_eq!(removed_item, 'h'); + assert_items_eq!(linked_chunk, ['a', 'b', 'c'] [] []); + assert_eq!(linked_chunk.len(), 3); + + assert_eq!( + linked_chunk.updates().unwrap().take(), + &[ + RemoveItem { at: Position(ChunkIdentifier(2), 0) }, + RemoveItem { at: Position(ChunkIdentifier(2), 0) }, + ] + ); + } + + // Remove all items from the same chunk. The chunk is empty after that. The + // chunk is NOT removed because we asked to keep it. + { + let position = linked_chunk.item_position(|item| *item == 'a').unwrap(); + let removed_item = linked_chunk.remove_item_at(position, EmptyChunk::Keep)?; + + assert_eq!(removed_item, 'a'); + assert_items_eq!(linked_chunk, ['b', 'c'] [] []); + assert_eq!(linked_chunk.len(), 2); + + let removed_item = linked_chunk.remove_item_at(position, EmptyChunk::Keep)?; + + assert_eq!(removed_item, 'b'); + assert_items_eq!(linked_chunk, ['c'] [] []); + assert_eq!(linked_chunk.len(), 1); + + let removed_item = linked_chunk.remove_item_at(position, EmptyChunk::Keep)?; + + assert_eq!(removed_item, 'c'); + assert_items_eq!(linked_chunk, [] [] []); + assert_eq!(linked_chunk.len(), 0); + + assert_eq!( + linked_chunk.updates().unwrap().take(), + &[ + RemoveItem { at: Position(ChunkIdentifier(0), 0) }, + RemoveItem { at: Position(ChunkIdentifier(0), 0) }, + RemoveItem { at: Position(ChunkIdentifier(0), 0) }, + ] + ); + } + + Ok(()) + } + #[test] fn test_insert_gap_at() -> Result<(), Error> { use super::Update::*; diff --git a/crates/matrix-sdk/src/event_cache/room/events.rs b/crates/matrix-sdk/src/event_cache/room/events.rs index 19d27b2e80d..0f714e9498b 100644 --- a/crates/matrix-sdk/src/event_cache/room/events.rs +++ b/crates/matrix-sdk/src/event_cache/room/events.rs @@ -20,7 +20,7 @@ use tracing::{debug, error, warn}; use super::super::{ deduplicator::{Decoration, Deduplicator}, - linked_chunk::{Chunk, ChunkIdentifier, Error, Iter, LinkedChunk, Position}, + linked_chunk::{Chunk, ChunkIdentifier, EmptyChunk, Error, Iter, LinkedChunk, Position}, }; /// An alias for the real event type. @@ -229,7 +229,14 @@ impl RoomEvents { }; self.chunks - .remove_item_at(event_position) + .remove_item_at( + event_position, + // If removing an event results in an empty chunk, the empty chunk is removed + // because nothing is going to be inserted in it apparently, otherwise the + // `Self::remove_events_and_update_insert_position` method would have been + // used. + EmptyChunk::Remove, + ) .expect("Failed to remove an event we have just found"); } } @@ -280,7 +287,12 @@ impl RoomEvents { }; self.chunks - .remove_item_at(event_position) + .remove_item_at( + event_position, + // If removing an event results in an empty chunk, the empty chunk is kept + // because maybe something is going to be inserted in it! + EmptyChunk::Keep, + ) .expect("Failed to remove an event we have just found"); // A `Position` is composed of a `ChunkIdentifier` and an index. @@ -405,6 +417,37 @@ mod tests { ); } + #[test] + fn test_push_events_with_duplicates_on_a_chunk_of_one_event() { + let (event_id_0, event_0) = new_event("$ev0"); + + let mut room_events = RoomEvents::new(); + + // The first chunk can never be removed, so let's create a gap, then a new + // chunk. + room_events.push_gap(Gap { prev_token: "hello".to_owned() }); + room_events.push_events([event_0.clone()]); + + assert_events_eq!( + room_events.events(), + [ + (event_id_0 at (2, 0)), + ] + ); + + // Everything is alright. Now let's push a duplicated event. + room_events.push_events([event_0]); + + // The event has been removed, then the chunk was empty, so removed, and a new + // chunk has been created with identifier 3. + assert_events_eq!( + room_events.events(), + [ + (event_id_0 at (3, 0)), + ] + ); + } + #[test] fn test_push_gap() { let (event_id_0, event_0) = new_event("$ev0"); @@ -511,6 +554,36 @@ mod tests { ); } + #[test] + fn test_insert_events_at_with_duplicates_on_a_chunk_of_one_event() { + let (event_id_0, event_0) = new_event("$ev0"); + + let mut room_events = RoomEvents::new(); + + // The first chunk can never be removed, so let's create a gap, then a new + // chunk. + room_events.push_gap(Gap { prev_token: "hello".to_owned() }); + room_events.push_events([event_0.clone()]); + + let position_of_event_0 = room_events + .events() + .find_map(|(position, event)| { + (event.event_id().unwrap() == event_id_0).then_some(position) + }) + .unwrap(); + + room_events.insert_events_at([event_0], position_of_event_0).unwrap(); + + // Event has been removed, the chunk was empty, but it was kept so that the + // position was still valid and the new event can be inserted. + assert_events_eq!( + room_events.events(), + [ + (event_id_0 at (2, 0)), + ] + ); + } + #[test] fn test_insert_gap_at() { let (event_id_0, event_0) = new_event("$ev0"); @@ -656,17 +729,20 @@ mod tests { // Push some events. let mut room_events = RoomEvents::new(); - room_events.push_events([event_0, event_1, event_2, event_3]); + room_events.push_events([event_0, event_1]); + room_events.push_gap(Gap { prev_token: "hello".to_owned() }); + room_events.push_events([event_2, event_3]); assert_events_eq!( room_events.events(), [ (event_id_0 at (0, 0)), (event_id_1 at (0, 1)), - (event_id_2 at (0, 2)), - (event_id_3 at (0, 3)), + (event_id_2 at (2, 0)), + (event_id_3 at (2, 1)), ] ); + assert_eq!(room_events.chunks().count(), 3); // Remove some events. room_events.remove_events(vec![event_id_1, event_id_3]); @@ -675,9 +751,20 @@ mod tests { room_events.events(), [ (event_id_0 at (0, 0)), - (event_id_2 at (0, 1)), + (event_id_2 at (2, 0)), + ] + ); + + // Ensure chunks are removed once empty. + room_events.remove_events(vec![event_id_2]); + + assert_events_eq!( + room_events.events(), + [ + (event_id_0 at (0, 0)), ] ); + assert_eq!(room_events.chunks().count(), 2); } #[test] @@ -717,6 +804,8 @@ mod tests { room_events.push_gap(Gap { prev_token: "raclette".to_owned() }); room_events.push_events([event_7, event_8]); + assert_eq!(room_events.chunks().count(), 3); + fn position_of(room_events: &RoomEvents, event_id: &EventId) -> Position { room_events .events() @@ -832,7 +921,7 @@ mod tests { { let previous_position = position; room_events.remove_events_and_update_insert_position( - vec![event_id_2, event_id_3, event_id_7], + vec![event_id_2, event_id_3, event_id_7, event_id_8], &mut position, ); @@ -848,9 +937,11 @@ mod tests { room_events.events(), [ (event_id_6 at (0, 0)), - (event_id_8 at (2, 0)), ] ); } + + // Ensure no chunk has been removed. + assert_eq!(room_events.chunks().count(), 3); } }