From 14c0c5eadb8c4ef91f33c95375175916946b25a0 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Tue, 2 Jul 2024 16:27:22 +0200 Subject: [PATCH] timeline: when an edit fails on a stale local echo, retry on the matching remote echo --- .../src/timeline/day_dividers.rs | 1 + .../src/timeline/event_handler.rs | 3 +- .../src/timeline/event_item/mod.rs | 1 + .../src/timeline/event_item/remote.rs | 7 ++- crates/matrix-sdk-ui/src/timeline/mod.rs | 53 +++++++++++++++---- 5 files changed, 54 insertions(+), 11 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/day_dividers.rs b/crates/matrix-sdk-ui/src/timeline/day_dividers.rs index 7c625b035ba..869be011871 100644 --- a/crates/matrix-sdk-ui/src/timeline/day_dividers.rs +++ b/crates/matrix-sdk-ui/src/timeline/day_dividers.rs @@ -622,6 +622,7 @@ mod tests { fn event_with_ts(timestamp: MilliSecondsSinceUnixEpoch) -> EventTimelineItem { let event_kind = EventTimelineItemKind::Remote(RemoteEventTimelineItem { event_id: owned_event_id!("$1"), + transaction_id: None, reactions: Default::default(), read_receipts: Default::default(), is_own: false, diff --git a/crates/matrix-sdk-ui/src/timeline/event_handler.rs b/crates/matrix-sdk-ui/src/timeline/event_handler.rs index 7749fc0d4c0..28a59a64580 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_handler.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_handler.rs @@ -891,7 +891,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { } .into(), - Flow::Remote { event_id, raw_event, position, .. } => { + Flow::Remote { event_id, raw_event, position, txn_id, .. } => { // Drop pending reactions if the message is redacted. if let TimelineItemContent::RedactedMessage = content { if !reactions.is_empty() { @@ -916,6 +916,7 @@ impl<'a, 'o> TimelineEventHandler<'a, 'o> { RemoteEventTimelineItem { event_id: event_id.clone(), + transaction_id: txn_id.clone(), reactions, read_receipts: self.ctx.read_receipts.clone(), is_own: self.ctx.is_own_event, diff --git a/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs b/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs index 8aadeeaa901..bce3d513984 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs @@ -140,6 +140,7 @@ impl EventTimelineItem { let event_kind = RemoteEventTimelineItem { event_id, + transaction_id: None, reactions, read_receipts, is_own, diff --git a/crates/matrix-sdk-ui/src/timeline/event_item/remote.rs b/crates/matrix-sdk-ui/src/timeline/event_item/remote.rs index 97cd074e89d..add84ba0cf3 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_item/remote.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_item/remote.rs @@ -19,7 +19,7 @@ use matrix_sdk::deserialized_responses::EncryptionInfo; use ruma::{ events::{receipt::Receipt, AnySyncTimelineEvent}, serde::Raw, - OwnedEventId, OwnedUserId, + OwnedEventId, OwnedTransactionId, OwnedUserId, }; use super::BundledReactions; @@ -30,6 +30,9 @@ pub(in crate::timeline) struct RemoteEventTimelineItem { /// The event ID. pub event_id: OwnedEventId, + /// If available, the transaction id we've used to send this event. + pub transaction_id: Option, + /// All bundled reactions about the event. pub reactions: BundledReactions, @@ -107,6 +110,7 @@ impl fmt::Debug for RemoteEventTimelineItem { // skip raw JSON, too noisy let Self { event_id, + transaction_id, reactions, read_receipts, is_own, @@ -119,6 +123,7 @@ impl fmt::Debug for RemoteEventTimelineItem { f.debug_struct("RemoteEventTimelineItem") .field("event_id", event_id) + .field("transaction_id", transaction_id) .field("reactions", reactions) .field("read_receipts", read_receipts) .field("is_own", is_own) diff --git a/crates/matrix-sdk-ui/src/timeline/mod.rs b/crates/matrix-sdk-ui/src/timeline/mod.rs index fd392887b1f..ac9f9801413 100644 --- a/crates/matrix-sdk-ui/src/timeline/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/mod.rs @@ -477,6 +477,22 @@ impl Timeline { }) } + /// Given a transaction id, try to find a remote echo that used this + /// transaction id upon sending. + async fn find_remote_by_transaction_id(&self, txn_id: &TransactionId) -> Option { + let items = self.inner.items().await; + + let (_, found) = rfind_event_item(&items, |item| { + if let Some(remote) = item.as_remote() { + remote.transaction_id.as_deref() == Some(txn_id) + } else { + false + } + })?; + + Some(found.event_id().expect("remote echoes have event id").to_owned()) + } + /// Edit an event. /// /// Only supports events for which [`EventTimelineItem::is_editable()`] @@ -503,19 +519,38 @@ impl Timeline { ) -> Result { let event_id = match edit_info.id { TimelineEventItemId::TransactionId(txn_id) => { - let Some(item) = self.item_by_transaction_id(&txn_id).await else { - warn!("Couldn't find the local echo anymore"); - return Ok(false); - }; + if let Some(item) = self.item_by_transaction_id(&txn_id).await { + let Some(handle) = item.as_local().and_then(|item| item.send_handle.clone()) + else { + warn!("No handle for a local echo; is this a test?"); + return Ok(false); + }; - if let Some(handle) = item.as_local().and_then(|item| item.send_handle.clone()) { // Assume no relations, since it's not been sent yet. - let new_content: RoomMessageEventContent = new_content.into(); - return Ok(handle.edit(new_content.into()).await?); + let new_content: RoomMessageEventContent = new_content.clone().into(); + + if handle.edit(new_content.into()).await? { + return Ok(true); + } } - warn!("No handle for a local echo; should only happen in testing situations"); - return Ok(false); + // We end up here in two cases: either there wasn't a local echo with this + // transaction id, or the send queue refused to edit the local echo (likely + // because it's sent). + // + // Try to find a matching local echo that now has an event id (it's been sent), + // or a remote echo with a matching transaction id, so as to + // send an actual edit. + if let Some(TimelineEventItemId::EventId(event_id)) = + self.item_by_transaction_id(&txn_id).await.map(|item| item.identifier()) + { + event_id + } else if let Some(event_id) = self.find_remote_by_transaction_id(&txn_id).await { + event_id + } else { + warn!("Couldn't find the local echo anymore, nor a matching remote echo"); + return Ok(false); + } } TimelineEventItemId::EventId(event_id) => event_id,