-
Notifications
You must be signed in to change notification settings - Fork 28.4k
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
Add AccessibilitySignal.terminalCommandSucceeded
and success.mp3
(issue #178989)
#204430
Add AccessibilitySignal.terminalCommandSucceeded
and success.mp3
(issue #178989)
#204430
Conversation
@microsoft-github-policy-service agree |
1 similar comment
@microsoft-github-policy-service agree |
@Maximetinu thanks for this. we cannot use externally provided sounds. We have a sound designer, @AFre100, that we use for such things. |
audioCues.terminalCommandSucceded
and success.mp3
(issue #178989)
Why was this closed, can't the sound just be replaced? This is the second PR opened for this (the first being #204408) and the work is essentially done but it's not's not clear what the next step is for this. Will a sound be created by the Code team? (FWIW, I think the sound in the video above really complements the existing failure sound 🙂) |
@meganrogge let's use this PR as a base regardless of whether we use the new sound of not. I'm not sure if there are rules around accepting external sounds but we can verify that, if we can accept it and plan on replacing it then we could merge this and mark it as experimental and then swap out the sound when it's ready. |
4f06511
to
747258b
Compare
@Tyriar @meganrogge It's very cool that someone made a sound externally! I see the logic of its design :) Unfortunately (booo), no matter how great it might be, rule of thumb is to never ship external sound assets in this manner due to multiple risks (legal, IP, copyright, accessibility, UX, brand, etc). Re marking this experimental -- this means including it in Insiders, right? It'd be beneficial to provide temporary placeholder sound(s), so long as it's ok to replace w/a different final sound in the future. If possible, we always avoid confusing users if there's a chance they'll get used to sound 1, only to have it changed to sound 2. This is particularly sensitive for audio assets, and has caused issues in the past. I'm pretty sure (?) it happened due to use in a stable build, not a short lived Insiders or testing situation -- happy to zero in on the parameters with you all more. (Btw, very happy to discuss any of this on a call if useful!). Couple things:
|
@AFre100 Thank you for the kind words about it, we really thinked about how to make a well implemented sound for vscode (firstly I tried to make some from scratch). Have a nice week you all! :) |
@JuanjeMC Appreciate that info and figured as such -- awesome design thinking and execution, no doubt. Unfortunately, as absurd as it sounds, that mitigates only a fraction of the many existing legal and copyright liabilities for a situation like this. Extremely punk rock and open source vibes, I know. Thanks again for doing a cool thing, and we will definitely consider that design concept. (It's also cool to see it mocked up because, crucially, it demonstrates the likelihood of rapid repetition.) cc @meganrogge @Tyriar sent you all and email about this :) |
@meganrogge et all, I'm thinking that the audio cue for Success might make more sense as a globally/generically usable "action complete" sound, which could cover other interactions that come up. This could reduce cognitive load, things to learn, clutter overall.
With the right sounds, how do you feel about Save being a use case of Action Complete, or do you feel it needs to be dedicated? |
@AFre100 I think we should use a different sound because that lessens cognitive burden. Save is an action completed by the user or the workbench (autosave) whereas terminal completion is dictated by the shell. |
great control flow call out -- thank you @meganrogge for setting the record straight! Totally agree there. And what do you think about two sounds: 1) global user-initiated success/completion (Save etc) and 2) global system-initiated/controlled success/completion (terminalCommandSucceeded etc)? |
@AFre100 what's the timeline here? |
@meganrogge I had to take off early today — I'll send an update on Monday :) |
@meganrogge In time for May's Insiders would be great — so what would that be, final by May 17? |
Yes @AFre100 |
Hello, can I do something to help moving this forward? Should I fix the conflicts with main? I've been using my own fork to have this feature for a while and I couldn't live without it now, thx for making it happen 👏 ! |
@Maximetinu thanks for asking, yes, if you could fix the conflicts, that would help. @AFre100 what's the status ? |
@meganrogge We're on track for a May 17 final delivery for Insiders. Will ping you offline early next week (possibly earlier) to align! |
@Maximetinu if you'll fix the conflicts, I have the sound we want to use, so we can likely get this in for May. |
93817c1
to
12e3f8c
Compare
audioCues.terminalCommandSucceded
and success.mp3
(issue #178989) AccessibilitySignal.terminalCommandSucceeded
and success.mp3
(issue #178989)
Co-Authored-By: JuanjeMC <101800960+juanjemc@users.noreply.github.com>
12e3f8c
to
7c30ba4
Compare
Done @meganrogge, conflicts resolved! 👌 Let me know if you need anything else, and feel free to make any modifications (you may have to replace the |
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.
Thank you! I'm seeing a permission issue when I try to push to your branch, so will merge the sound we want in the next PR.
@Maximetinu , I'm unable to push to your branch due to permissions issues. Could you pls fix the conflict, IE accept the incoming sound that I added, so this can be merged? Thanks! |
@meganrogge done! 👍 |
Corresponds to #178989 in Feature request.
This PR adds the ability to play a sound when a terminal command finishes with a
0
exit code, which is a useful feature for both multitasking and accessibility, like its similar, already existing, settingaudioCues.terminalCommandFailed
, which was added at PR #174621In the previous PR (#204408) I was using
terminalBell.mp3
:I'm using theterminalBell.mp3
sound because it feels the most succeed to me. It's also very similar to other OS terminal bells (on MacOS, when pressingCtrl+G
on the terminal you will hear it)I think it is ok to use this in 2 different places because they are very well differentiated.Actually, on a second thought, I also think it would be wrong to reuse the terminal bell sound because it means user mistyped (thx @meganrogge for the feedback)
So I've asked a friend to join the contribution and create a new
success.mp3
SFX for this 😎 (thx @JuanjeMC)We put special attention to make it similar to the already existing
warning.mp3
anderror.mp3
SFXs, so it sounds well integrateddemo.mov