Skip to content

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

Merged
merged 7 commits into from
Jun 4, 2025

Conversation

Johennes
Copy link
Contributor

@Johennes Johennes commented Jun 3, 2025

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.

  • Public API changes documented in changelogs (optional)

@Johennes Johennes changed the title feat(ffi): Expose sending method for sending galleries feat(ffi): Expose method for sending galleries Jun 3, 2025
@Johennes Johennes force-pushed the johannes/msc4274-step4 branch from 721b7aa to 816d087 Compare June 3, 2025 19:57
Copy link

codecov bot commented Jun 3, 2025

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.86%. Comparing base (d3be744) to head (0337fe9).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk-ui/src/timeline/futures.rs 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
@Johennes Johennes force-pushed the johannes/msc4274-step4 branch from 816d087 to 92b5d79 Compare June 4, 2025 07:49
@Johennes
Copy link
Contributor Author

Johennes commented Jun 4, 2025

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.

@Johennes Johennes marked this pull request as ready for review June 4, 2025 08:03
@Johennes Johennes requested a review from a team as a code owner June 4, 2025 08:03
@Johennes Johennes requested review from poljar and removed request for a team June 4, 2025 08:03
@bnjbvr
Copy link
Member

bnjbvr commented Jun 4, 2025

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.

Copy link
Member

@bnjbvr bnjbvr left a 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())
Copy link
Member

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)

Copy link
Contributor Author

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. 😕

@poljar poljar removed their request for review June 4, 2025 10:05
@Johennes Johennes requested a review from bnjbvr June 4, 2025 13:28
Copy link
Member

@bnjbvr bnjbvr left a 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 🥳

@bnjbvr bnjbvr merged commit d38f409 into matrix-org:main Jun 4, 2025
43 checks passed
@Johennes
Copy link
Contributor Author

Johennes commented Jun 4, 2025

Thanks a ton for reviewing all these bulky PRs! ❤️

andybalaam pushed a commit that referenced this pull request Jun 5, 2025
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants