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.

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

Stop delaying ElementCall until the next sync loop and only notify other participants when presumed to already be up to date. #3559

Merged
merged 2 commits into from
Nov 27, 2024
Merged

Stop delaying ElementCall until the next sync loop and only notify other participants when presumed to already be up to date. #3559

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c1cdaf
Stop delaying ElementCall until the next sync loop and only notify ot…
stefanceriu Nov 26, 2024
833fa7d
Move `notifyOtherParticipants` to the `ElementCallConfiguration`.
stefanceriu Nov 27, 2024
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
1 change: 1 addition & 0 deletions ElementX/Sources/Application/AppCoordinator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -658,6 +658,7 @@ class AppCoordinator: AppCoordinatorProtocol, AuthenticationFlowCoordinatorDeleg
let callScreenCoordinator = CallScreenCoordinator(parameters: .init(elementCallService: elementCallService,
configuration: configuration,
allowPictureInPicture: false,
notifyOtherParticipants: false,
appHooks: appHooks))

callScreenCoordinator.actions
Expand Down
21 changes: 12 additions & 9 deletions ElementX/Sources/FlowCoordinators/UserSessionFlowCoordinator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ class UserSessionFlowCoordinator: FlowCoordinatorProtocol {
case .userProfile(let userID):
stateMachine.processEvent(.showUserProfileScreen(userID: userID), userInfo: .init(animated: animated))
case .call(let roomID):
Task { await presentCallScreen(roomID: roomID) }
Task { await presentCallScreen(roomID: roomID, notifyOtherParticipants: false) }
case .genericCallLink(let url):
presentCallScreen(genericCallLink: url)
case .settings, .chatBackupSettings:
Expand Down Expand Up @@ -579,7 +579,8 @@ class UserSessionFlowCoordinator: FlowCoordinatorProtocol {

switch action {
case .presentCallScreen(let roomProxy):
presentCallScreen(roomProxy: roomProxy)
// 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)
Comment on lines +582 to +583
Copy link
Member

@pixlwave pixlwave Nov 26, 2024 •
edited
Loading

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?

Copy link
Member Author

@stefanceriu stefanceriu Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Might as well yeah 👍

Copy link
Member Author

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.

Copy link
Member

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 👍

case .finished:
stateMachine.processEvent(.deselectRoom)
}
Expand Down Expand Up @@ -654,29 +655,30 @@ class UserSessionFlowCoordinator: FlowCoordinatorProtocol {
// MARK: Calls

private func presentCallScreen(genericCallLink url: URL) {
presentCallScreen(configuration: .init(genericCallLink: url))
presentCallScreen(configuration: .init(genericCallLink: url), notifyOtherParticipants: false)
}

private func presentCallScreen(roomID: String) async {
private func presentCallScreen(roomID: String, notifyOtherParticipants: Bool) async {
guard case let .joined(roomProxy) = await userSession.clientProxy.roomForIdentifier(roomID) else {
return
}

presentCallScreen(roomProxy: roomProxy)
presentCallScreen(roomProxy: roomProxy, notifyOtherParticipants: notifyOtherParticipants)
}

private func presentCallScreen(roomProxy: JoinedRoomProxyProtocol) {
private func presentCallScreen(roomProxy: JoinedRoomProxyProtocol, notifyOtherParticipants: Bool) {
let colorScheme: ColorScheme = appMediator.windowManager.mainWindow.traitCollection.userInterfaceStyle == .light ? .light : .dark
presentCallScreen(configuration: .init(roomProxy: roomProxy,
clientProxy: userSession.clientProxy,
clientID: InfoPlistReader.main.bundleIdentifier,
elementCallBaseURL: appSettings.elementCallBaseURL,
elementCallBaseURLOverride: appSettings.elementCallBaseURLOverride,
colorScheme: colorScheme))
colorScheme: colorScheme),
notifyOtherParticipants: notifyOtherParticipants)
stefanceriu marked this conversation as resolved.
Show resolved Hide resolved
}

private var callScreenPictureInPictureController: AVPictureInPictureController?
private func presentCallScreen(configuration: ElementCallConfiguration) {
private func presentCallScreen(configuration: ElementCallConfiguration, notifyOtherParticipants: Bool) {
guard elementCallService.ongoingCallRoomIDPublisher.value != configuration.callRoomID else {
MXLog.info("Returning to existing call.")
callScreenPictureInPictureController?.stopPictureInPicture()
Expand All @@ -686,6 +688,7 @@ class UserSessionFlowCoordinator: FlowCoordinatorProtocol {
let callScreenCoordinator = CallScreenCoordinator(parameters: .init(elementCallService: elementCallService,
configuration: configuration,
allowPictureInPicture: true,
notifyOtherParticipants: notifyOtherParticipants,
appHooks: appHooks))

callScreenCoordinator.actions
Expand Down Expand Up @@ -899,7 +902,7 @@ class UserSessionFlowCoordinator: FlowCoordinatorProtocol {
navigationSplitCoordinator.setSheetCoordinator(nil)
stateMachine.processEvent(.selectRoom(roomID: roomID, via: [], entryPoint: .room))
case .startCall(let roomID):
Task { await self.presentCallScreen(roomID: roomID) }
Task { await self.presentCallScreen(roomID: roomID, notifyOtherParticipants: false) }
case .dismiss:
navigationSplitCoordinator.setSheetCoordinator(nil)
}
Expand Down
2 changes: 2 additions & 0 deletions ElementX/Sources/Screens/CallScreen/CallScreenCoordinator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ struct CallScreenCoordinatorParameters {
let elementCallService: ElementCallServiceProtocol
let configuration: ElementCallConfiguration
let allowPictureInPicture: Bool
let notifyOtherParticipants: Bool
let appHooks: AppHooks
}

Expand Down Expand Up @@ -43,6 +44,7 @@ final class CallScreenCoordinator: CoordinatorProtocol {
viewModel = CallScreenViewModel(elementCallService: parameters.elementCallService,
configuration: parameters.configuration,
allowPictureInPicture: parameters.allowPictureInPicture,
notifyOtherParticipants: parameters.notifyOtherParticipants,
appHooks: parameters.appHooks)
}

Expand Down
79 changes: 35 additions & 44 deletions ElementX/Sources/Screens/CallScreen/CallScreenViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ class CallScreenViewModel: CallScreenViewModelType, CallScreenViewModelProtocol
actionsSubject.eraseToAnyPublisher()
}

private var syncUpdateCancellable: AnyCancellable?

/// Designated initialiser
/// - Parameters:
/// - elementCallService: service responsible for setting up CallKit
Expand All @@ -35,6 +33,7 @@ class CallScreenViewModel: CallScreenViewModelType, CallScreenViewModelProtocol
init(elementCallService: ElementCallServiceProtocol,
configuration: ElementCallConfiguration,
allowPictureInPicture: Bool,
notifyOtherParticipants: Bool,
appHooks: AppHooks) {
self.elementCallService = elementCallService
self.configuration = configuration
Expand Down Expand Up @@ -103,7 +102,7 @@ class CallScreenViewModel: CallScreenViewModelType, CallScreenViewModelProtocol
}
.store(in: &cancellables)

setupCall()
setupCall(notifyOtherParticipants: notifyOtherParticipants)
}

override func process(viewAction: CallScreenViewAction) {
Expand Down Expand Up @@ -132,52 +131,44 @@ class CallScreenViewModel: CallScreenViewModelType, CallScreenViewModelProtocol

// MARK: - Private

private func setupCall() {
private func setupCall(notifyOtherParticipants: Bool) {
stefanceriu marked this conversation as resolved.
Show resolved Hide resolved
switch configuration.kind {
case .genericCallLink(let url):
state.url = url
// We need widget messaging to work before enabling CallKit, otherwise mute, hangup etc do nothing.

case .roomCall(let roomProxy, let clientProxy, let clientID, let elementCallBaseURL, let elementCallBaseURLOverride, let colorScheme):
// Wait for room states to be up to date before starting the call and notifying others
syncUpdateCancellable = clientProxy.actionsPublisher
.filter(\.isSyncUpdate)
.timeout(.seconds(5), scheduler: DispatchQueue.main)
.first() // Timeout will make the publisher complete, use first to handle both branches in the same place
.sink(receiveCompletion: { [weak self] _ in
Task { [weak self] in
guard let self else { return }

let baseURL = if let elementCallBaseURLOverride {
elementCallBaseURLOverride
} else if case .success(let wellKnown) = await clientProxy.getElementWellKnown(), let wellKnownCall = wellKnown?.call {
wellKnownCall.widgetURL
} else {
elementCallBaseURL
}

switch await widgetDriver.start(baseURL: baseURL, clientID: clientID, colorScheme: colorScheme) {
case .success(let url):
state.url = url
case .failure(let error):
MXLog.error("Failed starting ElementCall Widget Driver with error: \(error)")
state.bindings.alertInfo = .init(id: UUID(),
title: L10n.errorUnknown,
primaryButton: .init(title: L10n.actionOk) {
self.actionsSubject.send(.dismiss)
})

return
}

await elementCallService.setupCallSession(roomID: roomProxy.id,
roomDisplayName: roomProxy.infoPublisher.value.displayName ?? roomProxy.id)

_ = await roomProxy.sendCallNotificationIfNeeded()

syncUpdateCancellable = nil
}
}, receiveValue: { _ in })
Task { [weak self] in
guard let self else { return }

let baseURL = if let elementCallBaseURLOverride {
elementCallBaseURLOverride
} else if case .success(let wellKnown) = await clientProxy.getElementWellKnown(), let wellKnownCall = wellKnown?.call {
wellKnownCall.widgetURL
} else {
elementCallBaseURL
}

switch await widgetDriver.start(baseURL: baseURL, clientID: clientID, colorScheme: colorScheme) {
case .success(let url):
state.url = url
case .failure(let error):
MXLog.error("Failed starting ElementCall Widget Driver with error: \(error)")
state.bindings.alertInfo = .init(id: UUID(),
title: L10n.errorUnknown,
primaryButton: .init(title: L10n.actionOk) {
self.actionsSubject.send(.dismiss)
})
return
}

await elementCallService.setupCallSession(roomID: roomProxy.id,
roomDisplayName: roomProxy.infoPublisher.value.displayName ?? roomProxy.id)

if notifyOtherParticipants {
_ = await roomProxy.sendCallNotificationIfNeeded()
}
}
}
}

Expand Down
1 change: 1 addition & 0 deletions ElementX/Sources/Screens/CallScreen/View/CallScreen.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ struct CallScreen_Previews: PreviewProvider {
elementCallBaseURLOverride: nil,
colorScheme: .light),
allowPictureInPicture: false,
notifyOtherParticipants: false,
appHooks: AppHooks())
}()

Expand Down
Loading