-
Notifications
You must be signed in to change notification settings - Fork 155
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.
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
Use in-memory thumbnail APIs when possible #3817
Use in-memory thumbnail APIs when possible #3817
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3817 +/- ##
===========================================
- Coverage 82.91% 82.88% -0.03%
===========================================
Files 1772 1772
Lines 44534 44551 +17
Branches 5227 5235 +8
===========================================
+ Hits 36924 36928 +4
- Misses 5780 5784 +4
- Partials 1830 1839 +9 ☔ View full report in Codecov by Sentry. |
) | ||
} else { | ||
MediaRequestData( | ||
source = content.thumbnailSource, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep content.preferredMediaSource
and always use kind = MediaRequestData.Kind.Thumbnail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except, the thumbnailAPI is probably not working with GIF?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, it doesn't work properly... I guess we'll need to check the mime type here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already done in preferredMediaSource
, so maybe we could have a mediaRequestData
extension function in Content
, to pick the correct source
and kind
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried something like that in 6da362e.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why do you want to use Kind.File
when using mediaSource
(except for isMimeTypeAnimatedImage I mean).
I'd have done something like that :
val thumbnailMediaRequest: MediaRequestData by lazy {
if (mimeType.isMimeTypeAnimatedImage()) {
MediaRequestData(
source = mediaSource,
kind = MediaRequestData.Kind.File(
fileName = filename,
mimeType = mimeType
)
)
} else {
MediaRequestData(
source = thumbnailSource ?: mediaSource,
kind = MediaRequestData.Kind.Thumbnail(
width = thumbnailWidth?.toLong() ?: MAX_THUMBNAIL_WIDTH,
height = thumbnailHeight?.toLong() ?: MAX_THUMBNAIL_HEIGHT
),
)
}
}
and removed the preferredMediaSource
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used this in daba49c.
Also add `TimelineItemImageContent.thumbnailMediaRequest` lazy property.
8aaa467
to
6da362e
Compare
Rename `thumbnailMediaRequest` to `thumbnailMediaRequestData`.
Quality Gate passedIssues Measures |
Just a heads up: this is needed to fix a bug with the media upload + send queue implementation, so it should be merged soon-ish. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes, LGTM!
Content
Client.getMediaFIle(...)
to load thumbnails, useClient.getMediaThumbnail(...)
. This fixes a bug with upcoming new Rust APIs where thumbnails for videos weren't being displayed.TimelineItemImageContent
andTimelineItemVideoContent
.ByteArray.toUByteArray().toByteArray()
transformation. This was needed before because uniffi returned something likeArray<UByte>
instead, but it now returns a properByteArray
.Motivation and context
Fix issue in upcoming SDK versions, use the right APIs when possible.
Tests
Tested devices
Checklist