Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(timeline): fuse edit_by_id (resp redact_by_id) into their ID-less counterparts #4100

Merged
merged 5 commits into from
Oct 17, 2024
Merged

refactor(timeline): fuse edit_by_id (resp redact_by_id) into their ID-less counterparts #4100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8bae930
refactor(timeline): fuse `redact_by_id()` within `redact()`
bnjbvr Oct 15, 2024
6cdca96
refactor(timeline): fuse `edit_by_id()` within `edit()`
bnjbvr Oct 15, 2024
8a214d6
chore(ffi): revert to using a room method to edit if a remote event c…
bnjbvr Oct 16, 2024
5289dd6
refactor(timeline): return an invalid local echo state error if a loc…
bnjbvr Oct 17, 2024
5646e1c
refactor(timeline): don't return a bool in `Timeline::edit`
bnjbvr Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 25 additions & 9 deletions bindings/matrix-sdk-ffi/src/timeline/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use matrix_sdk::{
Error,
};
use matrix_sdk_ui::timeline::{
EventItemOrigin, LiveBackPaginationStatus, Profile, RepliedToEvent, TimelineDetails,
self, EventItemOrigin, LiveBackPaginationStatus, Profile, RepliedToEvent, TimelineDetails,
TimelineUniqueId as SdkTimelineUniqueId,
};
use mime::Mime;
Expand Down Expand Up @@ -494,11 +494,30 @@ impl Timeline {
&self,
event_or_transaction_id: EventOrTransactionId,
new_content: EditedContent,
) -> Result<bool, ClientError> {
self.inner
.edit_by_id(&event_or_transaction_id.try_into()?, new_content.try_into()?)
) -> Result<(), ClientError> {
bnjbvr marked this conversation as resolved.
Show resolved Hide resolved
match self
.inner
.edit(&event_or_transaction_id.clone().try_into()?, new_content.clone().try_into()?)
.await
.map_err(Into::into)
{
Ok(()) => Ok(()),
Err(timeline::Error::EventNotInTimeline(_)) => {
// If we couldn't edit, assume it was an (remote) event that wasn't in the
// timeline, and try to edit it via the room itself.
let event_id = match event_or_transaction_id {
EventOrTransactionId::EventId { event_id } => EventId::parse(event_id)?,
EventOrTransactionId::TransactionId { .. } => {
warn!("trying to apply an edit to a local echo that doesn't exist in this timeline, aborting");
return Ok(());
}
};
let room = self.inner.room();
let edit_event = room.make_edit_event(&event_id, new_content.try_into()?).await?;
room.send_queue().send(edit_event).await?;
Ok(())
}
Err(err) => Err(err)?,
}
}

pub async fn send_location(
Expand Down Expand Up @@ -610,10 +629,7 @@ impl Timeline {
event_or_transaction_id: EventOrTransactionId,
reason: Option<String>,
) -> Result<(), ClientError> {
self.inner
.redact_by_id(&(event_or_transaction_id.try_into()?), reason.as_deref())
.await
.map_err(Into::into)
Ok(self.inner.redact(&(event_or_transaction_id.try_into()?), reason.as_deref()).await?)
}

/// Load the reply details for the given event id.
Expand Down
27 changes: 22 additions & 5 deletions crates/matrix-sdk-ui/src/timeline/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,9 @@

use matrix_sdk::{
event_cache::{paginator::PaginatorError, EventCacheError},
room::edit::EditError,
send_queue::RoomSendQueueError,
HttpError,
};
use ruma::OwnedTransactionId;
use thiserror::Error;

use crate::timeline::{pinned_events_loader::PinnedEventsLoaderError, TimelineEventItemId};
Expand Down Expand Up @@ -77,18 +75,37 @@ pub enum Error {

/// An error happened while attempting to redact an event.
#[error(transparent)]
RedactError(RedactError),
RedactError(#[from] RedactError),
}

#[derive(Error, Debug)]
pub enum EditError {
/// The content types have changed.
#[error("the new content type ({new}) doesn't match that of the previous content ({original}")]
ContentMismatch { original: String, new: String },

/// The local echo we tried to edit has been lost.
#[error("Invalid state: the local echo we tried to abort has been lost.")]
InvalidLocalEchoState,

/// An error happened at a lower level.
#[error(transparent)]
RoomError(#[from] matrix_sdk::room::edit::EditError),
}

#[derive(Error, Debug)]
pub enum RedactError {
/// Local event to redact wasn't found for transaction id
#[error("Local event to redact wasn't found for transaction {0}")]
LocalEventNotFound(OwnedTransactionId),
#[error("Event to redact wasn't found for item id {0:?}")]
ItemNotFound(TimelineEventItemId),

/// An error happened while attempting to redact an event.
#[error(transparent)]
HttpError(#[from] HttpError),

/// The local echo we tried to abort has been lost.
#[error("Invalid state: the local echo we tried to abort has been lost.")]
InvalidLocalEchoState,
}

#[derive(Error, Debug)]
Expand Down
230 changes: 68 additions & 162 deletions crates/matrix-sdk-ui/src/timeline/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

use std::{path::PathBuf, pin::Pin, sync::Arc, task::Poll};

use event_item::{extract_room_msg_edit_content, EventTimelineItemKind, TimelineItemHandle};
use event_item::{extract_room_msg_edit_content, TimelineItemHandle};
use eyeball_im::VectorDiff;
use futures_core::Stream;
use imbl::Vector;
Expand Down Expand Up @@ -54,6 +54,7 @@ use ruma::{
};
use thiserror::Error;
use tracing::{error, instrument, trace, warn};
use util::rfind_event_by_item_id;

use crate::timeline::pinned_events_loader::PinnedEventsRoom;

Expand Down Expand Up @@ -238,17 +239,6 @@ impl Timeline {
self.controller.retry_event_decryption(self.room(), None).await;
}

/// Get the current timeline item for the given [`TimelineEventItemId`], if
/// any.
async fn event_by_timeline_id(&self, id: &TimelineEventItemId) -> Option<EventTimelineItem> {
match id {
TimelineEventItemId::EventId(event_id) => self.item_by_event_id(event_id).await,
TimelineEventItemId::TransactionId(transaction_id) => {
self.item_by_transaction_id(transaction_id).await
}
}
}

/// Get the current timeline item for the given event ID, if any.
///
/// Will return a remote event, *or* a local echo that has been sent but not
Expand Down Expand Up @@ -455,104 +445,70 @@ impl Timeline {
})
}

/// Returns a local or remote timeline item identified by this transaction
/// id.
async fn item_by_transaction_id(&self, txn_id: &TransactionId) -> Option<EventTimelineItem> {
let items = self.controller.items().await;

let (_, found) = rfind_event_item(&items, |item| match &item.kind {
EventTimelineItemKind::Local(local) => local.transaction_id == txn_id,
EventTimelineItemKind::Remote(remote) => {
remote.transaction_id.as_deref() == Some(txn_id)
}
})?;

Some(found.clone())
}

/// Edit an event given its [`TimelineEventItemId`] and some new content.
///
/// See [`Self::edit`] for more information.
pub async fn edit_by_id(
&self,
id: &TimelineEventItemId,
new_content: EditedContent,
) -> Result<bool, Error> {
let Some(event) = self.event_by_timeline_id(id).await else {
return Err(Error::EventNotInTimeline(id.clone()));
};

self.edit(&event, new_content).await
}

/// Edit an event.
///
/// Only supports events for which [`EventTimelineItem::is_editable()`]
/// returns `true`.
///
/// # Returns
///
/// - Returns `Ok(true)` if the edit was added to the send queue.
/// - Returns `Ok(false)` if the edit targets an item that has no local nor
/// matching remote item.
/// - Returns an error if there was an issue sending the redaction event, or
/// interacting with the sending queue.
#[instrument(skip(self, new_content))]
pub async fn edit(
&self,
item: &EventTimelineItem,
item_id: &TimelineEventItemId,
new_content: EditedContent,
) -> Result<bool, Error> {
let event_id = match item.identifier() {
TimelineEventItemId::TransactionId(txn_id) => {
// See if we have an up-to-date timeline item with that transaction id.
if let Some(item) = self.item_by_transaction_id(&txn_id).await {
match item.handle() {
TimelineItemHandle::Remote(event_id) => event_id.to_owned(),
TimelineItemHandle::Local(handle) => {
// Relations are filled by the editing code itself.
let new_content: AnyMessageLikeEventContent = match new_content {
EditedContent::RoomMessage(message) => {
if matches!(item.content, TimelineItemContent::Message(_)) {
AnyMessageLikeEventContent::RoomMessage(message.into())
} else {
warn!("New content (m.room.message) doesn't match previous event content.");
return Ok(false);
}
}
EditedContent::PollStart { new_content, .. } => {
if matches!(item.content, TimelineItemContent::Poll(_)) {
AnyMessageLikeEventContent::UnstablePollStart(
UnstablePollStartEventContent::New(
NewUnstablePollStartEventContent::new(new_content),
),
)
} else {
warn!("New content (poll start) doesn't match previous event content.");
return Ok(false);
}
}
};
return Ok(handle
.edit(new_content)
.await
.map_err(RoomSendQueueError::StorageError)?);
}
}
} else {
warn!("Couldn't find the local echo anymore, nor a matching remote echo");
return Ok(false);
}
}

TimelineEventItemId::EventId(event_id) => event_id,
) -> Result<(), Error> {
let items = self.items().await;
let Some((_pos, item)) = rfind_event_by_item_id(&items, item_id) else {
return Err(Error::EventNotInTimeline(item_id.clone()));
};

let content = self.room().make_edit_event(&event_id, new_content).await?;
match item.handle() {
TimelineItemHandle::Remote(event_id) => {
let content = self
.room()
.make_edit_event(event_id, new_content)
.await
.map_err(EditError::RoomError)?;
self.send(content).await?;
Ok(())
}

self.send(content).await?;
TimelineItemHandle::Local(handle) => {
// Relations are filled by the editing code itself.
let new_content: AnyMessageLikeEventContent = match new_content {
EditedContent::RoomMessage(message) => {
if matches!(item.content, TimelineItemContent::Message(_)) {
AnyMessageLikeEventContent::RoomMessage(message.into())
} else {
return Err(EditError::ContentMismatch {
original: item.content.debug_string().to_owned(),
new: "a message".to_owned(),
}
.into());
}
}
EditedContent::PollStart { new_content, .. } => {
if matches!(item.content, TimelineItemContent::Poll(_)) {
AnyMessageLikeEventContent::UnstablePollStart(
UnstablePollStartEventContent::New(
NewUnstablePollStartEventContent::new(new_content),
),
)
} else {
return Err(EditError::ContentMismatch {
original: item.content.debug_string().to_owned(),
new: "a poll".to_owned(),
}
.into());
}
}
};

Ok(true)
if !handle.edit(new_content).await.map_err(RoomSendQueueError::StorageError)? {
return Err(EditError::InvalidLocalEchoState.into());
}

Ok(())
}
}
}

/// Toggle a reaction on an event.
Expand Down Expand Up @@ -601,78 +557,28 @@ impl Timeline {

/// Redact an event given its [`TimelineEventItemId`] and an optional
/// reason.
///
/// See [`Self::redact`] for more info.
pub async fn redact_by_id(
pub async fn redact(
&self,
id: &TimelineEventItemId,
item_id: &TimelineEventItemId,
reason: Option<&str>,
) -> Result<(), Error> {
let event_id = match id {
TimelineEventItemId::TransactionId(transaction_id) => {
let Some(item) = self.item_by_transaction_id(transaction_id).await else {
return Err(Error::RedactError(RedactError::LocalEventNotFound(
transaction_id.to_owned(),
)));
};

match item.handle() {
TimelineItemHandle::Local(handle) => {
// If there is a local item that hasn't been sent yet, abort the upload
handle.abort().await.map_err(RoomSendQueueError::StorageError)?;
return Ok(());
}
TimelineItemHandle::Remote(event_id) => event_id.to_owned(),
}
}
TimelineEventItemId::EventId(event_id) => event_id.to_owned(),
let items = self.items().await;
let Some((_pos, event)) = rfind_event_by_item_id(&items, item_id) else {
return Err(RedactError::ItemNotFound(item_id.clone()).into());
};
self.room()
.redact(&event_id, reason, None)
.await
.map_err(|e| Error::RedactError(RedactError::HttpError(e)))?;

Ok(())
}

/// Redact an event.
///
/// # Returns
///
/// - Returns `Ok(true)` if the redact happened.
/// - Returns `Ok(false)` if the redact targets an item that has no local
/// nor matching remote item.
/// - Returns an error if there was an issue sending the redaction event, or
/// interacting with the sending queue.
pub async fn redact(
&self,
event: &EventTimelineItem,
reason: Option<&str>,
) -> Result<bool, Error> {
let event_id = match event.identifier() {
TimelineEventItemId::TransactionId(_) => {
// See if we have an up-to-date timeline item with that transaction id.
match event.handle() {
TimelineItemHandle::Remote(event_id) => event_id.to_owned(),
TimelineItemHandle::Local(handle) => {
return Ok(handle
.abort()
.await
.map_err(RoomSendQueueError::StorageError)?);
}
match event.handle() {
TimelineItemHandle::Remote(event_id) => {
self.room().redact(event_id, reason, None).await.map_err(RedactError::HttpError)?;
}
TimelineItemHandle::Local(handle) => {
if !handle.abort().await.map_err(RoomSendQueueError::StorageError)? {
return Err(RedactError::InvalidLocalEchoState.into());
}
}
}

TimelineEventItemId::EventId(event_id) => event_id,
};

self.room()
.redact(&event_id, reason, None)
.await
.map_err(RedactError::HttpError)
.map_err(Error::RedactError)?;

Ok(true)
Ok(())
}

/// Fetch unavailable details about the event with the given ID.
Expand Down
Loading
Loading