Skip to content

Commit 7d8e7af

Browse files
poljarpoljar
committed
Revert "chore(ui): Unify the logic for timeline item insertions"
This reverts commit d2ecd74.
1 parent 136522c commit 7d8e7af

File tree

1 file changed

+79
-102
lines changed

1 file changed

+79
-102
lines changed

‎crates/matrix-sdk-ui/src/timeline/event_handler.rs‎

Lines changed: 79 additions & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -1085,71 +1085,88 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
10851085
self.items.push_back(item);
10861086
}
10871087

1088-
Flow::Remote {
1089-
position: position @ TimelineItemPosition::Start { .. },
1090-
txn_id,
1091-
event_id,
1092-
..
1088+
Flow::Remote { position: TimelineItemPosition::Start { .. }, event_id, .. } => {
1089+
if self
1090+
.items
1091+
.iter()
1092+
.filter_map(|ev| ev.as_event()?.event_id())
1093+
.any(|id| id == event_id)
1094+
{
1095+
trace!("Skipping back-paginated event that has already been seen");
1096+
return;
1097+
}
1098+
1099+
trace!("Adding new remote timeline item at the start");
1100+
1101+
let item = self.meta.new_timeline_item(item);
1102+
self.items.push_front(item);
10931103
}
1094-
| Flow::Remote {
1095-
position: position @ TimelineItemPosition::End { .. },
1096-
txn_id,
1097-
event_id,
1098-
..
1104+
1105+
Flow::Remote {
1106+
position: TimelineItemPosition::End { .. }, txn_id, event_id, ..
10991107
} => {
1100-
// This block tries to find duplicated events.
1101-
1102-
let removed_event_item_id = {
1103-
// Look if we already have a corresponding item somewhere, based on the
1104-
// transaction id (if this is a local echo) or the event id (if this is a
1105-
// duplicate remote event).
1106-
let result = rfind_event_item(self.items, |it| {
1107-
txn_id.is_some() && it.transaction_id() == txn_id.as_deref()
1108-
|| it.event_id() == Some(event_id)
1109-
});
1110-
1111-
if let Some((idx, old_item)) = result {
1112-
if old_item.as_remote().is_some() {
1113-
// The item was previously received from the server. This should be very
1114-
// rare normally, but with the sliding- sync proxy, it is actually very
1115-
// common.
1116-
// NOTE: This is a SS proxy workaround.
1117-
trace!(?item, old_item = ?*old_item, "Received duplicate event");
1118-
1119-
if old_item.content.is_redacted() && !item.content.is_redacted() {
1120-
warn!("Got original form of an event that was previously redacted");
1121-
item.content = item.content.redact(&self.meta.room_version);
1122-
item.reactions.clear();
1123-
}
1108+
// Look if we already have a corresponding item somewhere, based on the
1109+
// transaction id (if a local echo) or the event id (if a
1110+
// duplicate remote event).
1111+
let result = rfind_event_item(self.items, |it| {
1112+
txn_id.is_some() && it.transaction_id() == txn_id.as_deref()
1113+
|| it.event_id() == Some(event_id)
1114+
});
1115+
1116+
let mut removed_event_item_id = None;
1117+
1118+
if let Some((idx, old_item)) = result {
1119+
if old_item.as_remote().is_some() {
1120+
// Item was previously received from the server. This should be very rare
1121+
// normally, but with the sliding- sync proxy, it is actually very
1122+
// common.
1123+
// NOTE: SS proxy workaround.
1124+
trace!(?item, old_item = ?*old_item, "Received duplicate event");
1125+
1126+
if old_item.content.is_redacted() && !item.content.is_redacted() {
1127+
warn!("Got original form of an event that was previously redacted");
1128+
item.content = item.content.redact(&self.meta.room_version);
1129+
item.reactions.clear();
11241130
}
1131+
}
11251132

1126-
// TODO: Check whether anything is different about the
1127-
// old and new item?
1133+
// TODO: Check whether anything is different about the
1134+
// old and new item?
11281135

1129-
transfer_details(&mut item, &old_item);
1136+
transfer_details(&mut item, &old_item);
11301137

1131-
let old_item_id = old_item.internal_id;
1138+
let old_item_id = old_item.internal_id;
11321139

1133-
if idx == self.items.len() - 1 {
1134-
// If the old item is the last one and no day divider
1135-
// changes need to happen, replace and return early.
1136-
trace!(idx, "Replacing existing event");
1137-
self.items.set(idx, TimelineItem::new(item, old_item_id.to_owned()));
1138-
return;
1139-
}
1140+
if idx == self.items.len() - 1 {
1141+
// If the old item is the last one and no day divider
1142+
// changes need to happen, replace and return early.
1143+
trace!(idx, "Replacing existing event");
1144+
self.items.set(idx, TimelineItem::new(item, old_item_id.to_owned()));
1145+
return;
1146+
}
11401147

1141-
// In more complex cases, remove the item before re-adding the item.
1142-
trace!("Removing local echo or duplicate timeline item");
1148+
// In more complex cases, remove the item before re-adding the item.
1149+
trace!("Removing local echo or duplicate timeline item");
1150+
removed_event_item_id = Some(self.items.remove(idx).internal_id.clone());
11431151

1144-
// no return here, the below logic for adding a new event
1145-
// will run to re-add the removed item
1152+
// no return here, below code for adding a new event
1153+
// will run to re-add the removed item
1154+
}
11461155

1147-
Some(self.items.remove(idx).internal_id.clone())
1148-
} else {
1149-
None
1150-
}
1151-
};
1156+
// Local echoes that are pending should stick to the bottom,
1157+
// find the latest event that isn't that.
1158+
let latest_event_idx = self
1159+
.items
1160+
.iter()
1161+
.enumerate()
1162+
.rev()
1163+
.find_map(|(idx, item)| (!item.as_event()?.is_local_echo()).then_some(idx));
1164+
1165+
// Insert the next item after the latest event item that's not a
1166+
// pending local echo, or at the start if there is no such item.
1167+
let insert_idx = latest_event_idx.map_or(0, |idx| idx + 1);
11521168

1169+
trace!("Adding new remote timeline item after all non-pending events");
11531170
let new_item = match removed_event_item_id {
11541171
// If a previous version of the same item (usually a local
11551172
// echo) was removed and we now need to add it again, reuse
@@ -1158,54 +1175,14 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
11581175
None => self.meta.new_timeline_item(item),
11591176
};
11601177

1161-
trace!("Adding new remote timeline item after all non-local events");
1162-
1163-
// We are about to insert the `new_item`, great! Though, we try to keep
1164-
// precise insertion semantics here, in this exact order:
1165-
//
1166-
// * _push back_ when the new item is inserted after all items,
1167-
// * _push front_ when the new item is inserted at index 0,
1168-
// * _insert_ otherwise.
1169-
//
1170-
// It means that the first inserted item will generate a _push back_ for
1171-
// example.
1172-
match position {
1173-
TimelineItemPosition::Start { .. } => {
1174-
trace!("Adding new remote timeline item at the front");
1175-
self.items.push_front(new_item);
1176-
}
1177-
1178-
TimelineItemPosition::End { .. } => {
1179-
// Local echoes that are pending should stick to the bottom,
1180-
// find the latest event that isn't that.
1181-
let latest_event_idx =
1182-
self.items.iter().enumerate().rev().find_map(|(idx, item)| {
1183-
(!item.as_event()?.is_local_echo()).then_some(idx)
1184-
});
1185-
1186-
// Insert the next item after the latest event item that's not a pending
1187-
// local echo, or at the start if there is no such item.
1188-
let insert_idx = latest_event_idx.map_or(0, |idx| idx + 1);
1189-
1190-
// Let's prioritize push backs because it's the hot path. Events are more
1191-
// generally added at the back because they come from the sync most of the
1192-
// time.
1193-
if insert_idx == self.items.len() {
1194-
trace!("Adding new remote timeline item at the back");
1195-
self.items.push_back(new_item);
1196-
} else if insert_idx == 0 {
1197-
trace!("Adding new remote timeline item at the front");
1198-
self.items.push_front(new_item);
1199-
} else {
1200-
trace!(insert_idx, "Adding new remote timeline item at specific index");
1201-
self.items.insert(insert_idx, new_item);
1202-
}
1203-
}
1204-
1205-
p => unreachable!(
1206-
"An unexpected `TimelineItemPosition` has been received: {p:?}"
1207-
),
1208-
};
1178+
// Keep push semantics, if we're inserting at the front or the back.
1179+
if insert_idx == self.items.len() {
1180+
self.items.push_back(new_item);
1181+
} else if insert_idx == 0 {
1182+
self.items.push_front(new_item);
1183+
} else {
1184+
self.items.insert(insert_idx, new_item);
1185+
}
12091186
}
12101187

12111188
Flow::Remote {

0 commit comments

Comments
 (0)