From 6dee4176f2e89e9ddd24a58c56151e28984cc93d Mon Sep 17 00:00:00 2001 From: Mauro Romito Date: Wed, 5 Nov 2025 16:05:09 +0100 Subject: [PATCH] pr suggestions and implemented a simpler way to clear the route, that always brings back to the initial coordinator --- ElementX.xcodeproj/project.pbxproj | 4 + .../Navigation/NavigationCoordinators.swift | 15 ++++ .../RoomMembersFlowCoordinator.swift | 24 ++++-- .../SpaceSettingsFlowCoordinator.swift | 14 ++-- .../Views/OverridableAvatarImage.swift | 34 -------- .../StateMachine/StateMachineFactory.swift | 15 ++++ .../RoomMembersFlowCoordinatorTests.swift | 80 +++++++++++++++++++ 7 files changed, 136 insertions(+), 50 deletions(-) create mode 100644 UnitTests/Sources/RoomMembersFlowCoordinatorTests.swift diff --git a/ElementX.xcodeproj/project.pbxproj b/ElementX.xcodeproj/project.pbxproj index 9482e0bbd..a366495fc 100644 --- a/ElementX.xcodeproj/project.pbxproj +++ b/ElementX.xcodeproj/project.pbxproj @@ -947,6 +947,7 @@ A74438ED16F8683A4B793E6A /* AnalyticsSettingsScreenViewModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0BCE3FAF40932AC7C7639AC4 /* AnalyticsSettingsScreenViewModel.swift */; }; A7D48E44D485B143AADDB77D /* Strings+Untranslated.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1A18F6CE4D694D21E4EA9B25 /* Strings+Untranslated.swift */; }; A7DB75E090542331F6668A23 /* CreateRoomScreenViewModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = CF19027E7FFA5E63D148873A /* CreateRoomScreenViewModel.swift */; }; + A7F6CE532EBB76A500450B70 /* RoomMembersFlowCoordinatorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = A7F6CE522EBB769D00450B70 /* RoomMembersFlowCoordinatorTests.swift */; }; A808DC3F72D15C6C5A52317E /* TimelineItemDebugView.swift in Sources */ = {isa = PBXBuildFile; fileRef = BCDA016D05107DED3B9495CB /* TimelineItemDebugView.swift */; }; A816F7087C495D85048AC50E /* RoomMemberDetailsScreenModels.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1B6E30BB748F3F480F077969 /* RoomMemberDetailsScreenModels.swift */; }; A851635B3255C6DC07034A12 /* RoomScreenCoordinator.swift in Sources */ = {isa = PBXBuildFile; fileRef = B8108C8F0ACF6A7EB72D0117 /* RoomScreenCoordinator.swift */; }; @@ -2368,6 +2369,7 @@ A7C4EA55DA62F9D0F984A2AE /* CollapsibleTimelineItem.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CollapsibleTimelineItem.swift; sourceTree = ""; }; A7D452AF7B5F7E3A0A7DB54C /* SessionVerificationScreenViewModelProtocol.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SessionVerificationScreenViewModelProtocol.swift; sourceTree = ""; }; A7E37072597F67C4DD8CC2DB /* ComposerDraftServiceProtocol.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ComposerDraftServiceProtocol.swift; sourceTree = ""; }; + A7F6CE522EBB769D00450B70 /* RoomMembersFlowCoordinatorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RoomMembersFlowCoordinatorTests.swift; sourceTree = ""; }; A84D413BF49F0E980F010A6B /* LogViewerScreenCoordinator.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LogViewerScreenCoordinator.swift; sourceTree = ""; }; A8558D41DD4B553A752C868A /* StackedAvatarsView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StackedAvatarsView.swift; sourceTree = ""; }; A861DA5932B128FE1DCB5CE2 /* InviteUsersScreenCoordinator.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = InviteUsersScreenCoordinator.swift; sourceTree = ""; }; @@ -4577,6 +4579,7 @@ 73CD9796729EB702B4DFA88C /* Sources */ = { isa = PBXGroup; children = ( + A7F6CE522EBB769D00450B70 /* RoomMembersFlowCoordinatorTests.swift */, 58C2527813FDAE23E72A9063 /* AnalyticsSettingsScreenViewModelTests.swift */, C687844F60BFF532D49A994C /* AnalyticsTests.swift */, E461B3C8BBBFCA400B268D14 /* AppRouteURLParserTests.swift */, @@ -7373,6 +7376,7 @@ 3582056513A384F110EC8274 /* MediaPlayerProviderTests.swift in Sources */, 167D00CAA13FAFB822298021 /* MediaProviderTests.swift in Sources */, B9A8C34A00D03094C0CF56F3 /* MediaUploadPreviewScreenViewModelTests.swift in Sources */, + A7F6CE532EBB76A500450B70 /* RoomMembersFlowCoordinatorTests.swift in Sources */, 23701DE32ACD6FD40AA992C3 /* MediaUploadingPreprocessorTests.swift in Sources */, F777C6FEE7D106136E2ED2B2 /* MessageForwardingScreenViewModelTests.swift in Sources */, 1146E9EDCF8344F7D6E0D553 /* MockCoder.swift in Sources */, diff --git a/ElementX/Sources/Application/Navigation/NavigationCoordinators.swift b/ElementX/Sources/Application/Navigation/NavigationCoordinators.swift index f6bf1965e..fe88cf6bb 100644 --- a/ElementX/Sources/Application/Navigation/NavigationCoordinators.swift +++ b/ElementX/Sources/Application/Navigation/NavigationCoordinators.swift @@ -574,6 +574,21 @@ private struct NavigationSplitCoordinatorView: View { } } + func pop(to coordinator: CoordinatorProtocol, animated: Bool = true) { + if rootCoordinator === coordinator { + popToRoot(animated: animated) + } else if stackCoordinators.contains(where: { $0 === coordinator }) { + var transaction = Transaction() + transaction.disablesAnimations = !animated + + withTransaction(transaction) { + while stackCoordinators.last !== coordinator, !stackCoordinators.isEmpty { + _ = stackModules.popLast() + } + } + } + } + /// Present a sheet on top of the stack. If this NavigationStackCoordinator is embedded within a NavigationSplitCoordinator /// then the presentation will be proxied to the split /// - Parameters: diff --git a/ElementX/Sources/FlowCoordinators/RoomMembersFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/RoomMembersFlowCoordinator.swift index a93437061..10f32cc27 100644 --- a/ElementX/Sources/FlowCoordinators/RoomMembersFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/RoomMembersFlowCoordinator.swift @@ -59,6 +59,8 @@ final class RoomMembersFlowCoordinator: FlowCoordinatorProtocol { private let entryPoint: RoomMembersFlowCoordinatorEntryPoint private let roomProxy: JoinedRoomProxyProtocol private let navigationStackCoordinator: NavigationStackCoordinator + /// The last coordinator in the `navigationStackCoordinator` stack at the initial state + private let initialCoordinator: CoordinatorProtocol? private let flowParameters: CommonFlowParameters private let stateMachine: StateMachine @@ -79,8 +81,9 @@ final class RoomMembersFlowCoordinator: FlowCoordinatorProtocol { self.roomProxy = roomProxy self.flowParameters = flowParameters self.navigationStackCoordinator = navigationStackCoordinator + initialCoordinator = navigationStackCoordinator.stackCoordinators.last ?? navigationStackCoordinator.rootCoordinator - stateMachine = .init(state: .initial) + stateMachine = flowParameters.stateMachineFactory.makeMembersFlowStateMachine(state: .initial) configureStateMachine() } @@ -123,14 +126,21 @@ final class RoomMembersFlowCoordinator: FlowCoordinatorProtocol { } func clearRoute(animated: Bool) { - if stateMachine.state == .inviteUsersScreen { + switch stateMachine.state { + case .inviteUsersScreen: navigationStackCoordinator.setSheetCoordinator(nil, animated: animated) - } else if let roomFlowCoordinator { - roomFlowCoordinator.clearRoute(animated: animated) + clearRoute(animated: animated) + case .roomFlow: + roomFlowCoordinator?.clearRoute(animated: animated) + clearRoute(animated: animated) + case .initial: + break + case .roomMemberDetails, .roomMembersList, .userProfile: + guard let initialCoordinator else { + return + } + navigationStackCoordinator.pop(to: initialCoordinator, animated: animated) } - // We don't support dismissing a sub flow by itself, only the entire chain. - // The presenter flow will take care of dismissing it - actionsSubject.send(.finished) } private func configureStateMachine() { diff --git a/ElementX/Sources/FlowCoordinators/SpaceSettingsFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/SpaceSettingsFlowCoordinator.swift index c3e510023..f46d00ade 100644 --- a/ElementX/Sources/FlowCoordinators/SpaceSettingsFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/SpaceSettingsFlowCoordinator.swift @@ -59,14 +59,11 @@ final class SpaceSettingsFlowCoordinator: FlowCoordinatorProtocol { private let roomProxy: JoinedRoomProxyProtocol private let navigationStackCoordinator: NavigationStackCoordinator + private let initialCoordinator: CoordinatorProtocol? private let flowParameters: CommonFlowParameters private let stateMachine: StateMachine private var cancellables = Set() - private var modalNavigationStackCoordinator: NavigationStackCoordinator? - - private var membersFlowCoordinator: RoomMembersFlowCoordinator? - private var rolesAndPermissionsFlowCoordinator: RoomRolesAndPermissionsFlowCoordinator? private var membersFlowCoordinator: RoomMembersFlowCoordinator? private var rolesAndPermissionsFlowCoordinator: RoomRolesAndPermissionsFlowCoordinator? @@ -82,6 +79,7 @@ final class SpaceSettingsFlowCoordinator: FlowCoordinatorProtocol { self.roomProxy = roomProxy self.flowParameters = flowParameters self.navigationStackCoordinator = navigationStackCoordinator + initialCoordinator = navigationStackCoordinator.stackCoordinators.last ?? navigationStackCoordinator.rootCoordinator stateMachine = .init(state: .initial) configureStateMachine() @@ -99,11 +97,9 @@ final class SpaceSettingsFlowCoordinator: FlowCoordinatorProtocol { switch stateMachine.state { case .initial: break - case .spaceSettings: - navigationStackCoordinator.pop(animated: animated) - case .securityAndPrivacy: - navigationStackCoordinator.pop(animated: animated) - clearRoute(animated: animated) + case .spaceSettings, .securityAndPrivacy: + guard let initialCoordinator else { return } + navigationStackCoordinator.pop(to: initialCoordinator, animated: animated) case .editDetailsScreen, .editAddress: navigationStackCoordinator.setSheetCoordinator(nil, animated: animated) clearRoute(animated: animated) diff --git a/ElementX/Sources/Other/SwiftUI/Views/OverridableAvatarImage.swift b/ElementX/Sources/Other/SwiftUI/Views/OverridableAvatarImage.swift index 2238ee2e5..30f9d7795 100644 --- a/ElementX/Sources/Other/SwiftUI/Views/OverridableAvatarImage.swift +++ b/ElementX/Sources/Other/SwiftUI/Views/OverridableAvatarImage.swift @@ -39,37 +39,3 @@ struct OverridableAvatarImage: View { } } } - -extension View { - func clipAvatar(isSpace: Bool, size: CGFloat) -> some View { - modifier(ClipAvatarModifier(isSpace: isSpace, size: size)) - } - - func clipAvatar(isSpace: Bool, scaledSize: ScaledMetric) -> some View { - modifier(ClipAvatarModifier(isSpace: isSpace, scaledSize: scaledSize)) - } -} - -struct ClipAvatarModifier: ViewModifier { - private let isSpace: Bool - @ScaledMetric private var scaledSize: CGFloat - - init(isSpace: Bool, size: CGFloat) { - self.isSpace = isSpace - _scaledSize = ScaledMetric(wrappedValue: size) - } - - init(isSpace: Bool, scaledSize: ScaledMetric) { - self.isSpace = isSpace - _scaledSize = scaledSize - } - - func body(content: Content) -> some View { - content - .clipShape(avatarShape) - } - - private var avatarShape: some Shape { - isSpace ? AnyShape(RoundedRectangle(cornerRadius: scaledSize / 4)) : AnyShape(Circle()) - } -} diff --git a/ElementX/Sources/Services/StateMachine/StateMachineFactory.swift b/ElementX/Sources/Services/StateMachine/StateMachineFactory.swift index 809618cdb..35a577bac 100644 --- a/ElementX/Sources/Services/StateMachine/StateMachineFactory.swift +++ b/ElementX/Sources/Services/StateMachine/StateMachineFactory.swift @@ -13,6 +13,7 @@ import SwiftState protocol StateMachineFactoryProtocol { func makeUserSessionFlowStateMachine(state: UserSessionFlowCoordinator.State) -> StateMachine func makeChatsFlowStateMachine() -> ChatsFlowCoordinatorStateMachine + func makeMembersFlowStateMachine(state: RoomMembersFlowCoordinator.State) -> StateMachine } struct StateMachineFactory: StateMachineFactoryProtocol { @@ -23,6 +24,10 @@ struct StateMachineFactory: StateMachineFactoryProtocol { func makeChatsFlowStateMachine() -> ChatsFlowCoordinatorStateMachine { .init() } + + func makeMembersFlowStateMachine(state: RoomMembersFlowCoordinator.State) -> StateMachine { + .init(state: state) + } } // MARK: For testing @@ -49,4 +54,14 @@ class PublishedStateMachineFactory: StateMachineFactoryProtocol { stateMachine.addTransitionHandler { [weak self] in self?.chatsFlowStatePublisher.send($0.toState) } return stateMachine } + + // MARK: MembersFlowCoordinator + + let membersFlowStatePublisher = PassthroughSubject() + + func makeMembersFlowStateMachine(state: RoomMembersFlowCoordinator.State) -> StateMachine { + let stateMachine = baseFactory.makeMembersFlowStateMachine(state: state) + stateMachine.addAnyHandler(.any => .any) { [weak self] in self?.membersFlowStatePublisher.send($0.toState) } + return stateMachine + } } diff --git a/UnitTests/Sources/RoomMembersFlowCoordinatorTests.swift b/UnitTests/Sources/RoomMembersFlowCoordinatorTests.swift new file mode 100644 index 000000000..0b5bb9a0a --- /dev/null +++ b/UnitTests/Sources/RoomMembersFlowCoordinatorTests.swift @@ -0,0 +1,80 @@ +// +// Copyright 2025 Element Creations Ltd. +// +// SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial +// Please see LICENSE files in the repository root for full details. +// + +import XCTest + +import Combine +@testable import ElementX + +@MainActor +class RoomMembersFlowCoordinatorTests: XCTestCase { + var membersFlowCoordinator: RoomMembersFlowCoordinator! + var navigationStackCoordinator: NavigationStackCoordinator! + let stateMachineFactory = PublishedStateMachineFactory() + + func testClearRoute() async throws { + try await setUp(entryPoint: .roomMembersList) + XCTAssertTrue(navigationStackCoordinator.stackCoordinators.last is RoomMembersListScreenCoordinator) + + var deferred = deferFulfillment(stateMachineFactory.membersFlowStatePublisher) { $0 == .roomMemberDetails(userID: "test", previousState: .roomMembersList) } + membersFlowCoordinator.handleAppRoute(.roomMemberDetails(userID: "test"), animated: false) + try await deferred.fulfill() + XCTAssertTrue(navigationStackCoordinator.stackCoordinators.last is RoomMemberDetailsScreenCoordinator) + + deferred = deferFulfillment(stateMachineFactory.membersFlowStatePublisher) { $0 == .roomMembersList } + let deferredAction = deferFulfillment(membersFlowCoordinator.actions) { action in + switch action { + case .finished: + true + default: + false + } + } + membersFlowCoordinator.clearRoute(animated: false) + try await deferred.fulfill() + try await deferredAction.fulfill() + XCTAssertTrue(navigationStackCoordinator.stackCoordinators.last is BlankFormCoordinator) + } + + private func setUp(entryPoint: RoomMembersFlowCoordinatorEntryPoint) async throws { + navigationStackCoordinator = NavigationStackCoordinator() + navigationStackCoordinator.setRootCoordinator(PlaceholderScreenCoordinator(hideBrandChrome: false)) + navigationStackCoordinator.push(BlankFormCoordinator()) + + let flowParameters = CommonFlowParameters(userSession: UserSessionMock(.init()), + bugReportService: BugReportServiceMock(.init()), + elementCallService: ElementCallServiceMock(.init()), + timelineControllerFactory: TimelineControllerFactoryMock(.init()), + emojiProvider: EmojiProvider(appSettings: ServiceLocator.shared.settings), + linkMetadataProvider: LinkMetadataProvider(), + appMediator: AppMediatorMock.default, + appSettings: ServiceLocator.shared.settings, + appHooks: AppHooks(), + analytics: ServiceLocator.shared.analytics, + userIndicatorController: UserIndicatorControllerMock(), + notificationManager: NotificationManagerMock(), + stateMachineFactory: stateMachineFactory) + + let roomProxy = JoinedRoomProxyMock(.init()) + membersFlowCoordinator = RoomMembersFlowCoordinator(entryPoint: entryPoint, + roomProxy: roomProxy, + navigationStackCoordinator: navigationStackCoordinator, + flowParameters: flowParameters) + + let deferred = deferFulfillment(stateMachineFactory.membersFlowStatePublisher) { state in + switch entryPoint { + case .roomMember(let userID): + state == .roomMemberDetails(userID: userID, previousState: .initial) + case .roomMembersList: + state == .roomMembersList + } + } + + membersFlowCoordinator.start() + try await deferred.fulfill() + } +}