fix(sdk): Do not always remove empty chunks from LinkedChunk.

This patch introduces `EmptyChunk`, a new enum used to represent whether
empty chunks must be removed/unlink or kept from the `LinkedChunk`. It
is used by `LinkedChunk::remove_item_at`.

Why is it important? For example, imagine the following situation:

- one inserts a single event in a new chunk (possible if a (sliding)
  sync is done with `timeline_limit=1`),
- one inserts many events at the position of the previous event,
  with one of the new events being a duplicate of the first event
  (possible if a (sliding) sync is done with `timeline_limit=10` this
  time),
- prior to this patch, the older event was removed, resulting in an
  empty chunk, which was removed from the `LinkedChunk`, invalidating
  the insertion position!

So, with this patch:

- `RoomEvents::remove_events` does remove empty chunks, but
- `RoomEvents::remove_events_and_update_insert_position` does NOT remove
  empty chunks, they are kept in case the position wants to insert in this
  same chunk.

Author: Hywan

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<Item>(
         accumulator: &mut Vector<Item>,
@@ -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");
                         }
                     }
                 }

crates/matrix-sdk/src/event_cache/linked_chunk/mod.rs

@@ -408,9 +408,18 @@ impl<const CAP: usize, Item, Gap> LinkedChunk<CAP, Item, Gap> {
 
     /// 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<Item, Error> {
+    /// `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<Item, Error> {
         let chunk_identifier = position.chunk_identifier();
         let item_index = position.index();
 
@@ -446,9 +455,9 @@ impl<const CAP: usize, Item, Gap> LinkedChunk<CAP, Item, Gap> {
                 }
             };
 
-            // 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,15 +1345,30 @@ 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;
 
     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,15 +2074,15 @@ 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]
             assert_items_eq!(linked_chunk, [] ['j']);
             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::*;