Respond to CR.

Signal/test/views/ImageEditorTest.swift

@@ -28,31 +28,23 @@ class ImageEditorTest: SignalBaseTest {
     func testImageEditorContents() {
         let contents = ImageEditorContents()
         XCTAssertEqual(0, contents.itemMap.count)
-        XCTAssertEqual(0, contents.itemIds.count)
 
         let item = ImageEditorItem(itemType: .test)
         contents.append(item: item)
         XCTAssertEqual(1, contents.itemMap.count)
-        XCTAssertEqual(1, contents.itemIds.count)
 
         let contentsCopy = contents.clone()
         XCTAssertEqual(1, contents.itemMap.count)
-        XCTAssertEqual(1, contents.itemIds.count)
         XCTAssertEqual(1, contentsCopy.itemMap.count)
-        XCTAssertEqual(1, contentsCopy.itemIds.count)
 
         contentsCopy.remove(item: item)
         XCTAssertEqual(1, contents.itemMap.count)
-        XCTAssertEqual(1, contents.itemIds.count)
         XCTAssertEqual(0, contentsCopy.itemMap.count)
-        XCTAssertEqual(0, contentsCopy.itemIds.count)
 
         let modifiedItem = ImageEditorItem(itemId: item.itemId, itemType: item.itemType)
         contents.replace(item: modifiedItem)
         XCTAssertEqual(1, contents.itemMap.count)
-        XCTAssertEqual(1, contents.itemIds.count)
         XCTAssertEqual(0, contentsCopy.itemMap.count)
-        XCTAssertEqual(0, contentsCopy.itemIds.count)
     }
 
     private func writeDummyImage() -> String {
@@ -61,23 +53,14 @@ class ImageEditorTest: SignalBaseTest {
             owsFail("Couldn't export dummy image.")
         }
         let filePath = OWSFileSystem.temporaryFilePath(withFileExtension: "png")
-        do {
+        try! data.write(to: URL(fileURLWithPath: filePath))
-            try data.write(to: URL(fileURLWithPath: filePath))
-        } catch {
-            owsFail("Couldn't write dummy image.")
-        }
         return filePath
     }
 
     func testImageEditor() {
         let imagePath = writeDummyImage()
 
-        let imageEditor: ImageEditorModel
+        let imageEditor = try! ImageEditorModel(srcImagePath: imagePath)
-        do {
-            imageEditor = try ImageEditorModel(srcImagePath: imagePath)
-        } catch {
-            owsFail("Couldn't create ImageEditorModel.")
-        }
         XCTAssertFalse(imageEditor.canUndo())
         XCTAssertFalse(imageEditor.canRedo())
         XCTAssertEqual(0, imageEditor.itemCount())

SignalMessaging/ViewControllers/AttachmentApprovalViewController.swift

@@ -74,7 +74,7 @@ class SignalAttachmentItem: Hashable {
             do {
                 imageEditorModel = try ImageEditorModel(srcImagePath: path)
             } catch {
-                // Usually not an error.
+                // Usually not an error; this usually indicates invalid input.
                 Logger.warn("Could not create image editor: \(error)")
             }
         }
@@ -582,7 +582,7 @@ public class AttachmentApprovalViewController: UIPageViewController, UIPageViewC
     }
 
     var attachments: [SignalAttachment] {
-        return attachmentItems.map { self.attachment(forAttachmentItem: $0) }
+        return attachmentItems.map { self.processedAttachment(forAttachmentItem: $0) }
     }
 
     // For any attachments edited with the image editor, returns a
@@ -592,7 +592,7 @@ public class AttachmentApprovalViewController: UIPageViewController, UIPageViewC
     // If any errors occurs in the export process, we fail over to
     // sending the original attachment.  This seems better than trying
     // to involve the user in resolving the issue.
-    func attachment(forAttachmentItem attachmentItem: SignalAttachmentItem) -> SignalAttachment {
+    func processedAttachment(forAttachmentItem attachmentItem: SignalAttachmentItem) -> SignalAttachment {
         guard let imageEditorModel = attachmentItem.imageEditorModel else {
             // Image was not edited.
             return attachmentItem.attachment

SignalMessaging/Views/ImageEditor/ImageEditorGestureRecognizer.swift

@@ -85,7 +85,7 @@ class ImageEditorGestureRecognizer: UIGestureRecognizer {
     override func touchesCancelled(_ touches: Set<UITouch>, with event: UIEvent) {
         super.touchesCancelled(touches, with: event)
 
-        state = .failed
+        state = .cancelled
     }
 
     public enum TouchType {

SignalMessaging/Views/ImageEditor/ImageEditorModel.swift

@@ -114,6 +114,103 @@ public class ImageEditorStrokeItem: ImageEditorItem {
 
 // MARK: -
 
+public class OrderedDictionary<ValueType>: NSObject {
+
+    public typealias KeyType = String
+
+    var keyValueMap = [KeyType: ValueType]()
+
+    var orderedKeys = [KeyType]()
+
+    public override init() {
+    }
+
+    // Used to clone copies of instances of this class.
+    public init(keyValueMap: [KeyType: ValueType],
+                orderedKeys: [KeyType]) {
+
+        self.keyValueMap = keyValueMap
+        self.orderedKeys = orderedKeys
+    }
+
+    // Since the contents are immutable, we only modify copies
+    // made with this method.
+    public func clone() -> OrderedDictionary<ValueType> {
+        return OrderedDictionary(keyValueMap: keyValueMap, orderedKeys: orderedKeys)
+    }
+
+    public func append(key: KeyType, value: ValueType) {
+        if keyValueMap[key] != nil {
+            owsFailDebug("Unexpected duplicate key in key map: \(key)")
+        }
+        keyValueMap[key] = value
+
+        if orderedKeys.contains(key) {
+            owsFailDebug("Unexpected duplicate key in key list: \(key)")
+        } else {
+            orderedKeys.append(key)
+        }
+
+        if orderedKeys.count != keyValueMap.count {
+            owsFailDebug("Invalid contents.")
+        }
+    }
+
+    public func replace(key: KeyType, value: ValueType) {
+        if keyValueMap[key] == nil {
+            owsFailDebug("Missing key in key map: \(key)")
+        }
+        keyValueMap[key] = value
+
+        if !orderedKeys.contains(key) {
+            owsFailDebug("Missing key in key list: \(key)")
+        }
+
+        if orderedKeys.count != keyValueMap.count {
+            owsFailDebug("Invalid contents.")
+        }
+    }
+
+    public func remove(key: KeyType) {
+        if keyValueMap[key] == nil {
+            owsFailDebug("Missing key in key map: \(key)")
+        } else {
+            keyValueMap.removeValue(forKey: key)
+        }
+
+        if !orderedKeys.contains(key) {
+            owsFailDebug("Missing key in key list: \(key)")
+        } else {
+            orderedKeys = orderedKeys.filter { $0 != key }
+        }
+
+        if orderedKeys.count != keyValueMap.count {
+            owsFailDebug("Invalid contents.")
+        }
+    }
+
+    public var count: Int {
+        if orderedKeys.count != keyValueMap.count {
+            owsFailDebug("Invalid contents.")
+        }
+        return orderedKeys.count
+    }
+
+    public func orderedValues() -> [ValueType] {
+        var values = [ValueType]()
+        for key in orderedKeys {
+            guard let value = self.keyValueMap[key] else {
+                owsFailDebug("Missing value")
+                continue
+            }
+            values.append(value)
+        }
+        return values
+    }
+}
+
+// MARK: -
+
 // ImageEditorContents represents a snapshot of canvas
 // state.
 //
@@ -121,114 +218,63 @@ public class ImageEditorStrokeItem: ImageEditorItem {
 // as immutable, once configured.
 public class ImageEditorContents: NSObject {
 
-    // This represents the current state of each item.
+    public typealias ItemMapType = OrderedDictionary<ImageEditorItem>
-    var itemMap = [String: ImageEditorItem]()
 
-    // This represents the back-to-front ordering of the items.
+    // This represents the current state of each item,
-    var itemIds = [String]()
+    // a mapping of [itemId : item].
+    var itemMap = ItemMapType()
 
-    @objc
+    // Used to create an initial, empty instances of this class.
     public override init() {
     }
 
-    @objc
+    // Used to clone copies of instances of this class.
-    public init(itemMap: [String: ImageEditorItem],
+    public init(itemMap: ItemMapType) {
-                itemIds: [String]) {
-
         self.itemMap = itemMap
-        self.itemIds = itemIds
     }
 
     // Since the contents are immutable, we only modify copies
     // made with this method.
-    @objc
     public func clone() -> ImageEditorContents {
-        return ImageEditorContents(itemMap: itemMap, itemIds: itemIds)
+        return ImageEditorContents(itemMap: itemMap.clone())
     }
 
     @objc
     public func append(item: ImageEditorItem) {
         Logger.verbose("\(item.itemId)")
 
-        if itemMap[item.itemId] != nil {
+        itemMap.append(key: item.itemId, value: item)
-            owsFail("Unexpected duplicate item in item map: \(item.itemId)")
-        }
-        itemMap[item.itemId] = item
-
-        if itemIds.contains(item.itemId) {
-            owsFail("Unexpected duplicate item in item list: \(item.itemId)")
-        } else {
-            itemIds.append(item.itemId)
-        }
-
-        if itemIds.count != itemMap.count {
-            owsFailDebug("Invalid contents.")
-        }
     }
 
     @objc
     public func replace(item: ImageEditorItem) {
         Logger.verbose("\(item.itemId)")
 
-        if itemMap[item.itemId] == nil {
+        itemMap.replace(key: item.itemId, value: item)
-            owsFail("Missing item in item map: \(item.itemId)")
-        }
-        itemMap[item.itemId] = item
-
-        if !itemIds.contains(item.itemId) {
-            owsFail("Missing item in item list: \(item.itemId)")
-        }
-
-        if itemIds.count != itemMap.count {
-            owsFailDebug("Invalid contents.")
-        }
     }
 
     @objc
     public func remove(item: ImageEditorItem) {
         Logger.verbose("\(item.itemId)")
 
-        remove(itemId: item.itemId)
+        itemMap.remove(key: item.itemId)
     }
 
     @objc
     public func remove(itemId: String) {
-        if itemMap[itemId] == nil {
+        Logger.verbose("\(itemId)")
-            owsFail("Missing item in item map: \(itemId)")
-        } else {
-            itemMap.removeValue(forKey: itemId)
-        }
 
-        if !itemIds.contains(itemId) {
+        itemMap.remove(key: itemId)
-            owsFail("Missing item in item list: \(itemId)")
-        } else {
-            itemIds = itemIds.filter { $0 != itemId }
-        }
-
-        if itemIds.count != itemMap.count {
-            owsFailDebug("Invalid contents.")
-        }
     }
 
     @objc
     public func itemCount() -> Int {
-        if itemIds.count != itemMap.count {
+        return itemMap.count
-            owsFailDebug("Invalid contents.")
-        }
-        return itemIds.count
     }
 
     @objc
     public func items() -> [ImageEditorItem] {
-        var items = [ImageEditorItem]()
+        return itemMap.orderedValues()
-        for itemId in itemIds {
-            guard let item = self.itemMap[itemId] else {
-                owsFailDebug("Missing item")
-                continue
-            }
-            items.append(item)
-        }
-        return items
     }
 }
 

SignalMessaging/Views/ImageEditor/ImageEditorView.swift

@@ -204,7 +204,7 @@ public class ImageEditorView: UIView, ImageEditorModelDelegate {
         let points = applySmoothing(to: unitSamples.map { (unitSample) in
             transformSampleToPoint(unitSample)
         })
-        var lastForwardVector = CGPoint.zero
+        var previousForwardVector = CGPoint.zero
         for index in 0..<points.count {
             let point = points[index]
 
@@ -215,22 +215,22 @@ public class ImageEditorView: UIView, ImageEditorModelDelegate {
                 forwardVector = CGPointSubtract(nextPoint, point)
             } else if index == points.count - 1 {
                 // Last sample.
-                let lastPoint = points[index - 1]
+                let previousPoint = points[index - 1]
-                forwardVector = CGPointSubtract(point, lastPoint)
+                forwardVector = CGPointSubtract(point, previousPoint)
             } else {
                 // Middle samples.
-                let lastPoint = points[index - 1]
+                let previousPoint = points[index - 1]
-                let lastForwardVector = CGPointSubtract(point, lastPoint)
+                let previousPointForwardVector = CGPointSubtract(point, previousPoint)
                 let nextPoint = points[index + 1]
-                let nextForwardVector = CGPointSubtract(nextPoint, point)
+                let nextPointForwardVector = CGPointSubtract(nextPoint, point)
-                forwardVector = CGPointScale(CGPointAdd(lastForwardVector, nextForwardVector), 0.5)
+                forwardVector = CGPointScale(CGPointAdd(previousPointForwardVector, nextPointForwardVector), 0.5)
             }
 
             if index == 0 {
                 // First sample.
                 bezierPath.move(to: point)
             } else {
-                let lastPoint = points[index - 1]
+                let previousPoint = points[index - 1]
                 // We apply more than one kind of smoothing.
                 // This smoothing avoids rendering "angled segments"
                 // by drawing the stroke as a series of curves.
@@ -243,12 +243,12 @@ public class ImageEditorView: UIView, ImageEditorModelDelegate {
                 //
                 // TODO: Tune this variable once we have stroke input.
                 let controlPointFactor: CGFloat = 0.25
-                let controlPoint1 = CGPointAdd(lastPoint, CGPointScale(lastForwardVector, +controlPointFactor))
+                let controlPoint1 = CGPointAdd(previousPoint, CGPointScale(previousForwardVector, +controlPointFactor))
                 let controlPoint2 = CGPointAdd(point, CGPointScale(forwardVector, -controlPointFactor))
                 // We're using Cubic curves.
                 bezierPath.addCurve(to: point, controlPoint1: controlPoint1, controlPoint2: controlPoint2)
             }
-            lastForwardVector = forwardVector
+            previousForwardVector = forwardVector
         }
 
         shapeLayer.path = bezierPath.cgPath
@@ -310,6 +310,7 @@ public class ImageEditorView: UIView, ImageEditorModelDelegate {
 
         let dstScale: CGFloat = 1.0 // The size is specified in pixels, not in points.
         UIGraphicsBeginImageContextWithOptions(dstSizePixels, !hasAlpha, dstScale)
+        defer { UIGraphicsEndImageContext() }
 
         guard let context = UIGraphicsGetCurrentContext() else {
             owsFailDebug("Could not create output context.")
@@ -338,7 +339,6 @@ public class ImageEditorView: UIView, ImageEditorModelDelegate {
         if scaledImage == nil {
             owsFailDebug("could not generate dst image.")
         }
-        UIGraphicsEndImageContext()
         return scaledImage
     }
 }