From 89b35fab6dca1b7dd84fe2346b3f10bc16451e1a Mon Sep 17 00:00:00 2001
From: Shadowfacts <me@shadowfacts.net>
Date: Mon, 26 Oct 2020 22:55:58 -0400
Subject: [PATCH] Move pruning of offscreen rows to when the VC disappears,
 instead of during scrolling

Prevents race when removing and adding cells in the willDisplay table
view delegate method.
---
 Tusker.xcodeproj/project.pbxproj              |   4 +
 Tusker/SceneDelegate.swift                    |   4 +
 .../Main/MainSplitViewController.swift        |  14 +++
 .../Main/MainTabBarViewController.swift       |   8 ++
 .../NotificationsTableViewController.swift    |  81 +++++++-----
 .../ProfileStatusesViewController.swift       |   2 -
 .../TimelineTableViewController.swift         | 119 ++++++++++--------
 .../BackgroundableViewController.swift        |  13 ++
 .../EnhancedNavigationViewController.swift    |   8 ++
 .../SegmentedPageViewController.swift         |   8 ++
 10 files changed, 174 insertions(+), 87 deletions(-)
 create mode 100644 Tusker/Screens/Utilities/BackgroundableViewController.swift

diff --git a/Tusker.xcodeproj/project.pbxproj b/Tusker.xcodeproj/project.pbxproj
index 9fb44a5c..f5ede7f8 100644
--- a/Tusker.xcodeproj/project.pbxproj
+++ b/Tusker.xcodeproj/project.pbxproj
@@ -145,6 +145,7 @@
 		D6531DEE246B81C9000F9538 /* GifvAttachmentView.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6531DED246B81C9000F9538 /* GifvAttachmentView.swift */; };
 		D6531DF0246B867E000F9538 /* GifvAttachmentViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6531DEF246B867E000F9538 /* GifvAttachmentViewController.swift */; };
 		D6538945214D6D7500E3CEFC /* TableViewSwipeActionProvider.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6538944214D6D7500E3CEFC /* TableViewSwipeActionProvider.swift */; };
+		D65C6BF525478A9C00A6E89C /* BackgroundableViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = D65C6BF425478A9C00A6E89C /* BackgroundableViewController.swift */; };
 		D65F613423AEAB6600F3CFD3 /* OnboardingTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = D65F613323AEAB6600F3CFD3 /* OnboardingTests.swift */; };
 		D6620ACE2511A0ED00312CA0 /* StatusStateResolver.swift in Sources */ = {isa = PBXBuildFile; fileRef = D6620ACD2511A0ED00312CA0 /* StatusStateResolver.swift */; };
 		D663625D2135C74800C9CBA2 /* ConversationMainStatusTableViewCell.xib in Resources */ = {isa = PBXBuildFile; fileRef = D663625C2135C74800C9CBA2 /* ConversationMainStatusTableViewCell.xib */; };
@@ -482,6 +483,7 @@
 		D6531DED246B81C9000F9538 /* GifvAttachmentView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GifvAttachmentView.swift; sourceTree = "<group>"; };
 		D6531DEF246B867E000F9538 /* GifvAttachmentViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GifvAttachmentViewController.swift; sourceTree = "<group>"; };
 		D6538944214D6D7500E3CEFC /* TableViewSwipeActionProvider.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TableViewSwipeActionProvider.swift; sourceTree = "<group>"; };
+		D65C6BF425478A9C00A6E89C /* BackgroundableViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BackgroundableViewController.swift; sourceTree = "<group>"; };
 		D65F612D23AE990C00F3CFD3 /* Embassy.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; path = Embassy.framework; sourceTree = BUILT_PRODUCTS_DIR; };
 		D65F613023AE99E000F3CFD3 /* Ambassador.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; path = Ambassador.framework; sourceTree = BUILT_PRODUCTS_DIR; };
 		D65F613323AEAB6600F3CFD3 /* OnboardingTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OnboardingTests.swift; sourceTree = "<group>"; };
@@ -1327,6 +1329,7 @@
 				D693DE5823FE24300061E07D /* InteractivePushTransition.swift */,
 				D6DFC69D242C490400ACC392 /* TrackpadScrollGestureRecognizer.swift */,
 				D6412B0224AFF6A600F5412E /* TabBarScrollableViewController.swift */,
+				D65C6BF425478A9C00A6E89C /* BackgroundableViewController.swift */,
 			);
 			path = Utilities;
 			sourceTree = "<group>";
@@ -1864,6 +1867,7 @@
 				D6BC9DDA232D8BE5002CA326 /* SearchResultsViewController.swift in Sources */,
 				D627FF7F217E95E000CC0648 /* DraftTableViewCell.swift in Sources */,
 				D6DF95C12533F5DE0027A9B6 /* RelationshipMO.swift in Sources */,
+				D65C6BF525478A9C00A6E89C /* BackgroundableViewController.swift in Sources */,
 				D6AEBB4A23216F0400E5038B /* UnfollowAccountActivity.swift in Sources */,
 				D663626421360D2300C9CBA2 /* AvatarStyle.swift in Sources */,
 				D6D3FDE024F41B8400FF50A5 /* ComposeContainerView.swift in Sources */,
diff --git a/Tusker/SceneDelegate.swift b/Tusker/SceneDelegate.swift
index 27eccb6d..075f1c30 100644
--- a/Tusker/SceneDelegate.swift
+++ b/Tusker/SceneDelegate.swift
@@ -108,6 +108,10 @@ class SceneDelegate: UIResponder, UIWindowSceneDelegate {
         // Use this method to save data, release shared resources, and store enough scene-specific state information
         // to restore the scene back to its current state.
         
+        if let rootVC = window?.rootViewController as? BackgroundableViewController {
+            rootVC.sceneDidEnterBackground()
+        }
+        
         try! scene.session.mastodonController?.persistentContainer.viewContext.save()
     }
     
diff --git a/Tusker/Screens/Main/MainSplitViewController.swift b/Tusker/Screens/Main/MainSplitViewController.swift
index 1aec0f08..7a6fd28f 100644
--- a/Tusker/Screens/Main/MainSplitViewController.swift
+++ b/Tusker/Screens/Main/MainSplitViewController.swift
@@ -349,3 +349,17 @@ extension MainSplitViewController: TuskerRootViewController {
         }
     }
 }
+
+@available(iOS 14.0, *)
+extension MainSplitViewController: BackgroundableViewController {
+    func sceneDidEnterBackground() {
+        if traitCollection.horizontalSizeClass == .compact {
+            tabBarViewController.sceneDidEnterBackground()
+        } else {
+            // todo: should this do the same for the sidebar VC as well?
+            if let contentVC = viewController(for: .secondary) as? BackgroundableViewController {
+                contentVC.sceneDidEnterBackground()
+            }
+        }
+    }
+}
diff --git a/Tusker/Screens/Main/MainTabBarViewController.swift b/Tusker/Screens/Main/MainTabBarViewController.swift
index 1f640d42..58ae420c 100644
--- a/Tusker/Screens/Main/MainTabBarViewController.swift
+++ b/Tusker/Screens/Main/MainTabBarViewController.swift
@@ -136,3 +136,11 @@ extension MainTabBarViewController: TuskerRootViewController {
         }
     }
 }
+
+extension MainTabBarViewController: BackgroundableViewController {
+    func sceneDidEnterBackground() {
+        if let selectedVC = selectedViewController as? BackgroundableViewController {
+            selectedVC.sceneDidEnterBackground()
+        }
+    }
+}
diff --git a/Tusker/Screens/Notifications/NotificationsTableViewController.swift b/Tusker/Screens/Notifications/NotificationsTableViewController.swift
index d11cb1f9..c8c17461 100644
--- a/Tusker/Screens/Notifications/NotificationsTableViewController.swift
+++ b/Tusker/Screens/Notifications/NotificationsTableViewController.swift
@@ -19,15 +19,18 @@ class NotificationsTableViewController: EnhancedTableViewController {
     
     weak var mastodonController: MastodonController!
     
-    let excludedTypes: [Pachyderm.Notification.Kind]
-    let groupTypes = [Notification.Kind.favourite, .reblog, .follow]
+    private let excludedTypes: [Pachyderm.Notification.Kind]
+    private let groupTypes = [Notification.Kind.favourite, .reblog, .follow]
     
     private var loaded = false
 
-    var groups: [NotificationGroup] = []
+    private var groups: [NotificationGroup] = []
     
-    var newer: RequestRange?
-    var older: RequestRange?
+    private let pageSize = 20
+    private var newer: RequestRange?
+    private var older: RequestRange?
+    
+    private var lastLastVisibleRow: IndexPath?
     
     init(allowedTypes: [Pachyderm.Notification.Kind], mastodonController: MastodonController) {
         self.excludedTypes = Array(Set(Pachyderm.Notification.Kind.allCases).subtracting(allowedTypes))
@@ -84,6 +87,41 @@ class NotificationsTableViewController: EnhancedTableViewController {
         }
     }
     
+    override func viewWillDisappear(_ animated: Bool) {
+        super.viewWillDisappear(animated)
+        
+        pruneOffscreenRows()
+    }
+    
+    private func pruneOffscreenRows() {
+        guard let lastVisibleRow = lastLastVisibleRow else {
+            return
+        }
+        let lastRowIndex = groups.count - 1
+        
+        if lastVisibleRow.row < lastRowIndex - pageSize {
+            // if there are more than 20 rows below the lats visible one
+            
+            let rowIndicesToRemove = (lastVisibleRow.row + pageSize)..<groups.count
+            
+            let groupsToRemove = groups[rowIndicesToRemove]
+            for group in groupsToRemove {
+                for notification in group.notifications {
+                    if let id = notification.status?.id {
+                        mastodonController.persistentContainer.status(for: id)?.decrementReferenceCount()
+                    }
+                }
+            }
+            
+            groups.removeSubrange(rowIndicesToRemove)
+            
+            let removedIndexPaths = rowIndicesToRemove.map { IndexPath(row: $0, section: 0) }
+            UIView.performWithoutAnimation {
+                tableView.deleteRows(at: removedIndexPaths, with: .none)
+            }
+        }
+    }
+    
     // MARK: - Table view data source
 
     override func numberOfSections(in tableView: UITableView) -> Int {
@@ -137,32 +175,7 @@ class NotificationsTableViewController: EnhancedTableViewController {
     // MARK: - Table view delegate
 
     override func tableView(_ tableView: UITableView, willDisplay cell: UITableViewCell, forRowAt indexPath: IndexPath) {
-        // see TimelineTableViewController.tableView(_:willDisplay:forRowAt:)
-        if !isCurrentlyScrollingToTop, scrollViewDirection < 0 {
-            let pageSize = 20
-            if groups.count > 2 * pageSize,
-                indexPath.row < groups.count - (2 * pageSize) {
-                let groupsToRemove = groups[groups.count - pageSize..<groups.count]
-                for group in groupsToRemove {
-                    for notification in group.notifications {
-                        // todo: reference count accounts
-//                        mastodonController.persistentContainer.account(for: notification.account.id)?.decrementReferenceCount()
-                        if let id = notification.status?.id {
-                            mastodonController.persistentContainer.status(for: id)?.decrementReferenceCount()
-                        }
-                    }
-                }
-                let removedIndexPaths = (groups.count - 20..<groups.count).map { IndexPath(row: $0, section: 0) }
-                
-                groups.removeLast(pageSize)
-                
-                DispatchQueue.main.async {
-                    UIView.performWithoutAnimation {
-                        tableView.deleteRows(at: removedIndexPaths, with: .none)
-                    }
-                }
-            }
-        }
+        lastLastVisibleRow = tableView.indexPathsForVisibleRows?.last
         
         if indexPath.row == groups.count - 1 {
             guard let older = older else { return }
@@ -305,3 +318,9 @@ extension NotificationsTableViewController: UITableViewDataSourcePrefetching {
         }
     }
 }
+
+extension NotificationsTableViewController: BackgroundableViewController {
+    func sceneDidEnterBackground() {
+        pruneOffscreenRows()
+    }
+}
diff --git a/Tusker/Screens/Profile/ProfileStatusesViewController.swift b/Tusker/Screens/Profile/ProfileStatusesViewController.swift
index 9c3d84ca..66048851 100644
--- a/Tusker/Screens/Profile/ProfileStatusesViewController.swift
+++ b/Tusker/Screens/Profile/ProfileStatusesViewController.swift
@@ -224,8 +224,6 @@ class ProfileStatusesViewController: EnhancedTableViewController {
     }
     
     override func tableView(_ tableView: UITableView, willDisplay cell: UITableViewCell, forRowAt indexPath: IndexPath) {
-        // todo: if scrolling up, remove statuses at bottom like timeline VC
-        
         // load older statuses if at bottom
         if timelineSegments.count > 0,
            indexPath.section == timelineSegments.count,
diff --git a/Tusker/Screens/Timeline/TimelineTableViewController.swift b/Tusker/Screens/Timeline/TimelineTableViewController.swift
index 171a9849..85914f7c 100644
--- a/Tusker/Screens/Timeline/TimelineTableViewController.swift
+++ b/Tusker/Screens/Timeline/TimelineTableViewController.swift
@@ -18,8 +18,11 @@ class TimelineTableViewController: EnhancedTableViewController, StatusTableViewC
     
     var timelineSegments: [[(id: String, state: StatusState)]] = []
     
-    var newer: RequestRange?
-    var older: RequestRange?
+    private let pageSize = 20
+    private var newer: RequestRange?
+    private var older: RequestRange?
+    
+    private var lastLastVisibleRow: IndexPath?
     
     init(for timeline: Timeline, mastodonController: MastodonController) {
         self.timeline = timeline
@@ -73,6 +76,12 @@ class TimelineTableViewController: EnhancedTableViewController, StatusTableViewC
         loadInitialStatuses()
     }
     
+    override func viewWillDisappear(_ animated: Bool) {
+        super.viewWillDisappear(animated)
+        
+        pruneOffscreenRows()
+    }
+    
     func loadInitialStatuses() {
         guard !loaded else { return }
         loaded = true
@@ -91,6 +100,52 @@ class TimelineTableViewController: EnhancedTableViewController, StatusTableViewC
             }
         }
     }
+    
+    private func pruneOffscreenRows() {
+        guard let lastVisibleRow = lastLastVisibleRow else {
+            return
+        }
+        let lastSectionIndex = timelineSegments.count - 1
+        
+        if lastVisibleRow.section < lastSectionIndex {
+            // if there is a section below the last visible one
+            
+            let sectionsToRemove = (lastVisibleRow.section + 1)...lastSectionIndex
+            
+            for section in sectionsToRemove {
+                for (id, _) in timelineSegments.remove(at: section) {
+                    mastodonController.persistentContainer.status(for: id)?.decrementReferenceCount()
+                }
+            }
+            
+            UIView.performWithoutAnimation {
+                tableView.deleteSections(IndexSet(sectionsToRemove), with: .none)
+            }
+            
+        } else if lastVisibleRow.section == lastSectionIndex {
+            let lastSection = timelineSegments.last!
+            let lastRowIndex = lastSection.count - 1
+            
+            if lastVisibleRow.row < lastRowIndex - 20 {
+                // if there are more than 20 rows in the current section below the last visible one
+                
+                let rowIndicesInLastSectionToRemove = (lastVisibleRow.row + 20)..<lastSection.count
+                
+                let statusesToRemove = lastSection[rowIndicesInLastSectionToRemove]
+                for (id, _) in statusesToRemove {
+                    mastodonController.persistentContainer.status(for: id)?.decrementReferenceCount()
+                }
+                
+                timelineSegments[lastSectionIndex].removeSubrange(rowIndicesInLastSectionToRemove)
+
+                let removedIndexPaths = rowIndicesInLastSectionToRemove.map { IndexPath(row: $0, section: lastSectionIndex) }
+                UIView.performWithoutAnimation {
+                    tableView.deleteRows(at: removedIndexPaths, with: .none)
+                }
+
+            }
+        }
+    }
 
     // MARK: - Table view data source
 
@@ -117,58 +172,8 @@ class TimelineTableViewController: EnhancedTableViewController, StatusTableViewC
     // MARK: - Table view delegate
     
     override func tableView(_ tableView: UITableView, willDisplay cell: UITableViewCell, forRowAt indexPath: IndexPath) {
-        // don't remove rows when jumping to the top, otherwise jumping back down might try to show removed rows
-        // when scrolling upwards, decrement reference counts for old statuses, if necessary
-        if !isCurrentlyScrollingToTop, scrollViewDirection < 0 {
-            if indexPath.section <= timelineSegments.count - 2 {
-                // decrement ref counts for all sections below the section below the current section
-                // (e.g., there exist sections 0, 1, 2 and we're currently scrolling upwards in section 0, we want to remove section 2)
-                
-                // todo: this is in the hot path for scrolling, possibly move this to a background thread?
-                let sectionsToRemove = indexPath.section + 1..<timelineSegments.count
-                for section in sectionsToRemove {
-                    for (id, _) in timelineSegments.remove(at: section) {
-                        mastodonController.persistentContainer.status(for: id)?.decrementReferenceCount()
-                    }
-                }
-                // see below comment about DispatchQueue.main.async
-                DispatchQueue.main.async {
-                    UIView.performWithoutAnimation {
-                        tableView.deleteSections(IndexSet(sectionsToRemove), with: .none)
-                    }
-                }
-            } else {
-                // we are scrolling in the last or second to last section
-                // grab the last section and, if there is more than one page in the last section
-                // (we never want to remove the only page in the last section unless there is another section between it and the user),
-                // remove the last page and decrement reference counts
-                let pageSize = 20 // todo: this should come from somewhere in Pachyderm
-                let lastSection = timelineSegments.last!
-                if lastSection.count > 2 * pageSize,
-                    indexPath.row < lastSection.count - (2 * pageSize) {
-                    // todo: this is in the hot path for scrolling, possibly move this to a background thread?
-                    let statusesToRemove = lastSection[lastSection.count - pageSize..<lastSection.count]
-                    for (id, _) in statusesToRemove {
-                        mastodonController.persistentContainer.status(for: id)?.decrementReferenceCount()
-                    }
-                    timelineSegments[timelineSegments.count - 1].removeLast(pageSize)
-                    
-                    let removedIndexPaths = (lastSection.count - 20..<lastSection.count).map { IndexPath(row: $0, section: timelineSegments.count - 1) }
-                    // Removing this DispatchQueue.main.async call causes things to break when scrolling
-                    // back down towards the removed rows. There would be a index out of bounds crash
-                    // because, while we've already removed the statuses from our model, the table view doesn't seem to know that.
-                    // It seems like tableView update calls made from inside tableView(_:willDisplay:forRowAt:) are silently ignored.
-                    // Calling tableView.numberOfRows(inSection: 0) when trapped in the debugger after the aforementioned IOOB
-                    // will produce an incorrect value (it will be some multiple of pageSize too high).
-                    // Deferring the tableView update until the next runloop iteration seems to solve that.
-                    DispatchQueue.main.async {
-                        UIView.performWithoutAnimation {
-                            tableView.deleteRows(at: removedIndexPaths, with: .none)
-                        }
-                    }
-                }
-            }
-        }
+        // this assumes that indexPathsForVisibleRows is always in order
+        lastLastVisibleRow = tableView.indexPathsForVisibleRows?.last
         
         // load older statuses, if necessary
         if indexPath.section == timelineSegments.count - 1,
@@ -282,3 +287,9 @@ extension TimelineTableViewController: UITableViewDataSourcePrefetching {
         }
     }
 }
+
+extension TimelineTableViewController: BackgroundableViewController {
+    func sceneDidEnterBackground() {
+        pruneOffscreenRows()
+    }
+}
diff --git a/Tusker/Screens/Utilities/BackgroundableViewController.swift b/Tusker/Screens/Utilities/BackgroundableViewController.swift
new file mode 100644
index 00000000..da50cdb6
--- /dev/null
+++ b/Tusker/Screens/Utilities/BackgroundableViewController.swift
@@ -0,0 +1,13 @@
+//
+//  BackgroundableViewController.swift
+//  Tusker
+//
+//  Created by Shadowfacts on 10/26/20.
+//  Copyright © 2020 Shadowfacts. All rights reserved.
+//
+
+import Foundation
+
+protocol BackgroundableViewController {
+    func sceneDidEnterBackground()
+}
diff --git a/Tusker/Screens/Utilities/EnhancedNavigationViewController.swift b/Tusker/Screens/Utilities/EnhancedNavigationViewController.swift
index c06ed147..bc23b2da 100644
--- a/Tusker/Screens/Utilities/EnhancedNavigationViewController.swift
+++ b/Tusker/Screens/Utilities/EnhancedNavigationViewController.swift
@@ -74,3 +74,11 @@ class EnhancedNavigationViewController: UINavigationController {
     }
 
 }
+
+extension EnhancedNavigationViewController: BackgroundableViewController {
+    func sceneDidEnterBackground() {
+        if let topVC = topViewController as? BackgroundableViewController {
+            topVC.sceneDidEnterBackground()
+        }
+    }
+}
diff --git a/Tusker/Screens/Utilities/SegmentedPageViewController.swift b/Tusker/Screens/Utilities/SegmentedPageViewController.swift
index 47ea6d51..6b1f2993 100644
--- a/Tusker/Screens/Utilities/SegmentedPageViewController.swift
+++ b/Tusker/Screens/Utilities/SegmentedPageViewController.swift
@@ -71,3 +71,11 @@ extension SegmentedPageViewController: TabBarScrollableViewController {
         }
     }
 }
+
+extension SegmentedPageViewController: BackgroundableViewController {
+    func sceneDidEnterBackground() {
+        if let current = pageControllers[currentIndex] as? BackgroundableViewController {
+            current.sceneDidEnterBackground()
+        }
+    }
+}