latest event: allow selecting an edit to the previous latest event as a viable latest event #3764
Conversation
A `LatestEvent` is produced by calling `is_suitable_for_latest_event`. In the caller of `Timeline::from_latest_event_content`, we unwrap the underlying event, pass it to that function, and then call `is_suitable_for_latest_event` again, which is weird. Instead, we can look at the event type, and pass it down the `from_suitable_...` functions directly.
…r as the new latest event
…_latest_event_content`
Not sure it should change behavior, but if we had bugs and the latest item appeared to be a day divider or a read marker, then that would return None even if there were event items before that.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3764 +/- ##
==========================================
- Coverage 84.08% 84.04% -0.05%
==========================================
Files 260 260
Lines 27031 27012 -19
==========================================
- Hits 22728 22701 -27
- Misses 4303 4311 +8 ☔ View full report in Codecov by Sentry. |
| prev_latest: Option<&EventId>, | ||
| ) -> PossibleLatestEvent<'a> { | ||
| use ruma::events::room::message::Relation; | ||
|
|
||
| match event { | ||
| // Suitable - we have an m.room.message that was not redacted or edited | ||
| AnySyncTimelineEvent::MessageLike(AnySyncMessageLikeEvent::RoomMessage(message)) => { | ||
| // Check if this is a replacement for another message. If it is, ignore it |
There was a problem hiding this comment.
This comment needs updating (right?)
| // We include messages that are not replacements; but we allow a replacement if | ||
| // that's the replacement to the previous known latest event. | ||
| let is_suitable = | ||
| original_message.content.relates_to.as_ref().map_or(true, |relates_to| { |
There was a problem hiding this comment.
If I read this right, it is equivalent to:
let relates_to = original_message.content.relates_to().as_ref();
let is_suitable = if let Some(relates_to) = relates_to {
// This is a relation: is it replacement of the previous latest event?
if let Relation::Replacement(c) = &relates_to {
// It is a replacement: suitable if it replaces the latest event
prev_latest == Some(&c.event_id)
else {
// Some other type of relation: not suitable
false
}
} else {
// Not a relation: definitely suitable
true
};
I find the original version really dense - what do you think about using a more expanded version like the above? I might even make it a separate function...
| } | ||
|
|
||
| #[test] | ||
| fn test_replacement_events_is_suitable_if_doesnt_replace_previous_latest_event() { |
There was a problem hiding this comment.
is_suitable -> is_not_suitable
| let event_content = event.content.clone(); | ||
|
|
||
| // We don't have access to any relations via the `AnySyncTimelineEvent` (I think | ||
| // - andyb) so we pretend there are none. This might be OK for | ||
| // the message preview use case. |
There was a problem hiding this comment.
Woohoo, glad to see this go!
| &timeline_items, | ||
| )) | ||
| } | ||
| let Some(event) = event.as_original() else { |
There was a problem hiding this comment.
Nice change! Note that you could use just if let here if you wanted. That would make the whole function one logical line of code, which some people like, but at the cost of one level of indentation.
|
Just tested this on iOS: it looks like it should work but in practice those edit just show up as empty |
|
Thanks for the review! This will have to wait for a bit, I need to add more testing because something is super weird there… |
|
I'm not satisfied with this technical solution, it was a workaround attempt until we do The Right Thing™ which is to handle the latest event in the event cache, and it's not working as intended, as demonstrated by the latest comment, so I'm closing this for now. |
It won't work in many cases (e.g. restart the whole SDK, receive many events from sync), but it should handle the simple case where we edit an event and get back to the room list.
Related element-hq/element-x-ios#2683