-
Notifications
You must be signed in to change notification settings - Fork 111
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
Stop delaying ElementCall until the next sync loop and only notify other participants when presumed to already be up to date. #3559
Conversation
…her participants when presumed to already be up to date.
Generated by 🚫 Danger Swift against 833fa7d |
❌ 1 Tests Failed:
View the full list of 1 ❄️ flaky tests
To view more test analytics, go to the Test Analytics Dashboard |
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.
Makes sense to me 📲
// Here we assume that the app is running and the call state is already up to date | ||
presentCallScreen(roomProxy: roomProxy, notifyOtherParticipants: !roomProxy.infoPublisher.value.hasRoomCall) |
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.
FWIW, I think it would signal the intent more clearly if notifyOtherParticipants
was passed up in the action, having its value derived from context.viewState.hasOngoingCall
in the room screen. WDYT?
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.
Might as well yeah 👍
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.
Okay no, I changed my mind. This would need to be passed through from the room, the room details and the user profile screens. I don't think it's worth it. The UserSessionFlowCoordinator
is perfectly capable of figuring out what's right here.
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.
Oh interesting, did think about that 👍
ElementX/Sources/FlowCoordinators/UserSessionFlowCoordinator.swift
Outdated
Show resolved
Hide resolved
Quality Gate passedIssues Measures |
We implementing ringing we observed that sometimes notifications were being sent multiple times, causing multiple incoming calls to appear. The reason for that was that the app wasn't necessarily up to date and the call member list wes still unknown.
The fix was to delay the starting of the call until the next sync loop but that in turn made entering an ElementCall slow. We are now reverting that and only, intentionally, sending a notification if the user presses the button and a call is not running yet. We're assuming that the app is already up to date at that point and that the call state is correctly known.
Relates to element-hq/voip-internal/issues/259