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.

Sign up for GitHub

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

Jump to bottom

MatrixRTC: refactor MatrixRTCSession MemberManager API #4610

Merged
merged 12 commits into from
Jan 13, 2025
Merged

MatrixRTC: refactor MatrixRTCSession MemberManager API #4610

merged 12 commits into from
Jan 13, 2025

Conversation

toger5
Copy link
Contributor

@toger5 toger5 commented Jan 9, 2025 •
edited
Loading

Can be reviewed commit by commit.

This changes the api between the session and the MembershipManager.
It also makes sure that the tests are only using this api and nothing specific to the current MembershipManager implementation.

It is planned to do the same thing with the encryption logic of the session.
This allows to exchange the MembershipManager with a different implementation.

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

@toger5 toger5 force-pushed the toger5/refactor-matrixrtcsession-part4-membershipManager-session-api branch from b292450 to 5946ff4 Compare January 9, 2025 17:27
@toger5 toger5 force-pushed the toger5/refactor-matrixrtcsession-part4-membershipManager-session-api branch from 5946ff4 to 99922e4 Compare January 9, 2025 17:37
@toger5 toger5 force-pushed the toger5/refactor-matrixrtcsession-part4-membershipManager-session-api branch from 99922e4 to df69930 Compare January 10, 2025 09:41
@toger5 toger5 force-pushed the toger5/refactor-matrixrtcsession-part3.5-seperate-myMembershipManager branch 2 times, most recently from 094b0f4 to e31e1cb Compare January 10, 2025 09:45
@toger5 toger5 force-pushed the toger5/refactor-matrixrtcsession-part4-membershipManager-session-api branch from df69930 to 5873e3a Compare January 10, 2025 09:45
@toger5 toger5 force-pushed the toger5/refactor-matrixrtcsession-part3.5-seperate-myMembershipManager branch 2 times, most recently from 9daacc1 to 24c8eb1 Compare January 10, 2025 10:09
@toger5 toger5 force-pushed the toger5/refactor-matrixrtcsession-part4-membershipManager-session-api branch from 5873e3a to 8231f75 Compare January 10, 2025 10:18
@toger5 toger5 force-pushed the toger5/refactor-matrixrtcsession-part4-membershipManager-session-api branch from 8231f75 to eefe219 Compare January 10, 2025 10:32
@toger5 toger5 force-pushed the toger5/refactor-matrixrtcsession-part4-membershipManager-session-api branch from eefe219 to 7ce0b77 Compare January 10, 2025 10:37
@toger5 toger5 marked this pull request as ready for review January 10, 2025 10:48
@toger5 toger5 requested a review from a team as a code owner January 10, 2025 10:48
@toger5 toger5 requested review from robintown and removed request for a team January 10, 2025 10:48
Base automatically changed from toger5/refactor-matrixrtcsession-part3.5-seperate-myMembershipManager to develop January 10, 2025 11:03
@toger5 toger5 force-pushed the toger5/refactor-matrixrtcsession-part4-membershipManager-session-api branch from 7ce0b77 to 9b6bc3f Compare January 10, 2025 11:05
toger5 added 3 commits January 10, 2025 12:08
@toger5
update join and leave internal api.
b5c84da
@toger5
rename onMembershipUpdate and triggerCallMembershipEventUpdate to onM…
99ccc62 
…embershipsUpdate

This makes it more clear that we do not talk about our own membership but all memberships in the session
@toger5
cleanup MembershipManager
10fc117 
 - add comments and interface how to test this class.
 - sort methods by public/private
 - make triggerCallMembershipEventUpdate private
@toger5 toger5 force-pushed the toger5/refactor-matrixrtcsession-part4-membershipManager-session-api branch from 9b6bc3f to ce420c9 Compare January 10, 2025 11:09
hughns
hughns requested changes Jan 10, 2025
View reviewed changes
src/matrixrtc/MatrixRTCSession.ts Outdated Show resolved Hide resolved
src/matrixrtc/MembershipManager.ts Outdated Show resolved Hide resolved
src/matrixrtc/MembershipManager.ts Outdated Show resolved Hide resolved
hughns
hughns reviewed Jan 10, 2025
View reviewed changes
Copy link
Member

@hughns hughns left a comment

Choose a reason for hiding this comment

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

The naming doesn't feel quite right here either.

We have an abstract class called MembershipManagerInterface.

@toger5
Copy link
Contributor Author

toger5 commented Jan 10, 2025 •
edited
Loading

Its an abstract class we use as an interface (but need to declare it as an abstract class to also have the constructor)
But I think ts is not even checking the constructor type correctly...

@toger5 toger5 changed the title MatrixRTC: refactor MatrixRTCSession api MatrixRTC: refactor MatrixRTCSession MemberManager api Jan 10, 2025
@toger5
Copy link
Contributor Author

toger5 commented Jan 10, 2025

@hughns we might want to make this an interface and do not care about the constructor.
I liked the very explicit constructor because it gave a good insight into how we would mock the client and room parts for this class.
A simple interface would also work however.

@toger5
Make the interface an actual interface.
e164f99 
The actual constructor of the class now contains the `Pick` to define what it needs from the client.
@hughns hughns added T-Task Tasks for the team like planning and removed T-Enhancement labels Jan 10, 2025
@hughns hughns changed the title MatrixRTC: refactor MatrixRTCSession MemberManager api MatrixRTC: refactor MatrixRTCSession MemberManager API Jan 10, 2025
@toger5 toger5 force-pushed the toger5/refactor-matrixrtcsession-part4-membershipManager-session-api branch from 5a23b28 to 00d4e72 Compare January 10, 2025 16:28
@toger5 toger5 force-pushed the toger5/refactor-matrixrtcsession-part4-membershipManager-session-api branch from 00d4e72 to c798a1f Compare January 10, 2025 16:29
hughns
hughns approved these changes Jan 13, 2025
View reviewed changes
Copy link
Member

@hughns hughns left a comment

Choose a reason for hiding this comment

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

The revised naming all looks good to me now 👍.

I've added a couple of suggested changes to tidy up some comments.

src/matrixrtc/MatrixRTCSession.ts Outdated Show resolved Hide resolved
src/matrixrtc/MatrixRTCSession.ts Outdated Show resolved Hide resolved
toger5 and others added 2 commits January 13, 2025 14:02
@toger5
Copy link
Contributor Author

toger5 commented Jan 13, 2025

I've added a couple of suggested changes to tidy up some comments.

They look like plain improvements so I merged both suggestions.

@github-actions github-actions bot deployed to PR Documentation Preview January 13, 2025 13:04 Active
@toger5 toger5 added this pull request to the merge queue Jan 13, 2025
Merged via the queue into develop with commit ffb228b Jan 13, 2025
30 checks passed
@toger5 toger5 deleted the toger5/refactor-matrixrtcsession-part4-membershipManager-session-api branch January 13, 2025 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hughns hughns hughns approved these changes

@robintown robintown Awaiting requested review from robintown robintown is a code owner automatically assigned from matrix-org/element-call-reviewers

Assignees
No one assigned
Labels
T-Task Tasks for the team like planning
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@toger5 @hughns