From d1cf942f7efdff1f213615f4ad2cd78c30dc540e Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 18 Dec 2018 10:53:33 -0500 Subject: [PATCH] Respond to CR. --- Signal/test/views/ImageEditorTest.swift | 21 +-- .../AttachmentApprovalViewController.swift | 6 +- .../ImageEditorGestureRecognizer.swift | 2 +- .../Views/ImageEditor/ImageEditorModel.swift | 178 +++++++++++------- .../Views/ImageEditor/ImageEditorView.swift | 22 +-- 5 files changed, 129 insertions(+), 100 deletions(-) diff --git a/Signal/test/views/ImageEditorTest.swift b/Signal/test/views/ImageEditorTest.swift index 1c2352b6c..5a4642a8d 100644 --- a/Signal/test/views/ImageEditorTest.swift +++ b/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)) - } catch { - owsFail("Couldn't write dummy image.") - } + try! data.write(to: URL(fileURLWithPath: filePath)) return filePath } func testImageEditor() { let imagePath = writeDummyImage() - let imageEditor: ImageEditorModel - do { - imageEditor = try ImageEditorModel(srcImagePath: imagePath) - } catch { - owsFail("Couldn't create ImageEditorModel.") - } + let imageEditor = try! ImageEditorModel(srcImagePath: imagePath) XCTAssertFalse(imageEditor.canUndo()) XCTAssertFalse(imageEditor.canRedo()) XCTAssertEqual(0, imageEditor.itemCount()) diff --git a/SignalMessaging/ViewControllers/AttachmentApprovalViewController.swift b/SignalMessaging/ViewControllers/AttachmentApprovalViewController.swift index 0b0d6c762..44a505589 100644 --- a/SignalMessaging/ViewControllers/AttachmentApprovalViewController.swift +++ b/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 diff --git a/SignalMessaging/Views/ImageEditor/ImageEditorGestureRecognizer.swift b/SignalMessaging/Views/ImageEditor/ImageEditorGestureRecognizer.swift index d62b9e8f7..3040fc804 100644 --- a/SignalMessaging/Views/ImageEditor/ImageEditorGestureRecognizer.swift +++ b/SignalMessaging/Views/ImageEditor/ImageEditorGestureRecognizer.swift @@ -85,7 +85,7 @@ class ImageEditorGestureRecognizer: UIGestureRecognizer { override func touchesCancelled(_ touches: Set, with event: UIEvent) { super.touchesCancelled(touches, with: event) - state = .failed + state = .cancelled } public enum TouchType { diff --git a/SignalMessaging/Views/ImageEditor/ImageEditorModel.swift b/SignalMessaging/Views/ImageEditor/ImageEditorModel.swift index 83508967b..9aa1d5896 100644 --- a/SignalMessaging/Views/ImageEditor/ImageEditorModel.swift +++ b/SignalMessaging/Views/ImageEditor/ImageEditorModel.swift @@ -114,6 +114,103 @@ public class ImageEditorStrokeItem: ImageEditorItem { // MARK: - +public class OrderedDictionary: 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 { + 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. - var itemMap = [String: ImageEditorItem]() + public typealias ItemMapType = OrderedDictionary - // This represents the back-to-front ordering of the items. - var itemIds = [String]() + // This represents the current state of each item, + // a mapping of [itemId : item]. + var itemMap = ItemMapType() - @objc + // Used to create an initial, empty instances of this class. public override init() { } - @objc - public init(itemMap: [String: ImageEditorItem], - itemIds: [String]) { - + // Used to clone copies of instances of this class. + public init(itemMap: ItemMapType) { 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 { - 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.") - } + itemMap.append(key: item.itemId, value: item) } @objc public func replace(item: ImageEditorItem) { Logger.verbose("\(item.itemId)") - if itemMap[item.itemId] == nil { - 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.") - } + itemMap.replace(key: item.itemId, value: item) } @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 { - owsFail("Missing item in item map: \(itemId)") - } else { - itemMap.removeValue(forKey: itemId) - } + Logger.verbose("\(itemId)") - if !itemIds.contains(itemId) { - owsFail("Missing item in item list: \(itemId)") - } else { - itemIds = itemIds.filter { $0 != itemId } - } - - if itemIds.count != itemMap.count { - owsFailDebug("Invalid contents.") - } + itemMap.remove(key: itemId) } @objc public func itemCount() -> Int { - if itemIds.count != itemMap.count { - owsFailDebug("Invalid contents.") - } - return itemIds.count + return itemMap.count } @objc public func items() -> [ImageEditorItem] { - var items = [ImageEditorItem]() - for itemId in itemIds { - guard let item = self.itemMap[itemId] else { - owsFailDebug("Missing item") - continue - } - items.append(item) - } - return items + return itemMap.orderedValues() } } diff --git a/SignalMessaging/Views/ImageEditor/ImageEditorView.swift b/SignalMessaging/Views/ImageEditor/ImageEditorView.swift index bffbae081..a6001f1c5 100644 --- a/SignalMessaging/Views/ImageEditor/ImageEditorView.swift +++ b/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..