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

feat(calls): add support for sending Matrix RTC call notifications (MSC4075) #3434

Merged
merged 1 commit into from
May 23, 2024

Conversation

stefanceriu
Copy link
Member

@stefanceriu stefanceriu commented May 18, 2024

This PR adds 2 new methods exposed all the way up to the ffi level that allow the client application to send call notifications when entering an Element call.

NB I had a serious go at writing a test for this but setting it up is beyond my rust sdk knowledge. If you really think it's necessary please provide tips, tricks and send thoughts and prayers.

Copy link

codecov bot commented May 18, 2024

Codecov Report

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

Project coverage is 83.22%. Comparing base (d7a8877) to head (f5c8276).
Report is 18 commits behind head on main.

Files Patch % Lines
crates/matrix-sdk/src/room/mod.rs 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3434      +/-   ##
==========================================
- Coverage   83.23%   83.22%   -0.01%     
==========================================
  Files         247      247              
  Lines       25051    25066      +15     
==========================================
+ Hits        20851    20862      +11     
- Misses       4200     4204       +4     

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

@andybalaam
Copy link
Member

The code looks sensible to me but I have asked for help from other team members with more context since I'm not confident I understand the implications of this.

@andybalaam
Copy link
Member

In terms of testing, it looks like you might be able to do it using a MockServer like test_cache_invalidation_while_encrypt does in crates/matrix-sdk/src/room/mod.rs

Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

OK, no-one else who's around at the moment has much knowledge on the Element Call integration, so I've reviewed the code as best I can.

It looks good, but I would like it to be tested. Please could you give it a go? Feel free to ping me for a call to try and pair on it if it proves tricky.

@stefanceriu
Copy link
Member Author

In terms of testing, it looks like you might be able to do it using a MockServer like test_cache_invalidation_while_encrypt does in crates/matrix-sdk/src/room/mod.rs

Thanks for the review. I already looked around but honestly all of this feels ridiculously complicated just to check a trivial piece of logic i.e. send a ring if it's a dm. Isn't there a simpler way to mock rooms in the rust sdk?

@stefanceriu stefanceriu force-pushed the stefan/send-call-notify branch from 21e4009 to ac7c4c2 Compare May 22, 2024 08:15
@andybalaam
Copy link
Member

Thanks for the review. I already looked around but honestly all of this feels ridiculously complicated just to check a trivial piece of logic i.e. send a ring if it's a dm. Isn't there a simpler way to mock rooms in the rust sdk?

It does seem unfortunate that Room holds a concrete Client reference rather than some kind of trait object or type param. I'll ask the team in the room what they think.

@stefanceriu
Copy link
Member Author

Alrighty, I managed to make something happen, please have another look

@stefanceriu stefanceriu force-pushed the stefan/send-call-notify branch from 3d3c5db to f5c8276 Compare May 23, 2024 09:29
Copy link
Member

@andybalaam andybalaam 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, thanks!

@jmartinesp
Copy link
Contributor

I think someone from the Rust team has to hit the green button 😅 .

@bnjbvr bnjbvr merged commit f672f17 into main May 23, 2024
38 checks passed
@bnjbvr bnjbvr deleted the stefan/send-call-notify branch May 23, 2024 13:07
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.

5 participants