Skip to content
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

MSC2530: added the ability to send media with captions #3226

Merged
merged 23 commits into from Mar 19, 2024

Conversation

surakin
Copy link
Contributor

@surakin surakin commented Mar 16, 2024

Now that there is some support for MSC2530, I gave adding sending captions a try. ( This is my first time with Rust 😄 )

I tried it on Element X with a hardcoded caption and it seems to work well
image

(It even got forwarded through mautrix-whatsapp and the caption was visible on the Whatsapp side)

  • Public API changes documented in changelogs (optional)

Signed-off-by: Marco Alvarez surakin@gmail.com

@surakin surakin marked this pull request as ready for review March 17, 2024 09:07
@surakin surakin requested a review from a team as a code owner March 17, 2024 09:07
@surakin surakin requested review from bnjbvr and removed request for a team March 17, 2024 09:07
Copy link

codecov bot commented Mar 17, 2024

Codecov Report

Attention: Patch coverage is 37.50000% with 35 lines in your changes are missing coverage. Please review.

Project coverage is 83.64%. Comparing base (876d323) to head (9f89e2d).
Report is 14 commits behind head on main.

❗ Current head 9f89e2d differs from pull request most recent head 58529c3. Consider uploading reports for the commit 58529c3 to get more accurate results

Files Patch % Lines
crates/matrix-sdk/src/media.rs 39.13% 14 Missing ⚠️
crates/matrix-sdk/src/encryption/mod.rs 0.00% 11 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/futures.rs 0.00% 3 Missing ⚠️
crates/matrix-sdk/src/attachment.rs 50.00% 3 Missing ⚠️
crates/matrix-sdk/src/room/mod.rs 57.14% 3 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/mod.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3226      +/-   ##
==========================================
- Coverage   83.67%   83.64%   -0.03%     
==========================================
  Files         236      236              
  Lines       24398    24415      +17     
==========================================
+ Hits        20414    20421       +7     
- Misses       3984     3994      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@zecakeh zecakeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick review of things that jumped out at me.

The comments usually apply to several identical parts of the code.

crates/matrix-sdk/src/room/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/room/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/encryption/mod.rs Outdated Show resolved Hide resolved
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.

Wow, that's really nice, thanks for working on this! I'm way too excited about getting captioned media in all apps 👀

There are a few CI tasks failing. To re-run them at home:

  • clippy: cargo xtask ci clippy
  • documentation task: cargo test --doc --features docsrs

Happy to take another look when the comments I've posted have been addressed 😊

crates/matrix-sdk/src/room/mod.rs Outdated Show resolved Hide resolved
bindings/matrix-sdk-ffi/src/timeline/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/encryption/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/encryption/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/room/mod.rs Outdated Show resolved Hide resolved
examples/image_bot/src/main.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/tests/integration/room/joined.rs Outdated Show resolved Hide resolved
@surakin surakin requested a review from bnjbvr March 18, 2024 16:40
@surakin
Copy link
Contributor Author

surakin commented Mar 18, 2024

All green 🎉

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.

Getting there, thanks for the changes! Only a few comments about doc comments, and we should be good to go.

crates/matrix-sdk-ui/src/timeline/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/encryption/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/media.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/room/mod.rs Outdated Show resolved Hide resolved
@bnjbvr
Copy link
Member

bnjbvr commented Mar 19, 2024

Thanks! Can you edit the original message to include the signoff, as requested in our contribution guidelines, please?

@bnjbvr
Copy link
Member

bnjbvr commented Mar 19, 2024

(CI failure is unrelated.)

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 🙏 Exciting!

@bnjbvr bnjbvr merged commit 10069fb into matrix-org:main Mar 19, 2024
32 of 33 checks passed
@surakin
Copy link
Contributor Author

surakin commented Mar 19, 2024

Yay! 🎉

@surakin surakin deleted the msc2530 branch March 19, 2024 10:10
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.

None yet

4 participants