diff --git a/src/session/view/account_settings/general_page/mod.rs b/src/session/view/account_settings/general_page/mod.rs index 811cc8b7..0f6d0c3e 100644 --- a/src/session/view/account_settings/general_page/mod.rs +++ b/src/session/view/account_settings/general_page/mod.rs @@ -21,7 +21,7 @@ use crate::{ prelude::*, session::model::Session, spawn, spawn_tokio, toast, - utils::{media::load_file, template_callbacks::TemplateCallbacks, OngoingAsyncAction}, + utils::{media::FileInfo, template_callbacks::TemplateCallbacks, OngoingAsyncAction}, }; mod imp { @@ -224,8 +224,18 @@ impl GeneralPage { let avatar = &imp.avatar; avatar.edit_in_progress(); - let (data, info) = match load_file(&file).await { - Ok(res) => res, + let info = match FileInfo::try_from_file(&file).await { + Ok(info) => info, + Err(error) => { + error!("Could not load user avatar file info: {error}"); + toast!(self, gettext("Could not load file")); + avatar.reset(); + return; + } + }; + + let data = match file.load_contents_future().await { + Ok((data, _)) => data, Err(error) => { error!("Could not load user avatar file: {error}"); toast!(self, gettext("Could not load file")); @@ -236,8 +246,12 @@ impl GeneralPage { let client = session.client(); let client_clone = client.clone(); - let handle = - spawn_tokio!(async move { client_clone.media().upload(&info.mime, data, None).await }); + let handle = spawn_tokio!(async move { + client_clone + .media() + .upload(&info.mime, data.into(), None) + .await + }); let uri = match handle.await.unwrap() { Ok(res) => res.content_uri, diff --git a/src/session/view/content/room_details/edit_details_subpage.rs b/src/session/view/content/room_details/edit_details_subpage.rs index f40d1cbb..597a8cf5 100644 --- a/src/session/view/content/room_details/edit_details_subpage.rs +++ b/src/session/view/content/room_details/edit_details_subpage.rs @@ -17,7 +17,7 @@ use crate::{ session::model::Room, spawn_tokio, toast, utils::{ - media::{image::ImageInfoLoader, load_file}, + media::{image::ImageInfoLoader, FileInfo}, template_callbacks::TemplateCallbacks, BoundObjectWeakRef, OngoingAsyncAction, }, @@ -173,8 +173,18 @@ mod imp { let avatar = &self.avatar; avatar.edit_in_progress(); - let (data, info) = match load_file(&file).await { - Ok(res) => res, + let info = match FileInfo::try_from_file(&file).await { + Ok(info) => info, + Err(error) => { + error!("Could not load room avatar file info: {error}"); + toast!(obj, gettext("Could not load file")); + avatar.reset(); + return; + } + }; + + let data = match file.load_contents_future().await { + Ok((data, _)) => data, Err(error) => { error!("Could not load room avatar file: {error}"); toast!(obj, gettext("Could not load file")); @@ -196,7 +206,9 @@ mod imp { }; let client = session.client(); let handle = - spawn_tokio!(async move { client.media().upload(&info.mime, data, None).await }); + spawn_tokio!( + async move { client.media().upload(&info.mime, data.into(), None).await } + ); let uri = match handle.await.unwrap() { Ok(res) => res.content_uri, diff --git a/src/session/view/content/room_history/message_row/audio.rs b/src/session/view/content/room_history/message_row/audio.rs index be599b4f..9ab16bf5 100644 --- a/src/session/view/content/room_history/message_row/audio.rs +++ b/src/session/view/content/room_history/message_row/audio.rs @@ -6,7 +6,7 @@ use gtk::{ }; use tracing::warn; -use super::ContentFormat; +use super::{content::MessageCacheKey, ContentFormat}; use crate::{ components::AudioPlayer, gettext_f, @@ -37,6 +37,12 @@ mod imp { /// The filename of the audio file. #[property(get)] filename: RefCell>, + /// The cache key for the current audio message. + /// + /// The audio is only reloaded if the cache key changes. This is to + /// avoid reloading the audio when the local echo is updated to a remote + /// echo. + cache_key: RefCell, /// The media file. file: RefCell>, /// The state of the audio file. @@ -137,13 +143,29 @@ mod imp { self.state_error.set_tooltip_text(Some(error)); } - /// Display the given audio message. + /// Set the cache key with the given value. + /// + /// Returns `true` if the audio should be reloaded. + fn set_cache_key(&self, key: MessageCacheKey) -> bool { + let should_reload = self.cache_key.borrow().should_reload(&key); + self.cache_key.replace(key); + + should_reload + } + + /// Display the given `audio` message. pub(super) fn audio( &self, message: MediaMessage, session: &Session, format: ContentFormat, + cache_key: MessageCacheKey, ) { + if !self.set_cache_key(cache_key) { + // We do not need to reload the audio. + return; + } + self.file.take(); self.set_filename(Some(message.filename())); @@ -212,8 +234,14 @@ impl MessageAudio { } /// Display the given `audio` message. - pub(crate) fn audio(&self, message: MediaMessage, session: &Session, format: ContentFormat) { - self.imp().audio(message, session, format); + pub(crate) fn audio( + &self, + message: MediaMessage, + session: &Session, + format: ContentFormat, + cache_key: MessageCacheKey, + ) { + self.imp().audio(message, session, format, cache_key); } } diff --git a/src/session/view/content/room_history/message_row/content.rs b/src/session/view/content/room_history/message_row/content.rs index fb77db87..a3a794b3 100644 --- a/src/session/view/content/room_history/message_row/content.rs +++ b/src/session/view/content/room_history/message_row/content.rs @@ -4,7 +4,7 @@ use gtk::{gdk, glib, glib::clone}; use matrix_sdk_ui::timeline::{ Message, RepliedToInfo, ReplyContent, TimelineDetails, TimelineItemContent, }; -use ruma::events::room::message::MessageType; +use ruma::{events::room::message::MessageType, OwnedEventId, OwnedTransactionId}; use tracing::{error, warn}; use super::{ @@ -158,6 +158,8 @@ impl MessageContent { ContentFormat::Compact, &replied_to_sender, replied_to_detect_at_room, + None, + event.reply_to_id(), ); build_content( reply.content(), @@ -165,6 +167,8 @@ impl MessageContent { ContentFormat::Natural, &event.sender(), detect_at_room, + event.transaction_id(), + event.event_id(), ); self.set_child(Some(&reply)); @@ -181,6 +185,8 @@ impl MessageContent { format, &event.sender(), detect_at_room, + event.transaction_id(), + event.event_id(), ); } @@ -192,7 +198,15 @@ impl MessageContent { let detect_at_room = message.can_contain_at_room() && sender.can_notify_room(); - build_message_content(self, message, self.format(), sender, detect_at_room); + build_message_content( + self, + message, + self.format(), + sender, + detect_at_room, + None, + Some(info.event_id().to_owned()), + ); } /// Get the texture displayed by this widget, if any. @@ -208,12 +222,22 @@ fn build_content( format: ContentFormat, sender: &Member, detect_at_room: bool, + transaction_id: Option, + event_id: Option, ) { let room = sender.room(); match content { TimelineItemContent::Message(message) => { - build_message_content(parent, &message, format, sender, detect_at_room); + build_message_content( + parent, + &message, + format, + sender, + detect_at_room, + transaction_id, + event_id, + ); } TimelineItemContent::Sticker(sticker) => { build_media_message_content( @@ -222,6 +246,11 @@ fn build_content( format, &room, detect_at_room, + MessageCacheKey { + transaction_id, + event_id, + is_edited: false, + }, ); } TimelineItemContent::UnableToDecrypt(_) => { @@ -265,11 +294,24 @@ fn build_message_content( format: ContentFormat, sender: &Member, detect_at_room: bool, + transaction_id: Option, + event_id: Option, ) { let room = sender.room(); if let Some(media_message) = MediaMessage::from_message(message.msgtype()) { - build_media_message_content(parent, media_message, format, &room, detect_at_room); + build_media_message_content( + parent, + media_message, + format, + &room, + detect_at_room, + MessageCacheKey { + transaction_id, + event_id, + is_edited: message.is_edited(), + }, + ); return; } @@ -364,6 +406,7 @@ fn build_media_message_content( format: ContentFormat, room: &Room, detect_at_room: bool, + cache_key: MessageCacheKey, ) { let Some(session) = room.session() else { return; @@ -387,11 +430,17 @@ fn build_media_message_content( detect_at_room, ); - let new_widget = - build_media_content(caption_widget.child(), media_message, format, &session); + let new_widget = build_media_content( + caption_widget.child(), + media_message, + format, + &session, + cache_key, + ); caption_widget.set_child(Some(new_widget)); } else { - let new_widget = build_media_content(parent.child(), media_message, format, &session); + let new_widget = + build_media_content(parent.child(), media_message, format, &session, cache_key); parent.set_child(Some(&new_widget)); } } @@ -404,6 +453,7 @@ fn build_media_content( media_message: MediaMessage, format: ContentFormat, session: &Session, + cache_key: MessageCacheKey, ) -> gtk::Widget { match media_message { MediaMessage::Audio(audio) => { @@ -411,7 +461,7 @@ fn build_media_content( .and_downcast::() .unwrap_or_default(); - widget.audio(audio.into(), session, format); + widget.audio(audio.into(), session, format, cache_key); widget.upcast() } @@ -429,7 +479,7 @@ fn build_media_content( .and_downcast::() .unwrap_or_default(); - widget.set_media_message(image.into(), session, format); + widget.set_media_message(image.into(), session, format, cache_key); widget.upcast() } @@ -438,7 +488,7 @@ fn build_media_content( .and_downcast::() .unwrap_or_default(); - widget.set_media_message(video.into(), session, format); + widget.set_media_message(video.into(), session, format, cache_key); widget.upcast() } @@ -447,9 +497,52 @@ fn build_media_content( .and_downcast::() .unwrap_or_default(); - widget.set_media_message(sticker.into(), session, format); + widget.set_media_message(sticker.into(), session, format, cache_key); widget.upcast() } } } + +/// The data used as a cache key for messages. +/// +/// This is used when there is no reliable way to detect if the content of a +/// message changed. For example, the URI of a media file might change between a +/// local echo and a remote echo, but we do not need to reload the media in this +/// case, and we have no other way to know that both URIs point to the same +/// file. +#[derive(Debug, Clone, Default)] +pub(crate) struct MessageCacheKey { + /// The transaction ID of the event. + /// + /// Local echo should keep its transaction ID after the message is sent, so + /// we do not need to reload the message if it did not change. + transaction_id: Option, + /// The global ID of the event. + /// + /// Local echo that was sent and remote echo should have the same event ID, + /// so we do not need to reload the message if it did not change. + event_id: Option, + /// Whether the message is edited. + /// + /// The message must be reloaded when it was edited. + is_edited: bool, +} + +impl MessageCacheKey { + /// Whether the given new `MessageCacheKey` should trigger a reload of the + /// mmessage compared to this one. + pub(super) fn should_reload(&self, new: &MessageCacheKey) -> bool { + if new.is_edited { + return true; + } + + let transaction_id_invalidated = self.transaction_id.is_none() + || new.transaction_id.is_none() + || self.transaction_id != new.transaction_id; + let event_id_invalidated = + self.event_id.is_none() || new.event_id.is_none() || self.event_id != new.event_id; + + transaction_id_invalidated && event_id_invalidated + } +} diff --git a/src/session/view/content/room_history/message_row/visual_media.rs b/src/session/view/content/room_history/message_row/visual_media.rs index f7c4ee0f..16e5d887 100644 --- a/src/session/view/content/room_history/message_row/visual_media.rs +++ b/src/session/view/content/room_history/message_row/visual_media.rs @@ -9,7 +9,7 @@ use matrix_sdk::Client; use ruma::api::client::media::get_content_thumbnail::v3::Method; use tracing::{error, warn}; -use super::ContentFormat; +use super::{content::MessageCacheKey, ContentFormat}; use crate::{ components::{AnimatedImagePaintable, VideoPlayer}, gettext_f, @@ -74,6 +74,11 @@ mod imp { #[property(get)] activatable: Cell, gesture_click: glib::WeakRef, + /// The cache key for the current media message. + /// + /// We only try to reload the media if the key changes. This is to avoid + /// reloading the media when a local echo changes to a remote echo. + cache_key: RefCell, /// The current video file, if any. file: RefCell>, paintable_animation_ref: RefCell>, @@ -310,13 +315,30 @@ mod imp { } } + /// Set the cache key with the given value. + /// + /// Returns `true` if the media should be reloaded. + fn set_cache_key(&self, key: MessageCacheKey) -> bool { + let should_reload = self.cache_key.borrow().should_reload(&key); + + self.cache_key.replace(key); + + should_reload + } + /// Build the content for the given media message. pub(super) fn build( &self, media_message: VisualMediaMessage, session: &Session, format: ContentFormat, + cache_key: MessageCacheKey, ) { + if !self.set_cache_key(cache_key) { + // We do not need to reload the media. + return; + } + self.file.take(); self.dimensions.set(media_message.dimensions()); @@ -525,8 +547,9 @@ impl MessageVisualMedia { media_message: VisualMediaMessage, session: &Session, format: ContentFormat, + cache_key: MessageCacheKey, ) { - self.imp().build(media_message, session, format); + self.imp().build(media_message, session, format, cache_key); } /// Get the texture displayed by this widget, if any. diff --git a/src/session/view/content/room_history/message_toolbar/mod.rs b/src/session/view/content/room_history/message_toolbar/mod.rs index 5ef3ba85..be6a501c 100644 --- a/src/session/view/content/room_history/message_toolbar/mod.rs +++ b/src/session/view/content/room_history/message_toolbar/mod.rs @@ -12,7 +12,7 @@ use matrix_sdk::{ attachment::{AttachmentConfig, AttachmentInfo, BaseFileInfo, Thumbnail}, room::edit::EditedContent, }; -use matrix_sdk_ui::timeline::{RepliedToInfo, TimelineItemContent}; +use matrix_sdk_ui::timeline::{AttachmentSource, RepliedToInfo, TimelineItemContent}; use ruma::{ events::{ room::message::{ @@ -43,8 +43,8 @@ use crate::{ spawn, spawn_tokio, toast, utils::{ media::{ - filename_for_mime, image::ImageInfoLoader, load_audio_info, load_file, - video::load_video_info, + filename_for_mime, image::ImageInfoLoader, load_audio_info, video::load_video_info, + FileInfo, }, template_callbacks::TemplateCallbacks, Location, LocationError, TokioDrop, @@ -665,9 +665,8 @@ mod imp { /// Send the attachment with the given data. async fn send_attachment( &self, - bytes: Vec, + source: AttachmentSource, mime: mime::Mime, - body: String, info: AttachmentInfo, thumbnail: Option, ) { @@ -677,11 +676,12 @@ mod imp { let config = AttachmentConfig::new().thumbnail(thumbnail).info(info); - let matrix_room = room.matrix_room().clone(); + let matrix_timeline = room.timeline().matrix_timeline(); let handle = spawn_tokio!(async move { - matrix_room - .send_attachment(&body, &mime, bytes, config) + matrix_timeline + .send_attachment(source, mime, config) + .use_send_queue() .await }); @@ -722,7 +722,11 @@ mod imp { base_info.size = filesize.map(Into::into); let info = AttachmentInfo::Image(base_info); - self.send_attachment(bytes.to_vec(), mime::IMAGE_PNG, filename, info, thumbnail) + let source = AttachmentSource::Data { + bytes: bytes.to_vec(), + filename, + }; + self.send_attachment(source, mime::IMAGE_PNG, info, thumbnail) .await; } @@ -779,10 +783,16 @@ mod imp { async fn send_file_inner(&self, file: gio::File) { let obj = self.obj(); - let (bytes, file_info) = match load_file(&file).await { - Ok(data) => data, + let Some(path) = file.path() else { + warn!("Could not read file: file does not have a path"); + toast!(obj, gettext("Error reading file")); + return; + }; + + let file_info = match FileInfo::try_from_file(&file).await { + Ok(file_info) => file_info, Err(error) => { - warn!("Could not read file: {error}"); + warn!("Could not read file info: {error}"); toast!(obj, gettext("Error reading file")); return; } @@ -818,7 +828,7 @@ mod imp { _ => (AttachmentInfo::File(BaseFileInfo { size }), None), }; - self.send_attachment(bytes, file_info.mime, file_info.filename, info, thumbnail) + self.send_attachment(path.into(), file_info.mime, info, thumbnail) .await; } diff --git a/src/utils/media/mod.rs b/src/utils/media/mod.rs index 3739269c..942db6bc 100644 --- a/src/utils/media/mod.rs +++ b/src/utils/media/mod.rs @@ -45,57 +45,54 @@ pub fn filename_for_mime(mime_type: Option<&str>, fallback: Option) .unwrap_or(name) } -/// Information about a file -pub struct FileInfo { +/// Information about a file. +pub(crate) struct FileInfo { /// The mime type of the file. - pub mime: Mime, + pub(crate) mime: Mime, /// The name of the file. - pub filename: String, + pub(crate) filename: String, /// The size of the file in bytes. - pub size: Option, + pub(crate) size: Option, } -/// Load a file and return its content and some information -pub async fn load_file(file: &gio::File) -> Result<(Vec, FileInfo), glib::Error> { - let attributes: &[&str] = &[ - gio::FILE_ATTRIBUTE_STANDARD_CONTENT_TYPE, - gio::FILE_ATTRIBUTE_STANDARD_DISPLAY_NAME, - gio::FILE_ATTRIBUTE_STANDARD_SIZE, - ]; - - // Read mime type. - let info = file - .query_info_future( - &attributes.join(","), - gio::FileQueryInfoFlags::NONE, - glib::Priority::DEFAULT, - ) - .await?; - - let mime = info - .content_type() - .and_then(|content_type| Mime::from_str(&content_type).ok()) - .unwrap_or(mime::APPLICATION_OCTET_STREAM); - - let filename = info.display_name().to_string(); - - let raw_size = info.size(); - let size = if raw_size >= 0 { - Some(raw_size.try_into().unwrap_or(u32::MAX)) - } else { - None - }; - - let (data, _) = file.load_contents_future().await?; +impl FileInfo { + /// Try to load information about the given file. + pub(crate) async fn try_from_file(file: &gio::File) -> Result { + let attributes: &[&str] = &[ + gio::FILE_ATTRIBUTE_STANDARD_CONTENT_TYPE, + gio::FILE_ATTRIBUTE_STANDARD_DISPLAY_NAME, + gio::FILE_ATTRIBUTE_STANDARD_SIZE, + ]; + + // Read mime type. + let info = file + .query_info_future( + &attributes.join(","), + gio::FileQueryInfoFlags::NONE, + glib::Priority::DEFAULT, + ) + .await?; + + let mime = info + .content_type() + .and_then(|content_type| Mime::from_str(&content_type).ok()) + .unwrap_or(mime::APPLICATION_OCTET_STREAM); + + let filename = info.display_name().to_string(); + + let raw_size = info.size(); + let size = if raw_size >= 0 { + Some(raw_size.try_into().unwrap_or(u32::MAX)) + } else { + None + }; - Ok(( - data.into(), - FileInfo { + Ok(FileInfo { mime, filename, size, - }, - )) + }) + } } /// Load information for the given media file.