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 bff2eecde6b..4a39e5d5a74 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 @@ -14,7 +14,7 @@ use std::{ collections::VecDeque, - ops::ControlFlow, + ops::{ControlFlow, Not}, sync::{Arc, RwLock}, }; @@ -22,7 +22,7 @@ use eyeball_im::VectorDiff; use super::{ updates::{ReaderToken, Update, UpdatesInner}, - ChunkContent, ChunkIdentifier, Iter, + ChunkContent, ChunkIdentifier, Iter, Position, }; /// A type alias to represent a chunk's length. This is purely for commodity. @@ -253,7 +253,8 @@ impl UpdateToVectorDiff { // // From the `VectorDiff` “point of view”, this optimisation aims at avoiding // removing items to push them again later. - let mut mute_push_items = false; + let mut reattaching = false; + let mut detaching = false; for update in updates { match update { @@ -329,46 +330,22 @@ impl UpdateToVectorDiff { } Update::PushItems { at: position, items } => { - let expected_chunk_identifier = position.chunk_identifier(); + let number_of_chunks = self.chunks.len(); + let (offset, (chunk_index, chunk_length)) = self.map_to_offset(position); - let (chunk_index, offset, chunk_length) = { - let control_flow = self.chunks.iter_mut().enumerate().try_fold( - position.index(), - |offset, (chunk_index, (chunk_identifier, chunk_length))| { - if chunk_identifier == &expected_chunk_identifier { - ControlFlow::Break((chunk_index, offset, chunk_length)) - } else { - ControlFlow::Continue(offset + *chunk_length) - } - }, - ); - - match control_flow { - // Chunk has been found, and all values have been calculated as - // expected. - ControlFlow::Break(values) => values, - - // Chunk has not been found. - ControlFlow::Continue(..) => { - // SAFETY: Assuming `LinkedChunk` and `ObservableUpdates` are not - // buggy, and assuming `Self::chunks` is correctly initialized, it - // is not possible to push items on a chunk that does not exist. If - // this predicate fails, it means `LinkedChunk` or - // `ObservableUpdates` contain a bug. - panic!("Pushing items: The chunk is not found"); - } - } - }; + let is_pushing_back = + chunk_index + 1 == number_of_chunks && position.index() >= *chunk_length; + // Add the number of items to the chunk in `self.chunks`. *chunk_length += items.len(); - // See `mute_push_items` to learn more. - if mute_push_items { + // See `reattaching` to learn more. + if reattaching { continue; } // Optimisation: we can emit a `VectorDiff::Append` in this particular case. - if chunk_index + 1 == self.chunks.len() { + if is_pushing_back && detaching.not() { diffs.push(VectorDiff::Append { values: items.into() }); } // No optimisation: let's emit `VectorDiff::Insert`. @@ -379,15 +356,30 @@ impl UpdateToVectorDiff { } } - Update::DetachLastItems { at } => { - let expected_chunk_identifier = at.chunk_identifier(); - let new_length = at.index(); + Update::RemoveItem { at: position } => { + let (offset, (_chunk_index, chunk_length)) = self.map_to_offset(position); + + // Remove one item to the chunk in `self.chunks`. + *chunk_length -= 1; - let length = self + // See `reattaching` to learn more. + if reattaching { + continue; + } + + // Let's emit a `VectorDiff::Remove`. + diffs.push(VectorDiff::Remove { index: offset }); + } + + Update::DetachLastItems { at: position } => { + let expected_chunk_identifier = position.chunk_identifier(); + let new_length = position.index(); + + let chunk_length = self .chunks .iter_mut() - .find_map(|(chunk_identifier, length)| { - (*chunk_identifier == expected_chunk_identifier).then_some(length) + .find_map(|(chunk_identifier, chunk_length)| { + (*chunk_identifier == expected_chunk_identifier).then_some(chunk_length) }) // SAFETY: Assuming `LinkedChunk` and `ObservableUpdates` are not buggy, and // assuming `Self::chunks` is correctly initialized, it is not possible to @@ -395,23 +387,63 @@ impl UpdateToVectorDiff { // it means `LinkedChunk` or `ObservableUpdates` contain a bug. .expect("Detach last items: The chunk is not found"); - *length = new_length; + *chunk_length = new_length; + + // Entering the _detaching_ mode. + detaching = true; } Update::StartReattachItems => { - // Entering the `reattaching` mode. - mute_push_items = true; + // Entering the _reattaching_ mode. + reattaching = true; } Update::EndReattachItems => { - // Exiting the `reattaching` mode. - mute_push_items = false; + // Exiting the _reattaching_ mode. + reattaching = false; + + // Exiting the _detaching_ mode. + detaching = false; } } } diffs } + + fn map_to_offset(&mut self, position: &Position) -> (usize, (usize, &mut usize)) { + let expected_chunk_identifier = position.chunk_identifier(); + + let (offset, (chunk_index, chunk_length)) = { + let control_flow = self.chunks.iter_mut().enumerate().try_fold( + position.index(), + |offset, (chunk_index, (chunk_identifier, chunk_length))| { + if chunk_identifier == &expected_chunk_identifier { + ControlFlow::Break((offset, (chunk_index, chunk_length))) + } else { + ControlFlow::Continue(offset + *chunk_length) + } + }, + ); + + match control_flow { + // Chunk has been found, and all values have been calculated as + // expected. + ControlFlow::Break(values) => values, + + // Chunk has not been found. + ControlFlow::Continue(..) => { + // SAFETY: Assuming `LinkedChunk` and `ObservableUpdates` are not buggy, and + // assuming `Self::chunks` is correctly initialized, it is not possible to work + // on a chunk that does not exist. If this predicate fails, it means + // `LinkedChunk` or `ObservableUpdates` contain a bug. + panic!("The chunk is not found"); + } + } + }; + + (offset, (chunk_index, chunk_length)) + } } #[cfg(test)] @@ -435,6 +467,9 @@ mod tests { match diff { VectorDiff::Insert { index, value } => accumulator.insert(index, value), VectorDiff::Append { values } => accumulator.append(values), + VectorDiff::Remove { index } => { + accumulator.remove(index); + } diff => unimplemented!("{diff:?}"), } } @@ -578,15 +613,77 @@ mod tests { &[VectorDiff::Insert { index: 0, value: 'm' }], ); + let removed_item = linked_chunk + .remove_item_at(linked_chunk.item_position(|item| *item == 'c').unwrap()) + .unwrap(); + assert_eq!(removed_item, 'c'); + assert_items_eq!( + linked_chunk, + ['m', 'a', 'w'] ['x'] ['y', 'z', 'b'] ['d'] ['i', 'j', 'k'] ['l'] ['e', 'f', 'g'] ['h'] + ); + + // From an `ObservableVector` point of view, it would look like: + // + // 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 + // +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+ + // | m | a | w | x | y | z | b | d | i | j | k | l | e | f | g | h | + // +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+ + // ^ + // | + // `c` has been removed + 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()) + .unwrap(); + assert_eq!(removed_item, 'z'); + assert_items_eq!( + linked_chunk, + ['m', 'a', 'w'] ['x'] ['y', 'b'] ['d'] ['i', 'j', 'k'] ['l'] ['e', 'f', 'g'] ['h'] + ); + + // From an `ObservableVector` point of view, it would look like: + // + // 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 + // +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+ + // | m | a | w | x | y | b | d | i | j | k | l | e | f | g | h | + // +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+ + // ^ + // | + // `z` has been removed + apply_and_assert_eq(&mut accumulator, as_vector.take(), &[VectorDiff::Remove { index: 5 }]); + + linked_chunk + .insert_items_at(['z'], linked_chunk.item_position(|item| *item == 'h').unwrap()) + .unwrap(); + + assert_items_eq!( + linked_chunk, + ['m', 'a', 'w'] ['x'] ['y', 'b'] ['d'] ['i', 'j', 'k'] ['l'] ['e', 'f', 'g'] ['z', 'h'] + ); + + // From an `ObservableVector` point of view, it would look like: + // + // 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 + // +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+ + // | m | a | w | x | y | b | d | i | j | k | l | e | f | g | z | h | + // +---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+---+ + // ^^^^ + // | + // new! + apply_and_assert_eq( + &mut accumulator, + as_vector.take(), + &[VectorDiff::Insert { index: 14, value: 'z' }], + ); + drop(linked_chunk); assert!(as_vector.take().is_empty()); // Finally, ensure the “reconstitued” vector is the one expected. assert_eq!( accumulator, - vector![ - 'm', 'a', 'w', 'x', 'y', 'z', 'b', 'c', 'd', 'i', 'j', 'k', 'l', 'e', 'f', 'g', 'h' - ] + vector!['m', 'a', 'w', 'x', 'y', 'b', 'd', 'i', 'j', 'k', 'l', 'e', 'f', 'g', 'z', 'h'] ); } @@ -622,6 +719,7 @@ mod tests { PushItems { items: Vec }, PushGap, ReplaceLastGap { items: Vec }, + RemoveItem { item: char }, } fn as_vector_operation_strategy() -> impl Strategy { @@ -633,13 +731,16 @@ mod tests { 1 => prop::collection::vec(prop::char::ranges(vec!['a'..='z', 'A'..='Z'].into()), 0..=25) .prop_map(|items| AsVectorOperation::ReplaceLastGap { items }), + + 1 => prop::char::ranges(vec!['a'..='z', 'A'..='Z'].into()) + .prop_map(|item| AsVectorOperation::RemoveItem { item }), ] } proptest! { #[test] fn as_vector_is_correct( - operations in prop::collection::vec(as_vector_operation_strategy(), 10..=50) + operations in prop::collection::vec(as_vector_operation_strategy(), 50..=200) ) { let mut linked_chunk = LinkedChunk::<10, char, ()>::new_with_update_history(); let mut as_vector = linked_chunk.as_vector().unwrap(); @@ -662,7 +763,17 @@ mod tests { continue; }; - linked_chunk.replace_gap_at(items, gap_identifier).unwrap(); + linked_chunk.replace_gap_at(items, gap_identifier).expect("Failed to replace a gap"); + } + + AsVectorOperation::RemoveItem { item: expected_item } => { + let Some(position) = linked_chunk + .items().find_map(|(position, item)| (*item == expected_item).then_some(position)) + else { + continue; + }; + + linked_chunk.remove_item_at(position).expect("Failed to remove an item"); } } } @@ -678,6 +789,9 @@ mod tests { vector_from_diffs.append(&mut values); } + VectorDiff::Remove { index } => { + vector_from_diffs.remove(index); + } _ => unreachable!(), } } 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 ce70c12475d..1bfda5f9df7 100644 --- a/crates/matrix-sdk/src/event_cache/linked_chunk/mod.rs +++ b/crates/matrix-sdk/src/event_cache/linked_chunk/mod.rs @@ -406,6 +406,79 @@ impl LinkedChunk { Ok(()) } + /// 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 { + let chunk_identifier = position.chunk_identifier(); + let item_index = position.index(); + + let mut chunk_ptr = None; + let removed_item; + + { + let chunk = self + .links + .chunk_mut(chunk_identifier) + .ok_or(Error::InvalidChunkIdentifier { identifier: chunk_identifier })?; + + let can_unlink_chunk = match &mut chunk.content { + ChunkContent::Gap(..) => { + return Err(Error::ChunkIsAGap { identifier: chunk_identifier }) + } + + ChunkContent::Items(current_items) => { + let current_items_length = current_items.len(); + + if item_index > current_items_length { + return Err(Error::InvalidItemIndex { index: item_index }); + } + + removed_item = current_items.remove(item_index); + + if let Some(updates) = self.updates.as_mut() { + updates + .push(Update::RemoveItem { at: Position(chunk_identifier, item_index) }) + } + + current_items.is_empty() + } + }; + + // 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() { + // Unlink `chunk`. + chunk.unlink(&mut self.updates); + + chunk_ptr = Some(chunk.as_ptr()); + + // We need to update `self.last` if and only if `chunk` _is_ the last chunk. The + // new last chunk is the chunk before `chunk`. + if chunk.is_last_chunk() { + self.links.last = chunk.previous; + } + } + + self.length -= 1; + + // Stop borrowing `chunk`. + } + + if let Some(chunk_ptr) = chunk_ptr { + // `chunk` has been unlinked. + + // Re-box the chunk, and let Rust does its job. + // + // SAFETY: `chunk` is unlinked and not borrowed anymore. `LinkedChunk` doesn't + // use it anymore, it's a leak. It is time to re-`Box` it and drop it. + let _chunk_boxed = unsafe { Box::from_raw(chunk_ptr.as_ptr()) }; + } + + Ok(removed_item) + } + /// Insert a gap at a specified position in the [`LinkedChunk`]. /// /// Because the `position` can be invalid, this method returns a @@ -1852,6 +1925,206 @@ mod tests { Ok(()) } + #[test] + fn test_remove_item_at() -> 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', 'i', 'j', 'k']); + assert_items_eq!(linked_chunk, ['a', 'b', 'c'] ['d', 'e', 'f'] ['g', 'h', 'i'] ['j', 'k']); + assert_eq!(linked_chunk.len(), 11); + + // Ignore previous updates. + let _ = linked_chunk.updates().unwrap().take(); + + // Remove the last item of the middle chunk, 3 times. The chunk is empty after + // 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)?; + + 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)?; + + 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)?; + + assert_eq!(removed_item, 'd'); + assert_items_eq!(linked_chunk, ['a', 'b', 'c'] ['g', 'h', 'i'] ['j', 'k']); + assert_eq!(linked_chunk.len(), 8); + + assert_eq!( + linked_chunk.updates().unwrap().take(), + &[ + RemoveItem { at: Position(ChunkIdentifier(1), 2) }, + RemoveItem { at: Position(ChunkIdentifier(1), 1) }, + RemoveItem { at: Position(ChunkIdentifier(1), 0) }, + RemoveChunk(ChunkIdentifier(1)), + ] + ); + } + + // Remove the first item of the first chunk, 3 times. The chunk is empty after + // 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)?; + + 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)?; + + 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)?; + + assert_eq!(removed_item, 'c'); + assert_items_eq!(linked_chunk, [] ['g', 'h', 'i'] ['j', 'k']); + assert_eq!(linked_chunk.len(), 5); + + 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) }, + ] + ); + } + + // Remove the first item of the middle chunk, 3 times. The chunk is empty after + // 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)?; + + 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)?; + + 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)?; + + assert_eq!(removed_item, 'i'); + assert_items_eq!(linked_chunk, [] ['j', 'k']); + assert_eq!(linked_chunk.len(), 2); + + assert_eq!( + linked_chunk.updates().unwrap().take(), + &[ + RemoveItem { at: Position(ChunkIdentifier(2), 0) }, + RemoveItem { at: Position(ChunkIdentifier(2), 0) }, + RemoveItem { at: Position(ChunkIdentifier(2), 0) }, + RemoveChunk(ChunkIdentifier(2)), + ] + ); + } + + // Remove the last item of the last chunk, twice. The chunk is empty after that. + // 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)?; + + 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)?; + + assert_eq!(removed_item, 'j'); + assert_items_eq!(linked_chunk, []); + assert_eq!(linked_chunk.len(), 0); + + assert_eq!( + linked_chunk.updates().unwrap().take(), + &[ + RemoveItem { at: Position(ChunkIdentifier(3), 1) }, + RemoveItem { at: Position(ChunkIdentifier(3), 0) }, + RemoveChunk(ChunkIdentifier(3)), + ] + ); + } + + // Add a couple more items, delete one, add a gap, and delete more items. + { + linked_chunk.push_items_back(['a', 'b', 'c', 'd']); + + #[rustfmt::skip] + assert_items_eq!(linked_chunk, ['a', 'b', 'c'] ['d']); + assert_eq!(linked_chunk.len(), 4); + + let position_of_c = linked_chunk.item_position(|item| *item == 'c').unwrap(); + linked_chunk.insert_gap_at((), position_of_c)?; + + assert_items_eq!(linked_chunk, ['a', 'b'] [-] ['c'] ['d']); + assert_eq!(linked_chunk.len(), 4); + + // Ignore updates. + 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)?; + + 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)?; + + 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)?; + + 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)?; + + assert_eq!(removed_item, 'b'); + assert_items_eq!(linked_chunk, [] [-]); + assert_eq!(linked_chunk.len(), 0); + + assert_eq!( + linked_chunk.updates().unwrap().take(), + &[ + RemoveItem { at: Position(ChunkIdentifier(6), 0) }, + RemoveChunk(ChunkIdentifier(6)), + RemoveItem { at: Position(ChunkIdentifier(4), 0) }, + RemoveChunk(ChunkIdentifier(4)), + 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/linked_chunk/updates.rs b/crates/matrix-sdk/src/event_cache/linked_chunk/updates.rs index 68da551c640..5a143b94486 100644 --- a/crates/matrix-sdk/src/event_cache/linked_chunk/updates.rs +++ b/crates/matrix-sdk/src/event_cache/linked_chunk/updates.rs @@ -76,6 +76,12 @@ pub enum Update { items: Vec, }, + /// An item has been removed inside a chunk of kind Items. + RemoveItem { + /// The [`Position`] of the item. + at: Position, + }, + /// The last items of a chunk have been detached, i.e. the chunk has been /// truncated. DetachLastItems {