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

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
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

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

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 @@ -657,22 +658,23 @@ class UserSessionFlowCoordinator: FlowCoordinatorProtocol {
presentCallScreen(configuration: .init(genericCallLink: url))
}

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))
}

private var callScreenPictureInPictureController: AVPictureInPictureController?
Expand Down Expand Up @@ -899,7 +901,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
78 changes: 34 additions & 44 deletions ElementX/Sources/Screens/CallScreen/CallScreenViewModel.swift
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 @@ -43,7 +41,7 @@ class CallScreenViewModel: CallScreenViewModelType, CallScreenViewModelProtocol
switch configuration.kind {
case .genericCallLink(let url):
widgetDriver = GenericCallLinkWidgetDriver(url: url)
case .roomCall(let roomProxy, let clientProxy, _, _, _, _):
case .roomCall(let roomProxy, let clientProxy, _, _, _, _, _):
guard let deviceID = clientProxy.deviceID else { fatalError("Missing device ID for the call.") }
widgetDriver = roomProxy.elementCallWidgetDriver(deviceID: deviceID)
}
Expand Down Expand Up @@ -137,47 +135,39 @@ class CallScreenViewModel: CallScreenViewModelType, CallScreenViewModelProtocol
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 })

case .roomCall(let roomProxy, let clientProxy, let clientID, let elementCallBaseURL, let elementCallBaseURLOverride, let colorScheme, let notifyOtherParticipants):
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
3 changes: 2 additions & 1 deletion ElementX/Sources/Screens/CallScreen/View/CallScreen.swift
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,8 @@ struct CallScreen_Previews: PreviewProvider {
clientID: "io.element.elementx",
elementCallBaseURL: "https://call.element.io",
elementCallBaseURLOverride: nil,
colorScheme: .light),
colorScheme: .light,
notifyOtherParticipants: false),
allowPictureInPicture: false,
appHooks: AppHooks())
}()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ struct ElementCallConfiguration {
clientID: String,
elementCallBaseURL: URL,
elementCallBaseURLOverride: URL?,
colorScheme: ColorScheme)
colorScheme: ColorScheme,
notifyOtherParticipants: Bool)
}

/// The type of call being configured i.e. whether it's an external URL or an internal room call.
Expand Down Expand Up @@ -58,21 +59,23 @@ struct ElementCallConfiguration {
clientID: String,
elementCallBaseURL: URL,
elementCallBaseURLOverride: URL?,
colorScheme: ColorScheme) {
colorScheme: ColorScheme,
notifyOtherParticipants: Bool) {
kind = .roomCall(roomProxy: roomProxy,
clientProxy: clientProxy,
clientID: clientID,
elementCallBaseURL: elementCallBaseURL,
elementCallBaseURLOverride: elementCallBaseURLOverride,
colorScheme: colorScheme)
colorScheme: colorScheme,
notifyOtherParticipants: notifyOtherParticipants)
}

/// A string representing the call being configured.
var callRoomID: String {
switch kind {
case .genericCallLink(let url):
url.absoluteString
case .roomCall(let roomProxy, _, _, _, _, _):
case .roomCall(let roomProxy, _, _, _, _, _, _):
roomProxy.id
}
}
Expand Down
Loading