From d51a9b3a2b926cad1c7d3034612b243cb5e4e90a Mon Sep 17 00:00:00 2001 From: Stefan Ceriu Date: Tue, 31 Jan 2023 11:51:56 +0200 Subject: [PATCH] Memory management (#503) * Correctly tear down the user session on signing out * Fix session verification <-> user session retain cycle, visible range debouncer leak * Manually clean up coordinators retained within SwiftUI's NavigationStacks * Slightly refactor the timeline content menu builder and prevent it from retaining the view model. Cleanup now unnecessarily optional RoomScreenCoordinator instance vars * Move coordinator dismissal logic to the navigation modules --- .../Sources/Application/AppCoordinator.swift | 35 ++++--- .../Navigation/NavigationCoordinators.swift | 99 +++++++++++-------- .../Navigation/NavigationModule.swift | 20 +++- .../NavigationRootCoordinator.swift | 11 ++- .../HomeScreen/HomeScreenViewModel.swift | 8 +- .../RoomScreen/RoomScreenCoordinator.swift | 41 ++------ .../Screens/RoomScreen/RoomScreenModels.swift | 3 +- .../RoomScreen/RoomScreenViewModel.swift | 37 +++---- .../RoomScreenViewModelProtocol.swift | 2 - .../View/TimelineItemContextMenu.swift | 27 ++--- .../View/TimelineTableViewController.swift | 10 +- .../RoomScreen/View/TimelineView.swift | 2 +- .../Services/Session/UserSession.swift | 4 +- 13 files changed, 152 insertions(+), 147 deletions(-) diff --git a/ElementX/Sources/Application/AppCoordinator.swift b/ElementX/Sources/Application/AppCoordinator.swift index cd44cce8d..13541d359 100644 --- a/ElementX/Sources/Application/AppCoordinator.swift +++ b/ElementX/Sources/Application/AppCoordinator.swift @@ -29,8 +29,9 @@ class AppCoordinator: AppCoordinatorProtocol { private var userSession: UserSessionProtocol! { didSet { - deobserveUserSessionChanges() - if let userSession, !userSession.isSoftLogout { + userSessionCancellables.removeAll() + + if userSession != nil { configureNotificationManager() observeUserSessionChanges() } @@ -145,14 +146,16 @@ class AppCoordinator: AppCoordinatorProtocol { self.setupUserSession() case (_, .signOut, .signingOut): self.showLoadingIndicator() - self.tearDownUserSession() + self.logout(isSoftLogout: false) case (.signingOut, .completedSigningOut, .signedOut): + self.tearDownUserSession() self.presentSplashScreen() self.hideLoadingIndicator() case (_, .remoteSignOut(let isSoft), .remoteSigningOut): self.showLoadingIndicator() - self.tearDownUserSession(isSoftLogout: isSoft) + self.logout(isSoftLogout: isSoft) case (.remoteSigningOut(let isSoft), .completedSigningOut, .signedOut): + self.tearDownUserSession() self.presentSplashScreen(isSoftLogout: isSoft) self.hideLoadingIndicator() default: @@ -251,34 +254,34 @@ class AppCoordinator: AppCoordinatorProtocol { navigationRootCoordinator.setRootCoordinator(navigationSplitCoordinator) } - private func tearDownUserSession(isSoftLogout: Bool = false) { + private func logout(isSoftLogout: Bool) { userSession.clientProxy.stopSync() userSessionFlowCoordinator?.stop() - deobserveUserSessionChanges() - guard !isSoftLogout else { stateMachine.processEvent(.completedSigningOut) return } Task { - showLoadingIndicator() - // first log out from the server _ = await userSession.clientProxy.logout() // regardless of the result, clear user data userSessionStore.logout(userSession: userSession) - userSession = nil - notificationManager?.delegate = nil - notificationManager = nil stateMachine.processEvent(.completedSigningOut) - - hideLoadingIndicator() } } + + private func tearDownUserSession() { + userSession = nil + + userSessionFlowCoordinator = nil + + notificationManager?.delegate = nil + notificationManager = nil + } private func presentSplashScreen(isSoftLogout: Bool = false) { navigationRootCoordinator.setRootCoordinator(SplashScreenCoordinator()) @@ -340,10 +343,6 @@ class AppCoordinator: AppCoordinatorProtocol { } .store(in: &userSessionCancellables) } - - private func deobserveUserSessionChanges() { - userSessionCancellables.removeAll() - } // MARK: Toasts and loading indicators diff --git a/ElementX/Sources/Application/Navigation/NavigationCoordinators.swift b/ElementX/Sources/Application/Navigation/NavigationCoordinators.swift index 8923283ef..bb0574d4f 100644 --- a/ElementX/Sources/Application/Navigation/NavigationCoordinators.swift +++ b/ElementX/Sources/Application/Navigation/NavigationCoordinators.swift @@ -28,13 +28,12 @@ class NavigationSplitCoordinator: CoordinatorProtocol, ObservableObject, CustomS didSet { if let oldValue { logPresentationChange("Remove sidebar", oldValue) - oldValue.coordinator.stop() - oldValue.dismissalCallback?() + oldValue.tearDown() } if let sidebarModule { logPresentationChange("Set sidebar", sidebarModule) - sidebarModule.coordinator.start() + sidebarModule.coordinator?.start() } updateCompactLayoutComponents() @@ -50,13 +49,12 @@ class NavigationSplitCoordinator: CoordinatorProtocol, ObservableObject, CustomS didSet { if let oldValue { logPresentationChange("Remove detail", oldValue) - oldValue.coordinator.stop() - oldValue.dismissalCallback?() + oldValue.tearDown() } if let detailModule { logPresentationChange("Set detail", detailModule) - detailModule.coordinator.start() + detailModule.coordinator?.start() } updateCompactLayoutComponents() @@ -72,13 +70,12 @@ class NavigationSplitCoordinator: CoordinatorProtocol, ObservableObject, CustomS didSet { if let oldValue { logPresentationChange("Remove sheet", oldValue) - oldValue.coordinator.stop() - oldValue.dismissalCallback?() + oldValue.tearDown() } if let sheetModule { logPresentationChange("Set sheet", sheetModule) - sheetModule.coordinator.start() + sheetModule.coordinator?.start() } updateCompactLayoutComponents() @@ -94,13 +91,12 @@ class NavigationSplitCoordinator: CoordinatorProtocol, ObservableObject, CustomS didSet { if let oldValue { logPresentationChange("Remove fullscreen cover", oldValue) - oldValue.coordinator.stop() - oldValue.dismissalCallback?() + oldValue.tearDown() } if let fullScreenCoverModule { logPresentationChange("Set fullscreen cover", fullScreenCoverModule) - fullScreenCoverModule.coordinator.start() + fullScreenCoverModule.coordinator?.start() } updateCompactLayoutComponents() @@ -123,7 +119,7 @@ class NavigationSplitCoordinator: CoordinatorProtocol, ObservableObject, CustomS @Published internal var compactLayoutStackModules: [NavigationModule] = [] var compactLayoutStackCoordinators: [any CoordinatorProtocol] { - compactLayoutStackModules.map(\.coordinator) + compactLayoutStackModules.compactMap(\.coordinator) } /// Default NavigationSplitCoordinator initialiser @@ -190,6 +186,10 @@ class NavigationSplitCoordinator: CoordinatorProtocol, ObservableObject, CustomS AnyView(NavigationSplitCoordinatorView(navigationSplitCoordinator: self)) } + func stop() { + releaseAllCoordinatorReferences() + } + // MARK: - CustomStringConvertible var description: String { @@ -207,8 +207,27 @@ class NavigationSplitCoordinator: CoordinatorProtocol, ObservableObject, CustomS // MARK: - Private + /// The NavigationStack has a tendency to hold on to path items for longer than needed. We work around that by manually nilling the coordinator + /// when a NavigationModule is dismissed. As the NavigationModule is just a wrapper multiple instances of it continuing living is of no consequence + /// https://stackoverflow.com/questions/73885353/found-a-strange-behaviour-of-state-when-combined-to-the-new-navigation-stack/ + /// + /// For added complexity, the NavigationSplitCoordinator has an internal compact layout NavigationStack for which we need to manually nil things again + private func releaseAllCoordinatorReferences() { + sidebarModule?.coordinator = nil + detailModule?.coordinator = nil + sheetModule?.coordinator = nil + fullScreenCoverModule?.coordinator = nil + + compactLayoutRootModule?.coordinator = nil + compactLayoutStackModules.forEach { module in + module.coordinator = nil + } + } + private func logPresentationChange(_ change: String, _ module: NavigationModule) { - MXLog.info("\(self) \(change): \(module.coordinator)") + if let coordinator = module.coordinator { + MXLog.info("\(self) \(change): \(coordinator)") + } } /// We need to update the compact layout whenever anything changes within the split coordinator or @@ -335,11 +354,11 @@ private struct NavigationSplitCoordinatorView: View { // Embedded NavigationStackCoordinators will present their sheets // through the NavigationSplitCoordinator as well. .sheet(item: $navigationSplitCoordinator.sheetModule) { module in - module.coordinator.toPresentable() + module.coordinator?.toPresentable() .tint(.element.accent) } .fullScreenCover(item: $navigationSplitCoordinator.fullScreenCoverModule) { module in - module.coordinator.toPresentable() + module.coordinator?.toPresentable() .tint(.element.accent) } } @@ -347,9 +366,9 @@ private struct NavigationSplitCoordinatorView: View { /// The NavigationStack that will be used in compact layouts var navigationStack: some View { NavigationStack(path: $navigationSplitCoordinator.compactLayoutStackModules) { - navigationSplitCoordinator.compactLayoutRootModule?.coordinator.toPresentable() + navigationSplitCoordinator.compactLayoutRootModule?.coordinator?.toPresentable() .navigationDestination(for: NavigationModule.self) { module in - module.coordinator.toPresentable() + module.coordinator?.toPresentable() } } } @@ -358,20 +377,20 @@ private struct NavigationSplitCoordinatorView: View { var navigationSplitView: some View { NavigationSplitView(columnVisibility: $columnVisibility) { if let sidebarModule = navigationSplitCoordinator.sidebarModule { - sidebarModule.coordinator.toPresentable() + sidebarModule.coordinator?.toPresentable() } else { - navigationSplitCoordinator.placeholderModule.coordinator.toPresentable() + navigationSplitCoordinator.placeholderModule.coordinator?.toPresentable() } } detail: { if let detailModule = navigationSplitCoordinator.detailModule { - detailModule.coordinator.toPresentable() + detailModule.coordinator?.toPresentable() } else { - navigationSplitCoordinator.placeholderModule.coordinator.toPresentable() + navigationSplitCoordinator.placeholderModule.coordinator?.toPresentable() } } .navigationSplitViewStyle(.balanced) .navigationDestination(for: NavigationModule.self) { module in - module.coordinator.toPresentable() + module.coordinator?.toPresentable() } } } @@ -386,13 +405,12 @@ class NavigationStackCoordinator: ObservableObject, CoordinatorProtocol, CustomS didSet { if let oldValue { logPresentationChange("Remove root", oldValue) - oldValue.coordinator.stop() - oldValue.dismissalCallback?() + oldValue.tearDown() } if let rootModule { logPresentationChange("Set root", rootModule) - rootModule.coordinator.start() + rootModule.coordinator?.start() } } } @@ -406,13 +424,12 @@ class NavigationStackCoordinator: ObservableObject, CoordinatorProtocol, CustomS didSet { if let oldValue { logPresentationChange("Remove sheet", oldValue) - oldValue.coordinator.stop() - oldValue.dismissalCallback?() + oldValue.tearDown() } if let sheetModule { logPresentationChange("Set sheet", sheetModule) - sheetModule.coordinator.start() + sheetModule.coordinator?.start() } } } @@ -433,13 +450,12 @@ class NavigationStackCoordinator: ObservableObject, CoordinatorProtocol, CustomS didSet { if let oldValue { logPresentationChange("Remove fullscreen cover", oldValue) - oldValue.coordinator.stop() - oldValue.dismissalCallback?() + oldValue.tearDown() } if let fullScreenCoverModule { logPresentationChange("Set fullscreen cover", fullScreenCoverModule) - fullScreenCoverModule.coordinator.start() + fullScreenCoverModule.coordinator?.start() } } } @@ -461,11 +477,10 @@ class NavigationStackCoordinator: ObservableObject, CoordinatorProtocol, CustomS switch change { case .insert(_, let module, _): logPresentationChange("Push", module) - module.coordinator.start() + module.coordinator?.start() case .remove(_, let module, _): logPresentationChange("Pop", module) - module.coordinator.stop() - module.dismissalCallback?() + module.tearDown() } } } @@ -473,7 +488,7 @@ class NavigationStackCoordinator: ObservableObject, CoordinatorProtocol, CustomS // The current navigation stack. Excludes the rootCoordinator var stackCoordinators: [any CoordinatorProtocol] { - stackModules.map(\.coordinator) + stackModules.compactMap(\.coordinator) } /// If this NavigationStackCoordinator will be embedded into a NavigationSplitCoordinator pass it here @@ -592,7 +607,9 @@ class NavigationStackCoordinator: ObservableObject, CoordinatorProtocol, CustomS // MARK: - Private private func logPresentationChange(_ change: String, _ module: NavigationModule) { - MXLog.info("\(self) \(change): \(module.coordinator)") + if let coordinator = module.coordinator { + MXLog.info("\(self) \(change): \(coordinator)") + } } } @@ -601,17 +618,17 @@ private struct NavigationStackCoordinatorView: View { var body: some View { NavigationStack(path: $navigationStackCoordinator.stackModules) { - navigationStackCoordinator.rootModule?.coordinator.toPresentable() + navigationStackCoordinator.rootModule?.coordinator?.toPresentable() .navigationDestination(for: NavigationModule.self) { module in - module.coordinator.toPresentable() + module.coordinator?.toPresentable() } } .sheet(item: $navigationStackCoordinator.sheetModule) { module in - module.coordinator.toPresentable() + module.coordinator?.toPresentable() .tint(.element.accent) } .fullScreenCover(item: $navigationStackCoordinator.fullScreenCoverModule) { module in - module.coordinator.toPresentable() + module.coordinator?.toPresentable() .tint(.element.accent) } } diff --git a/ElementX/Sources/Application/Navigation/NavigationModule.swift b/ElementX/Sources/Application/Navigation/NavigationModule.swift index e1d19a75e..061fc331d 100644 --- a/ElementX/Sources/Application/Navigation/NavigationModule.swift +++ b/ElementX/Sources/Application/Navigation/NavigationModule.swift @@ -18,9 +18,15 @@ import Foundation /// A CoordinatorProtocol wrapper and type erasing component that allows /// dynamically presenting arbitrary screens -struct NavigationModule: Identifiable, Hashable { +@MainActor +class NavigationModule: Identifiable, Hashable { let id = UUID() - let coordinator: any CoordinatorProtocol + + /// The NavigationStack has a tendency to hold on to path items for longer than needed. We work around that by manually nilling the coordinator + /// when a NavigationModule is dismissed, reason why this is an optional property + /// As the NavigationModule is just a wrapper multiple instances of it continuing living is of no consequence + /// https://stackoverflow.com/questions/73885353/found-a-strange-behaviour-of-state-when-combined-to-the-new-navigation-stack/ + var coordinator: (any CoordinatorProtocol)? let dismissalCallback: (() -> Void)? init(_ coordinator: any CoordinatorProtocol, dismissalCallback: (() -> Void)? = nil) { @@ -28,11 +34,17 @@ struct NavigationModule: Identifiable, Hashable { self.dismissalCallback = dismissalCallback } - static func == (lhs: NavigationModule, rhs: NavigationModule) -> Bool { + func tearDown() { + coordinator?.stop() + dismissalCallback?() + coordinator = nil + } + + nonisolated static func == (lhs: NavigationModule, rhs: NavigationModule) -> Bool { lhs.id == rhs.id } - func hash(into hasher: inout Hasher) { + nonisolated func hash(into hasher: inout Hasher) { hasher.combine(id) } } diff --git a/ElementX/Sources/Application/Navigation/NavigationRootCoordinator.swift b/ElementX/Sources/Application/Navigation/NavigationRootCoordinator.swift index 8ebf5afe7..7bcae3af8 100644 --- a/ElementX/Sources/Application/Navigation/NavigationRootCoordinator.swift +++ b/ElementX/Sources/Application/Navigation/NavigationRootCoordinator.swift @@ -20,13 +20,12 @@ class NavigationRootCoordinator: ObservableObject, CoordinatorProtocol, CustomSt @Published fileprivate var rootModule: NavigationModule? { didSet { if let oldValue { - oldValue.coordinator.stop() - oldValue.dismissalCallback?() + oldValue.tearDown() } if let rootModule { logPresentationChange("Set root", rootModule) - rootModule.coordinator.start() + rootModule.coordinator?.start() } } } @@ -66,7 +65,9 @@ class NavigationRootCoordinator: ObservableObject, CoordinatorProtocol, CustomSt // MARK: - Private private func logPresentationChange(_ change: String, _ module: NavigationModule) { - MXLog.info("\(self) \(change): \(module.coordinator)") + if let coordinator = module.coordinator { + MXLog.info("\(self) \(change): \(coordinator)") + } } } @@ -75,7 +76,7 @@ private struct NavigationRootCoordinatorView: View { var body: some View { ZStack { - rootCoordinator.rootModule?.coordinator.toPresentable() + rootCoordinator.rootModule?.coordinator?.toPresentable() } .animation(.elementDefault, value: rootCoordinator.rootModule) } diff --git a/ElementX/Sources/Screens/HomeScreen/HomeScreenViewModel.swift b/ElementX/Sources/Screens/HomeScreen/HomeScreenViewModel.swift index 619ae9b14..6b6a557b0 100644 --- a/ElementX/Sources/Screens/HomeScreen/HomeScreenViewModel.swift +++ b/ElementX/Sources/Screens/HomeScreen/HomeScreenViewModel.swift @@ -35,8 +35,6 @@ class HomeScreenViewModel: HomeScreenViewModelType, HomeScreenViewModelProtocol var callback: ((HomeScreenViewModelAction) -> Void)? - // MARK: - Setup - // swiftlint:disable:next function_body_length init(userSession: UserSessionProtocol, attributedStringBuilder: AttributedStringBuilderProtocol) { self.userSession = userSession @@ -65,8 +63,8 @@ class HomeScreenViewModel: HomeScreenViewModelType, HomeScreenViewModelProtocol visibleItemRangePublisher .debounce(for: 0.1, scheduler: DispatchQueue.main) .removeDuplicates() - .sink { range in - self.updateVisibleRange(range) + .sink { [weak self] range in + self?.updateVisibleRange(range) } .store(in: &cancellables) @@ -93,7 +91,7 @@ class HomeScreenViewModel: HomeScreenViewModelType, HomeScreenViewModelProtocol visibleRoomsSummaryProvider.roomListPublisher) .receive(on: DispatchQueue.main) .sink { [weak self] state, totalCount, rooms in - guard let self = self else { return } + guard let self else { return } let isLoadingData = state != .live && (totalCount == 0 || rooms.count != totalCount) let hasNoRooms = state == .live && totalCount == 0 diff --git a/ElementX/Sources/Screens/RoomScreen/RoomScreenCoordinator.swift b/ElementX/Sources/Screens/RoomScreen/RoomScreenCoordinator.swift index 2b3e7902a..b48b47c65 100644 --- a/ElementX/Sources/Screens/RoomScreen/RoomScreenCoordinator.swift +++ b/ElementX/Sources/Screens/RoomScreen/RoomScreenCoordinator.swift @@ -25,15 +25,11 @@ struct RoomScreenCoordinatorParameters { } final class RoomScreenCoordinator: CoordinatorProtocol { - private var parameters: RoomScreenCoordinatorParameters? + private var parameters: RoomScreenCoordinatorParameters - private var viewModel: RoomScreenViewModelProtocol? + private var viewModel: RoomScreenViewModelProtocol private var navigationStackCoordinator: NavigationStackCoordinator { - guard let parameters else { - fatalError() - } - - return parameters.navigationStackCoordinator + parameters.navigationStackCoordinator } init(parameters: RoomScreenCoordinatorParameters) { @@ -49,7 +45,7 @@ final class RoomScreenCoordinator: CoordinatorProtocol { // MARK: - Public func start() { - viewModel?.callback = { [weak self] action in + viewModel.callback = { [weak self] action in guard let self else { return } switch action { @@ -64,18 +60,11 @@ final class RoomScreenCoordinator: CoordinatorProtocol { } func stop() { - viewModel?.context.send(viewAction: .markRoomAsRead) - viewModel?.stop() - viewModel = nil - parameters = nil + viewModel.context.send(viewAction: .markRoomAsRead) } func toPresentable() -> AnyView { - guard let context = viewModel?.context else { - fatalError() - } - - return AnyView(RoomScreen(context: context)) + AnyView(RoomScreen(context: viewModel.context)) } // MARK: - Private @@ -91,14 +80,9 @@ final class RoomScreenCoordinator: CoordinatorProtocol { } private func displayEmojiPickerScreen(for itemId: String) { - guard let emojiProvider = parameters?.emojiProvider, - let timelineController = parameters?.timelineController else { - fatalError() - } - let emojiPickerNavigationStackCoordinator = NavigationStackCoordinator() - let params = EmojiPickerScreenCoordinatorParameters(emojiProvider: emojiProvider, + let params = EmojiPickerScreenCoordinatorParameters(emojiProvider: parameters.emojiProvider, itemId: itemId) let coordinator = EmojiPickerScreenCoordinator(parameters: params) coordinator.callback = { [weak self] action in @@ -107,7 +91,7 @@ final class RoomScreenCoordinator: CoordinatorProtocol { MXLog.debug("Selected \(emoji) for \(itemId)") self?.navigationStackCoordinator.setSheetCoordinator(nil) Task { - await timelineController.sendReaction(emoji, to: itemId) + await self?.parameters.timelineController.sendReaction(emoji, to: itemId) } case .dismiss: self?.navigationStackCoordinator.setSheetCoordinator(nil) @@ -121,14 +105,9 @@ final class RoomScreenCoordinator: CoordinatorProtocol { } private func displayRoomDetails() { - guard let roomProxy = parameters?.roomProxy, - let mediaProvider = parameters?.mediaProvider else { - return - } - let params = RoomDetailsCoordinatorParameters(navigationStackCoordinator: navigationStackCoordinator, - roomProxy: roomProxy, - mediaProvider: mediaProvider) + roomProxy: parameters.roomProxy, + mediaProvider: parameters.mediaProvider) let coordinator = RoomDetailsCoordinator(parameters: params) coordinator.callback = { [weak self] _ in self?.navigationStackCoordinator.pop() diff --git a/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift b/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift index 27df699cd..23fe76d0b 100644 --- a/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift +++ b/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift @@ -44,6 +44,7 @@ enum RoomScreenViewAction { case cancelEdit /// Mark the entire room as read - this is heavy handed as a starting point for now. case markRoomAsRead + case contextMenuAction(itemID: String, action: TimelineItemContextMenuAction) } struct RoomScreenViewState: BindableState { @@ -56,7 +57,7 @@ struct RoomScreenViewState: BindableState { var showLoading = false var bindings: RoomScreenViewStateBindings - var contextMenuBuilder: (@MainActor (_ itemId: String) -> TimelineItemContextMenu)? + var contextMenuActionProvider: (@MainActor (_ itemId: String) -> TimelineItemContextMenuActions?)? var composerMode: RoomScreenComposerMode = .default diff --git a/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift b/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift index cb7d5e49a..97d8a30e0 100644 --- a/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift +++ b/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift @@ -28,9 +28,7 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol private let timelineController: RoomTimelineControllerProtocol private let timelineViewFactory: RoomTimelineViewFactoryProtocol - - // MARK: - Setup - + init(timelineController: RoomTimelineControllerProtocol, timelineViewFactory: RoomTimelineViewFactoryProtocol, mediaProvider: MediaProviderProtocol, @@ -72,7 +70,13 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol } .store(in: &cancellables) - state.contextMenuBuilder = buildContextMenuForItemId(_:) + state.contextMenuActionProvider = { [weak self] itemId -> TimelineItemContextMenuActions? in + guard let self else { + return nil + } + + return self.contextMenuActionsForItemId(itemId) + } buildTimelineViews() } @@ -109,13 +113,10 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol state.bindings.composerText = "" case .markRoomAsRead: await markRoomAsRead() + case .contextMenuAction(let itemID, let action): + processContentMenuAction(action, itemID: itemID) } } - - func stop() { - cancellables.removeAll() - state.contextMenuBuilder = nil - } // MARK: - Private @@ -197,22 +198,16 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol // MARK: ContextMenus - private func buildContextMenuForItemId(_ itemId: String) -> TimelineItemContextMenu { - TimelineItemContextMenu(contextMenuActions: contextMenuActionsForItemId(itemId)) { [weak self] action in - self?.processContentMenuAction(action, itemId: itemId) - } - } - - private func contextMenuActionsForItemId(_ itemId: String) -> TimelineItemContextMenuActions { + private func contextMenuActionsForItemId(_ itemId: String) -> TimelineItemContextMenuActions? { guard let timelineItem = timelineController.timelineItems.first(where: { $0.id == itemId }), let item = timelineItem as? EventBasedTimelineItemProtocol else { // Don't show a context menu for non-event based items. - return .empty + return nil } if timelineItem is StateRoomTimelineItem { // Don't show a context menu for state events. - return .empty + return nil } var actions: [TimelineItemContextMenuAction] = [ @@ -238,8 +233,8 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol } // swiftlint:disable:next cyclomatic_complexity - private func processContentMenuAction(_ action: TimelineItemContextMenuAction, itemId: String) { - guard let timelineItem = timelineController.timelineItems.first(where: { $0.id == itemId }), + private func processContentMenuAction(_ action: TimelineItemContextMenuAction, itemID: String) { + guard let timelineItem = timelineController.timelineItems.first(where: { $0.id == itemID }), let item = timelineItem as? EventBasedTimelineItemProtocol else { return } @@ -265,7 +260,7 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol } case .redact: Task { - await timelineController.redact(itemId) + await timelineController.redact(itemID) } case .reply: state.bindings.composerFocused = true diff --git a/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModelProtocol.swift b/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModelProtocol.swift index 7503da91f..c90575394 100644 --- a/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModelProtocol.swift +++ b/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModelProtocol.swift @@ -20,6 +20,4 @@ import Foundation protocol RoomScreenViewModelProtocol { var callback: ((RoomScreenViewModelAction) -> Void)? { get set } var context: RoomScreenViewModelType.Context { get } - - func stop() } diff --git a/ElementX/Sources/Screens/RoomScreen/View/TimelineItemContextMenu.swift b/ElementX/Sources/Screens/RoomScreen/View/TimelineItemContextMenu.swift index 20bd1662d..8d6360d04 100644 --- a/ElementX/Sources/Screens/RoomScreen/View/TimelineItemContextMenu.swift +++ b/ElementX/Sources/Screens/RoomScreen/View/TimelineItemContextMenu.swift @@ -17,11 +17,17 @@ import SwiftUI struct TimelineItemContextMenuActions { - static var empty: TimelineItemContextMenuActions { .init(actions: [], debugActions: []) } - let actions: [TimelineItemContextMenuAction] let debugActions: [TimelineItemContextMenuAction] - var isEmpty: Bool { actions.isEmpty && debugActions.isEmpty } + + init?(actions: [TimelineItemContextMenuAction], debugActions: [TimelineItemContextMenuAction]) { + if actions.isEmpty, debugActions.isEmpty { + return nil + } + + self.actions = actions + self.debugActions = debugActions + } } enum TimelineItemContextMenuAction: Identifiable, Hashable { @@ -52,16 +58,11 @@ public struct TimelineItemContextMenu: View { let callback: (TimelineItemContextMenuAction) -> Void public var body: some View { - if contextMenuActions.isEmpty { - // When there are no actions make sure then menu isn't shown. - EmptyView() - } else { - viewsForActions(contextMenuActions.actions) - Menu { - viewsForActions(contextMenuActions.debugActions) - } label: { - Label("Developer", systemImage: "hammer") - } + viewsForActions(contextMenuActions.actions) + Menu { + viewsForActions(contextMenuActions.debugActions) + } label: { + Label("Developer", systemImage: "hammer") } } diff --git a/ElementX/Sources/Screens/RoomScreen/View/TimelineTableViewController.swift b/ElementX/Sources/Screens/RoomScreen/View/TimelineTableViewController.swift index b4d58f873..c43184c0c 100644 --- a/ElementX/Sources/Screens/RoomScreen/View/TimelineTableViewController.swift +++ b/ElementX/Sources/Screens/RoomScreen/View/TimelineTableViewController.swift @@ -72,7 +72,7 @@ class TimelineTableViewController: UIViewController { } } - var contextMenuBuilder: (@MainActor (_ itemId: String) -> TimelineItemContextMenu)? + var contextMenuActionProvider: (@MainActor (_ itemId: String) -> TimelineItemContextMenuActions?)? @Binding private var scrollToBottomButtonVisible: Bool @@ -189,7 +189,7 @@ class TimelineTableViewController: UIViewController { // A local reference to avoid capturing self in the cell configuration. let coordinator = self.coordinator let opacity = self.opacity(for: timelineItem) - let contextMenuBuilder = self.contextMenuBuilder + let contextMenuActionProvider = self.contextMenuActionProvider cell.item = timelineItem cell.contentConfiguration = UIHostingConfiguration { @@ -198,7 +198,11 @@ class TimelineTableViewController: UIViewController { .frame(maxWidth: .infinity, alignment: .leading) .opacity(opacity) .contextMenu { - contextMenuBuilder?(timelineItem.id) + contextMenuActionProvider?(timelineItem.id).map { actions in + TimelineItemContextMenu(contextMenuActions: actions) { action in + coordinator.send(viewAction: .contextMenuAction(itemID: timelineItem.id, action: action)) + } + } } .onAppear { coordinator.send(viewAction: .itemAppeared(id: timelineItem.id)) diff --git a/ElementX/Sources/Screens/RoomScreen/View/TimelineView.swift b/ElementX/Sources/Screens/RoomScreen/View/TimelineView.swift index a75331814..88862a914 100644 --- a/ElementX/Sources/Screens/RoomScreen/View/TimelineView.swift +++ b/ElementX/Sources/Screens/RoomScreen/View/TimelineView.swift @@ -70,7 +70,7 @@ struct TimelineView: UIViewControllerRepresentable { } // Doesn't have an equatable conformance :( - tableViewController.contextMenuBuilder = context.viewState.contextMenuBuilder + tableViewController.contextMenuActionProvider = context.viewState.contextMenuActionProvider } func send(viewAction: RoomScreenViewAction) { diff --git a/ElementX/Sources/Services/Session/UserSession.swift b/ElementX/Sources/Services/Session/UserSession.swift index f1fdd7d6d..439ce0980 100644 --- a/ElementX/Sources/Services/Session/UserSession.swift +++ b/ElementX/Sources/Services/Session/UserSession.swift @@ -66,10 +66,10 @@ class UserSession: UserSessionProtocol { self.sessionVerificationController = sessionVerificationController - sessionVerificationController.callbacks.sink { callback in + sessionVerificationController.callbacks.sink { [weak self] callback in switch callback { case .finished: - self.callbacks.send(.didVerifySession) + self?.callbacks.send(.didVerifySession) default: break }