Merge branch 'mkirk/fix-crash-after-delete'

Signal/src/ViewControllers/MediaGalleryViewController.swift

@@ -176,11 +176,11 @@ protocol MediaGalleryDataSource: class {
     func showAllMedia(focusedItem: MediaGalleryItem)
     func dismissMediaDetailViewController(_ mediaDetailViewController: MediaPageViewController, animated isAnimated: Bool, completion: (() -> Void)?)
 
-    func delete(items: [MediaGalleryItem])
+    func delete(items: [MediaGalleryItem], initiatedBy: MediaGalleryDataSourceDelegate)
 }
 
 protocol MediaGalleryDataSourceDelegate: class {
-    func mediaGalleryDataSource(_ mediaGalleryDataSource: MediaGalleryDataSource, willDelete items: [MediaGalleryItem])
+    func mediaGalleryDataSource(_ mediaGalleryDataSource: MediaGalleryDataSource, willDelete items: [MediaGalleryItem], initiatedBy: MediaGalleryDataSourceDelegate)
     func mediaGalleryDataSource(_ mediaGalleryDataSource: MediaGalleryDataSource, deletedSections: IndexSet, deletedItems: [IndexPath])
 }
 
@@ -280,6 +280,7 @@ class MediaGalleryViewController: UINavigationController, MediaGalleryDataSource
         self.initialDetailItem = initialDetailItem
 
         let pageViewController = MediaPageViewController(initialItem: initialDetailItem, mediaGalleryDataSource: self, uiDatabaseConnection: self.uiDatabaseConnection, options: self.options)
+        self.addDataSourceDelegate(pageViewController)
 
         self.pageViewController = pageViewController
         self.setViewControllers([pageViewController], animated: false)
@@ -467,7 +468,7 @@ class MediaGalleryViewController: UINavigationController, MediaGalleryDataSource
         if isAnimated {
             UIView.animate(withDuration: changedItems ? 0.25 : 0.18,
                            delay: 0.0,
-                           options:.curveEaseOut,
+                           options: .curveEaseOut,
                            animations: {
                             // Move back over it's original location
                             self.presentationView.superview?.layoutIfNeeded()
@@ -535,8 +536,8 @@ class MediaGalleryViewController: UINavigationController, MediaGalleryDataSource
 
         self.presentationViewConstraints += self.presentationView.autoSetDimensions(to: convertedRect.size)
         self.presentationViewConstraints += [
-            self.presentationView.autoPinEdge(toSuperviewEdge: .top, withInset:convertedRect.origin.y),
+            self.presentationView.autoPinEdge(toSuperviewEdge: .top, withInset: convertedRect.origin.y),
-            self.presentationView.autoPinEdge(toSuperviewEdge: .left, withInset:convertedRect.origin.x)
+            self.presentationView.autoPinEdge(toSuperviewEdge: .left, withInset: convertedRect.origin.x)
         ]
     }
 
@@ -573,10 +574,7 @@ class MediaGalleryViewController: UINavigationController, MediaGalleryDataSource
         let vc = MediaTileViewController(mediaGalleryDataSource: self, uiDatabaseConnection: self.uiDatabaseConnection)
         vc.delegate = self
 
-        // dataSourceDelegate will either be this tile view, or the MessageDetailView, but they should
+        self.addDataSourceDelegate(vc)
-        // be mutually exclusive
-        assert(self.dataSourceDelegate == nil)
-        self.dataSourceDelegate = vc
 
         return vc
     }()
@@ -735,15 +733,21 @@ class MediaGalleryViewController: UINavigationController, MediaGalleryDataSource
         }
     }
 
-    weak var dataSourceDelegate: MediaGalleryDataSourceDelegate?
+    var dataSourceDelegates: [Weak<MediaGalleryDataSourceDelegate>] = []
-    var deletedMessages: Set<TSMessage> = Set()
+    func addDataSourceDelegate(_ dataSourceDelegate: MediaGalleryDataSourceDelegate) {
+        dataSourceDelegates.append(Weak(value: dataSourceDelegate))
+    }
 
-    func delete(items: [MediaGalleryItem]) {
+    var deletedMessages: Set<TSMessage> = Set()
+    var deletedGalleryItems: Set<MediaGalleryItem> = Set()
+
+    func delete(items: [MediaGalleryItem], initiatedBy: MediaGalleryDataSourceDelegate) {
         AssertIsOnMainThread()
 
-        Logger.info("\(logTag) in \(#function) with items: \(items)")
+        Logger.info("\(logTag) in \(#function) with items: \(items.map { ($0.attachmentStream, $0.message.timestamp) })")
 
-        dataSourceDelegate?.mediaGalleryDataSource(self, willDelete: items)
+        deletedGalleryItems.formUnion(items)
+        dataSourceDelegates.forEach { $0.value?.mediaGalleryDataSource(self, willDelete: items, initiatedBy: initiatedBy) }
 
         self.editingDatabaseConnection.asyncReadWrite { transaction in
             for item in items {
@@ -812,7 +816,7 @@ class MediaGalleryViewController: UINavigationController, MediaGalleryDataSource
             }
         }
 
-        dataSourceDelegate?.mediaGalleryDataSource(self, deletedSections: deletedSections, deletedItems: deletedIndexPaths)
+        dataSourceDelegates.forEach { $0.value?.mediaGalleryDataSource(self, deletedSections: deletedSections, deletedItems: deletedIndexPaths) }
     }
 
     let kGallerySwipeLoadBatchSize: UInt = 5
@@ -828,7 +832,17 @@ class MediaGalleryViewController: UINavigationController, MediaGalleryDataSource
         }
 
         let index: Int = galleryItems.index(after: currentIndex)
-        return galleryItems[safe: index]
+        guard let nextItem = galleryItems[safe: index] else {
+            // already at last item
+            return nil
+        }
+
+        guard !deletedGalleryItems.contains(nextItem) else {
+            Logger.debug("\(self.logTag) in \(#function) nextItem was deleted - Recursing.")
+            return galleryItem(after: nextItem)
+        }
+
+        return nextItem
     }
 
     internal func galleryItem(before currentItem: MediaGalleryItem) -> MediaGalleryItem? {
@@ -842,7 +856,17 @@ class MediaGalleryViewController: UINavigationController, MediaGalleryDataSource
         }
 
         let index: Int = galleryItems.index(before: currentIndex)
-        return galleryItems[safe: index]
+        guard let previousItem = galleryItems[safe: index] else {
+            // already at first item
+            return nil
+        }
+
+        guard !deletedGalleryItems.contains(previousItem) else {
+            Logger.debug("\(self.logTag) in \(#function) previousItem was deleted - Recursing.")
+            return galleryItem(before: previousItem)
+        }
+
+        return previousItem
     }
 
     var galleryItemCount: Int {

Signal/src/ViewControllers/MediaPageViewController.swift

@@ -18,7 +18,7 @@ public class GalleryItemBox: NSObject {
     }
 }
 
-fileprivate class Box<A> {
+private class Box<A> {
     var value: A
     init(_ val: A) {
         self.value = val
@@ -31,7 +31,7 @@ fileprivate extension MediaDetailViewController {
     }
 }
 
-class MediaPageViewController: UIPageViewController, UIPageViewControllerDataSource, UIPageViewControllerDelegate, MediaDetailViewControllerDelegate {
+class MediaPageViewController: UIPageViewController, UIPageViewControllerDataSource, UIPageViewControllerDelegate, MediaDetailViewControllerDelegate, MediaGalleryDataSourceDelegate {
 
     private weak var mediaGalleryDataSource: MediaGalleryDataSource?
 
@@ -171,7 +171,7 @@ class MediaPageViewController: UIPageViewController, UIPageViewControllerDataSou
         self.view.addSubview(footerBar)
         footerBar.autoPinWidthToSuperview()
         footerBar.autoPin(toBottomLayoutGuideOf: self, withInset: 0)
-        footerBar.autoSetDimension(.height, toSize:kFooterHeight)
+        footerBar.autoSetDimension(.height, toSize: kFooterHeight)
 
         // Gestures
 
@@ -244,7 +244,7 @@ class MediaPageViewController: UIPageViewController, UIPageViewControllerDataSou
 
             // Hiding the status bar affects the positioning of the navbar. We don't want to show that in an animation, it's
             // better to just have everythign "flit" in/out.
-            UIApplication.shared.setStatusBarHidden(shouldHideToolbars, with:.none)
+            UIApplication.shared.setStatusBarHidden(shouldHideToolbars, with: .none)
             self.navigationController?.setNavigationBarHidden(shouldHideToolbars, animated: false)
 
             // We don't animate the background color change because the old color shows through momentarily
@@ -267,20 +267,20 @@ class MediaPageViewController: UIPageViewController, UIPageViewControllerDataSou
         }
 
         var toolbarItems: [UIBarButtonItem] = [
-            UIBarButtonItem(barButtonSystemItem: .action, target:self, action: #selector(didPressShare)),
+            UIBarButtonItem(barButtonSystemItem: .action, target: self, action: #selector(didPressShare)),
-            UIBarButtonItem(barButtonSystemItem: .flexibleSpace, target:nil, action:nil)
+            UIBarButtonItem(barButtonSystemItem: .flexibleSpace, target: nil, action: nil)
         ]
 
         if (self.currentItem.isVideo) {
             toolbarItems += [
                 isPlayingVideo ? self.videoPauseBarButton : self.videoPlayBarButton,
-                UIBarButtonItem(barButtonSystemItem: .flexibleSpace, target:nil, action:nil)
+                UIBarButtonItem(barButtonSystemItem: .flexibleSpace, target: nil, action: nil)
             ]
         }
 
         toolbarItems.append(UIBarButtonItem(barButtonSystemItem: .trash,
-                                            target:self,
+                                            target: self,
-                                            action:#selector(didPressDelete)))
+                                            action: #selector(didPressDelete)))
 
         self.footerBar.setItems(toolbarItems, animated: false)
     }
@@ -317,18 +317,7 @@ class MediaPageViewController: UIPageViewController, UIPageViewControllerDataSou
         let deleteAction = UIAlertAction(title: NSLocalizedString("TXT_DELETE_TITLE", comment: ""),
                                          style: .destructive) { _ in
                                             let deletedItem = currentViewController.galleryItem
-
+                                            mediaGalleryDataSource.delete(items: [deletedItem], initiatedBy: self)
-                                            if !self.sliderEnabled {
-                                                // In message details, which doesn't use the slider, so don't swap pages.
-                                            } else if let nextItem = mediaGalleryDataSource.galleryItem(after: deletedItem) {
-                                                self.setCurrentItem(nextItem, direction: .forward, animated: true)
-                                            } else if let previousItem = mediaGalleryDataSource.galleryItem(before: deletedItem) {
-                                                self.setCurrentItem(previousItem, direction: .reverse, animated: true)
-                                            } else {
-                                                // else we deleted the last piece of media, return to the conversation view
-                                                self.dismissSelf(animated: true)
-                                            }
-                                            mediaGalleryDataSource.delete(items: [deletedItem])
         }
         actionSheet.addAction(OWSAlerts.cancelAction)
         actionSheet.addAction(deleteAction)
@@ -336,6 +325,43 @@ class MediaPageViewController: UIPageViewController, UIPageViewControllerDataSou
         self.present(actionSheet, animated: true)
     }
 
+    // MARK: MediaGalleryDataSourceDelegate
+
+    func mediaGalleryDataSource(_ mediaGalleryDataSource: MediaGalleryDataSource, willDelete items: [MediaGalleryItem], initiatedBy: MediaGalleryDataSourceDelegate) {
+        Logger.debug("\(self.logTag) in \(#function)")
+
+        guard let currentItem = self.currentItem else {
+              owsFail("\(logTag) in \(#function) currentItem was unexpectedly nil")
+            return
+        }
+
+        guard items.contains(currentItem) else {
+            Logger.debug("\(self.logTag) in \(#function) irrelevant item")
+            return
+        }
+
+        // If we setCurrentItem with (animated: true) while this VC is in the background, then
+        // the next/previous cache isn't expired, and we're able to swipe back to the just-deleted vc.
+        // So to get the correct behavior, we should only animate these transitions when this
+        // vc is in the foreground
+        let isAnimated = initiatedBy === self
+
+        if !self.sliderEnabled {
+            // In message details, which doesn't use the slider, so don't swap pages.
+        } else if let nextItem = mediaGalleryDataSource.galleryItem(after: currentItem) {
+            self.setCurrentItem(nextItem, direction: .forward, animated: isAnimated)
+        } else if let previousItem = mediaGalleryDataSource.galleryItem(before: currentItem) {
+            self.setCurrentItem(previousItem, direction: .reverse, animated: isAnimated)
+        } else {
+            // else we deleted the last piece of media, return to the conversation view
+            self.dismissSelf(animated: true)
+        }
+    }
+
+    func mediaGalleryDataSource(_ mediaGalleryDataSource: MediaGalleryDataSource, deletedSections: IndexSet, deletedItems: [IndexPath]) {
+        // no-op
+    }
+
     @objc
     public func didPressPlayBarButton(_ sender: Any) {
         guard let currentViewController = self.viewControllers?[0] as? MediaDetailViewController else {
@@ -512,7 +538,7 @@ class MediaPageViewController: UIPageViewController, UIPageViewControllerDataSou
         }
 
         dismissSelf(animated: true) {
-            mediaGalleryDataSource.delete(items: [galleryItem])
+            mediaGalleryDataSource.delete(items: [galleryItem], initiatedBy: self)
         }
     }
 

Signal/src/ViewControllers/MediaTileViewController.swift

@@ -94,13 +94,13 @@ public class MediaTileViewController: UICollectionViewController, MediaGalleryDa
         let footerBar = UIToolbar()
         self.footerBar = footerBar
         let deleteButton = UIBarButtonItem(barButtonSystemItem: .trash,
-                                          target:self,
+                                          target: self,
-                                          action:#selector(didPressDelete))
+                                          action: #selector(didPressDelete))
         self.deleteButton =  deleteButton
         let footerItems = [
-            UIBarButtonItem(barButtonSystemItem: .flexibleSpace, target:nil, action:nil),
+            UIBarButtonItem(barButtonSystemItem: .flexibleSpace, target: nil, action: nil),
             deleteButton,
-            UIBarButtonItem(barButtonSystemItem: .flexibleSpace, target:nil, action:nil),
+            UIBarButtonItem(barButtonSystemItem: .flexibleSpace, target: nil, action: nil)
         ]
         footerBar.setItems(footerItems, animated: false)
 
@@ -111,9 +111,6 @@ public class MediaTileViewController: UICollectionViewController, MediaGalleryDa
         self.footerBarBottomConstraint = footerBar.autoPinEdge(toSuperviewEdge: .bottom, withInset: -kFooterBarHeight)
 
         updateSelectButton()
-
-        self.view.layoutIfNeeded()
-        scrollToBottom(animated: false)
     }
 
     private func indexPath(galleryItem: MediaGalleryItem) -> IndexPath? {
@@ -140,7 +137,9 @@ public class MediaTileViewController: UICollectionViewController, MediaGalleryDa
         }
 
         Logger.debug("\(logTag) scrolling to focused item at indexPath: \(indexPath)")
+        self.view.layoutIfNeeded()
         self.collectionView?.scrollToItem(at: indexPath, at: .centeredVertically, animated: false)
+        self.autoLoadMoreIfNecessary()
     }
 
     // MARK: UICollectionViewDelegate
@@ -482,6 +481,10 @@ public class MediaTileViewController: UICollectionViewController, MediaGalleryDa
 
     @objc
     func didCancelSelect(_ sender: Any) {
+        endSelectMode()
+    }
+
+    func endSelectMode() {
         isInBatchSelectMode = false
 
         guard let collectionView = self.collectionView else {
@@ -536,7 +539,8 @@ public class MediaTileViewController: UICollectionViewController, MediaGalleryDa
         }()
 
         let deleteAction = UIAlertAction(title: confirmationTitle, style: .destructive) { _ in
-            mediaGalleryDataSource.delete(items: items)
+            mediaGalleryDataSource.delete(items: items, initiatedBy: self)
+            self.endSelectMode()
         }
 
         let actionSheet = UIAlertController(title: nil, message: nil, preferredStyle: .actionSheet)
@@ -553,7 +557,7 @@ public class MediaTileViewController: UICollectionViewController, MediaGalleryDa
 
     // MARK: MediaGalleryDataSourceDelegate
 
-    func mediaGalleryDataSource(_ mediaGalleryDataSource: MediaGalleryDataSource, willDelete items: [MediaGalleryItem]) {
+    func mediaGalleryDataSource(_ mediaGalleryDataSource: MediaGalleryDataSource, willDelete items: [MediaGalleryItem], initiatedBy: MediaGalleryDataSourceDelegate) {
         Logger.debug("\(self.logTag) in \(#function)")
 
         guard let collectionView = self.collectionView else {
@@ -709,20 +713,6 @@ public class MediaTileViewController: UICollectionViewController, MediaGalleryDa
             }
         }
     }
-
-    // MARK: Util
-
-    private func scrollToBottom(animated isAnimated: Bool) {
-        guard let collectionView = self.collectionView else {
-            owsFail("\(self.logTag) in \(#function) collectionView was unexpectedly nil")
-            return
-        }
-
-        let yOffset: CGFloat = collectionView.contentSize.height - collectionView.bounds.size.height + collectionView.contentInset.bottom
-        let offset: CGPoint  = CGPoint(x: 0, y: yOffset)
-
-        collectionView.setContentOffset(offset, animated: isAnimated)
-    }
 }
 
 // MARK: - Private Helper Classes
@@ -730,7 +720,7 @@ public class MediaTileViewController: UICollectionViewController, MediaGalleryDa
 // Accomodates remaining scrolled to the same "apparent" position when new content is insterted
 // into the top of a collectionView. There are multiple ways to solve this problem, but this
 // is the only one which avoided a perceptible flicker.
-fileprivate class MediaTileViewLayout: UICollectionViewFlowLayout {
+private class MediaTileViewLayout: UICollectionViewFlowLayout {
 
     fileprivate var isInsertingCellsToTop: Bool = false
     fileprivate var contentSizeBeforeInsertingToTop: CGSize?
@@ -751,7 +741,7 @@ fileprivate class MediaTileViewLayout: UICollectionViewFlowLayout {
     }
 }
 
-fileprivate class MediaGallerySectionHeader: UICollectionReusableView {
+private class MediaGallerySectionHeader: UICollectionReusableView {
 
     static let reuseIdentifier = "MediaGallerySectionHeader"
 
@@ -813,7 +803,7 @@ fileprivate class MediaGallerySectionHeader: UICollectionReusableView {
     }
 }
 
-fileprivate class MediaGalleryStaticHeader: UICollectionViewCell {
+private class MediaGalleryStaticHeader: UICollectionViewCell {
 
     static let reuseIdentifier = "MediaGalleryStaticHeader"
 
@@ -843,7 +833,7 @@ fileprivate class MediaGalleryStaticHeader: UICollectionViewCell {
     }
 }
 
-fileprivate class MediaGalleryCell: UICollectionViewCell {
+private class MediaGalleryCell: UICollectionViewCell {
 
     static let reuseIdentifier = "MediaGalleryCell"
 

Signal/src/ViewControllers/MessageDetailViewController.swift

@@ -758,7 +758,7 @@ class MessageDetailViewController: OWSViewController, UIScrollViewDelegate, Medi
 
     // MediaGalleryDataSourceDelegate
 
-    func mediaGalleryDataSource(_ mediaGalleryDataSource: MediaGalleryDataSource, willDelete items: [MediaGalleryItem]) {
+    func mediaGalleryDataSource(_ mediaGalleryDataSource: MediaGalleryDataSource, willDelete items: [MediaGalleryItem], initiatedBy: MediaGalleryDataSourceDelegate) {
         Logger.info("\(self.logTag) in \(#function)")
 
         guard (items.map({ $0.message }) == [self.message]) else {
@@ -785,7 +785,7 @@ class MessageDetailViewController: OWSViewController, UIScrollViewDelegate, Medi
         }
 
         let mediaGalleryViewController = MediaGalleryViewController(thread: self.thread, uiDatabaseConnection: self.uiDatabaseConnection)
-        mediaGalleryViewController.dataSourceDelegate = self
+        mediaGalleryViewController.addDataSourceDelegate(self)
         mediaGalleryViewController.presentDetailView(fromViewController: self, mediaMessage: self.message, replacingView: fromView)
     }
 }