-
Notifications
You must be signed in to change notification settings - Fork 315
feat(ffi): Expose method for sending galleries #5163
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
Conversation
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>
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. |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😕
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 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>
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.