-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
Play a sound whenever a breakpoint is hit. #140044
Conversation
a0f4f34
to
7e6d71d
Compare
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.
Awesome work!
I have done an initial review. Once you tackle this I can try it out and provide more feedback.
Thanks a lot
src/vs/workbench/services/soundNotification/browser/audioNotificationService.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/services/soundNotification/browser/audioNotificationService.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/services/soundNotification/browser/audioNotificationService.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/services/soundNotification/browser/audioNotificationService.ts
Outdated
Show resolved
Hide resolved
@@ -913,6 +915,10 @@ export class DebugSession implements IDebugSession { | |||
})); | |||
|
|||
this.rawListeners.push(this.raw.onDidStop(async event => { | |||
if (event.body.reason === 'breakpoint') { |
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.
The current problem with the breakpoints is not that the users are unaware that the breakpoint is hit, but that the breakpoint is on a line.
So the logic should be different.
We need a line change listener, and on each line change to check if a breakpoint is on a line. If it is we play the sound. We will do the same approach with errors.
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.
I suggest to introduce a new minimalistic WorkbenchContribution
just for this
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.
Ah, I misunderstood the problem. But #135908 is about playing a sound when a breakpoint is hit.
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.
@hediet good point. Yeah I then agree we also want this.
I feel like this is mostly requested by non screen reader users. And what I mention is requested by screen reader users.
So in the end we will need both, and the sound we play for those might be different. Though to start we can play the same sound.
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.
I believe this would not work from our real builds that use a stripped down version of ffmpeg
, i.e. we ship no codecs, @deepak1556 would know for sure.
src/vs/workbench/services/soundNotification/browser/audioNotificationService.ts
Outdated
Show resolved
Hide resolved
I think his comment here means webm should be supported. |
I think a service for this is overkill, why not |
Yup |
7e6d71d
to
fdf9147
Compare
fdf9147
to
675014e
Compare
This PR fixes #135908
Use
"audioNotifications.enabled": true
to enable the feature.Use
"audioNotifications.breakpointHit": false
to enable audio cues for breakpoints.Later TODO's: