feat(ffi): Expose method for sending galleries#5163
Conversation
Johennes
changed the title
Johennes
force-pushed
the
johannes/msc4274-step4
branch
from
721b7aa to
816d087
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5163 +/- ##
==========================================
- Coverage 85.86% 85.86% -0.01%
==========================================
Files 336 336
Lines 36582 36582
==========================================
- Hits 31412 31410 -2
- Misses 5170 5172 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Johennes
force-pushed
the
johannes/msc4274-step4
branch
from
816d087 to
92b5d79
Compare
|
I think the CI failures are caused by complement-crypto using uniffi 0.25.0. Is there a reason why it's not using 0.28.0 like the SDK? Apart from that, I think this is ready for review. |
Yeah, complement-crypto uses the Go bindings generator, which is slightly outdated and requires using uniffi 0.25.0, so it is our least common denominator here. In the meanwhile, the code has to be compatible with uniffi 0.25.0, until this is upgraded in complement-crypto. |
bnjbvr
left a comment
bnjbvr
left a comment
There was a problem hiding this comment.
Looks great! only requesting changes because we need to make it work for complement-crypto, as alluded to in the previous comment (unless you're interested in a side quest to bump the go bindings in complement-crypto :D).
| impl From<ImageMessageContent> for RumaImageMessageEventContent { | ||
| fn from(value: ImageMessageContent) -> Self { | ||
| let (body, filename) = get_body_and_filename(value.filename, value.caption); | ||
| let mut event_content = Self::new(body, (*value.source).clone().into()) |
There was a problem hiding this comment.
no ideas if that idea applies cleanly, but: is there no way to avoid the clone here? It seems spurious since the value is passed by ownership and consumer by this method.
(and it happens a few times below as well)
There was a problem hiding this comment.
Hm, I haven't found a way to avoid the clone. Self::new requires a plain MediaSource but value.source wraps it in an Arc. So I have to clone the inner value because I cannot move it out of the Arc. I'm not sure I'm fully following why because value is also consumed by the method. My Rust-fu might just be too weak. 😕
bnjbvr
left a comment
bnjbvr
left a comment
There was a problem hiding this comment.
Thanks a bunch, and congrats on shipping the gallery feature on the Rust SDK, end-to-end 🥳
|
Thanks a ton for reviewing all these bulky PRs! ❤️ |
Looks like I forgot adding this in #5163, sorry. Everything below the FFI layer was already prepared for replies. Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
2 participants
Addendum to #5125 to allow sending galleries from the FFI crate. This is the final PR for galleries (apart from possible enhancements to report the upload status in #5008).
This is somewhat lengthy again, apologies. Most of the changes are just wrappers and type mapping, however. So I was hoping that it's relatively straightforward to review in bulk. Happy to try and elaborate on the changes or break them up into smaller commits if that helps, however.