-
Notifications
You must be signed in to change notification settings - Fork 260
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
Media: Add support for media captions. #4081
Conversation
bindings/matrix-sdk-ffi/src/ruma.rs
Outdated
event_content.filename = content.filename; | ||
event_content.filename = Some(content.filename); |
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'm not 100% about these first 4 mappings going back to Ruma. Is it important to store the non-computed filename in the FFI type so that we can pass that back here, or is that an unnecessary step?
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 this is technically only used for edits and media events can't have them AFAICT, so it might not be needed. However, I don't see the harm in passing it around in case we might need it in the future.
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've added it back as raw_filename
.
633d9b1
to
0c01fec
Compare
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.
LGTM, thanks!
bindings/matrix-sdk-ffi/src/ruma.rs
Outdated
event_content.filename = content.filename; | ||
event_content.filename = Some(content.filename); |
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 this is technically only used for edits and media events can't have them AFAICT, so it might not be needed. However, I don't see the harm in passing it around in case we might need it in the future.
The tests need some love though 😅 . |
787a049
to
5519dae
Compare
Just updated adding in the |
Also includes the computed filename too.
5519dae
to
204c45e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4081 +/- ##
==========================================
+ Coverage 84.66% 84.67% +0.01%
==========================================
Files 269 269
Lines 28694 28694
==========================================
+ Hits 24294 24297 +3
+ Misses 4400 4397 -3 ☔ View full report in Codecov by Sentry. |
@@ -537,6 +543,7 @@ async fn test_sync_resumes_from_previous_state() -> Result<(), Error> { | |||
"lists": { | |||
ALL_ROOMS: { | |||
"ranges": [[0, 9]], | |||
"timeline_limit": 0, |
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.
All these timeline_limit
to 0 is a bug, fixed in #4039 :-).
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.
Ooops, I thought it was mocks not being fully fleshed out 🙈
This PR makes 2 changes: