Skip to content

Commit

Permalink
Memory management (#503)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@stefanceriu
stefanceriu authored Jan 31, 2023
1 parent 70cff44 commit d51a9b3
Show file tree
Hide file tree
Showing 13 changed files with 152 additions and 147 deletions.
35 changes: 17 additions & 18 deletions ElementX/Sources/Application/AppCoordinator.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -340,10 +343,6 @@ class AppCoordinator: AppCoordinatorProtocol {
}
.store(in: &userSessionCancellables)
}

private func deobserveUserSessionChanges() {
userSessionCancellables.removeAll()
}

// MARK: Toasts and loading indicators

Expand Down
99 changes: 58 additions & 41 deletions ElementX/Sources/Application/Navigation/NavigationCoordinators.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -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()
Expand All @@ -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
Expand Down Expand Up @@ -190,6 +186,10 @@ class NavigationSplitCoordinator: CoordinatorProtocol, ObservableObject, CustomS
AnyView(NavigationSplitCoordinatorView(navigationSplitCoordinator: self))
}

func stop() {
releaseAllCoordinatorReferences()
}

// MARK: - CustomStringConvertible

var description: String {
Expand All @@ -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
Expand Down Expand Up @@ -335,21 +354,21 @@ 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)
}
}

/// 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()
}
}
}
Expand All @@ -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()
}
}
}
Expand All @@ -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()
}
}
}
Expand All @@ -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()
}
}
}
Expand All @@ -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()
}
}
}
Expand All @@ -461,19 +477,18 @@ 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()
}
}
}
}

// 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
Expand Down Expand Up @@ -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)")
}
}
}

Expand All @@ -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)
}
}
Expand Down
20 changes: 16 additions & 4 deletions ElementX/Sources/Application/Navigation/NavigationModule.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,33 @@ 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) {
self.coordinator = coordinator
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)
}
}
Loading

0 comments on commit d51a9b3

Please sign in to comment.