Skip to main content

Merge pull request #17 from alexwlchan/tidy-up-event-handler

ID
d6cc5e9
date
2023-06-11 19:59:53+00:00
author
Alex Chan <alex@alexwlchan.net>
parents
1a15e4b, f6262e5
message
Merge pull request #17 from alexwlchan/tidy-up-event-handler

Don't play the "funk" sound when you handle keyboard events
changed files
3 files, 54 additions, 14 deletions

Changed files

BlinkReviewer/Blink/Photos/PhotosLibrary.swift (6986) → BlinkReviewer/Blink/Photos/PhotosLibrary.swift (7877)

diff --git a/BlinkReviewer/Blink/Photos/PhotosLibrary.swift b/BlinkReviewer/Blink/Photos/PhotosLibrary.swift
index 1060b72..9a8d844 100644
--- a/BlinkReviewer/Blink/Photos/PhotosLibrary.swift
+++ b/BlinkReviewer/Blink/Photos/PhotosLibrary.swift
@@ -125,6 +125,24 @@ class PhotosLibrary: NSObject, ObservableObject, PHPhotoLibraryChangeObserver {
         }
     }
     
+    func asset(at index: Int) -> PHAsset {
+        assets.object(at: index)
+    }
+    
+    /// Get the review state of a given asset.
+    ///
+    /// These methods are called repeatedly on every view (when we get the
+    /// state of thumbnails), so they need to be *fast*.
+    ///
+    /// This is why we cache the list of rejected/needs action/approved assets --
+    /// to make this method fast and performant.
+    ///
+    /// Note: it's possibly for an asset to be in multiple albums if the user
+    /// fiddles with it, so we show the "most destructive" state first -- the
+    /// state that might cause data loss if the user deletes all their rejected
+    /// images.  If they toggle the state in the app, we'll fix it.
+    ///
+    /// TODO: Log a warning here? Resolve somehow?
     func state(of asset: PHAsset) -> ReviewState? {
         if self.rejectedAssets.contains(asset) {
             return .Rejected
@@ -141,6 +159,10 @@ class PhotosLibrary: NSObject, ObservableObject, PHPhotoLibraryChangeObserver {
         return nil
     }
     
+    func state(ofAssetAtIndex index: Int) -> ReviewState? {
+        state(of: asset(at: index))
+    }
+    
     // Implements a basic cache for thumbnail images.
     //
     // Thumbnail images are small and easily reused; I've put them here because

BlinkReviewer/Blink/Views/PhotoReviewer.swift (14313) → BlinkReviewer/Blink/Views/PhotoReviewer.swift (14929)

diff --git a/BlinkReviewer/Blink/Views/PhotoReviewer.swift b/BlinkReviewer/Blink/Views/PhotoReviewer.swift
index f0b3fd7..fb5670c 100644
--- a/BlinkReviewer/Blink/Views/PhotoReviewer.swift
+++ b/BlinkReviewer/Blink/Views/PhotoReviewer.swift
@@ -26,7 +26,7 @@ struct PhotoReviewer: View {
     @State var _focusedAsset: PHAsset? = nil
     
     var focusedAsset: PHAsset {
-        return photosLibrary.assets.object(at: focusedAssetIndex)
+        return photosLibrary.asset(at: focusedAssetIndex)
     }
     
     @State var showStatistics: Bool = false
@@ -83,10 +83,10 @@ struct PhotoReviewer: View {
             }
             .background(.black)
             .onAppear {
-                NSEvent.addLocalMonitorForEvents(matching: .keyDown) { event in
-                    handleKeyDown(event)
-                    return event
-                }
+                NSEvent.addLocalMonitorForEvents(
+                    matching: .keyDown,
+                    handler: handleKeyDown
+                )
             }
             // These two lines update the big image that fills most of the window.
             // See the comments on FocusedImage for more explanation of why this is
@@ -126,7 +126,7 @@ struct PhotoReviewer: View {
         //
         // e.g. the change is about album data, or all the changes are further
         // along than the focused asset.
-        if photosLibrary.assets.object(at: focusedAssetIndex) == self._focusedAsset {
+        if photosLibrary.asset(at: focusedAssetIndex) == self._focusedAsset {
             logger.debug("Focused asset is in the same place as before, nothing to do [\(changeId, privacy: .public)]")
             return
         }
@@ -175,7 +175,7 @@ struct PhotoReviewer: View {
         // If we've got a delta, check to see if it points us to the right asset.
         //
         // If it does, we're done!
-        if photosLibrary.assets.object(at: focusedAssetIndex + (delta ?? 0)) == self._focusedAsset {
+        if photosLibrary.asset(at: focusedAssetIndex + (delta ?? 0)) == self._focusedAsset {
             logger.debug("Incremental changes found the new position of the asset [\(changeId, privacy: .public)]")
             focusedAssetIndex += delta ?? 0
             return
@@ -196,7 +196,7 @@ struct PhotoReviewer: View {
         let matchingAssetInUpdatedLibrary =
             (0..<photosLibrary.assets.count)
                 .first(where: {
-                    photosLibrary.assets.object(at: $0).localIdentifier ==
+                    photosLibrary.asset(at: $0).localIdentifier ==
                         self._focusedAsset?.localIdentifier
                 })
         
@@ -217,7 +217,14 @@ struct PhotoReviewer: View {
         self.focusedAssetIndex += (delta ?? 0)
     }
 
-    private func handleKeyDown(_ event: NSEvent) {
+    /// Handle any keypresses in the app.
+    ///
+    /// Note: this function should return `nil` for any events that it
+    /// processes; any events it returns will be passed to other event handlers
+    /// to see if anything else knows what to do with them.  Among other
+    /// issues, this results in an annoying "funk" sound playing on
+    /// every event, because the OS thinks the event is unhandled.
+    private func handleKeyDown(_ event: NSEvent) -> NSEvent? {
         let logger = Logger()
         
         switch event {
@@ -226,12 +233,14 @@ struct PhotoReviewer: View {
                 if focusedAssetIndex < photosLibrary.assets.count - 1 {
                     focusedAssetIndex += 1
                 }
+                return nil
             
             case let e where e.specialKey == NSEvent.SpecialKey.rightArrow:
                 print("to the right!")
                 if focusedAssetIndex > 0 {
                     focusedAssetIndex -= 1
                 }
+                return nil
             
             case let e where e.characters == "1" || e.characters == "2" || e.characters == "3":
                 print("time to review!")
@@ -277,6 +286,7 @@ struct PhotoReviewer: View {
                 if focusedAssetIndex < photosLibrary.assets.count - 1 {
                     focusedAssetIndex += 1
                 }
+                return nil
             
             case let e where e.characters == "c":
                 let crossStitch = getAlbum(withName: "Cross stitch")
@@ -285,43 +295,51 @@ struct PhotoReviewer: View {
                     focusedAsset.toggle(inAlbum: crossStitch)
                 }
             
+                return nil
+            
             case let e where e.characters == "f":
                 try! PHPhotoLibrary.shared().performChangesAndWait {
                     PHAssetChangeRequest(for: focusedAsset).isFavorite = !focusedAsset.isFavorite
                 }
+            
+                return nil
 
             case let e where e.characters == "d":
                 showDebug.toggle()
+                return nil
             
             case let e where e.characters == "s":
                 showStatistics.toggle()
+                return nil
             
             case let e where e.characters == "i":
                 showInfo.toggle()
+                return nil
             
             case let e where e.characters == "u":
                 if photosLibrary.state(of: focusedAsset) != nil {
                     if let lastUnreviewed = (focusedAssetIndex..<photosLibrary.assets.count).first(where: { index in
-                        photosLibrary.state(of: photosLibrary.assets.object(at: index)) == nil
+                        photosLibrary.state(ofAssetAtIndex: index) == nil
                     }) {
                         focusedAssetIndex = lastUnreviewed
                     }
                 }
+                return nil
             
             case let e where e.characters == "?":
                 while true {
                     let randomIndex = (0..<photosLibrary.assets.count).randomElement()!
                     
-                    if photosLibrary.state(of: photosLibrary.assets.object(at: randomIndex)) == nil {
+                    if photosLibrary.state(ofAssetAtIndex: randomIndex) == nil {
                         focusedAssetIndex = randomIndex
                         break
                     }
                 }
-            
+                return nil
 
             default:
                 logger.info("Received unhandled keyboard event: \(event, privacy: .public)")
-                break
+                return event
         }
     }
 }

BlinkReviewer/Blink/Views/Thumbnails/ThumbnailList.swift (1083) → BlinkReviewer/Blink/Views/Thumbnails/ThumbnailList.swift (1075)

diff --git a/BlinkReviewer/Blink/Views/Thumbnails/ThumbnailList.swift b/BlinkReviewer/Blink/Views/Thumbnails/ThumbnailList.swift
index 29c228c..efcc95b 100644
--- a/BlinkReviewer/Blink/Views/Thumbnails/ThumbnailList.swift
+++ b/BlinkReviewer/Blink/Views/Thumbnails/ThumbnailList.swift
@@ -26,7 +26,7 @@ struct ThumbnailList: View {
             .onChange(of: focusedAssetIndex, perform: { newIndex in
                 withAnimation {
                     proxy.scrollTo(
-                        photosLibrary.assets.object(at: newIndex).localIdentifier,
+                        photosLibrary.asset(at: newIndex).localIdentifier,
                         anchor: .center
                     )
                 }