-
Notifications
You must be signed in to change notification settings - Fork 349
feat(rtc): Remove deprecated CallNotify in favour of RtcNotification #5668
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
CodSpeed Performance ReportMerging #5668 will not alter performanceComparing Summary
|
e4c8d51 to
e68ddbc
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5668 +/- ##
==========================================
- Coverage 88.37% 88.36% -0.01%
==========================================
Files 355 355
Lines 97476 97476
Branches 97476 97476
==========================================
- Hits 86146 86138 -8
- Misses 7264 7272 +8
Partials 4066 4066 ☔ View full report in Codecov by Sentry. |
e68ddbc to
d270dcd
Compare
bnjbvr
left a comment
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. Two minor comments below, and a few extra questions:
- being unaware of the MSC et al., does this maintain backwards compatibility with the existing system and events?
- linked to the other question: does it require a timed merge with an Element Call release, or can this PR be merged at any time?
bindings/matrix-sdk-ffi/src/ruma.rs
Outdated
| pub enum NotifyType { | ||
| pub enum RtcNotificationType { | ||
| Ring, | ||
| Notify, |
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.
Should it be renamed Notification to align with Ruma?
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.
right, done 9d53bc1
| content.relates_to = rtc_membership_event_id.map(Reference::new); | ||
| content.mentions = mentions; |
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.
The more idiomatic way with the EventFactory builder pattern would be to add fn mentions() and fn rtc_membership_event() methods on impl EventBuilder<RtcNotificationEventContent>, so that we don't have Optional parameters in this ctor. Also, I would rather make it so that the timestamp and durations are configurable (aka function parameters), so we don't have tight coupling between the test framework and the tests themselves (or, at least let's rename rtc_notification to predefined_rtc_notification or something like that)
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 for all the tips! I missed the fact that one could do impl EventBuilder<CONTENT>.
Done here 0505ce2
Good Q. Given that EC is already sending both the old |
bnjbvr
left a comment
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.
Perfect then, thanks!
CallNotifyevent has been deprecated in favour ofRtcNotificationevent ruma/ruma#2199Needs an update on ruma dependency
Signed-off-by: