Fix RootViewModel memory leaks #623

justinswart · 2019-03-01T22:56:25Z

📲 What

@ifbarrera and I paired on bringing back the .ksr_userLocalePreferencesChanged and fixing the memory leaks that we suspected were being created by retaining view controllers in Signals in RootViewModel.

🤔 Why

In order for currency selection changes to have immediate effect in the app, we need to recreate the underlying view controllers on the tab bar when a currency preference change is made. For this reason we post the .ksr_userLocalePreferencesChanged notification and respond to that in RootViewModel by emitting a new array of root UIViewControllers.

Previously this was causing these view controllers to leak due to the way the Signals were set up, they were retaining their last emission in some cases.

🛠 How

Updated the Signals that previously emitted actual UIViewControllerarrays to instead emit an enum which we've called RootViewControllerData so that the instantiation of those view controllers could be handled outside of the view model and thereby not be retained and leaked.

NB: This is a change to our app's navigation so it should be tested fairly thoroughly for regressions.

✅ Acceptance criteria

Tests pass
Navigation works as before
Scroll to top works as before

…gnals

ifbarrera · 2019-03-04T15:02:46Z

Re-add "userLocaleChanged" notification after user changes currency

ifbarrera · 2019-03-13T14:55:12Z

Fix root view controller memory leak

…select-currency-locale-notification

ifbarrera · 2019-03-13T15:41:05Z

@dusi this is now ready for review at your convenience :)

dusi

Requested couple questions

dusi · 2019-04-04T18:16:53Z

Kickstarter-iOS/ViewModels/RootViewModel.swift

  /// controller it is, and setting its `contentOffset`.
-  var scrollToTop: Signal<UIViewController, NoError> { get }
+  var scrollViewControllerAtIndexToTop: Signal<Int, NoError> { get }


Looks like we have renamed this signal to be more verbose and explicitly state that we're dealing with view controller at index... but still haven't renamed the other signals.

I wonder if using a more specific type would help us be more descriptive?

i.e.

typealias ViewControllerAtIndex = Int // or typealias RootViewControllerIndex = Int var filterDiscovery: Signal<(ViewControllerAtIndex, DiscoveryParams), NoError> { get } var scrollToTop: Signal<ViewControllerAtIndex, NoError> { get } ...

Sure, can make that change.

dusi · 2019-04-04T18:22:17Z

Kickstarter-iOS/ViewModels/RootViewModel.swift

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


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

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

isDashboard? 😄

dusi · 2019-04-04T18:41:46Z

Kickstarter-iOS/ViewModels/RootViewModel.swift


  /// Emits an index that the tab bar should be switched to.
  var selectedIndex: Signal<Int, 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 }


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

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

dusi · 2019-04-04T18:46:21Z

Kickstarter-iOS/ViewModels/RootViewModelTests.swift

+      .map { $0.map { $0.viewController }.compact() }
+
+    Signal.combineLatest(viewControllers, self.vm.outputs.scrollViewControllerAtIndexToTop)
+      .map { $0[$1] }


Wonder if .map { (vcs, idx) in vcs[idx] } would make this more readable 🤔

dusi · 2019-04-04T18:48:06Z

Kickstarter-iOS/ViewModels/RootViewModelTests.swift

@@ -162,10 +166,6 @@ final class RootViewModelTests: TestCase {
    self.vm.inputs.didSelect(index: 0)

    self.selectedIndex.assertValues([0, 1, 0], "Selects index immediately.")
-
-    self.vm.inputs.didSelect(index: 10)


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).

Sure, can't remember why I removed it.

dusi · 2019-04-04T18:53:19Z

Kickstarter-iOS/Views/Controllers/RootTabBarViewController.swift

+    self.viewModel.outputs.scrollViewControllerAtIndexToTop
+      .observeForControllerAction()
+      .map { [weak self] index -> UIViewController? in
+        guard let vc = self?.viewControllers?[index] else { return nil }


Should we make sure index is smaller than viewControllers.count - 1?

Again, can't remember because it's been pretty long since I made this change but I think I determined that because of this change this should be impossible, but doesn't hurt to keep it in 👍

dusi · 2019-04-04T18:55:10Z

Kickstarter-iOS/Views/Controllers/RootTabBarViewController.swift

      .observeValues { $0.filter(with: $1) }

    self.viewModel.outputs.switchDashboardProject
      .observeForControllerAction()
-      .observeValues { $0.`switch`(toProject: $1) }
+      .map { [weak self] index, param -> (DashboardViewController, Param)? in


Looks like duplicate logic happening in these two signals...should we extract this to a private helper function to keep it more DRY?

dusi · 2019-04-04T18:57:14Z

Kickstarter-iOS/Views/Controllers/RootTabBarViewController.swift

@@ -291,3 +313,22 @@ private func strokedRoundImage(fromImage image: UIImage?,

  return UIGraphicsGetImageFromCurrentImageContext()?.withRenderingMode(.alwaysOriginal)
 }
+
+private func first<VC: UIViewController>(_ viewController: VC.Type) -> ([UIViewController]) -> VC? {


Looks like the code compiles even without this func 🤔

dusi · 2019-04-04T23:13:31Z

Kickstarter-iOS/ViewModels/RootViewModel.swift

+  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>


Is this Int or RootViewControllerIndex?

dusi · 2019-04-04T23:14:06Z

Kickstarter-iOS/ViewModels/RootViewModelTests.swift

+      .map { $0.map { $0.viewController }.compact() }
+
+    Signal.combineLatest(viewControllers, self.vm.outputs.scrollToTop)
+      .map { (vcs, idx) in vcs[idx] }


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

dusi · 2019-04-04T23:15:22Z

Kickstarter-iOS/Views/Controllers/RootTabBarViewController.swift

@@ -126,6 +143,11 @@ public final class RootTabBarViewController: UITabBarController {
    self.viewModel.inputs.switchToSearch()
  }

+  private func viewControllerAndParam<T, P>(with index: RootViewControllerIndex, param: P) -> (T, P)? {
+    guard let vc = self.viewControllers?[index] as? T else { return nil }


Same index check here (index < self.viewControllers.count - 1)?

dusi

🚢 🚢 🚢
🚢 🚢 🚢
🚢 🚢 🚢

ifbarrera and others added 2 commits March 1, 2019 12:20

Update user locale notification on change currency

cce5422

Refactor RootViewModel so that no view controllers are retained by si…

a8f6952

…gnals

justinswart assigned dusi Mar 1, 2019

justinswart requested review from dusi and ifbarrera March 1, 2019 22:56

Merge branch 'master' of https://github.com/kickstarter/ios-oss into …

2bceffd

…select-currency-locale-notification

ifbarrera marked this pull request as ready for review March 13, 2019 15:39

ifbarrera added the needs review label Mar 13, 2019

ifbarrera changed the title ~~Post notification on currency change and fix memory leaks~~ Fix RootViewModel memory leaks Mar 13, 2019

Merge branch 'master' into select-currency-locale-notification

6ac2fc8

dusi added blocked a PR that is blocked for external reasons and removed needs review labels Mar 21, 2019

Merge branch 'master' into select-currency-locale-notification

0129450

dusi reviewed Apr 4, 2019

View reviewed changes

justinswart added 7 commits April 4, 2019 14:03

Add RootViewControllerIndex typealias

8768479

Put tests back

57d44ac

Improve guard and remove unused code

549c67b

swiftlint

9af9a53

Use generics to make code more DRY

c585002

Use RootViewControllerIndex

a1e35da

swiftlint

2079f91

dusi reviewed Apr 4, 2019

View reviewed changes

dusi removed the blocked a PR that is blocked for external reasons label Apr 5, 2019

justinswart added 2 commits April 5, 2019 09:49

Add index clamping

040fd22

Merge branch 'master' into select-currency-locale-notification

695e5bb

dusi approved these changes Apr 5, 2019

View reviewed changes

justinswart merged commit f0bf3cf into master Apr 5, 2019

justinswart deleted the select-currency-locale-notification branch April 5, 2019 17:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix RootViewModel memory leaks #623

Fix RootViewModel memory leaks #623

justinswart commented Mar 1, 2019 •

edited by dusi

Loading

ifbarrera commented Mar 4, 2019

ifbarrera commented Mar 13, 2019

ifbarrera commented Mar 13, 2019

dusi left a comment

dusi Apr 4, 2019

justinswart Apr 4, 2019

dusi Apr 4, 2019

justinswart Apr 4, 2019

dusi Apr 4, 2019

dusi Apr 4, 2019

justinswart Apr 4, 2019

dusi Apr 4, 2019

dusi Apr 4, 2019

justinswart Apr 4, 2019

dusi Apr 4, 2019

justinswart Apr 4, 2019

dusi Apr 4, 2019

dusi Apr 4, 2019

dusi Apr 4, 2019

dusi Apr 4, 2019

dusi Apr 4, 2019

dusi left a comment

Fix RootViewModel memory leaks #623

Fix RootViewModel memory leaks #623

Conversation

justinswart commented Mar 1, 2019 • edited by dusi Loading

📲 What

🤔 Why

🛠 How

✅ Acceptance criteria

ifbarrera commented Mar 4, 2019

ifbarrera commented Mar 13, 2019

ifbarrera commented Mar 13, 2019

dusi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dusi left a comment

Choose a reason for hiding this comment

justinswart commented Mar 1, 2019 •

edited by dusi

Loading