Remove warnings and deprecate captureImage method #40

robinsalehjan · 2019-06-07T10:06:46Z

Why?

We had 20 or so warnings related to using old APIs from iOS 9
Deprecate the captureImage method in favour of delegation
Move private methods into extensions
Remove fileprivate modifier in favour of private
Update Xcode with recommended settings for localisation
Ran SwiftLint to have consistency across the project

bstien · 2019-06-07T13:04:50Z

Sources/Data/PhotoStorage.swift

-    fileprivate let assetLibrary = ALAssetsLibrary()
+    private let baseURL: URL
+    private let queue = DispatchQueue(label: "no.finn.finjonon.disk-cache-writes", attributes: [])
+    private let resizeQueue = DispatchQueue(label: "no.finn.finjonon.disk-cache-resizes", attributes: DispatchQueue.Attributes.concurrent)


Could this be refactored to DispatchQueue(label: "no.finn.finjonon.disk-cache-resizes", attributes: .concurrent) instead?

bstien · 2019-06-07T13:16:15Z

Sources/PhotoCaptureViewController/CaptureManager.swift

+            discoverySession = AVCaptureDevice.DiscoverySession(deviceTypes: [.builtInTrueDepthCamera, .builtInDualCamera, .builtInWideAngleCamera], mediaType: .video, position: .unspecified)
+        } else {
+            discoverySession = AVCaptureDevice.DiscoverySession(deviceTypes: [.builtInWideAngleCamera], mediaType: .video, position: .unspecified)
+        }


Could this rather be

let deviceTypes: [AVCaptureDevice.DeviceType] if #available(iOS 11.2, *) { deviceTypes = [.builtInTrueDepthCamera, .builtInDualCamera, .builtInWideAngleCamera] } else { deviceTypes = [.builtInWideAngleCamera] } let discoverySession = AVCaptureDevice.DiscoverySession(deviceTypes: deviceTypes, mediaType: .video, position: .unspecified)

Nitpicking, but I think it makes it a bit easier to read since the only difference is the list of deviceTypes :)

bstien · 2019-06-07T13:20:01Z

Sources/PhotoCaptureViewController/CaptureManager.swift

+            didCaptureImageCompletion = nil
+        }
+
+        if error == nil {


Saves you an indentation😄

guard error == nil else { NSLog("Failed capturing still images: \(String(describing: error))") return }

bstien · 2019-06-07T13:21:42Z

Sources/PhotoCaptureViewController/CaptureManager.swift

+        if error == nil {
+            if let sampleBuffer = photoSampleBuffer, let data = AVCapturePhotoOutput.jpegPhotoDataRepresentation(forJPEGSampleBuffer: sampleBuffer, previewPhotoSampleBuffer: nil) {
+                if let metadata = CMCopyDictionaryOfAttachments(allocator: nil, target: sampleBuffer, attachmentMode: CMAttachmentMode(kCMAttachmentMode_ShouldPropagate)) as NSDictionary? {
+                    DispatchQueue.main.async {


Should probably use [weak self] here

Good catch 👍

bstien · 2019-06-07T13:22:43Z

Sources/PhotoCaptureViewController/CaptureManager.swift

+            if let sampleBuffer = photoSampleBuffer, let data = AVCapturePhotoOutput.jpegPhotoDataRepresentation(forJPEGSampleBuffer: sampleBuffer, previewPhotoSampleBuffer: nil) {
+                if let metadata = CMCopyDictionaryOfAttachments(allocator: nil, target: sampleBuffer, attachmentMode: CMAttachmentMode(kCMAttachmentMode_ShouldPropagate)) as NSDictionary? {
+                    DispatchQueue.main.async {
+                        if self.didCaptureImageCompletion == nil {


And an if let here

So we avoid the ❗️unwrap a couple lines down

vadymmarkov · 2019-06-11T12:10:16Z

Sources/Data/PhotoStorage.swift

+
+            imageManager.requestImageData(for: asset, options: imageRequestOptions) { (imageData, _, _, _) in
+                guard let imageData = imageData else { return }
+                self.createAssetFromImageData(imageData as Data, completion: completion)


Should we use weak self here?

vadymmarkov · 2019-06-11T12:12:25Z

Sources/PhotoCaptureViewController/CaptureManager.swift

@@ -11,17 +11,17 @@ enum CaptureManagerViewfinderMode {
 }

 protocol CaptureManagerDelegate: AnyObject {
+    func captureManager(_ manager: CaptureManager, didCaptureImage data: Data?, withMetadata metadata: NSDictionary?)


Perhaps didCaptureImageData is more descriptive since it's an instance of Data?

vadymmarkov · 2019-06-11T12:13:55Z

Sources/PhotoCaptureViewController/CaptureManager.swift

-                    NSLog("Failed capturing still images: \(String(describing: error))")
-                }
-            })
+            self.cameraSettings = self.createCapturePhotoSettingsObject()


Should we use weak self here?

vadymmarkov · 2019-06-11T12:14:50Z

Sources/PhotoCaptureViewController/CaptureManager.swift

-                }
-            })
+            self.cameraSettings = self.createCapturePhotoSettingsObject()
+            self.cameraOutput.capturePhoto(with: self.cameraSettings!, delegate: self)


Should we guard cameraSettings instead of force unwrapping?

vadymmarkov · 2019-06-11T12:15:30Z

Sources/PhotoCaptureViewController/CaptureManager.swift

+    func createCapturePhotoSettingsObject() -> AVCapturePhotoSettings {
+        var newCameraSettings: AVCapturePhotoSettings
+
+        if let currentCameraSettings = self.cameraSettings {


No need for self here.

vadymmarkov · 2019-06-11T12:16:16Z

Sources/PhotoCaptureViewController/CaptureManager.swift

-
-            if self.session.canAddOutput(self.stillImageOutput!) {
-                self.session.addOutput(self.stillImageOutput!)
+            if self.session.canAddOutput(self.cameraOutput) {


Should we use weak self in this closure?

vadymmarkov · 2019-06-11T12:19:52Z

Sources/PhotoCaptureViewController/CaptureManager.swift

+        let discoverySession = AVCaptureDevice.DiscoverySession(deviceTypes: deviceTypes, mediaType: .video, position: .unspecified)
+        let availableCameraDevices = discoverySession.devices
+
+        guard availableCameraDevices.isEmpty == false else { fatalError("No camera devices found") }


I would rather return instead of calling fatalError here, mainly because it's annoying when simulator crashes while opening this view in the app.

vadymmarkov · 2019-06-11T12:21:42Z

Sources/PhotoCaptureViewController/CaptureManager.swift

+        }
+
+        guard error == nil else {
+            NSLog("Failed capturing still images: \(String(describing: error))")


Not sure how useful it is, but we could also add a delegate method to report back about errors.

I agree 👍

vadymmarkov · 2019-06-11T12:22:58Z

Sources/PhotoCaptureViewController/PhotoCaptureViewController.swift

+        createAssetFromImageData(data as Data, completion: { (asset: Asset) in
+            var mutableAsset = asset
+            mutableAsset.imageDataSourceType = .camera
+            self.didAddAsset(mutableAsset)


How about weak self?

vadymmarkov

Great work @robinsalehjan 💪

robinsalehjan added 4 commits June 5, 2019 13:08

Group similar files into groups

3a0fb16

Remove warnings and deprecate captureImage method

c7e2bf7

Update to recommended settings

4ff85d9

Merge branch 'master' into fix/remove-deprecation-warnings

510c0a1

robinsalehjan requested a review from a team June 7, 2019 10:06

bstien reviewed Jun 7, 2019

View reviewed changes

Address PR comments

7e17d3a

robinsalehjan force-pushed the fix/remove-deprecation-warnings branch from d56578d to 7e17d3a Compare June 11, 2019 07:06

vadymmarkov reviewed Jun 11, 2019

View reviewed changes

robinsalehjan added 2 commits June 11, 2019 14:47

Address PR comments

5f30408

Use weak self in queue, resizeQueue and captureQueue

d6a41bb

vadymmarkov approved these changes Jun 11, 2019

View reviewed changes

Create delegate method for propagating errors

8a327c6

robinsalehjan merged commit 399ab1d into master Jun 12, 2019

robinsalehjan deleted the fix/remove-deprecation-warnings branch June 12, 2019 06:06

This was referenced Jun 12, 2019

New deprecation warnings #34

Closed

Fix deprecation warnings #32

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove warnings and deprecate captureImage method #40

Remove warnings and deprecate captureImage method #40

robinsalehjan commented Jun 7, 2019 •

edited

Loading

bstien Jun 7, 2019

bstien Jun 7, 2019

bstien Jun 7, 2019 •

edited

Loading

bstien Jun 7, 2019

bstien Jun 7, 2019

robinsalehjan Jun 11, 2019

bstien Jun 7, 2019

bstien Jun 7, 2019

vadymmarkov Jun 11, 2019

vadymmarkov Jun 11, 2019

vadymmarkov Jun 11, 2019

vadymmarkov Jun 11, 2019

vadymmarkov Jun 11, 2019

vadymmarkov Jun 11, 2019

vadymmarkov Jun 11, 2019

vadymmarkov Jun 11, 2019

robinsalehjan Jun 11, 2019

vadymmarkov Jun 11, 2019

vadymmarkov left a comment

Remove warnings and deprecate captureImage method #40

Remove warnings and deprecate captureImage method #40

Conversation

robinsalehjan commented Jun 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bstien Jun 7, 2019 • edited Loading

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

vadymmarkov left a comment

Choose a reason for hiding this comment

robinsalehjan commented Jun 7, 2019 •

edited

Loading

bstien Jun 7, 2019 •

edited

Loading