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

MatrixRTC: move MatrixRTCSession logic into LocalMembershipManager #4608

Conversation

toger5
Copy link
Contributor

@toger5 toger5 commented Jan 8, 2025

Can be reviewed commit by commit.

This PR does not change the actual logic, but instead lowers the complexity of the MatrixRTCSession class.
It moves all the logic into a new class LocalMembershipManager but keeps all the logic paths the same.
This results in a awkward api between the manager and the session and will be tackled in a follow up PR.

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-part3.5-seperate-myMembershipManager branch from dd398ab to b1fd6a3 Compare January 8, 2025 17:37
@toger5 toger5 force-pushed the toger5/refactor-matrixrtcsession-part3.5-seperate-myMembershipManager branch from b1fd6a3 to f31dbf6 Compare January 8, 2025 17:51
@toger5 toger5 force-pushed the toger5/refactor-matrixrtcsession-part3.5-seperate-myMembershipManager branch from f31dbf6 to 6f119f7 Compare January 8, 2025 17:52
@toger5 toger5 marked this pull request as ready for review January 8, 2025 17:52
@toger5 toger5 requested a review from a team as a code owner January 8, 2025 17:52
@toger5 toger5 requested a review from hughns January 8, 2025 17:52
@hughns hughns added the T-Task Tasks for the team like planning label Jan 9, 2025
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.

I've made a few comments on the style and some naming.

Otherwise this refactor looks solid and a good step in the right direction. 👍

src/matrixrtc/MatrixRTCSession.ts Outdated Show resolved Hide resolved
src/matrixrtc/MatrixRTCSession.ts Outdated Show resolved Hide resolved
src/matrixrtc/MatrixRTCMyMembershipManager.ts Outdated Show resolved Hide resolved
src/matrixrtc/MatrixRTCMyMembershipManager.ts Outdated Show resolved Hide resolved
@hughns hughns changed the title [WIP] Refactor matrixRTCSession Part3(v2): Move own membership logic into MyMembershipManager MatrixRTC: refactor MatrixRTCSession logic for own membership management Jan 9, 2025
Copy link
Member

@robintown robintown left a comment

Choose a reason for hiding this comment

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

Notes from our little pairing session:

spec/unit/matrixrtc/MatrixRTCSession.spec.ts Outdated Show resolved Hide resolved
@toger5 toger5 changed the title MatrixRTC: refactor MatrixRTCSession logic for own membership management MatrixRTC: move MatrixRTCSession logic into LocalMembershipManagemer Jan 9, 2025
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.

Latest changes look good.

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.

Latest changes look good.

@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-part3.5-seperate-myMembershipManager branch from e31e1cb to 9daacc1 Compare January 10, 2025 09:50
@toger5 toger5 requested a review from a team as a code owner January 10, 2025 09:50
@toger5 toger5 force-pushed the toger5/refactor-matrixrtcsession-part3.5-seperate-myMembershipManager branch from 9daacc1 to 24c8eb1 Compare January 10, 2025 10:09
@hughns hughns self-requested a review January 10, 2025 10:16
@toger5 toger5 changed the title MatrixRTC: move MatrixRTCSession logic into LocalMembershipManagemer MatrixRTC: move MatrixRTCSession logic into LocalMembershipManager Jan 10, 2025
@toger5 toger5 added this pull request to the merge queue Jan 10, 2025
Merged via the queue into develop with commit 7697338 Jan 10, 2025
50 checks passed
@toger5 toger5 deleted the toger5/refactor-matrixrtcsession-part3.5-seperate-myMembershipManager branch January 10, 2025 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants