Skip to content
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

Restore Video Call Visualizer on enqueuing if it is running #1189

Open
wants to merge 1 commit into
base: feature/entry-widget-and-secure-conversations-v2
Choose a base branch
from

Conversation

ykyivskyi-gl
Copy link
Contributor

What was solved?
This PR restores Video Call Visualizer on enqueuing if it is running

Release notes:

  • Feature
  • Ignore
  • Release notes (Is it clear from the description here?)
  • Migration guide (If changes are needed for integrator already using the SDK - what needs to be communicated? Add underneath please)

Additional info:

  • Is the feature sufficiently tested? All tests fixed? Necessary unit, acceptance, snapshots added? Check that at least new public classes & methods are covered with unit tests
  • Did you add logging beneficial for troubleshooting of customer issues?
  • Did you add new logging? We would like the logging between platforms to be similar. Refer to Logging from iOS SDKsThings to consider for newly added logs in Confluence for more information.

Copy link
Contributor

@EgorovEI EgorovEI left a comment

Choose a reason for hiding this comment

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

From the added logic I see that if there is ongoing CV without video and resolveEngagementState is called, snackbar will not be shown. But our diagrams requires this. But maybe I missed something.
It would also be good to add unit test for showing snackbar in this case. If it is not added yet.

@ykyivskyi-gl
Copy link
Contributor Author

@EgorovEI It seems that it's either restoring Video or snackbar. handleOngoingCallVisualizer checks if it's video CV so try to restore Video if it is chat/audio/messaging - show snackbar. Here is the test that checks if snackbar is showing

func test_testEnqueuingWhenCallVisualizerIsActiveShouldShowSnackbar() throws {

Screenshot 2025-01-20 at 17 36 09

@EgorovEI
Copy link
Contributor

@EgorovEI It seems that it's either restoring Video or snackbar. handleOngoingCallVisualizer checks if it's video CV so try to restore Video if it is chat/audio/messaging - show snackbar. Here is the test that checks if snackbar is showing

func test_testEnqueuingWhenCallVisualizerIsActiveShouldShowSnackbar() throws {

Screenshot 2025-01-20 at 17 36 09

handleOngoingCallVisualizer checks if videoCall engagement kind was initiated by EngagementLauncher, not existing CV video. So If there is non-video CV and videoCall is chosen, then it tries to restore CV video screen and fails, because engagement.mediaStreams.video is nil.

@ykyivskyi-gl ykyivskyi-gl force-pushed the MOB-3948-open-video-for-ongoing-CV branch from 192a4bd to 505d157 Compare January 22, 2025 13:10
@sm-deployer sm-deployer force-pushed the MOB-3948-open-video-for-ongoing-CV branch from 505d157 to 7e9fc2f Compare January 22, 2025 13:10
@sm-deployer sm-deployer force-pushed the MOB-3948-open-video-for-ongoing-CV branch from 7e9fc2f to ae8cb19 Compare January 22, 2025 13:11
@ykyivskyi-gl
Copy link
Contributor Author

@EgorovEI basically was right that there is an issue with restoring non video CV engagement with video engagement type selected. During fixing this issue I decided to add a test for every restoring CV engagement case(audio, chat, video engagement) it may be a bit redundant but will keep us safe as there is different behavior for video and non-video engagements.

Thanks Egor!

)
throw GliaError.callVisualizerEngagementExists
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As Android team does not throw error here and it there is no sense to do it we decided to align it between platforms.

engagement: Engagement,
snackBarStyle: Theme.SnackBarStyle
) {
if engagementKind == .videoCall && engagement.mediaStreams.video != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not come up with better solution so these checks happen twice: here and in Call Visualizer. It has to be done inside Call Visualizer as it is internal method and may be used somewhere else.

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

Successfully merging this pull request may close these issues.

3 participants