-
-
Notifications
You must be signed in to change notification settings - Fork 612
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
Improve MatrixCall
media handling and internally remove CallType
s
#1911
Improve MatrixCall
media handling and internally remove CallType
s
#1911
Conversation
ee97cae
to
d916d3f
Compare
Signed-off-by: Šimon Brandner <[email protected]>
Signed-off-by: Šimon Brandner <[email protected]>
Signed-off-by: Šimon Brandner <[email protected]>
d916d3f
to
7143ef8
Compare
This looks really good @SimonBrandner! I'll let someone else review the breaking change, but it looks good to me. One comment, if we wanted to expose changing audio/video devices mid-call, I assume we'd want to expose that here. It'd also need a reference to the active calls to swap out the media streams and renegotiate. That could be a good future PR building on this work. |
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.
Overall this looks fine, thank you! The breaking change feels like a good quality of life improvement and version numbers are cheap, so I'm okay with this going through.
public async placeCall(audio: boolean, video: boolean): Promise<void> { | ||
logger.debug(`placeCall audio=${audio} video=${video}`); | ||
if (!audio) { | ||
throw new Error("You CANNOT start a call without audio"); |
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.
hmm.. why would it be a boolean option then?
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.
Because this should be possible in the future and I'd like to avoid another breaking change. In general, I feel like this will make things smoother
Type: task
Split out of #1827
Requires matrix-org/matrix-react-sdk#6781
Notes: This PR removes
setMatrixCallAudioInput()
andsetMatrixCallVideoInput()
in favour ofMediaHandler::setAudioInput()
andMediaHandler::setVideoInput()
. TheMediaHandler
can be accessed through theMatrixClient
usingMatrixClient::getMediaHandler()
This change is marked as an internal change (Task), so will not be included in the changelog.