Skip to content

Dismiss video effects error when camera is off #3137

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.

Sign up for GitHub

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

Merged
prabhjot-msft merged 8 commits into from
Jun 8, 2023
Merged

Dismiss video effects error when camera is off #3137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
01f791d
Dismiss video effects error when camera is off
prabhjot-msft Jun 1, 2023
83401fa
Merge remote-tracking branch 'origin' into prabhjot/dismiss-effects-e…
prabhjot-msft Jun 1, 2023
2e18194
Change files
prabhjot-msft Jun 1, 2023
7b5b6de
Duplicate change files for beta release
prabhjot-msft Jun 1, 2023
193cd42
Merge branch 'main' into prabhjot/dismiss-effects-error
prabhjot-msft Jun 7, 2023
34c03d6
Merge branch 'main' into prabhjot/dismiss-effects-error
prabhjot-msft Jun 8, 2023
6c4510c
Merge branch 'main' into prabhjot/dismiss-effects-error
prabhjot-msft Jun 8, 2023
cffd285
Merge branch 'main' into prabhjot/dismiss-effects-error
prabhjot-msft Jun 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions change-beta/@azure-communication-react-dd247789-2124-4327-bacc-c5d1bcc70f94.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "Dismiss video effects error when camera is off",
"packageName": "@azure/communication-react",
"email": "97124699+prabhjot-msft@users.noreply.github.com",
"dependentChangeType": "patch"
}
7 changes: 7 additions & 0 deletions change/@azure-communication-react-dd247789-2124-4327-bacc-c5d1bcc70f94.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "prerelease",
"comment": "Dismiss video effects error when camera is off",
"packageName": "@azure/communication-react",
"email": "97124699+prabhjot-msft@users.noreply.github.com",
"dependentChangeType": "patch"
}
7 changes: 6 additions & 1 deletion packages/react-composites/src/composites/CallComposite/components/CallArrangement.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ import { getPipStyles } from '../../common/styles/ModalLocalAndRemotePIP.styles'
import { useMinMaxDragPosition } from '../../common/utils';
import { MobileChatSidePaneTabHeaderProps } from '../../common/TabHeader';
import { CommonCallControlOptions } from '../../common/types/CommonCallControlOptions';
/* @conditional-compile-remove(video-background-effects) */
import { localVideoSelector } from '../../CallComposite/selectors/localVideoStreamSelector';

/**
* @private
Expand Down Expand Up @@ -218,6 +220,9 @@ export const CallArrangement = (props: CallArrangementProps): JSX.Element => {

let errorBarProps = props.errorBarProps;

/* @conditional-compile-remove(video-background-effects) */
const isCameraOn = useSelector(localVideoSelector).isAvailable;

/* @conditional-compile-remove(rooms) */
// TODO: move this logic to the error bar selector once role is plumbed from the headless SDK
if (!rolePermissions.cameraButton && props.errorBarProps) {
Expand All @@ -230,7 +235,7 @@ export const CallArrangement = (props: CallArrangementProps): JSX.Element => {
}

/* @conditional-compile-remove(video-background-effects) */
if (useIsParticularSidePaneOpen('videoeffects') && props.errorBarProps) {
if ((useIsParticularSidePaneOpen('videoeffects') || !isCameraOn) && props.errorBarProps) {
Copy link
Copy Markdown
Member

@JamesBurnside JamesBurnside Jun 1, 2023

Choose a reason for hiding this comment

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

We haven't used this anywhere in the codebase yet, but instead of hiding the error when the camera is off, could we clear the error in the the handler for toggleCamera using newClearCallErrorsModifier?
image

Honestly no idea if that works just curious if its possible.

Copy link
Copy Markdown
Member

@JamesBurnside JamesBurnside Jun 1, 2023

Choose a reason for hiding this comment

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

The way its currently implemented, does the error re-show when the camera is turned back on?

Copy link
Copy Markdown
Contributor Author

@prabhjot-msft prabhjot-msft Jun 7, 2023

Choose a reason for hiding this comment

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

  • For 1st suggestion, using newClearCallErrorsModifier, this modifier is not actually exposed in the calling stateful layer. If we are planning to expose this modifier, it would be called in the stateful layer mostly in the call context by introducing new set errors method. We can trigger this method whenever we want to delete certain errors. I can pursue this as a separate task if we feel strongly about this approach.

  • For 2nd question, as part of the second change for auto-dismiss(Auto-dismiss video effect errors  #3160). Whenever camera is turned on, it will reset a new active effect and if the error is generated again it would be shown and otherwise no error will be shown.

Copy link
Copy Markdown
Contributor

@edwardlee-msft edwardlee-msft Jun 1, 2023

Choose a reason for hiding this comment

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

is this supposed to be !isCameraOn?
The title indicates that it should dismiss when camera is off, but this is setting an activemessage when camera is off.

I see that in videoEffectsPane.tsx line 149 new change it does what I expected.
Correct me if I'm wrong here.

Copy link
Copy Markdown
Contributor Author

@prabhjot-msft prabhjot-msft Jun 7, 2023

Choose a reason for hiding this comment

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

Yeah, so in this scenario, if videoeffectsSidepane is open and camera is off we are filtering out the video effects error from latest errors.
For videoeffectsPane, if the videoeffectsError exists and camera is on we are showing the error.

errorBarProps = {
...props.errorBarProps,
activeErrorMessages: props.errorBarProps.activeErrorMessages.filter((e) => e.type !== 'unableToStartVideoEffect')
Expand Down
7 changes: 6 additions & 1 deletion packages/react-composites/src/composites/CallComposite/pages/ConfigurationPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ import { SidePane } from '../components/SidePane/SidePane';
import { SidePaneRenderer } from '../components/SidePane/SidePaneProvider';
/* @conditional-compile-remove(video-background-effects) */
import { useIsParticularSidePaneOpen } from '../components/SidePane/SidePaneProvider';
/* @conditional-compile-remove(video-background-effects) */
import { localVideoSelector } from '../../CallComposite/selectors/localVideoStreamSelector';

/**
* @private
Expand Down Expand Up @@ -103,6 +105,9 @@ export const ConfigurationPage = (props: ConfigurationPageProps): JSX.Element =>
/* @conditional-compile-remove(rooms) */
const rolePermissions = _usePermissions();

/* @conditional-compile-remove(video-background-effects) */
const isCameraOn = useSelector(localVideoSelector).isAvailable;

/* @conditional-compile-remove(rooms) */
// TODO: move this logic to the error bar selector once role is plumbed from the headless SDK
if (!rolePermissions.cameraButton) {
Expand All @@ -115,7 +120,7 @@ export const ConfigurationPage = (props: ConfigurationPageProps): JSX.Element =>
}

/* @conditional-compile-remove(video-background-effects) */
if (useIsParticularSidePaneOpen('videoeffects') && errorBarProps) {
if ((useIsParticularSidePaneOpen('videoeffects') || !isCameraOn) && errorBarProps) {
errorBarProps = {
...errorBarProps,
activeErrorMessages: errorBarProps.activeErrorMessages.filter((e) => e.type !== 'unableToStartVideoEffect')
Expand Down
2 changes: 1 addition & 1 deletion packages/react-composites/src/composites/common/VideoEffectsPane.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ const VideoEffectsPaneTrampoline = (
/* @conditional-compile-remove(video-background-effects) */
return (
<Stack horizontalAlign="center">
{videoEffectError && (
{videoEffectError && isCameraOn && (
<MessageBar messageBarType={MessageBarType.error} onDismiss={() => onDismissError(videoEffectError)}>
{locale.strings.call.unableToStartVideoEffect}
</MessageBar>
Expand Down
⚡