feat(media)!: Make all fields of Thumbnail required

It seems sensible to assume that if a client is able to generate a thumbnail,
it should be able to get all this information for it too.
A thumbnail with no information is not really useful, as we don't know when it could be used instead of the original image.

Removes `BaseThumbnailInfo`.

Signed-off-by: Kévin Commaille <[email protected]>

Author: zecakeh

bindings/matrix-sdk-ffi/src/ruma.rs

@@ -15,9 +15,7 @@
 use std::{collections::BTreeSet, sync::Arc, time::Duration};
 
 use extension_trait::extension_trait;
-use matrix_sdk::attachment::{
-    BaseAudioInfo, BaseFileInfo, BaseImageInfo, BaseThumbnailInfo, BaseVideoInfo,
-};
+use matrix_sdk::attachment::{BaseAudioInfo, BaseFileInfo, BaseImageInfo, BaseVideoInfo};
 use ruma::{
     assign,
     events::{
@@ -747,21 +745,6 @@ impl From<ThumbnailInfo> for RumaThumbnailInfo {
     }
 }
 
-impl TryFrom<&ThumbnailInfo> for BaseThumbnailInfo {
-    type Error = MediaInfoError;
-
-    fn try_from(value: &ThumbnailInfo) -> Result<Self, MediaInfoError> {
-        let height = UInt::try_from(value.height.ok_or(MediaInfoError::MissingField)?)
-            .map_err(|_| MediaInfoError::InvalidField)?;
-        let width = UInt::try_from(value.width.ok_or(MediaInfoError::MissingField)?)
-            .map_err(|_| MediaInfoError::InvalidField)?;
-        let size = UInt::try_from(value.size.ok_or(MediaInfoError::MissingField)?)
-            .map_err(|_| MediaInfoError::InvalidField)?;
-
-        Ok(BaseThumbnailInfo { height: Some(height), width: Some(width), size: Some(size) })
-    }
-}
-
 #[derive(Clone, uniffi::Record)]
 pub struct NoticeMessageContent {
     pub body: String,

bindings/matrix-sdk-ffi/src/timeline/mod.rs

@@ -24,7 +24,7 @@ use matrix_sdk::crypto::CollectStrategy;
 use matrix_sdk::{
     attachment::{
         AttachmentConfig, AttachmentInfo, BaseAudioInfo, BaseFileInfo, BaseImageInfo,
-        BaseThumbnailInfo, BaseVideoInfo, Thumbnail,
+        BaseVideoInfo, Thumbnail,
     },
     deserialized_responses::{ShieldState as SdkShieldState, ShieldStateCode},
     room::edit::EditedContent as SdkEditedContent,
@@ -53,7 +53,7 @@ use ruma::{
         },
         AnyMessageLikeEventContent,
     },
-    EventId,
+    EventId, UInt,
 };
 use tokio::{
     sync::Mutex,
@@ -144,19 +144,26 @@ fn build_thumbnail_info(
             let thumbnail_data =
                 fs::read(thumbnail_url).map_err(|_| RoomError::InvalidThumbnailData)?;
 
-            let base_thumbnail_info = BaseThumbnailInfo::try_from(&thumbnail_info)
-                .map_err(|_| RoomError::InvalidAttachmentData)?;
+            let height = thumbnail_info
+                .height
+                .and_then(|u| UInt::try_from(u).ok())
+                .ok_or(RoomError::InvalidAttachmentData)?;
+            let width = thumbnail_info
+                .width
+                .and_then(|u| UInt::try_from(u).ok())
+                .ok_or(RoomError::InvalidAttachmentData)?;
+            let size = thumbnail_info
+                .width
+                .and_then(|u| UInt::try_from(u).ok())
+                .ok_or(RoomError::InvalidAttachmentData)?;
 
             let mime_str =
                 thumbnail_info.mimetype.as_ref().ok_or(RoomError::InvalidAttachmentMimeType)?;
             let mime_type =
                 mime_str.parse::<Mime>().map_err(|_| RoomError::InvalidAttachmentMimeType)?;
 
-            let thumbnail = Thumbnail {
-                data: thumbnail_data,
-                content_type: mime_type,
-                info: Some(base_thumbnail_info),
-            };
+            let thumbnail =
+                Thumbnail { data: thumbnail_data, content_type: mime_type, height, width, size };
 
             Ok(AttachmentConfig::with_thumbnail(thumbnail))
         }

crates/matrix-sdk/src/attachment.rs

@@ -148,48 +148,30 @@ impl From<AttachmentInfo> for FileInfo {
     }
 }
 
-#[derive(Debug, Default, Clone)]
-/// Base metadata about a thumbnail.
-pub struct BaseThumbnailInfo {
-    /// The height of the thumbnail in pixels.
-    pub height: Option<UInt>,
-    /// The width of the thumbnail in pixels.
-    pub width: Option<UInt>,
-    /// The file size of the thumbnail in bytes.
-    pub size: Option<UInt>,
-}
-
-impl From<BaseThumbnailInfo> for ThumbnailInfo {
-    fn from(info: BaseThumbnailInfo) -> Self {
-        assign!(ThumbnailInfo::new(), {
-            height: info.height,
-            width: info.width,
-            size: info.size,
-        })
-    }
-}
-
 /// A thumbnail to upload and send for an attachment.
 #[derive(Debug)]
 pub struct Thumbnail {
     /// The raw bytes of the thumbnail.
     pub data: Vec<u8>,
     /// The type of the thumbnail, this will be used as the content-type header.
     pub content_type: mime::Mime,
-    /// The metadata of the thumbnail.
-    pub info: Option<BaseThumbnailInfo>,
+    /// The height of the thumbnail in pixels.
+    pub height: UInt,
+    /// The width of the thumbnail in pixels.
+    pub width: UInt,
+    /// The file size of the thumbnail in bytes.
+    pub size: UInt,
 }
 
 impl Thumbnail {
     /// Convert this `Thumbnail` into a `(data, content_type, info)` tuple.
     pub fn into_parts(self) -> (Vec<u8>, mime::Mime, Box<ThumbnailInfo>) {
-        let thumbnail_info = assign!(
-            self.info
-                .as_ref()
-                .map(|info| ThumbnailInfo::from(info.clone()))
-                .unwrap_or_default(),
-            { mimetype: Some(self.content_type.to_string()) }
-        );
+        let thumbnail_info = assign!(ThumbnailInfo::new(), {
+            height: Some(self.height),
+            width: Some(self.width),
+            size: Some(self.size),
+            mimetype: Some(self.content_type.to_string())
+        });
         (self.data, self.content_type, Box::new(thumbnail_info))
     }
 }

crates/matrix-sdk/src/room/mod.rs

@@ -1949,14 +1949,9 @@ impl Room {
 
         // If necessary, store caching data for the thumbnail ahead of time.
         let thumbnail_cache_info = if store_in_cache {
-            // Use a small closure returning Option to avoid an unnecessary complicated
-            // chain of map/and_then.
-            let get_info = || {
-                let thumbnail = thumbnail.as_ref()?;
-                let info = thumbnail.info.as_ref()?;
-                Some((thumbnail.data.clone(), info.height?, info.width?))
-            };
-            get_info()
+            thumbnail
+                .as_ref()
+                .map(|thumbnail| (thumbnail.data.clone(), thumbnail.height, thumbnail.width))
         } else {
             None
         };

crates/matrix-sdk/src/send_queue/upload.rs

@@ -31,7 +31,7 @@ use ruma::{
         },
         AnyMessageLikeEventContent,
     },
-    uint, OwnedMxcUri, OwnedTransactionId, TransactionId, UInt,
+    OwnedMxcUri, OwnedTransactionId, TransactionId, UInt,
 };
 use tracing::{debug, error, instrument, trace, warn, Span};
 
@@ -182,23 +182,17 @@ impl RoomSendQueue {
 
             // Process the thumbnail, if it's been provided.
             if let Some(thumbnail) = config.thumbnail.take() {
-                // Create the information required for filling the thumbnail section of the
-                // media event.
-                let (data, content_type, thumbnail_info) = thumbnail.into_parts();
-
                 // Normalize information to retrieve the thumbnail in the cache store.
-                let height = thumbnail_info.height.unwrap_or_else(|| {
-                    trace!("thumbnail height is unknown, using 0 for the cache entry");
-                    uint!(0)
-                });
-                let width = thumbnail_info.width.unwrap_or_else(|| {
-                    trace!("thumbnail width is unknown, using 0 for the cache entry");
-                    uint!(0)
-                });
+                let height = thumbnail.height;
+                let width = thumbnail.width;
 
                 let txn = TransactionId::new();
                 trace!(upload_thumbnail_txn = %txn, thumbnail_size = ?(height, width), "attachment has a thumbnail");
 
+                // Create the information required for filling the thumbnail section of the
+                // media event.
+                let (data, content_type, thumbnail_info) = thumbnail.into_parts();
+
                 // Cache thumbnail in the cache store.
                 let thumbnail_media_request =
                     make_local_thumbnail_media_request(&txn, height, width);
@@ -320,27 +314,18 @@ impl QueueStorage {
                 let from_req =
                     make_local_thumbnail_media_request(&info.txn, info.height, info.width);
 
-                if info.height == uint!(0) || info.width == uint!(0) {
-                    trace!(from = ?from_req.source, "removing thumbnail with unknown dimension from cache store");
-
-                    cache_store
-                        .remove_media_content(&from_req)
-                        .await
-                        .map_err(RoomSendQueueStorageError::EventCacheStoreError)?;
-                } else {
-                    trace!(from = ?from_req.source, to = ?new_source, "renaming thumbnail file key in cache store");
-
-                    // Reuse the same format for the cached thumbnail with the final MXC ID.
-                    let new_format = from_req.format.clone();
-
-                    cache_store
-                        .replace_media_key(
-                            &from_req,
-                            &MediaRequestParameters { source: new_source, format: new_format },
-                        )
-                        .await
-                        .map_err(RoomSendQueueStorageError::EventCacheStoreError)?;
-                }
+                trace!(from = ?from_req.source, to = ?new_source, "renaming thumbnail file key in cache store");
+
+                // Reuse the same format for the cached thumbnail with the final MXC ID.
+                let new_format = from_req.format.clone();
+
+                cache_store
+                    .replace_media_key(
+                        &from_req,
+                        &MediaRequestParameters { source: new_source, format: new_format },
+                    )
+                    .await
+                    .map_err(RoomSendQueueStorageError::EventCacheStoreError)?;
             }
         }
 

crates/matrix-sdk/tests/integration/room/attachment/mod.rs

@@ -1,10 +1,7 @@
 use std::time::Duration;
 
 use matrix_sdk::{
-    attachment::{
-        AttachmentConfig, AttachmentInfo, BaseImageInfo, BaseThumbnailInfo, BaseVideoInfo,
-        Thumbnail,
-    },
+    attachment::{AttachmentConfig, AttachmentInfo, BaseImageInfo, BaseVideoInfo, Thumbnail},
     media::{MediaFormat, MediaRequestParameters, MediaThumbnailSettings},
     test_utils::mocks::MatrixMockServer,
 };
@@ -210,11 +207,9 @@ async fn test_room_attachment_send_info_thumbnail() {
     let config = AttachmentConfig::with_thumbnail(Thumbnail {
         data: b"Thumbnail".to_vec(),
         content_type: mime::IMAGE_JPEG,
-        info: Some(BaseThumbnailInfo {
-            height: Some(uint!(360)),
-            width: Some(uint!(480)),
-            size: Some(uint!(3600)),
-        }),
+        height: uint!(360),
+        width: uint!(480),
+        size: uint!(3600),
     })
     .info(AttachmentInfo::Image(BaseImageInfo {
         height: Some(uint!(600)),