-
Notifications
You must be signed in to change notification settings - Fork 155
Make Element Call widgets compatible with Matrix 2.0 mode #3686
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
This gives Element Call widgets the ability to send and receive sticky RTC membership events.
4c22402 to
d516f0f
Compare
toger5
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.
This looks great.
We cannot yet approve since we need
- the correct js-sdk version
- Decide what we do with the test coverage (I think a simple test to confirm that we call createRoomWidgetClient with the correct params would be good. Everything on top would just test already tested (in the js-sdk) code)
|
Ah, I was going to override the coverage requirement. It's a 3 line change, and I generally don't see value in tests that are a close copy of what the code already says. |
What do you mean but close to the code? Wouldn't it be good to have a test that check what the EC widget is calling the expected list of permissions? when creating the room widget client? |
BillCarsonFr
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.
Thx for all the tests. Maybe in the future would protect us from unoticed removal of some capability ;)
| const sendEvent = [ | ||
| EventType.CallNotify, // Sent as a deprecated fallback | ||
| EventType.RTCNotification, | ||
| ]; | ||
| const sendRecvEvent = [ | ||
| "org.matrix.rageshake_request", | ||
| EventType.CallEncryptionKeysPrefix, | ||
| EventType.Reaction, | ||
| EventType.RoomRedaction, | ||
| ElementCallReactionEventType, | ||
| EventType.RTCDecline, | ||
| EventType.RTCMembership, | ||
| ]; | ||
|
|
||
| const sendState = [ |
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.
@BillCarsonFr For the record this is what I was referring to with the test being a close copy of the code: The obvious way to write it has all the same lists from widget.ts duplicated here, with an expect at the end rather than a function call.
My take is that writing tests like this is busywork, since they don't tell the reviewer anything that they wouldn't have already known from reading the source file. In general my attitude toward tests - maybe an unpopular opinion :) - is that properties of code that are trivial or self-evident do not need testing.
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.
I also can get behind that. But there is some hardcoded values in there that we also check.
I was considering just writing down the full list as raw strings. But for the sake of time made it like that.
But what makes this a bit of a maintenance. burden also has the advantage, that if I mess around with the widget file, the test will force me to rethink if that is what I actually want to do.
This gives Element Call widgets the ability to send and receive sticky RTC membership events.
Depends on matrix-org/matrix-js-sdk#5142
Closes #3677
To-do: