Add volume control to screen shares#3747
Add volume control to screen shares#3747JakeTripplJ wants to merge 10 commits intoelement-hq:livekitfrom
Conversation
…MediaViewModel Signed-off-by: Jake Janicke <jaketripplj@gmail.com>
…king reliably with ScreenShareViewModel Signed-off-by: Jake Janicke <jaketripplj@gmail.com>
Signed-off-by: Jake Janicke <jaketripplj@gmail.com>
Signed-off-by: Jake Janicke <jaketripplj@gmail.com>
Signed-off-by: Jake Janicke <jaketripplj@gmail.com>
|
Thanks for the contribution. |
Signed-off-by: Jake Janicke <jaketripplj@gmail.com>
Signed-off-by: Jake Janicke <jaketripplj@gmail.com>
Signed-off-by: Jake Janicke <jaketripplj@gmail.com>
Signed-off-by: Jake Janicke <jaketripplj@gmail.com>
|
If the playwright CI test fails again then I'll have to host a backend server to actually properly run the tests locally and see what it's getting hung up on. It seems like playwright tries clicking the spotlight view button after entering a call as part of a test and nothing happens, although it works fine when I perform the same sequence on the Netlify-hosted PR version. Almost all of my local testing was done on the embedded version through element-web, so maybe it's an issue that only shows up on the non-embedded version. Let me know if you have any ideas/suggestions. |
There was a problem hiding this comment.
Really glad to have this feature contributed, it's giving us the motivation now to properly work out what the UI for these extra screen share controls should look like. Stay tuned as there are some changes we'd like to request to have it work more similarly to the volume controls on normal user tiles - but I need to finalize that with our design team first.
You don't actually have to run the Playwright tests yourself to see how they failed: You can download the playwright-report.zip artifact from the CI run here, extract it, and then serve it over HTTP to access the "trace viewer" which will show you screenshots, logs, and everything.
src/tile/SpotlightTile.tsx
Outdated
| const currentMedia = media[visibleIndex]; | ||
| const isScreenShare = currentMedia instanceof ScreenShareViewModel; | ||
| const hasAudio$ = useBehavior(currentMedia.audioEnabled$); |
There was a problem hiding this comment.
In this case the Playwright tests are crashing with: TypeError: Cannot read properties of undefined (reading 'audioEnabled$'), which indicates that currentMedia can be undefined here. (The test switches to spotlight layout so quickly that even the user's own tile hasn't yet appeared in the interface!)
|
Sorry that the "Publish SwiftPM Library" action is failing by the way, you can ignore it. |
Signed-off-by: Jake Janicke <jaketripplj@gmail.com>
|
I didn't know that the CI served a report with actual traces, I was just going off of the logs it spat out (which weren't very descriptive). That makes things much easier to debug, thanks. As for the actual UI for the slider, I was initially going to put it in its own context menu like the user tiles, but I ended up just going down the easier route of putting it right on the tile itself (which was ironically harder to get working due to finding out the hard way that onPointerUp was required to commit the value). The actual volume control logic for ScreenShareViewModel should still be useful in either case though. Let me know what everyone eventually decides on for the actual UX, I can always tweak it if needed. |
Fixes #2337
This PR adds volume control and muting capabilities to remote screen shares using the built-in slider wrapper to maintain a cohesive look for the UI. The slider only shows up in spotlight view, and also only shows up when a screen share includes audio. It also remembers the previous volume level upon toggling mute by clicking the speaker icon.