-
Notifications
You must be signed in to change notification settings - Fork 3
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
Support the Repl.it Audio messages #3
Conversation
This change: a) Adds a button to toggle the audio stream. This is needed since most browsers require the audio play action to be run in a user event handler, so having the toggle audio be an explicit button achieves that (and is also to avoid annoying users). b) Drops support for the QEMU audio extension and instead uses the Repl.it Audio messages, which allows for explicit negotiation for audio compression. c) Makes the audio library more robust and now works in Chrome and Firefox.
This PR is getting big. |
note: the failures are somewhat expected since the CI setup in this fork isn't quite the same as the one upstream. |
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.
🔉 Seems pretty legit to me
document.getElementById('noVNC_audio_button') | ||
.classList.remove("noVNC_hidden"); | ||
document.getElementById('noVNC_audio_button') | ||
.classList.remove("noVNC_selected"); |
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.
How often is updateCapabilities
called? Should we always remove this selected class?
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.
at most four times per connection: once after the connection is established (to clear everything), once when the power capabilities are advertised by the server, once when the server says that it supports audio (only once if the user opted in), and once more when the connection is lost (to clean everything again for good measure).
it should be roughly fine to always attempt to remove the selected class since classList.remove()
is a no-op if the class is not there, and other UI functions follow roughly the same pattern (like updatePowerButton()
, another funciton that's called from updateCapabilities()
).
which is a very long way of saying i guess it's fine?
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.
if they do this elsewhere, sounds good to me haha!
unbooping: approved |
This change:
a) Adds a button to toggle the audio stream. This is needed since most
browsers require the audio play action to be run in a user event
handler, so having the toggle audio be an explicit button achieves
that (and is also to avoid annoying users).
b) Drops support for the QEMU audio extension and instead uses the
Repl.it Audio messages, which allows for explicit negotiation for
audio compression.
c) Makes the audio library more robust and now works in Chrome and
Firefox.
(cherry-pick of novnc#1525)