Use web audio api for bell sound #1200
Conversation
src/utils/Sounds.ts
Outdated
| @@ -7,4 +7,4 @@ | |||
| // This sound is released under the Creative Commons Attribution 3.0 Unported | |||
| // (CC BY 3.0) license. It was created by 'altemark'. No modifications have been | |||
| // made, apart from the conversion to base64. | |||
| export const BellSound = 'data:audio/wav;base64,UklGRigBAABXQVZFZm10IBAAAAABAAEARKwAAIhYAQACABAAZGF0YQQBAADpAFgCwAMlBZoG/wdmCcoKRAypDQ8PbRDBEQQTOxRtFYcWlBePGIUZXhoiG88bcBz7HHIdzh0WHlMeZx51HmkeUx4WHs8dah0AHXwc3hs9G4saxRnyGBIYGBcQFv8U4RPAEoYRQBACD70NWwwHC6gJOwjWBloF7gOBAhABkf8b/qv8R/ve+Xf4Ife79W/0JfPZ8Z/wde9N7ijtE+wU6xvqM+lb6H7nw+YX5mrlxuQz5Mzje+Ma49fioeKD4nXiYeJy4pHitOL04j/jn+MN5IPkFOWs5U3mDefM55/ogOl36m7rdOyE7abuyu8D8Unyj/Pg9D/2qfcb+Yn6/vuK/Qj/lAAlAg=='; | |||
| export const BellSound = 'UklGRigBAABXQVZFZm10IBAAAAABAAEARKwAAIhYAQACABAAZGF0YQQBAADpAFgCwAMlBZoG/wdmCcoKRAypDQ8PbRDBEQQTOxRtFYcWlBePGIUZXhoiG88bcBz7HHIdzh0WHlMeZx51HmkeUx4WHs8dah0AHXwc3hs9G4saxRnyGBIYGBcQFv8U4RPAEoYRQBACD70NWwwHC6gJOwjWBloF7gOBAhABkf8b/qv8R/ve+Xf4Ife79W/0JfPZ8Z/wde9N7ijtE+wU6xvqM+lb6H7nw+YX5mrlxuQz5Mzje+Ma49fioeKD4nXiYeJy4pHitOL04j/jn+MN5IPkFOWs5U3mDefM55/ogOl36m7rdOyE7abuyu8D8Unyj/Pg9D/2qfcb+Yn6/vuK/Qj/lAAlAg=='; | |||
There was a problem hiding this comment.
We would need ti handle the data uri case otherwise this represents an API change and would need to wait for v4
src/Terminal.ts
Outdated
| @@ -1797,7 +1794,19 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT | |||
| */ | |||
| public bell(): void { | |||
| this.emit('bell'); | |||
| if (this.soundBell()) this.bellAudioElement.play(); | |||
| if (this.soundBell()) { | |||
| if (!this.audioContext) { | |||
There was a problem hiding this comment.
We need feature detection just in case. Browser support seems pretty good so I'm fine with dropping the DOM-based sound loading https://caniuse.com/#feat=audio-api
| @@ -1797,7 +1794,19 @@ export class Terminal extends EventEmitter implements ITerminal, IInputHandlingT | |||
| */ | |||
| public bell(): void { | |||
| this.emit('bell'); | |||
| if (this.soundBell()) this.bellAudioElement.play(); | |||
| if (this.soundBell()) { | |||
There was a problem hiding this comment.
I think it would be a good idea to move all the sound-related code (including base64ToArrayBuffer) into it's own file now that the DOM interactions are gone.
|
I refactored the code a bit. For the feature detection, it's only printing a message to the console, should i do something else? As for the typings, even after updating the ts version, they still don't seem to be working.. I'll try a few more things when i have more time 馃槈 |
|
@dgriffen FYI this PR will drop terminal bell support for IE11, I don't think you're even using it yet 馃槂 |
|
@nikonso I merged from master resolving the conflicts and then fixed some lint/style problems. Could you give this a once over to make sure everything looks good before I merge this in? |
|
@Tyriar Everything seems to be Ok 馃憤 Except there are a bunch of changes in the package-lock.json, are they Ok to merge in? |
|
Yeah I think I accidentally added that, I think we need to add the file to .gitignore or something. Need to do some reading on best practices. |
Implementation of the feature requested in #1181
Feedback on what to change is welcome! 馃槃
Btw: the typings aren't recognizing AudioContext || webkitAudioContext