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

Change media devices mid-call #6935

Closed
wants to merge 1 commit into from

Conversation

robertlong
Copy link

@robertlong robertlong commented Oct 12, 2021

Companion to matrix-org/matrix-js-sdk#1977 in which we add support for changing media devices mid-call. In the matrix-react-sdk this requires a one line change which updates the stream in the VideoFeed when it changes.


Here's what your changelog entry will look like:

✨ Features

Preview: https://6165fcc949fb1e009f8278fd--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.

@robertlong robertlong requested a review from a team as a code owner October 12, 2021 21:19
@robertlong robertlong added T-Task Refactoring, enabling or disabling functionality, other engineering tasks T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements and removed T-Task Refactoring, enabling or disabling functionality, other engineering tasks labels Oct 12, 2021
Copy link
Contributor

@SimonBrandner SimonBrandner left a comment

Choose a reason for hiding this comment

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

Some methods of MediaHandler are now async, could we please also update the ones in MediaDeviceHandler to be async too?

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 lgtm

@SimonBrandner - updating the class to be async feels like a different PR's concern? If we're not using the async functionality in the react-sdk, it's fine to keep it sync.

@SimonBrandner
Copy link
Contributor

this lgtm

@SimonBrandner - updating the class to be async feels like a different PR's concern? If we're not using the async functionality in the react-sdk, it's fine to keep it sync.

I guess so, though it would be nice to do it before we (or I at least) forget they're async :D

@turt2live
Copy link
Member

Your IDE settings may need tuning so it can warn you.

image

@SimonBrandner
Copy link
Contributor

Your IDE settings may need tuning so it can warn you.

True though the react-sdk class has wrapper functions which are not async which the IDE won't help with

@turt2live
Copy link
Member

@robertlong fyi we don't merge people's changes for them if they have commit access to the repo, just in case there's sensitivity to things landing. This appears to be open still - was it intending to be merged?

@SimonBrandner
Copy link
Contributor

@robertlong fyi we don't merge people's changes for them if they have commit access to the repo, just in case there's sensitivity to things landing. This appears to be open still - was it intending to be merged?

IIRC, @robertlong has mentioned there were some unresolved concerns with this. Though I would appreciate this moving forward as #7173 is blocked on this

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants