Skip to content

Catch the permission exception on the audio component#2330

Merged
pngwn merged 8 commits into
gradio-app:mainfrom
Ian-GL:igl-fix-catch-audio-permission-err
Sep 29, 2022
Merged

Catch the permission exception on the audio component#2330
pngwn merged 8 commits into
gradio-app:mainfrom
Ian-GL:igl-fix-catch-audio-permission-err

Conversation

@Ian-GL
Copy link
Copy Markdown
Contributor

@Ian-GL Ian-GL commented Sep 24, 2022

Description

Currently the Audio component fails silently if the microphone permissions were not granted and the UI seems as if it was recording. This fix catches the permission exception and raises an alert that tells the user the permissions are needed. It also prevents the UI from showing the stop button as if it had started recording.

Screen Shot 2022-09-24 at 9 51 50

Closes: #2325

Checklist:

  • I have performed a self-review of my own code
  • My code follows the style guidelines of this project
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@abidlabs
Copy link
Copy Markdown
Member

Thanks @Ian-GL for the fix! This looks pretty good, but I wonder if it would be possible to use Gradio's Error instead of alert() for a better UI? cc @aliabid94

@Ian-GL
Copy link
Copy Markdown
Contributor Author

Ian-GL commented Sep 26, 2022

Thanks @Ian-GL for the fix! This looks pretty good, but I wonder if it would be possible to use Gradio's Error instead of alert() for a better UI? cc @aliabid94

Hey @abidlabs , I wasn't aware of Gradio's Error. Yes, if you have an example of another component that uses it I could use that as a guide to add it here.

Comment thread ui/packages/audio/src/Audio.svelte Outdated
Comment on lines +60 to +74
const stream = await navigator.mediaDevices
.getUserMedia({ audio: true })
.then((stream) => {
return stream;
})
.catch((err) => {
if (err instanceof DOMException && err.name == "NotAllowedError") {
alert(
"Please allow the use of microphone if you wish to record audio."
);
return null;
} else {
throw err;
}
});
Copy link
Copy Markdown
Member

@pngwn pngwn Sep 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't be mixing await and then chaining. Lets wrap this in a try catch block instead.

Additionally should we think of something less intrusive than an alert which blocks all interaction with the page? cc @gary149 @abidlabs

@aliabid94
Copy link
Copy Markdown
Contributor

I'll make a PR against this fork to launch a gradio.Error from within a component - we don't have any examples of doing this before so I'll take care of that.

@aliabid94 aliabid94 requested a review from pngwn September 29, 2022 04:01
@aliabid94
Copy link
Copy Markdown
Contributor

aliabid94 commented Sep 29, 2022

Just made the changes to use gradio error messaging (and added errors to webcam as well), @pngwn please rereview

Screen Shot 2022-09-28 at 9 02 00 PM

@pngwn
Copy link
Copy Markdown
Member

pngwn commented Sep 29, 2022

Pushed a change to convert the chained promises to try catch style, so we aren't mixing await + promise chaining, and fixed and issue with the type.

Looks good, thanks @Ian-GL and @aliabid94!

@pngwn
Copy link
Copy Markdown
Member

pngwn commented Sep 29, 2022

Will merge when CI passes with everything merged in.

@pngwn pngwn merged commit 662a811 into gradio-app:main Sep 29, 2022
@Ian-GL
Copy link
Copy Markdown
Contributor Author

Ian-GL commented Sep 29, 2022

Thanks for taking care of the changes, @pngwn and @aliabid94 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Microphone cannot be stopped after starting without permissions

4 participants