day dividers: record the insert position when applying an operation b…

…ased on the previous item

We could end up, like in the regression test, with a sequence of
operations like that:

- remove day divider @ i+1 (because it's redundant with one @ i)
- remove day divider @ i (because it's useless, since the event before
the day divider and after the day divider use the same date).

In that case, it would break the non-decreasing invariant: we'd apply an
operation on the array @ i+1, then @ i, which troubles the offset
computation.

Instead, when doing an operation based on the "prev_item" (now with a
small helper struct, to facilitate understanding of each field), we also
record the insertion order for the operation itself: it's always "at the
end of the operation order, at the time we're looking at it", so
equivalent to a "push_back" if there's no operation in between; but that
ensures that we'll do the operation in a non-decreasing order. For
instance in the above test case, the Remove(i) is now inserted before
the Remove(i+1), instead of after.

crates/matrix-sdk-ui/src/timeline/day_dividers.rs

@@ -58,6 +58,18 @@ impl Default for DayDividerAdjuster {
  }
 }
 
+/// A descriptor for a previous item.
+struct PrevItemDesc<'a> {
+ /// The index of the item in the `self.items` array.
+ item_index: usize,
+
+ /// The previous timeline item.
+ item: &'a Arc<TimelineItem>,
+
+ // The insert position of the operation in the `ops` array.
+ insert_op_at: usize,
+}
+
 impl DayDividerAdjuster {
  /// Marks this [`DayDividerAdjuster`] as used, which means it'll require a
  /// call to [`DayDividerAdjuster::run`] before getting dropped.
@@ -79,32 +91,42 @@ impl DayDividerAdjuster {
  // recorded offsets will change as we're iterating over the array. The
  // only way this is possible is because we're recording operations
  // happening in non-decreasing order of the indices, i.e. we can't do an
- // operation onindex I and then on any index J<I later.
+ // operation on index I and then on any index J<I later.
  //
- // Note we can't just iterate in reverse order, because we may have a
+ // Note we can't "just" iterate in reverse order, because we may have a
  // `Remove(i)` followed by a `Replace((i+1) -1)`, which wouldn't do what
  // we want, if running in reverse order.
+ //
+ // Also note that we can remove a few items at position J, then later decide to
+ // replace/remove an item (in `handle_event`) at position I, with I<J. That
+ // would break the above invariant (that operations happen in
+ // non-decreasing order of the indices), so we must record the insert
+ // position for an operation related to the previous item.
 
- let mut prev_item_pair: Option<(usize, &Arc<TimelineItem>)> = None;
+ let mut prev_item: Option<PrevItemDesc<'_>> = None;
  let mut latest_event_ts = None;
 
  for (i, item) in items.iter().enumerate() {
  match item.kind() {
  TimelineItemKind::Virtual(VirtualTimelineItem::DayDivider(ts)) => {
  // Record what the last alive item pair is only if we haven't removed the day
  // divider.
- if !self.handle_day_divider(i, *ts, prev_item_pair.as_ref().map(|pair| pair.1))
- {
- prev_item_pair = Some((i, item));
+ if !self.handle_day_divider(i, *ts, prev_item.as_ref().map(|desc| desc.item)) {
+ prev_item = Some(PrevItemDesc {
+ item_index: i,
+ item,
+ insert_op_at: self.ops.len(),
+ });
  }
  }
 
  TimelineItemKind::Event(event) => {
  let ts = event.timestamp();
 
- self.handle_event(i, ts, prev_item_pair, latest_event_ts);
+ self.handle_event(i, ts, prev_item, latest_event_ts);
 
- prev_item_pair = Some((i, item));
+ prev_item =
+ Some(PrevItemDesc { item_index: i, item, insert_op_at: self.ops.len() });
  latest_event_ts = Some(ts);
  }
 
@@ -195,18 +217,18 @@ impl DayDividerAdjuster {
  &mut self,
  i: usize,
  ts: MilliSecondsSinceUnixEpoch,
- prev_item_pair: Option<(usize, &Arc<TimelineItem>)>,
+ prev_item_desc: Option<PrevItemDesc<'_>>,
  latest_event_ts: Option<MilliSecondsSinceUnixEpoch>,
  ) {
- let Some((prev_index, prev_item)) = prev_item_pair else {
+ let Some(PrevItemDesc { item_index, insert_op_at, item }) = prev_item_desc else {
  // The event was the first item, so there wasn't any day divider before it:
  // insert one.
  trace!("inserting the first day divider @ {}", i);
  self.ops.push(DayDividerOperation::Insert(i, ts));
  return;
  };
 
- match prev_item.kind() {
+ match item.kind() {
  TimelineItemKind::Event(prev_event) => {
  // The event is preceded by another event. If they're not the same date,
  // insert a day divider.
@@ -228,16 +250,16 @@ impl DayDividerAdjuster {
  if let Some(last_event_ts) = latest_event_ts {
  if timestamp_to_date(last_event_ts) == event_date {
  // There's a previous event with the same date: remove the divider.
- trace!("removed day divider @ {prev_index} between two events that have the same date");
- self.ops.push(DayDividerOperation::Remove(prev_index));
+ trace!("removed day divider @ {item_index} between two events that have the same date");
+ self.ops.insert(insert_op_at, DayDividerOperation::Remove(item_index));
  return;
  }
  }
 
  // There's no previous event or there's one with a different date: replace
  // the current divider.
- trace!("replacing day divider @ {prev_index} with new timestamp from event");
- self.ops.push(DayDividerOperation::Replace(prev_index, ts));
+ trace!("replacing day divider @ {item_index} with new timestamp from event");
+ self.ops.insert(insert_op_at, DayDividerOperation::Replace(item_index, ts));
  }
  }
 
@@ -686,4 +708,34 @@ mod tests {
  assert!(iter.next().unwrap().is_remote_event());
  assert!(iter.next().is_none());
  }
+
+ #[test]
+ fn test_remove_all_day_dividers() {
+ let mut items = ObservableVector::new();
+ let mut txn = items.transaction();
+
+ let mut meta = TimelineInnerMetadata::new(ruma::RoomVersionId::V11, None, None);
+
+ let timestamp = MilliSecondsSinceUnixEpoch(uint!(42));
+ let timestamp_next_day =
+ MilliSecondsSinceUnixEpoch((42 + 3600 * 24 * 1000).try_into().unwrap());
+ assert_ne!(timestamp_to_date(timestamp), timestamp_to_date(timestamp_next_day));
+
+ txn.push_back(meta.new_timeline_item(event_with_ts(timestamp_next_day)));
+ txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp)));
+ txn.push_back(meta.new_timeline_item(VirtualTimelineItem::DayDivider(timestamp)));
+ txn.push_back(meta.new_timeline_item(event_with_ts(timestamp_next_day)));
+
+ let mut adjuster = DayDividerAdjuster::default();
+ adjuster.run(&mut txn, &mut meta);
+
+ txn.commit();
+
+ let mut iter = items.iter();
+
+ assert!(iter.next().unwrap().is_day_divider());
+ assert!(iter.next().unwrap().is_remote_event());
+ assert!(iter.next().unwrap().is_remote_event());
+ assert!(iter.next().is_none());
+ }
 }