Skip to content

Commit 0122d11

Browse files
committed
refactor(ui): EventHandler uses regions to improve the code and avoid bugs.
This patch updates `EventHandler` to use the correct regions where appropriate, thus reducing the complexity of the code, and removing classes of bugs. In the case of `Flow::Remote { position: TimelineItemPosition::At { … }}`, we no longer need to skip the local timeline items, and to handle the presence of the `TimelineStart` timeline item. The code is less complex. In the case of `Flow::Remote { position: TimelineItemPosition::End { … }}`, that's exactly the same at the previous case. In the case of `recycle_local_or_create_item`, the `try_fold` approach is replaced entirely with a simple `iter_locals_region`, reducing the size of the comments explaining the code, reducing the complexity of the code, and reducing the surface of bugs.
1 parent 52b8ac0 commit 0122d11

File tree

1 file changed

+18
-57
lines changed

1 file changed

+18
-57
lines changed

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

Lines changed: 18 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15-
use std::{ops::ControlFlow, sync::Arc};
15+
use std::sync::Arc;
1616

1717
use as_variant::as_variant;
1818
use indexmap::IndexMap;
@@ -1171,23 +1171,15 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
11711171
// so we are inserting at the last non-local item position as a fallback.
11721172
let timeline_item_index = timeline_item_index.unwrap_or_else(|| {
11731173
self.items
1174-
.iter()
1175-
.enumerate()
1174+
.iter_remotes_region()
11761175
.rev()
11771176
.find_map(|(timeline_item_index, timeline_item)| {
1178-
(!timeline_item.as_event()?.is_local_echo())
1179-
.then_some(timeline_item_index + 1)
1177+
timeline_item.as_event().and_then(|_| Some(timeline_item_index + 1))
11801178
})
11811179
.unwrap_or_else(|| {
1182-
// We don't have any local echo, so we could insert at 0. However, in
1183-
// the case of an insertion caused by a pagination, we
1184-
// may have already pushed the start of the timeline item, so we need
1185-
// to check if the first item is that, and insert after it otherwise.
1186-
if self.items.get(0).is_some_and(|item| item.is_timeline_start()) {
1187-
1
1188-
} else {
1189-
0
1190-
}
1180+
// There is no remote timeline item, so we could insert at the start of
1181+
// the remotes region.
1182+
self.items.first_remotes_region_index()
11911183
})
11921184
});
11931185

@@ -1211,28 +1203,18 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
12111203
txn_id.as_deref(),
12121204
);
12131205

1214-
// Local events are always at the bottom. Let's find the latest remote event
1215-
// and insert after it, otherwise, if there is no remote event, insert at 0 or 1
1216-
// depending of the presence of the `TimelineStart` virtual item.
1206+
// Let's find the latest remote event and insert after it
12171207
let timeline_item_index = self
12181208
.items
1219-
.iter()
1220-
.enumerate()
1209+
.iter_remotes_region()
12211210
.rev()
12221211
.find_map(|(timeline_item_index, timeline_item)| {
1223-
(!timeline_item.as_event()?.is_local_echo())
1224-
.then_some(timeline_item_index + 1)
1212+
timeline_item.as_event().and_then(|_| Some(timeline_item_index + 1))
12251213
})
12261214
.unwrap_or_else(|| {
1227-
// We don't have any local echo, so we could insert at 0. However, in
1228-
// the case of an insertion caused by a pagination, we
1229-
// may have already pushed the start of the timeline item, so we need
1230-
// to check if the first item is that, and insert after it otherwise.
1231-
if self.items.get(0).is_some_and(|item| item.is_timeline_start()) {
1232-
1
1233-
} else {
1234-
0
1235-
}
1215+
// There is no remote timeline item, so we could insert at the start of
1216+
// the remotes region.
1217+
self.items.first_remotes_region_index()
12361218
});
12371219

12381220
let event_index = self
@@ -1306,46 +1288,25 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> {
13061288
) -> Arc<TimelineItem> {
13071289
// Detect a local timeline item that matches `event_id` or `transaction_id`.
13081290
if let Some((local_timeline_item_index, local_timeline_item)) = items
1309-
.iter()
1310-
// Get the index of each item.
1311-
.enumerate()
1291+
// Iterate the locals region.
1292+
.iter_locals_region()
13121293
// Iterate from the end to the start.
13131294
.rev()
1314-
// Use a `Iterator::try_fold` to produce a single value, and to stop the iterator
1315-
// when a non local event timeline item is met. We want to stop iterating when:
1316-
//
1317-
// - a duplicate local event timeline item has been found,
1318-
// - a non local event timeline item is met,
1319-
// - a non event timeline is met.
1320-
//
1321-
// Indeed, it is a waste of time to iterate over all items in `items`. Local event
1322-
// timeline items are necessarily at the end of `items`: as soon as they have been
1323-
// iterated, we can stop the entire iteration.
1324-
.try_fold((), |(), (nth, timeline_item)| {
1325-
let Some(event_timeline_item) = timeline_item.as_event() else {
1326-
// Not an event timeline item? Stop iterating here.
1327-
return ControlFlow::Break(None);
1328-
};
1329-
1330-
// Not a local event timeline item? Stop iterating here.
1331-
if !event_timeline_item.is_local_echo() {
1332-
return ControlFlow::Break(None);
1333-
}
1295+
.find_map(|(nth, timeline_item)| {
1296+
let event_timeline_item = timeline_item.as_event()?;
13341297

13351298
if Some(event_id) == event_timeline_item.event_id()
13361299
|| (transaction_id.is_some()
13371300
&& transaction_id == event_timeline_item.transaction_id())
13381301
{
13391302
// A duplicate local event timeline item has been found!
1340-
ControlFlow::Break(Some((nth, event_timeline_item)))
1303+
Some((nth, event_timeline_item))
13411304
} else {
13421305
// This local event timeline is not the one we are looking for. Continue our
13431306
// search.
1344-
ControlFlow::Continue(())
1307+
None
13451308
}
13461309
})
1347-
.break_value()
1348-
.flatten()
13491310
{
13501311
trace!(
13511312
?event_id,

0 commit comments

Comments
 (0)