From f2c1f8fdea9fe7cb1d71788f116649e13178310e Mon Sep 17 00:00:00 2001 From: Alfonso Grillo Date: Wed, 31 May 2023 18:24:12 +0200 Subject: [PATCH] Fix navigation back from room's details (#988) * Fix navigation to room details * Improve tests * Delete unused action in RoomDetailsScreen * Cleanup * Try fix flaky tests --- .../RoomFlowCoordinator.swift | 77 ++++++++++++------- .../RoomDetailsScreenCoordinator.swift | 3 - .../RoomDetailsScreenModels.swift | 1 - .../Sources/RoomFlowCoordinatorTests.swift | 56 +++++++++++--- 4 files changed, 98 insertions(+), 39 deletions(-) diff --git a/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift index 90f21814c..1e73c7e9e 100644 --- a/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift @@ -86,12 +86,12 @@ class RoomFlowCoordinator: FlowCoordinatorProtocol { return .initial case (.presentRoomDetails(let roomID), .initial): - return .roomDetails(roomID: roomID) - case (.presentRoomDetails(let roomID), .room): - return .roomDetails(roomID: roomID) - case (.presentRoomDetails(let roomID), .roomDetails): - return .roomDetails(roomID: roomID) - case (.dismissRoomDetails, .roomDetails(let roomID)): + return .roomDetails(roomID: roomID, isRoot: true) + case (.presentRoomDetails(let roomID), .room(let currentRoomID)): + return .roomDetails(roomID: roomID, isRoot: roomID != currentRoomID) + case (.presentRoomDetails(let roomID), .roomDetails(let currentRoomID, _)): + return .roomDetails(roomID: roomID, isRoot: roomID != currentRoomID) + case (.dismissRoomDetails, .roomDetails(let roomID, _)): return .room(roomID: roomID) case (.dismissRoom, .roomDetails): return .initial @@ -134,17 +134,20 @@ class RoomFlowCoordinator: FlowCoordinatorProtocol { let animated = (context.userInfo as? EventUserInfo)?.animated ?? true switch (context.fromState, context.event, context.toState) { + case (.roomDetails(roomID: let currentRoomID, true), .presentRoom(let roomID), .room) where currentRoomID == roomID: + dismissRoom(animated: animated) + presentRoom(roomID, animated: animated) case (_, .presentRoom(let roomID), .room): presentRoom(roomID, animated: animated) case (.room, .dismissRoom, .initial): dismissRoom(animated: animated) - case (.initial, .presentRoomDetails, .roomDetails(let roomID)), - (.room, .presentRoomDetails, .roomDetails(let roomID)), - (.roomDetails, .presentRoomDetails, .roomDetails(let roomID)): - Task { - await self.presentRoomDetails(roomID: roomID, animated: animated) - } + case (.roomDetails(let currentRoomID, _), .presentRoomDetails, .roomDetails(let roomID, _)) where currentRoomID == roomID: + break + case (.initial, .presentRoomDetails, .roomDetails(let roomID, let isRoot)), + (.room, .presentRoomDetails, .roomDetails(let roomID, let isRoot)), + (.roomDetails, .presentRoomDetails, .roomDetails(let roomID, let isRoot)): + self.presentRoomDetails(roomID: roomID, isRoot: isRoot, animated: animated) case (.roomDetails, .dismissRoomDetails, .room): break case (.roomDetails, .dismissRoom, .initial): @@ -275,20 +278,30 @@ class RoomFlowCoordinator: FlowCoordinatorProtocol { } private func dismissRoom(animated: Bool) { - navigationStackCoordinator.popToRoot(animated: true) - navigationSplitCoordinator.setDetailCoordinator(nil) + navigationStackCoordinator.popToRoot(animated: animated) + navigationSplitCoordinator.setDetailCoordinator(nil, animated: animated) roomProxy = nil actionsSubject.send(.dismissedRoom) } - private func presentRoomDetails(roomID: String, animated: Bool) async { - if roomProxy?.id != roomID { - await asyncPresentRoom(roomID, animated: true) + private func presentRoomDetails(roomID: String, isRoot: Bool, animated: Bool) { + Task { + await asyncPresentRoomDetails(roomID: roomID, isRoot: isRoot, animated: animated) + } + } + + private func asyncPresentRoomDetails(roomID: String, isRoot: Bool, animated: Bool) async { + if isRoot { + roomProxy = await userSession.clientProxy.roomForIdentifier(roomID) + } else { + await asyncPresentRoom(roomID, animated: animated) } guard let roomProxy else { - fatalError() + MXLog.error("Invalid room identifier: \(roomID)") + stateMachine.tryEvent(.dismissRoom) + return } let params = RoomDetailsScreenCoordinatorParameters(navigationStackCoordinator: navigationStackCoordinator, @@ -298,18 +311,30 @@ class RoomFlowCoordinator: FlowCoordinatorProtocol { let coordinator = RoomDetailsScreenCoordinator(parameters: params) coordinator.callback = { [weak self] action in switch action { - case .cancel: - self?.navigationStackCoordinator.pop() case .leftRoom: self?.dismissRoom(animated: animated) } } - - navigationStackCoordinator.push(coordinator) { [weak self] in - guard let self else { return } + + if isRoot { + navigationStackCoordinator.setRootCoordinator(coordinator, animated: animated) { [weak self] in + guard let self else { return } + if case .roomDetails(let detailsRoomID, _) = stateMachine.state, detailsRoomID == roomID { + stateMachine.tryEvent(.dismissRoom) + } + } - if case .roomDetails = stateMachine.state { - stateMachine.tryEvent(.dismissRoomDetails) + if navigationSplitCoordinator.detailCoordinator == nil { + navigationSplitCoordinator.setDetailCoordinator(navigationStackCoordinator, animated: animated) + } + + actionsSubject.send(.presentedRoom(roomID)) + } else { + navigationStackCoordinator.push(coordinator, animated: animated) { [weak self] in + guard let self else { return } + if case .roomDetails = stateMachine.state { + stateMachine.tryEvent(.dismissRoomDetails) + } } } } @@ -445,7 +470,7 @@ private extension RoomFlowCoordinator { case room(roomID: String) case mediaViewer(roomID: String, file: MediaFileHandleProxy, title: String?) case reportContent(roomID: String, itemID: String, senderID: String) - case roomDetails(roomID: String) + case roomDetails(roomID: String, isRoot: Bool) case mediaUploadPicker(roomID: String, source: MediaPickerScreenSource) case mediaUploadPreview(roomID: String, fileURL: URL) case emojiPicker(roomID: String, itemID: String) diff --git a/ElementX/Sources/Screens/RoomDetailsScreen/RoomDetailsScreenCoordinator.swift b/ElementX/Sources/Screens/RoomDetailsScreen/RoomDetailsScreenCoordinator.swift index 21600c5f5..e5e04f00a 100644 --- a/ElementX/Sources/Screens/RoomDetailsScreen/RoomDetailsScreenCoordinator.swift +++ b/ElementX/Sources/Screens/RoomDetailsScreen/RoomDetailsScreenCoordinator.swift @@ -25,7 +25,6 @@ struct RoomDetailsScreenCoordinatorParameters { } enum RoomDetailsScreenCoordinatorAction { - case cancel case leftRoom } @@ -58,8 +57,6 @@ final class RoomDetailsScreenCoordinator: CoordinatorProtocol { self.presentRoomMembersList(members) case .requestInvitePeoplePresentation(let members): self.presentInviteUsersScreen(members: members) - case .cancel: - self.callback?(.cancel) case .leftRoom: self.callback?(.leftRoom) case .requestEditDetailsPresentation(let accountOwner): diff --git a/ElementX/Sources/Screens/RoomDetailsScreen/RoomDetailsScreenModels.swift b/ElementX/Sources/Screens/RoomDetailsScreen/RoomDetailsScreenModels.swift index 76502f3bd..bf54854ea 100644 --- a/ElementX/Sources/Screens/RoomDetailsScreen/RoomDetailsScreenModels.swift +++ b/ElementX/Sources/Screens/RoomDetailsScreen/RoomDetailsScreenModels.swift @@ -25,7 +25,6 @@ enum RoomDetailsScreenViewModelAction { case requestMemberDetailsPresentation([RoomMemberProxyProtocol]) case requestInvitePeoplePresentation([RoomMemberProxyProtocol]) case leftRoom - case cancel case requestEditDetailsPresentation(RoomMemberProxyProtocol) } diff --git a/UnitTests/Sources/RoomFlowCoordinatorTests.swift b/UnitTests/Sources/RoomFlowCoordinatorTests.swift index 3387abc11..91d7c2990 100644 --- a/UnitTests/Sources/RoomFlowCoordinatorTests.swift +++ b/UnitTests/Sources/RoomFlowCoordinatorTests.swift @@ -60,9 +60,7 @@ class RoomFlowCoordinatorTests: XCTestCase { func testRoomDetailsPresentation() async { await process(route: .roomDetails(roomID: "1"), expectedAction: .presentedRoom("1")) - XCTAssert(navigationStackCoordinator.rootCoordinator is RoomScreenCoordinator) - XCTAssertEqual(navigationStackCoordinator.stackCoordinators.count, 1) - XCTAssert(navigationStackCoordinator.stackCoordinators.first is RoomDetailsScreenCoordinator) + XCTAssert(navigationStackCoordinator.rootCoordinator is RoomDetailsScreenCoordinator) await process(route: .roomList, expectedAction: .dismissedRoom) XCTAssertNil(navigationStackCoordinator.rootCoordinator) @@ -70,18 +68,58 @@ class RoomFlowCoordinatorTests: XCTestCase { func testStackUnwinding() async { await process(route: .roomDetails(roomID: "1"), expectedAction: .presentedRoom("1")) - XCTAssert(navigationStackCoordinator.rootCoordinator is RoomScreenCoordinator) - XCTAssertEqual(navigationStackCoordinator.stackCoordinators.count, 1) - XCTAssert(navigationStackCoordinator.stackCoordinators.first is RoomDetailsScreenCoordinator) + XCTAssert(navigationStackCoordinator.rootCoordinator is RoomDetailsScreenCoordinator) await process(route: .room(roomID: "2"), expectedAction: .presentedRoom("2")) XCTAssert(navigationStackCoordinator.rootCoordinator is RoomScreenCoordinator) } + func testNoOp() async { + await process(route: .roomDetails(roomID: "1"), expectedAction: .presentedRoom("1")) + XCTAssert(navigationStackCoordinator.rootCoordinator is RoomDetailsScreenCoordinator) + + await process(route: .roomDetails(roomID: "1"), expectedAction: nil) + XCTAssert(navigationStackCoordinator.rootCoordinator is RoomDetailsScreenCoordinator) + } + + func testSwitchToDifferentDetails() async { + await process(route: .roomDetails(roomID: "1"), expectedAction: .presentedRoom("1")) + XCTAssert(navigationStackCoordinator.rootCoordinator is RoomDetailsScreenCoordinator) + + await process(route: .roomDetails(roomID: "2"), expectedAction: .presentedRoom("2")) + XCTAssert(navigationStackCoordinator.rootCoordinator is RoomDetailsScreenCoordinator) + } + + func testPushDetails() async { + await process(route: .room(roomID: "1"), expectedAction: .presentedRoom("1")) + XCTAssert(navigationStackCoordinator.rootCoordinator is RoomScreenCoordinator) + + await process(route: .roomDetails(roomID: "1"), expectedAction: nil) + XCTAssert(navigationStackCoordinator.rootCoordinator is RoomScreenCoordinator) + XCTAssertEqual(navigationStackCoordinator.stackCoordinators.count, 1) + XCTAssert(navigationStackCoordinator.stackCoordinators.first is RoomDetailsScreenCoordinator) + } + + func testReplaceDetailsWithTimeline() async { + await process(route: .roomDetails(roomID: "1"), expectedAction: .presentedRoom("1")) + XCTAssert(navigationStackCoordinator.rootCoordinator is RoomDetailsScreenCoordinator) + + await process(route: .room(roomID: "1"), expectedAction: .presentedRoom("1")) + XCTAssert(navigationStackCoordinator.rootCoordinator is RoomScreenCoordinator) + } + // MARK: - Private - func process(route: AppRoute, expectedAction: RoomFlowCoordinatorAction) async { - Task { try? await Task.sleep(for: .seconds(0.1)); roomFlowCoordinator.handleAppRoute(route, animated: true) } - _ = await roomFlowCoordinator.actions.values.first(where: { $0 == expectedAction }) + func process(route: AppRoute, expectedAction: RoomFlowCoordinatorAction?) async { + let routeTask = Task.detached(priority: .low) { + try? await Task.sleep(for: .seconds(0.2)) + await self.roomFlowCoordinator.handleAppRoute(route, animated: true) + } + + if let expectedAction { + _ = await roomFlowCoordinator.actions.values.first(where: { $0 == expectedAction }) + } else { + await routeTask.value + } } }