From cbf804cda664bf3eeaa37c89bdba9b2ba68009cd Mon Sep 17 00:00:00 2001 From: Mauro Romito Date: Thu, 2 Oct 2025 15:39:36 +0200 Subject: [PATCH] pr suggestions and some code improvements --- .../RoomFlowCoordinator.swift | 50 ++++++++++--------- ElementX/Sources/Mocks/ClientProxyMock.swift | 4 +- .../Mocks/Generated/GeneratedMocks.swift | 50 +++++++++---------- .../Services/Room/JoinedRoomProxy.swift | 2 +- .../Services/Room/RoomProxyProtocol.swift | 2 +- .../Sources/RoomFlowCoordinatorTests.swift | 10 ++-- 6 files changed, 60 insertions(+), 58 deletions(-) diff --git a/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift index 4a145dc34..3cd3e7909 100644 --- a/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift @@ -68,7 +68,7 @@ class RoomFlowCoordinator: FlowCoordinatorProtocol { private var roomProxy: JoinedRoomProxyProtocol! private var roomScreenCoordinator: RoomScreenCoordinator? - private var childThreads: [ThreadTimelineScreenCoordinator] = [] + private var childThreadScreenCoordinators: [ThreadTimelineScreenCoordinator] = [] private weak var joinRoomScreenCoordinator: JoinRoomScreenCoordinator? // periphery:ignore - used to avoid deallocation @@ -167,27 +167,27 @@ class RoomFlowCoordinator: FlowCoordinatorProtocol { showLoadingIndicator(delay: .seconds(0.5)) Task { defer { hideLoadingIndicator() } - switch await roomProxy.loadOrFetchEvent(for: eventID) { + switch await roomProxy.loadOrFetchEventDetails(for: eventID) { case .success(let event): if flowParameters.appSettings.threadsEnabled, let threadRootEventID = event.threadRootEventId() { - if case .thread(threadRootEventID: threadRootEventID, _) = stateMachine.state, let threadCoordinator = childThreads.last { + if case .thread(threadRootEventID: threadRootEventID, _) = stateMachine.state, let threadCoordinator = childThreadScreenCoordinators.last { threadCoordinator.focusOnEvent(eventID: eventID) } else { - // If we are showing the room timeline, we want to focus the thread root - if childThreads.isEmpty { + // If we are showing the room timeline, we want to focus the thread root. + if childThreadScreenCoordinators.isEmpty { roomScreenCoordinator?.focusOnEvent(.init(eventID: threadRootEventID, shouldSetPin: false)) } stateMachine.tryEvent(.presentThread(threadRootEventID: threadRootEventID, focusEventID: eventID)) } - } else if !childThreads.isEmpty { - // If we are showing a child thread, and we are navigating to a non threaded event - // of the same room, we want to push the room on top of the thread + } else if !childThreadScreenCoordinators.isEmpty { + // If we are showing a child thread and we are navigating to a non threaded event + // of the same room, we want to push the room on top of the thread. stateMachine.tryEvent(.startChildFlow(roomID: roomID, via: via, entryPoint: .eventID(eventID)), userInfo: EventUserInfo(animated: animated)) } else { roomScreenCoordinator?.focusOnEvent(.init(eventID: eventID, shouldSetPin: false)) } case .failure: - showError() + showErrorIndicator() } } } @@ -258,22 +258,24 @@ class RoomFlowCoordinator: FlowCoordinatorProtocol { switch room { case .joined(let roomProxy): await storeAndSubscribeToRoomProxy(roomProxy) - if case let .eventFocus(focusEvent) = presentationAction { - switch await roomProxy.loadOrFetchEvent(for: focusEvent.eventID) { - case .success(let event): - guard flowParameters.appSettings.threadsEnabled, let threadRootEventID = event.threadRootEventId() else { - break - } + guard case let .eventFocus(focusEvent) = presentationAction else { + // If is not a focus event just handle the presentation action directly in `presentRoom` + stateMachine.tryEvent(.presentRoom(presentationAction: presentationAction), userInfo: EventUserInfo(animated: animated)) + return + } + // Otherwise check if the focussed event exists to handle a possible error or theaded event. + switch await roomProxy.loadOrFetchEventDetails(for: focusEvent.eventID) { + case .success(let event): + if flowParameters.appSettings.threadsEnabled, let threadRootEventID = event.threadRootEventId() { stateMachine.tryEvent(.presentRoom(presentationAction: .eventFocus(.init(eventID: threadRootEventID, shouldSetPin: false))), userInfo: EventUserInfo(animated: animated)) stateMachine.tryEvent(.presentThread(threadRootEventID: threadRootEventID, focusEventID: focusEvent.eventID), userInfo: EventUserInfo(animated: false)) - return - case .failure: - showError() - stateMachine.tryEvent(.presentRoom(presentationAction: nil), userInfo: EventUserInfo(animated: animated)) - return + } else { + stateMachine.tryEvent(.presentRoom(presentationAction: presentationAction), userInfo: EventUserInfo(animated: animated)) } + case .failure: + showErrorIndicator() + stateMachine.tryEvent(.presentRoom(presentationAction: nil), userInfo: EventUserInfo(animated: animated)) } - stateMachine.tryEvent(.presentRoom(presentationAction: presentationAction), userInfo: EventUserInfo(animated: animated)) default: stateMachine.tryEvent(.presentJoinRoomScreen(via: via), userInfo: EventUserInfo(animated: animated)) } @@ -737,10 +739,10 @@ class RoomFlowCoordinator: FlowCoordinatorProtocol { navigationStackCoordinator.push(coordinator, animated: animated) { [weak self] in guard let self else { return } stateMachine.tryEvent(.dismissThread) - childThreads.removeAll { $0 === coordinator } + childThreadScreenCoordinators.removeAll { $0 === coordinator } } - childThreads.append(coordinator) + childThreadScreenCoordinators.append(coordinator) } private func presentJoinRoomScreen(via: [String], animated: Bool) { @@ -1625,7 +1627,7 @@ class RoomFlowCoordinator: FlowCoordinatorProtocol { flowParameters.userIndicatorController.retractIndicatorWithId(Self.loadingIndicatorID) } - private func showError() { + private func showErrorIndicator() { flowParameters.userIndicatorController.submitIndicator(.init(title: L10n.errorUnknown)) } } diff --git a/ElementX/Sources/Mocks/ClientProxyMock.swift b/ElementX/Sources/Mocks/ClientProxyMock.swift index 217c3fb0f..6eaefa481 100644 --- a/ElementX/Sources/Mocks/ClientProxyMock.swift +++ b/ElementX/Sources/Mocks/ClientProxyMock.swift @@ -97,11 +97,11 @@ extension ClientProxyMock { roomForIdentifierClosure = { [weak self] identifier in if let room = self?.roomSummaryProvider.roomListPublisher.value.first(where: { $0.id == identifier }) { let roomProxy = await JoinedRoomProxyMock(.init(id: room.id, name: room.name)) - roomProxy.loadOrFetchEventForReturnValue = .success(TimelineEventSDKMock()) + roomProxy.loadOrFetchEventDetailsForReturnValue = .success(TimelineEventSDKMock()) return .joined(roomProxy) } else if let spaceRoomProxy = configuration.joinedSpaceRooms.first(where: { $0.id == identifier }) { let roomProxy = await JoinedRoomProxyMock(.init(id: spaceRoomProxy.id, name: spaceRoomProxy.name)) - roomProxy.loadOrFetchEventForReturnValue = .success(TimelineEventSDKMock()) + roomProxy.loadOrFetchEventDetailsForReturnValue = .success(TimelineEventSDKMock()) return .joined(roomProxy) } else { return nil diff --git a/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift b/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift index a11a7230d..6d4d01b0b 100644 --- a/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift +++ b/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift @@ -6483,17 +6483,17 @@ class JoinedRoomProxyMock: JoinedRoomProxyProtocol, @unchecked Sendable { return threadTimelineEventIDReturnValue } } - //MARK: - loadOrFetchEvent + //MARK: - loadOrFetchEventDetails - var loadOrFetchEventForUnderlyingCallsCount = 0 - var loadOrFetchEventForCallsCount: Int { + var loadOrFetchEventDetailsForUnderlyingCallsCount = 0 + var loadOrFetchEventDetailsForCallsCount: Int { get { if Thread.isMainThread { - return loadOrFetchEventForUnderlyingCallsCount + return loadOrFetchEventDetailsForUnderlyingCallsCount } else { var returnValue: Int? = nil DispatchQueue.main.sync { - returnValue = loadOrFetchEventForUnderlyingCallsCount + returnValue = loadOrFetchEventDetailsForUnderlyingCallsCount } return returnValue! @@ -6501,29 +6501,29 @@ class JoinedRoomProxyMock: JoinedRoomProxyProtocol, @unchecked Sendable { } set { if Thread.isMainThread { - loadOrFetchEventForUnderlyingCallsCount = newValue + loadOrFetchEventDetailsForUnderlyingCallsCount = newValue } else { DispatchQueue.main.sync { - loadOrFetchEventForUnderlyingCallsCount = newValue + loadOrFetchEventDetailsForUnderlyingCallsCount = newValue } } } } - var loadOrFetchEventForCalled: Bool { - return loadOrFetchEventForCallsCount > 0 + var loadOrFetchEventDetailsForCalled: Bool { + return loadOrFetchEventDetailsForCallsCount > 0 } - var loadOrFetchEventForReceivedEventID: String? - var loadOrFetchEventForReceivedInvocations: [String] = [] + var loadOrFetchEventDetailsForReceivedEventID: String? + var loadOrFetchEventDetailsForReceivedInvocations: [String] = [] - var loadOrFetchEventForUnderlyingReturnValue: Result! - var loadOrFetchEventForReturnValue: Result! { + var loadOrFetchEventDetailsForUnderlyingReturnValue: Result! + var loadOrFetchEventDetailsForReturnValue: Result! { get { if Thread.isMainThread { - return loadOrFetchEventForUnderlyingReturnValue + return loadOrFetchEventDetailsForUnderlyingReturnValue } else { var returnValue: Result? = nil DispatchQueue.main.sync { - returnValue = loadOrFetchEventForUnderlyingReturnValue + returnValue = loadOrFetchEventDetailsForUnderlyingReturnValue } return returnValue! @@ -6531,26 +6531,26 @@ class JoinedRoomProxyMock: JoinedRoomProxyProtocol, @unchecked Sendable { } set { if Thread.isMainThread { - loadOrFetchEventForUnderlyingReturnValue = newValue + loadOrFetchEventDetailsForUnderlyingReturnValue = newValue } else { DispatchQueue.main.sync { - loadOrFetchEventForUnderlyingReturnValue = newValue + loadOrFetchEventDetailsForUnderlyingReturnValue = newValue } } } } - var loadOrFetchEventForClosure: ((String) async -> Result)? + var loadOrFetchEventDetailsForClosure: ((String) async -> Result)? - func loadOrFetchEvent(for eventID: String) async -> Result { - loadOrFetchEventForCallsCount += 1 - loadOrFetchEventForReceivedEventID = eventID + func loadOrFetchEventDetails(for eventID: String) async -> Result { + loadOrFetchEventDetailsForCallsCount += 1 + loadOrFetchEventDetailsForReceivedEventID = eventID DispatchQueue.main.async { - self.loadOrFetchEventForReceivedInvocations.append(eventID) + self.loadOrFetchEventDetailsForReceivedInvocations.append(eventID) } - if let loadOrFetchEventForClosure = loadOrFetchEventForClosure { - return await loadOrFetchEventForClosure(eventID) + if let loadOrFetchEventDetailsForClosure = loadOrFetchEventDetailsForClosure { + return await loadOrFetchEventDetailsForClosure(eventID) } else { - return loadOrFetchEventForReturnValue + return loadOrFetchEventDetailsForReturnValue } } //MARK: - messageFilteredTimeline diff --git a/ElementX/Sources/Services/Room/JoinedRoomProxy.swift b/ElementX/Sources/Services/Room/JoinedRoomProxy.swift index cef6b078a..270c9a95a 100644 --- a/ElementX/Sources/Services/Room/JoinedRoomProxy.swift +++ b/ElementX/Sources/Services/Room/JoinedRoomProxy.swift @@ -190,7 +190,7 @@ class JoinedRoomProxy: JoinedRoomProxyProtocol { } } - func loadOrFetchEvent(for eventID: String) async -> Result { + func loadOrFetchEventDetails(for eventID: String) async -> Result { do { let event = try await room.loadOrFetchEvent(eventId: eventID) return .success(event) diff --git a/ElementX/Sources/Services/Room/RoomProxyProtocol.swift b/ElementX/Sources/Services/Room/RoomProxyProtocol.swift index 5725a3ac6..d9433f5a6 100644 --- a/ElementX/Sources/Services/Room/RoomProxyProtocol.swift +++ b/ElementX/Sources/Services/Room/RoomProxyProtocol.swift @@ -97,7 +97,7 @@ protocol JoinedRoomProxyProtocol: RoomProxyProtocol { func threadTimeline(eventID: String) async -> Result - func loadOrFetchEvent(for eventID: String) async -> Result + func loadOrFetchEventDetails(for eventID: String) async -> Result func messageFilteredTimeline(focus: TimelineFocus, allowedMessageTypes: [TimelineAllowedMessageType], diff --git a/UnitTests/Sources/RoomFlowCoordinatorTests.swift b/UnitTests/Sources/RoomFlowCoordinatorTests.swift index 41859940a..52f3d3bd5 100644 --- a/UnitTests/Sources/RoomFlowCoordinatorTests.swift +++ b/UnitTests/Sources/RoomFlowCoordinatorTests.swift @@ -235,7 +235,7 @@ class RoomFlowCoordinatorTests: XCTestCase { var mockedEvent = TimelineEventSDKMock() mockedEvent.threadRootEventIdReturnValue = "1" - roomProxy.loadOrFetchEventForReturnValue = .success(mockedEvent) + roomProxy.loadOrFetchEventDetailsForReturnValue = .success(mockedEvent) clientProxy.roomForIdentifierClosure = { _ in .joined(roomProxy) @@ -258,7 +258,7 @@ class RoomFlowCoordinatorTests: XCTestCase { // From the thread screen, navigate to another threaded event in the same room, but in a different thread. mockedEvent = TimelineEventSDKMock() mockedEvent.threadRootEventIdReturnValue = "4" - roomProxy.loadOrFetchEventForReturnValue = .success(mockedEvent) + roomProxy.loadOrFetchEventDetailsForReturnValue = .success(mockedEvent) try await process(route: .childEvent(eventID: "5", roomID: "1", via: [])) XCTAssert(navigationStackCoordinator.rootCoordinator is RoomScreenCoordinator) XCTAssertEqual(navigationStackCoordinator.stackCoordinators.count, 2) @@ -274,7 +274,7 @@ class RoomFlowCoordinatorTests: XCTestCase { mockedEvent = TimelineEventSDKMock() mockedEvent.threadRootEventIdReturnValue = "1" - roomProxy.loadOrFetchEventForReturnValue = .success(mockedEvent) + roomProxy.loadOrFetchEventDetailsForReturnValue = .success(mockedEvent) clientProxy.roomForIdentifierClosure = { _ in .joined(roomProxy) @@ -291,7 +291,7 @@ class RoomFlowCoordinatorTests: XCTestCase { // From the thread screen, navigate to an event of the same room that is not threaded mockedEvent = TimelineEventSDKMock() mockedEvent.threadRootEventIdReturnValue = nil - roomProxy.loadOrFetchEventForReturnValue = .success(mockedEvent) + roomProxy.loadOrFetchEventDetailsForReturnValue = .success(mockedEvent) try await process(route: .childEvent(eventID: "3", roomID: "2", via: [])) XCTAssert(navigationStackCoordinator.rootCoordinator is RoomScreenCoordinator) @@ -377,7 +377,7 @@ class RoomFlowCoordinatorTests: XCTestCase { try await fulfillment.fulfill() } - + // MARK: - Private private func process(route: AppRoute) async throws {