Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix RootViewModel memory leaks #623

Merged
merged 14 commits into from
Apr 5, 2019
Merged

Fix RootViewModel memory leaks #623

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
cce5422
Update user locale notification on change currency
ifbarrera Mar 1, 2019
a8f6952
Refactor RootViewModel so that no view controllers are retained by si…
justinswart Mar 1, 2019
2bceffd
Merge branch 'master' of https://github.com/kickstarter/ios-oss into …
ifbarrera Mar 13, 2019
6ac2fc8
Merge branch 'master' into select-currency-locale-notification
ifbarrera Mar 20, 2019
0129450
Merge branch 'master' into select-currency-locale-notification
justinswart Mar 29, 2019
8768479
Add RootViewControllerIndex typealias
justinswart Apr 4, 2019
57d44ac
Put tests back
justinswart Apr 4, 2019
549c67b
Improve guard and remove unused code
justinswart Apr 4, 2019
9af9a53
swiftlint
justinswart Apr 4, 2019
c585002
Use generics to make code more DRY
justinswart Apr 4, 2019
a1e35da
Use RootViewControllerIndex
justinswart Apr 4, 2019
2079f91
swiftlint
justinswart Apr 4, 2019
040fd22
Add index clamping
justinswart Apr 5, 2019
695e5bb
Merge branch 'master' into select-currency-locale-notification
justinswart Apr 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
216 changes: 123 additions & 93 deletions Kickstarter-iOS/ViewModels/RootViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,29 +5,97 @@ import ReactiveSwift
import Result
import UIKit

typealias RootViewControllerIndex = Int

internal enum RootViewControllerData: Equatable {
case discovery
case activities
case search
case dashboard(isMember: Bool)
case profile(isLoggedIn: Bool)

var viewController: UIViewController? {
switch self {
case .discovery:
return DiscoveryViewController.instantiate()
case .activities:
return ActivitiesViewController.instantiate()
case.search:
return SearchViewController.instantiate()
case .dashboard(let isMember):
return isMember ? DashboardViewController.instantiate() : nil
case .profile(let isLoggedIn):
return isLoggedIn
? BackerDashboardViewController.instantiate()
: LoginToutViewController.configuredWith(loginIntent: .generic)
}
}

static func == (lhs: RootViewControllerData, rhs: RootViewControllerData) -> Bool {
switch (lhs, rhs) {
case (.discovery, .discovery): return true
case (.activities, .activities): return true
case (.search, .search): return true
case (.dashboard(let lhsIsMember), .dashboard(let rhsIsMember)):
return lhsIsMember == rhsIsMember
case (.profile(let lhsIsLoggedIn), .profile(let rhsIsLoggedIn)):
return lhsIsLoggedIn == rhsIsLoggedIn
default:
return false
}
}

var isNil: Bool {
switch self {
case .dashboard(let isMember):
return !isMember
default:
return false
}
}

var isDashBoard: Bool {
switch self {
case .dashboard:
return true
default:
return false
}
}

var isProfile: Bool {
switch self {
case .profile:
return true
default:
return false
}
}
}

internal struct TabBarItemsData {
internal let items: [TabBarItem]
internal let isLoggedIn: Bool
internal let isMember: Bool
}

internal enum TabBarItem {
case activity(index: Int)
case dashboard(index: Int)
case home(index: Int)
case profile(avatarUrl: URL?, index: Int)
case search(index: Int)
case activity(index: RootViewControllerIndex)
case dashboard(index: RootViewControllerIndex)
case home(index: RootViewControllerIndex)
case profile(avatarUrl: URL?, index: RootViewControllerIndex)
case search(index: RootViewControllerIndex)
}

internal protocol RootViewModelInputs {
/// Call when the controller has received a user updated notification.
func currentUserUpdated()

/// Call before selected tab bar index changes.
func shouldSelect(index: Int?)
func shouldSelect(index: RootViewControllerIndex?)

/// Call when selected tab bar index changes.
func didSelect(index: Int)
func didSelect(index: RootViewControllerIndex)

/// Call when we should switch to the activities tab.
func switchToActivities()
Expand Down Expand Up @@ -62,20 +130,20 @@ internal protocol RootViewModelInputs {

internal protocol RootViewModelOutputs {
/// Emits when the discovery VC should filter with specific params.
var filterDiscovery: Signal<(DiscoveryViewController, DiscoveryParams), NoError> { get }
var filterDiscovery: Signal<(RootViewControllerIndex, DiscoveryParams), NoError> { get }

/// Emits a controller that should be scrolled to the top. This requires figuring out what kind of
/// Emits a controller index that should be scrolled to the top. This requires figuring out what kind of
/// controller it is, and setting its `contentOffset`.
var scrollToTop: Signal<UIViewController, NoError> { get }
var scrollToTop: Signal<RootViewControllerIndex, NoError> { get }

/// Emits an index that the tab bar should be switched to.
var selectedIndex: Signal<Int, NoError> { get }
var selectedIndex: Signal<RootViewControllerIndex, NoError> { get }

/// Emits the array of view controllers that should be set on the tab bar.
var setViewControllers: Signal<[UIViewController], NoError> { get }
var setViewControllers: Signal<[RootViewControllerData], NoError> { get }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to rename this to viewControllersData or similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this type is specific to the RootViewModel and RootTabBarViewController so it shouldn't really be used anywhere else.


/// Emits when the dashboard should switch projects.
var switchDashboardProject: Signal<(DashboardViewController, Param), NoError> { get }
var switchDashboardProject: Signal<(RootViewControllerIndex, Param), NoError> { get }

/// Emits data for setting tab bar item styles.
var tabBarItemsData: Signal<TabBarItemsData, NoError> { get }
Expand Down Expand Up @@ -103,25 +171,22 @@ internal final class RootViewModel: RootViewModelType, RootViewModelInputs, Root

let standardViewControllers = self.viewDidLoadProperty.signal.map { generateStandardViewControllers() }
let personalizedViewControllers = userState.map { generatePersonalizedViewControllers(userState: $0) }
.map { $0.compact() }

let viewControllers = Signal.combineLatest(standardViewControllers, personalizedViewControllers).map(+)

let refreshedViewControllers = userState.takeWhen(self.userLocalePreferencesChangedProperty.signal)
.map { userState -> [UIViewController?] in
.map { userState -> [RootViewControllerData] in
let standard = generateStandardViewControllers()
let personalized = generatePersonalizedViewControllers(userState: userState)

return [standard, personalized].flatMap { $0 }
return standard + personalized
}
.map { $0.compact() }

self.setViewControllers = Signal.merge(
viewControllers,
refreshedViewControllers
).map {
$0.map(UINavigationController.init(rootViewController:))
}
)
.map { $0.filter { !$0.isNil } }

let loginState = userState.map { $0.isLoggedIn }
let vcCount = self.setViewControllers.map { $0.count }
Expand All @@ -136,60 +201,50 @@ internal final class RootViewModel: RootViewModelType, RootViewModelInputs, Root
.filter { isTrue($1) }
.map(first)

let discovery = viewControllers
.map(first(DiscoveryViewController.self))
let discoveryControllerIndex = self.setViewControllers
.map { $0.index(of: .discovery) }
.skipNil()

self.filterDiscovery = discovery
self.filterDiscovery = discoveryControllerIndex
.takePairWhen(self.switchToDiscoveryProperty.signal.skipNil())

let dashboard = viewControllers
.map(first(DashboardViewController.self))
let dashboardControllerIndex = self.setViewControllers
.map { $0.index(where: { $0.isDashBoard }) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there actually a need for isDashBoard property? Can't we simply do $0.index(of: .dashboard)? Also looks like we don't use DashBoard casing anywhere else (mostly dashboard or Dashboard) in case we're keeping this property

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.map { $0.index(of: .dashboard) } doesn't work because dashboard has an associated value. Also, we can't make it isdashboard 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isDashboard? 😄

.skipNil()

self.switchDashboardProject =
Signal.combineLatest(dashboard, self.switchToDashboardProperty.signal.skipNil(),
loginState)
self.switchDashboardProject = Signal
.combineLatest(dashboardControllerIndex, self.switchToDashboardProperty.signal.skipNil(), loginState)
.filter { _, _, loginState in
isTrue(loginState)
}
.map { dashboard, param, _ in
(dashboard, param)
}

self.selectedIndex =
Signal.combineLatest(
.merge(
self.viewDidLoadProperty.signal.mapConst(0),
self.didSelectIndexProperty.signal,
self.switchToActivitiesProperty.signal.mapConst(1),
self.switchToDiscoveryProperty.signal.mapConst(0),
self.switchToSearchProperty.signal.mapConst(2),
switchToLogin,
switchToProfile,
self.switchToDashboardProperty.signal.mapConst(3)
),
self.setViewControllers,
self.viewDidLoadProperty.signal)
.map { idx, vcs, _ in clamp(0, vcs.count - 1)(idx) }
self.selectedIndex = Signal.combineLatest(
.merge(
self.viewDidLoadProperty.signal.mapConst(0),
self.didSelectIndexProperty.signal,
self.switchToActivitiesProperty.signal.mapConst(1),
self.switchToDiscoveryProperty.signal.mapConst(0),
self.switchToSearchProperty.signal.mapConst(2),
switchToLogin,
switchToProfile,
self.switchToDashboardProperty.signal.mapConst(3)
),
self.setViewControllers,
self.viewDidLoadProperty.signal
)
.map { idx, vcs, _ in clamp(0, vcs.count - 1)(idx) }

let shouldSelectIndex = self.shouldSelectIndexProperty.signal
.skipNil()

let selectedTabAgain = self.selectedIndex
self.scrollToTop = self.selectedIndex
.takePairWhen(shouldSelectIndex)
.filter { prev, next in prev == next }
.map { $1 }

self.scrollToTop = Signal.combineLatest(
self.setViewControllers,
selectedTabAgain
)
.filter { vcs, idx in idx < vcs.count }
.map { vcs, idx in vcs[idx] }
.map(extractViewController)
.skipNil()

self.tabBarItemsData = Signal.combineLatest(currentUser, .merge(
self.viewDidLoadProperty.signal,
self.userLocalePreferencesChangedProperty.signal.ignoreValues())
Expand All @@ -212,26 +267,32 @@ internal final class RootViewModel: RootViewModelType, RootViewModelInputs, Root
internal func didSelect(index: Int) {
self.didSelectIndexProperty.value = index
}

fileprivate let switchToActivitiesProperty = MutableProperty(())
internal func switchToActivities() {
self.switchToActivitiesProperty.value = ()
}

fileprivate let switchToDashboardProperty = MutableProperty<Param?>(nil)
internal func switchToDashboard(project param: Param?) {
self.switchToDashboardProperty.value = param
}

fileprivate let switchToDiscoveryProperty = MutableProperty<DiscoveryParams?>(nil)
internal func switchToDiscovery(params: DiscoveryParams?) {
self.switchToDiscoveryProperty.value = params
}

fileprivate let switchToLoginProperty = MutableProperty(())
internal func switchToLogin() {
self.switchToLoginProperty.value = ()
}

fileprivate let switchToProfileProperty = MutableProperty(())
internal func switchToProfile() {
self.switchToProfileProperty.value = ()
}

fileprivate let switchToSearchProperty = MutableProperty(())
internal func switchToSearch() {
self.switchToSearchProperty.value = ()
Expand All @@ -256,33 +317,24 @@ internal final class RootViewModel: RootViewModelType, RootViewModelInputs, Root
self.viewDidLoadProperty.value = ()
}

internal let filterDiscovery: Signal<(DiscoveryViewController, DiscoveryParams), NoError>
internal let scrollToTop: Signal<UIViewController, NoError>
internal let selectedIndex: Signal<Int, NoError>
internal let setViewControllers: Signal<[UIViewController], NoError>
internal let switchDashboardProject: Signal<(DashboardViewController, Param), NoError>
internal let filterDiscovery: Signal<(RootViewControllerIndex, DiscoveryParams), NoError>
internal let scrollToTop: Signal<RootViewControllerIndex, NoError>
internal let selectedIndex: Signal<RootViewControllerIndex, NoError>
internal let setViewControllers: Signal<[RootViewControllerData], NoError>
internal let switchDashboardProject: Signal<(Int, Param), NoError>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this Int or RootViewControllerIndex?

internal let tabBarItemsData: Signal<TabBarItemsData, NoError>

internal var inputs: RootViewModelInputs { return self }
internal var outputs: RootViewModelOutputs { return self }
}

private func generateStandardViewControllers() -> [UIViewController] {
return [
DiscoveryViewController.instantiate(),
ActivitiesViewController.instantiate(),
SearchViewController.instantiate()
]
private func generateStandardViewControllers() -> [RootViewControllerData] {
return [.discovery, .activities, .search]
}

private func generatePersonalizedViewControllers(userState: (isMember: Bool, isLoggedIn: Bool))
-> [UIViewController?] {
let dashboardViewController: UIViewController? = userState.isMember
? DashboardViewController.instantiate() : nil
let loginProfileViewController: UIViewController = userState.isLoggedIn
? profileController() : LoginToutViewController.configuredWith(loginIntent: .generic)

return [dashboardViewController, loginProfileViewController]
-> [RootViewControllerData] {
return [.dashboard(isMember: userState.isMember), .profile(isLoggedIn: userState.isLoggedIn)]
}

private func tabData(forUser user: User?) -> TabBarItemsData {
Expand Down Expand Up @@ -324,25 +376,3 @@ extension TabBarItem: Equatable {
}
}
}

private func first<VC: UIViewController>(_ viewController: VC.Type) -> ([UIViewController]) -> VC? {

return { viewControllers in
viewControllers
.index { $0 is VC }
.flatMap { viewControllers[$0] as? VC }
}
}

private func profileController() -> UIViewController {

return BackerDashboardViewController.instantiate()
}

private func extractViewController(_ viewController: UIViewController) -> UIViewController? {
if let navigationController = viewController as? UINavigationController {
return navigationController.viewControllers.count == 1 ? navigationController.viewControllers.first : nil
} else {
return viewController
}
}
18 changes: 13 additions & 5 deletions Kickstarter-iOS/ViewModels/RootViewModelTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ final class RootViewModelTests: TestCase {
let vm: RootViewModelType = RootViewModel()
let viewControllerNames = TestObserver<[String], NoError>()
let filterDiscovery = TestObserver<DiscoveryParams, NoError>()
let selectedIndex = TestObserver<Int, NoError>()
let selectedIndex = TestObserver<RootViewControllerIndex, NoError>()
let scrollToTopControllerName = TestObserver<String, NoError>()
let switchDashboardProject = TestObserver<Param, NoError>()
let tabBarItemsData = TestObserver<TabBarItemsData, NoError>()
Expand All @@ -29,7 +29,11 @@ final class RootViewModelTests: TestCase {
self.vm.outputs.selectedIndex.observe(self.selectedIndex.observer)
self.vm.outputs.switchDashboardProject.map(second).observe(self.switchDashboardProject.observer)

self.vm.outputs.scrollToTop
let viewControllers = self.vm.outputs.setViewControllers
.map { $0.map { $0.viewController }.compact() }

Signal.combineLatest(viewControllers, self.vm.outputs.scrollToTop)
.map { (vcs, idx) in vcs[idx] }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about also checkin whether index < vcs.count - 1

.map(extractName)
.observe(self.scrollToTopControllerName.observer)

Expand Down Expand Up @@ -165,7 +169,7 @@ final class RootViewModelTests: TestCase {

self.vm.inputs.didSelect(index: 10)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we still keep this test? I'm thinking about the scenario where a creator is logged in (5 tabs) and logs out (4 tabs).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, can't remember why I removed it.


self.selectedIndex.assertValues([0, 1, 0, 3], "Selecting index out of range safely clamps to bounds.")
self.selectedIndex.assertValues([0, 1, 0, 3], "Selects index immediately.")
}

func testScrollToTop() {
Expand Down Expand Up @@ -328,8 +332,12 @@ final class RootViewModelTests: TestCase {
}
}

private func extractRootNames(_ vcs: [UIViewController]) -> [String] {
return vcs.compactMap(extractRootName)
private func extractRootNames(_ vcs: [RootViewControllerData]) -> [String] {
return vcs
.map { $0.viewController }
.compact()
.map(UINavigationController.init(rootViewController:))
.compactMap(extractRootName)
}

private func extractRootName(_ vc: UIViewController) -> String? {
Expand Down
Loading