fix(plugin-meetings): Windows/Chrome still asks for camera permissions even when not selecting videoEnabled and shareVideoEnabled #3831
Conversation
Parimala032
changed the title
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
Parimala032
changed the title
| const deviceKind = | ||
| (isVideoEnabled && Media.DeviceKind.VIDEO_INPUT) || | ||
| (isAudioEnabled && Media.DeviceKind.AUDIO_INPUT) || | ||
| undefined; |
There was a problem hiding this comment.
@mkesavan13 Im unable to understand this logic. When isVideoEnabled is false and isAudioENabled is false, then shouldnt deviceKind = undefined. And that should call getDevice(undfiened) -> which means we ask for both the permissions.
There was a problem hiding this comment.
@Parimala032 can answer to this. Logically that is how it is supposed to work, yes
| @@ -4196,6 +4200,8 @@ describe('plugin-meetings', () => { | |||
| // and that these were the only /media requests that were sent | |||
| assert.calledTwice(locusMediaRequestStub); | |||
| assert.calledOnce(handleDeviceLoggingSpy); | |||
| assert.calledWith(handleDeviceLoggingSpy, false); | |||
|
|
|||
| }); | |||
|
|
|||
There was a problem hiding this comment.
Please add test for the following conditions
- Only asked for camera permission
- Only asked for mic permission
- Ask for both permissions
- Ask for neither permissions.
There was a problem hiding this comment.
Please change the title to reflect only the change this commit is doing. The PR description can cover the rest of the details. This title is quite long.
This pull request addresses the issue with a kitchen sink app.
Isn't this an issue with the web client too?
Changes have been made in the plugin-meetings
Could we be more specific about what the changes were inside plugin meetings? I don't see anything about the function that actually had the problem.
Please also add a root cause to the PR description.
| * @returns {Promise<void>} | ||
| */ | ||
| private static async handleDeviceLogging(): Promise<void> { | ||
| private static async handleDeviceLogging( | ||
| isAudioEnabled = false, |
There was a problem hiding this comment.
Why do we need to set default values for these booleans?
| try { | ||
| const devices = await getDevices(); | ||
| const deviceKind = | ||
| (isVideoEnabled && Media.DeviceKind.VIDEO_INPUT) || |
There was a problem hiding this comment.
Does enabling video automatically ask for audio too?
What's the reason for undefined being present here?
There was a problem hiding this comment.
Please address @sreenara & @Shreyas281299 comments. Meanwhile, we can have tests written and demo recorded for following cases as well,
- Video is enabled and Audio disabled
- Both video and audio enabled
- Both video and audio disabled
COMPLETES #< SPARK-539683 >
This pull request addresses
This pull request addresses the issue with a kitchen sink app. When the user tries to disable the videoEnabled checkbox and sharVideoEnabled checkbox, the sample app still asks for camera access. This pull request resolves this problem with the following changes:
by making the following changes
Changes have been made in the plugin-meetings where previously getDevices() was being called without considering the user's usage. Now, getDevices is being called with parameters based on the user's requests.
PR.3831.mp4
Change Type
The following scenarios where tested
Tested this Pull request by
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.