feat(media)!: Make all fields of Thumbnail required
#4324
Conversation
… the encrypted thumbnail's content type Signed-off-by: Kévin Commaille <[email protected]>
Signed-off-by: Kévin Commaille <[email protected]>
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]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4324 +/- ##
==========================================
+ Coverage 85.07% 85.10% +0.02%
==========================================
Files 275 275
Lines 30314 30297 -17
==========================================
- Hits 25789 25783 -6
+ Misses 4525 4514 -11 ☔ View full report in Codecov by Sentry. |
| 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)?; |
There was a problem hiding this comment.
Since we're failing if any's missing, maybe the FFI's ThumbnailInfo could also have non-optional fields for height/width/size? (also the last one is wrong, let size = thumbnail_info.width)
|
@jmartinesp @stefanceriu can you confirm it's OK to always require a height/width/size for a thumbnail? |
I think it's fine, yes, we were already enforcing them on the FFI level on the same |
On Android we weren't, but I think it makes sense. However, I'm not sure if this makes sense if this info comes from the HS: if we get the mxc url of the thumbnail for a timeline event and we don't have a size, I think we could still display this thumbnail without those if the other client didn't provide them. |
Ah yeah; forgot to say, this is only in the context of sending, so you could still receive thumbnails that lack dimensions, IIUC. |
|
So I should create a new type for sending? If I'm not mistaken that also means creating new |
Unless I'm mistaken, I think your change is fine ad we don't need a new type for sending, because it only touches places where we're sending a |
|
I was talking about the FFI crate, if that's not clear. |
|
Ah yeah, I missed that the Thanks @zecakeh as usual for the nice PR! |
Thumbnailis used to provide the data and information when uploading a thumbnail along an attachment. 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.
It allows to simplify the send queue to not have to handle the case where the dimensions of the thumbnail are missing.
Removes
BaseThumbnailInfoin the process by merging its fields inThumbnaildirectly.There are also 2 pre-commits to refactor part of the code to simplify the changes.
cc @bnjbvr.