Merge pull request #997 from mpretty-cyro/fix/attachment-item-hash

Fixed a SignalAttachment hash uniqueness issue, fixed attachment UX issues

Session/Media Viewing & Editing/ImagePickerController.swift

@@ -30,6 +30,7 @@ class ImagePickerGridController: UICollectionViewController, PhotoLibraryDelegat
     private var photoCollection: PhotoCollection
     private var photoCollectionContents: PhotoCollectionContents
     private let photoMediaSize = PhotoMediaSize()
+    private var firstSelectedIndexPath: IndexPath?
 
     var collectionViewFlowLayout: UICollectionViewFlowLayout
     var titleView: TitleView!
@@ -212,6 +213,12 @@ class ImagePickerGridController: UICollectionViewController, PhotoLibraryDelegat
         let cellSize = collectionViewFlowLayout.itemSize
         photoMediaSize.thumbnailSize = CGSize(width: cellSize.width * scale, height: cellSize.height * scale)
  
+        // When we select the first item we immediately deselect it so it doesn't look odd when pushing to the
+        // next screen, but this in turn looks odd if the user returns and the item is deselected
+        if let firstSelectedIndexPath: IndexPath = firstSelectedIndexPath {
+            collectionView.cellForItem(at: firstSelectedIndexPath)?.isSelected = true
+        }
+        
         if !hasEverAppeared {
             scrollToBottom(animated: false)
         }
@@ -249,6 +256,13 @@ class ImagePickerGridController: UICollectionViewController, PhotoLibraryDelegat
         DispatchQueue.main.async {
             // pre-layout collectionPicker for snappier response
             self.collectionPickerController.view.layoutIfNeeded()
+            
+            // We also need to actually inform the collectionView that the item should be selected (if we don't
+            // then the user won't be able to deselect it)
+            if let firstSelectedIndexPath: IndexPath = self.firstSelectedIndexPath {
+                self.collectionView.selectItem(at: firstSelectedIndexPath, animated: false, scrollPosition: .centeredHorizontally)
+                self.collectionView.cellForItem(at: firstSelectedIndexPath)?.isSelected = true
+            }
         }
     }
 
@@ -489,9 +503,11 @@ class ImagePickerGridController: UICollectionViewController, PhotoLibraryDelegat
             didSelectAsset: asset,
             attachmentPublisher: photoCollectionContents.outgoingAttachment(for: asset)
         )
+        firstSelectedIndexPath = nil
 
         if !delegate.isInBatchSelectMode {
             // Don't show "selected" badge unless we're in batch mode
+            firstSelectedIndexPath = indexPath
             collectionView.deselectItem(at: indexPath, animated: false)
             delegate.imagePickerDidCompleteSelection(self)
         }
@@ -511,6 +527,7 @@ class ImagePickerGridController: UICollectionViewController, PhotoLibraryDelegat
         }
         
         delegate.imagePicker(self, didDeselectAsset: asset)
+        firstSelectedIndexPath = nil
     }
 
     override func collectionView(_ collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int {

Session/Media Viewing & Editing/SendMediaNavigationController.swift

@@ -691,6 +691,7 @@ private class DoneButton: UIView {
     func updateCount() {
         guard let delegate = delegate else { return }
 
+        badge.themeBackgroundColor = (delegate.doneButtonCount > 0 ? .primary : .disabled)
         badgeLabel.text = numberFormatter.string(for: delegate.doneButtonCount)
     }
     
@@ -729,11 +730,14 @@ private class DoneButton: UIView {
     }
 
     @objc func didTap(tapGesture: UITapGestureRecognizer) {
+        guard (delegate?.doneButtonCount ?? 0) > 0 else { return }
+        
         delegate?.doneButtonWasTapped(self)
     }
     
     override func touchesBegan(_ touches: Set<UITouch>, with event: UIEvent?) {
         guard
+            (delegate?.doneButtonCount ?? 0) > 0,
             isUserInteractionEnabled,
             let location: CGPoint = touches.first?.location(in: self),
             bounds.contains(location)

SessionMessagingKit/Sending & Receiving/Attachments/SignalAttachment.swift

@@ -1140,11 +1140,11 @@ public class SignalAttachment: Equatable, Hashable {
         isConvertibleToContactShare.hash(into: &hasher)
         isVoiceMessage.hash(into: &hasher)
         
-        /// There was a crash in `AttachmentApprovalViewController when trying to generate the hash
+        /// There was a crash in `AttachmentApprovalViewController` when trying to generate the hash
         /// value to store in a dictionary, I'm guessing it's due to either `dataSource`, `cachedImage` or `cachedVideoPreview`
         /// so, instead of trying to hash them directly which involves unknown behaviours due to `NSObject` & `UIImage` types, this
         /// has been reworked to use primitives
-        dataSource.sourceFilename.hash(into: &hasher)
+        dataSource.uniqueId.hash(into: &hasher)
         cachedImage?.size.width.hash(into: &hasher)
         cachedImage?.size.height.hash(into: &hasher)
         cachedVideoPreview?.size.width.hash(into: &hasher)

SessionUtilitiesKit/Media/DataSource.h

@@ -10,6 +10,7 @@ NS_ASSUME_NONNULL_BEGIN
 @interface DataSource : NSObject
 
 @property (nonatomic, nullable) NSString *sourceFilename;
+@property (nonatomic) NSUUID *uniqueId;
 
 // Should not be called unless necessary as it can involve an expensive read.
 - (NSData *)data;

SessionUtilitiesKit/Media/DataSource.m

@@ -124,6 +124,7 @@ NS_ASSUME_NONNULL_BEGIN
     instance.dataValue = data;
     instance.fileExtension = fileExtension;
     instance.shouldDeleteOnDeallocation = YES;
+    instance.uniqueId = [NSUUID UUID];
     return instance;
 }
 
@@ -246,6 +247,7 @@ NS_ASSUME_NONNULL_BEGIN
     DataSourcePath *instance = [DataSourcePath new];
     instance.filePath = fileUrl.path;
     instance.shouldDeleteOnDeallocation = shouldDeleteOnDeallocation;
+    instance.uniqueId = [NSUUID UUID];
     return instance;
 }
 
@@ -259,6 +261,7 @@ NS_ASSUME_NONNULL_BEGIN
     DataSourcePath *instance = [DataSourcePath new];
     instance.filePath = filePath;
     instance.shouldDeleteOnDeallocation = shouldDeleteOnDeallocation;
+    instance.uniqueId = [NSUUID UUID];
     return instance;
 }