-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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
Implement Audio cues on cell execution completed #165442
Implement Audio cues on cell execution completed #165442
Conversation
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.
Thanks for tackling this! Looks fine, but it makes me wonder- if you click "Run all" (or run above/below, or select a range and run...) should it sound the audio cue for every cell individually, or just once at the end of the "batch"? I don't know
cc @meganrogge for that question, not sure whether there is a similar concept anywhere else that uses audio cues
Good point! I didn't think about it. I would lean more toward one sound per batch because it can be annoying when you have multiple small cells. On the other hand, having a sound for each cells could provide more informative feedback (I'm thinking of the possibility of counting the cues for knowing which cell is currently being executed). There is also the option of having the user deciding it with an additional setting. |
@roblourens we sort of encountered this with the build task audio cues and decided to just play one sound at a time instead of capturing the requests and playing them sequentially. #164921 Do the cells get run in parallel? I imagine they don't necessarily finish in order, so it could be pretty confusing for the user if multiple sounds play... for example: Cell 1 Cell 2 succeeds Could sound misleading given the order the cells appear in is not consistent w the order in which the noise gets played. |
I've changed the behavior of the audio cues. Now the successful execution cue is played on the last cell scheduled, while the failed one always plays whenever a cell fails, since a failed execution interrupts the all the remaining scheduled ones. |
src/vs/workbench/contrib/notebook/browser/services/notebookExecutionStateServiceImpl.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/contrib/notebook/browser/services/notebookExecutionStateServiceImpl.ts
Outdated
Show resolved
Hide resolved
9b3ebee
to
7ad0f0c
Compare
I'm using the The checks for the 2 cues are done at the level of the audioCueService: vscode/src/vs/workbench/contrib/audioCues/browser/audioCueService.ts Lines 42 to 46 in b982536
So to bypass it the options are:
There is also the option to scrap the new setting completely since cell execution can be considered a "task", and, probably users who turn on the options would likely want to have cues played for notebooks. What do you think? I'm open to every option. |
A notebook cell is not a task. Looking at the code, there are more to audio cues than I thought- the notebook AudioCues should be registered with the others here: so that they should show up in the "List Audio Cues" command like the others. This isn't duplicating stuff- notebook cell execution is its own feature and should be listed and controlled independently. |
I've added the audioCues and removed the old setting. The settings for the cues will appear as |
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.
Love it, thanks!
Fixes #165085
This PR implement an option in the settings
notebook.audioCuesEnabled
that enables audio cues on cell execution both in case of success and in case of failure.