modifying testing image protocol to return square and full-screen images #102

jaimejiyepark · 2019-12-03T22:41:32Z

[RFC]

set a imageFullScreen bool that can only be set by the TestingImageDataSource protocol implementer
captureOutputWork checks:
- if testing image exists
  - if full screen image -> then get squared and full image.
  - if cropped -> send straight to ocr
- if frames from camera
  - get squared and full image

jaimejiyepark · 2019-12-03T22:44:14Z

Did not implement cropping the square in OCR to a credit card size yet since I want to get this portion done first.

…ardscan-ios into squared_full_images merge

jaimejiyepark · 2019-12-04T00:37:15Z

[RFC]

removed the objc attribute to use optionals
made an extension to scanBaseviewController that returns an optional CGImage array from the testingImageDataSource
added logic to captureOutputWork if testing images exist

kingst

good start, left a few comments

kingst · 2019-12-04T04:34:26Z

CardScan/Classes/ScanBaseViewController.swift

@@ -18,7 +19,7 @@ import Vision
    }


-    @objc public weak var testingImageDataSource: TestingImageDataSource?
+    public var testingImageDataSource: TestingImageDataSource?


I think we need to leave the weak in here to avoid a memory leak, but I'm not 100% sure

it wont let me put the weak in there :-/ . i get the errors 'weak' must not be applied to non-class-bound 'TestingImageDataSource'.

we should understand this more clearly. the "safe" thing to do here is to add the @objc and weak back and be done with it, but if you want to make a change it's going to require some deeper analysis

im going to see if theres a workaround in the next commit to just add the @objc back. it's all because of that optional return

CardScan/Classes/ScanBaseViewController.swift

CardScan/Classes/TestingImageExtension.swift

Example/CardScan/ViewController.swift

jaimejiyepark · 2019-12-04T19:36:45Z

I'll get on the end-end test once we approve the protocol design

kingst

More comments, looking better

kingst · 2019-12-04T20:05:53Z

CardScan/Classes/ScanBaseViewController.swift

@@ -18,7 +19,7 @@ import Vision
    }


-    @objc public weak var testingImageDataSource: TestingImageDataSource?
+    public var testingImageDataSource: TestingImageDataSource?


we should understand this more clearly. the "safe" thing to do here is to add the @objc and weak back and be done with it, but if you want to make a change it's going to require some deeper analysis

CardScan/Classes/ScanBaseViewController.swift

Example/CardScan/ViewController.swift

kingst

More comments

Example/CardScan/ViewController.swift

jaimejiyepark · 2019-12-04T21:48:13Z

AnyObject allows the protocol to only be conformed by class-types which allows us to put in that weak for the delegate.

https://developer.apple.com/documentation/swift/anyobject

kingst

More comments, getting closer. My last hesitation is around having a protocol that inherits from something -- it seems hacky.

kingst · 2019-12-04T22:24:17Z

CardScan/Classes/ScanBaseViewController.swift


        if #available(iOS 11.2, *) {
            if self.scanQrCode {
                self.blockingQrModel(pixelBuffer: pixelBuffer)
            } else {
-                self.blockingOcrModel(squareCardImage: squareCardImage, fullCardImage: fullCardImage)
+                self.blockingOcrModel(squareCardImage: squareAndFullCardImage.0, fullCardImage: squareAndFullCardImage.1)


naming individual tuple elements here doesn't seem great people would need to know that .0 is square and .1 is full. For squareAndFullCardImage why not have variables directly, something like let (square, full) = self.testingImageDataSource...

kingst · 2019-12-04T22:25:27Z

CardScan/Classes/ScanBaseViewController.swift

@@ -521,7 +496,6 @@ import Vision
        let rect = CGRect(x: cx - width / 2.0, y: cy - height / 2.0, width: width, height: height)

        self.currentImageRect = rect
-


Can you remove whitespace changes from here and the few other places that they show up?

jaimejiyepark · 2019-12-04T23:00:25Z

I would argue that its actually not hacky.
https://docs.swift.org/swift-book/LanguageGuide/Protocols.html#//apple_ref/doc/uid/TP40014097-CH25-ID267
The first portion of the delegate section shows an example of a protocol inheriting AnyObject

kingst

I stand corrected on protocols inheriting from AnyObject, looks good. #shipit

mod testing image protocol/ add squared image

8388512

jaimejiyepark requested a review from kingst December 3, 2019 22:41

igor-makarov and others added 5 commits December 3, 2019 15:17

adapt to iOS 9

617c5e3

mod testing image protocol/ add squared image

644a3d7

Merge branch 'squared_full_images' of https://github.com/getbouncer/c…

f2196f2

…ardscan-ios into squared_full_images merge

merge

b4fc244

[RFC] added extension

d14eda1

kingst reviewed Dec 4, 2019

View reviewed changes

jaimejiyepark added 2 commits December 4, 2019 11:05

merge

f32d972

nextSquareAndFullCardImage protocol

fc4955c

kingst reviewed Dec 4, 2019

View reviewed changes

Example/CardScan/ViewController.swift Outdated Show resolved Hide resolved

jaimejiyepark added 2 commits December 4, 2019 13:22

add objc back to protocol

e094b08

hopefully the last commit

7304509

kingst reviewed Dec 4, 2019

View reviewed changes

jaimejiyepark added 2 commits December 4, 2019 15:14

rid of all whitespace i hope

171634b

missed one whitespace

a6799db

kingst approved these changes Dec 4, 2019

View reviewed changes

jaimejiyepark merged commit f1815dc into master Dec 4, 2019

jaimejiyepark deleted the squared_full_images branch December 4, 2019 23:35

jaimejiyepark changed the title ~~mod testing image protocol/ add squared image~~ modifying testing image protocol to return square and full-screen images Jan 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modifying testing image protocol to return square and full-screen images #102

modifying testing image protocol to return square and full-screen images #102

jaimejiyepark commented Dec 3, 2019

jaimejiyepark commented Dec 3, 2019

jaimejiyepark commented Dec 4, 2019 •

edited

Loading

kingst left a comment

kingst Dec 4, 2019

jaimejiyepark Dec 4, 2019

kingst Dec 4, 2019

jaimejiyepark Dec 4, 2019

jaimejiyepark commented Dec 4, 2019

kingst left a comment

kingst Dec 4, 2019

kingst left a comment

jaimejiyepark commented Dec 4, 2019

kingst left a comment

kingst Dec 4, 2019

kingst Dec 4, 2019

jaimejiyepark commented Dec 4, 2019

kingst left a comment

		@@ -521,7 +496,6 @@ import Vision
		let rect = CGRect(x: cx - width / 2.0, y: cy - height / 2.0, width: width, height: height)

		self.currentImageRect = rect

modifying testing image protocol to return square and full-screen images #102

modifying testing image protocol to return square and full-screen images #102

Conversation

jaimejiyepark commented Dec 3, 2019

jaimejiyepark commented Dec 3, 2019

jaimejiyepark commented Dec 4, 2019 • edited Loading

kingst left a comment

Choose a reason for hiding this comment

kingst Dec 4, 2019

Choose a reason for hiding this comment

jaimejiyepark Dec 4, 2019

Choose a reason for hiding this comment

kingst Dec 4, 2019

Choose a reason for hiding this comment

jaimejiyepark Dec 4, 2019

Choose a reason for hiding this comment

jaimejiyepark commented Dec 4, 2019

kingst left a comment

Choose a reason for hiding this comment

kingst Dec 4, 2019

Choose a reason for hiding this comment

kingst left a comment

Choose a reason for hiding this comment

jaimejiyepark commented Dec 4, 2019

kingst left a comment

Choose a reason for hiding this comment

kingst Dec 4, 2019

Choose a reason for hiding this comment

kingst Dec 4, 2019

Choose a reason for hiding this comment

jaimejiyepark commented Dec 4, 2019

kingst left a comment

Choose a reason for hiding this comment

jaimejiyepark commented Dec 4, 2019 •

edited

Loading