Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Make CallHandler more EventEmittery #6704

Merged
merged 19 commits into from
Nov 30, 2021
Merged

Make CallHandler more EventEmittery #6704

merged 19 commits into from
Nov 30, 2021

Conversation

SimonBrandner
Copy link
Contributor

@SimonBrandner SimonBrandner commented Aug 28, 2021 •
edited by github-actions bot
Loading

Fixes element-hq/element-web#18812
Type: task

For the reviewer: please review carefully as this could lead to regressions. Also, this should be reviewable commit-by-commit

This is more TypeScript friendly since it's easier to type than the dispatcher actions. It's easier to read, imo and quite a bit shorter. It should also theoretically be better for performance but it's not like that would be noticeable :D

This change is marked as an internal change (Task), so will not be included in the changelog.

Preview: https://61a0f0ec203e4619c6da8a42--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

@github-actions github-actions bot added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Aug 28, 2021
@SimonBrandner SimonBrandner mentioned this pull request Aug 28, 2021
Closed
@SimonBrandner SimonBrandner force-pushed the task/call-handler-emittery branch 2 times, most recently from 977df3e to cd4ca57 Compare August 28, 2021 14:10
@SimonBrandner
sharedInstance() -> instance
61d5bd8 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Use CallState event instead of dispatching
790d079 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Simplifie some code
2d6160c 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Use a method to start a call instead of the dispatcher
c96ecc3 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Use a method instead of place_conference_call
4424371 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Make terminateCallApp() and hangupCallApp() public
447f4d6 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Use hangupAllCalls() instead of the dispatcher
c9e42bb 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Make dialNumber(), startTransferToMatrixID() and startTransferToPhone…
09882b2 
…Number() public instead of using the dispatcher

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Use answerCall() instead of using the dispatcher
9b86230 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Use hangupOrReject() instead of the dispatcher
969e7bc 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Update docs
b8dcf1c 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner SimonBrandner marked this pull request as ready for review August 28, 2021 15:06
@SimonBrandner SimonBrandner requested a review from a team as a code owner August 28, 2021 15:06
turt2live
turt2live approved these changes Sep 3, 2021
View reviewed changes
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

This looks fine, though I believe there's some hidden requirements for stuff in this area so going to ask for a second opinion review.

Thanks for taking a look at this! Down with the dispatcher!

@SimonBrandner
Merge remote-tracking branch 'upstream/develop' into task/call-handle…
ec9123c 
…r-emittery

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Copy link
Contributor Author

@turt2live, let me know if there is someone I can talk to about this, in case there are some internal requirements

@SimonBrandner
Improve TS
be5619f 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Dispatch call_state, see element-hq/element-web#18823 (comment)
2acc662 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@turt2live turt2live requested a review from dbkr September 14, 2021 03:01
@SimonBrandner SimonBrandner mentioned this pull request Sep 14, 2021
Closed
@SimonBrandner
Merge remote-tracking branch 'upstream/develop' into task/call-handle…
c1c91f3 
…r-emittery

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@dbkr
Copy link
Member

dbkr commented Sep 22, 2021

While I'm not necessarily saying the dispatcher is the best thing, it's still nominally our way of handling actions from the user, so is there a reason to call the methods directly for thing like placing / answering / hanging up calls rather than go via the dispatcher?

alsoa cc @robertlong as this is probably going to cause conflicts with your stuff

@SimonBrandner
Copy link
Contributor Author

While I'm not necessarily saying the dispatcher is the best thing, it's still nominally our way of handling actions from the user, so is there a reason to call the methods directly for thing like placing / answering / hanging up calls rather than go via the dispatcher?

As I said in the description:

This is more TypeScript friendly since it's easier to type than the dispatcher actions. It's easier to read, imo and quite a bit shorter. It should also theoretically be better for performance but it's not like that would be noticeable :D

I generally find this much easier to work with, navigating the dispatches just feels much harder and this feels much cleaner. It also cuts down the number of lines

@SimonBrandner
Copy link
Contributor Author

I see this a bit from the other perspective, why would you want to use the dispatcher for this :p

@dbkr
Copy link
Member

dbkr commented Sep 22, 2021

OK, but I think this is something we need to coordinate on across the app. I'm not really a fan of the dispatcher either, but if we're going to stop using it we should decide to do so in general rather than have some bits of the app using it and some not.

@SimonBrandner
Copy link
Contributor Author

OK, but I think this is something we need to coordinate on across the app. I'm not really a fan of the dispatcher either, but if we're going to stop using it we should decide to do so in general rather than have some bits of the app using it and some not.

I definitely agree though I would say we already are in a state where parts of the app use and other parts don't...

Is there anything I can do to help this..?

@dbkr
Copy link
Member

dbkr commented Sep 22, 2021

I'll ask around & see what people think

@SimonBrandner
Copy link
Contributor Author

I'll ask around & see what people think

Thank you!

@SimonBrandner SimonBrandner mentioned this pull request Oct 13, 2021
Closed
@SimonBrandner
Merge remote-tracking branch 'upstream/develop' into task/call-handle…
ea6c6c9 
…r-emittery

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Merge remote-tracking branch 'upstream/develop' into task/call-handle…
216cd49 
…r-emittery

Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Merge remote-tracking branch 'upstream/develop' into task/call-handle…
d20bba0 
…r-emittery
@SimonBrandner
Add missing import
3de75e7 
Signed-off-by: Šimon Brandner <simon.bra.ag@gmail.com>
@dbkr
Copy link
Member

dbkr commented Nov 29, 2021

Well, nobody seemed to object to going this way so let's do it. Let's merge this post RC.

@SimonBrandner SimonBrandner added the X-Blocked The PR cannot move forward in any capacity until an action is made label Nov 29, 2021
@dbkr dbkr merged commit cbb34d8 into matrix-org:develop Nov 30, 2021
@SimonBrandner SimonBrandner deleted the task/call-handler-emittery branch November 30, 2021 18:11
@SimonBrandner SimonBrandner removed the X-Blocked The PR cannot move forward in any capacity until an action is made label Nov 30, 2021
Palid pushed a commit that referenced this pull request Dec 2, 2021
Merge remote-tracking branch 'origin/develop' into palid/19609/fix-ro…
0c65b9f 
…om-alias

* origin/develop: (29 commits)
  Tweak roving tab index focus behaviour (#7254)
  Fix textual message stripping new line (#7239)
  Fix broken i18n in Forgot & Change password (#7252)
  Fix setBotPower to not use `.content` (#7179)
  Break long words in pinned messages to prevent overflow (#7251)
  Send read receipts for events in thread's timeline (#7229)
  Autofocus device panel entry when renaming device (#7249)
  Fix user menu bottom stroke (#7248)
  Disallow sending empty feedbacks (#7240)
  Fix wrongly sized default sub-space icons in space panel (#7243)
  Hide clear cache and reload button if crash is before client init (#7242)
  Don't show edit button for hidden edit events (#7226)
  Update Space Panel scrollable region (#7245)
  Replace breadcrumbs with recently viewed menu (#7073)
  Fix automatic space switching wrongly going via Home for room aliases (#7247)
  Make e2e tests pass in CI by using an older Synapse version (#7246)
  Make `CallHandler` more `EventEmitter`y (#6704)
  Fix links being parsed as markdown links improperly (#7200)
  Tweaks to informational architecture 1.1 (#7052)
  Upgrade allchange to 1.0.6 (#7238)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@turt2live turt2live turt2live approved these changes

@dbkr dbkr Awaiting requested review from dbkr

Assignees
No one assigned
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make CallHandler more EventEmittery
3 participants
@SimonBrandner @dbkr @turt2live