Respond to CR.

Signal/src/ViewControllers/ConversationView/Cells/AttachmentUploadView.h

@@ -1,5 +1,5 @@
 //
-//  Copyright (c) 2017 Open Whisper Systems. All rights reserved.
+//  Copyright (c) 2018 Open Whisper Systems. All rights reserved.
 //
 
 NS_ASSUME_NONNULL_BEGIN
@@ -19,7 +19,6 @@ typedef void (^AttachmentStateBlock)(BOOL isAttachmentReady);
 @interface AttachmentUploadView : UIView
 
 - (instancetype)initWithAttachment:(TSAttachmentStream *)attachment
-                         superview:(UIView *)superview
            attachmentStateCallback:(AttachmentStateBlock _Nullable)attachmentStateCallback;
 
 @end

Signal/src/ViewControllers/ConversationView/Cells/AttachmentUploadView.m

@@ -1,5 +1,5 @@
 //
-//  Copyright (c) 2017 Open Whisper Systems. All rights reserved.
+//  Copyright (c) 2018 Open Whisper Systems. All rights reserved.
 //
 
 #import "AttachmentUploadView.h"
@@ -32,21 +32,16 @@ NS_ASSUME_NONNULL_BEGIN
 @implementation AttachmentUploadView
 
 - (instancetype)initWithAttachment:(TSAttachmentStream *)attachment
-                         superview:(UIView *)superview
            attachmentStateCallback:(AttachmentStateBlock _Nullable)attachmentStateCallback
 {
     self = [super init];
 
     if (self) {
         OWSAssert(attachment);
-        OWSAssert(superview);
 
         self.attachment = attachment;
         self.attachmentStateCallback = attachmentStateCallback;
 
-        [superview addSubview:self];
-        [self autoPinToSuperviewEdges];
-
         _bezierPathView = [OWSBezierPathView new];
         self.bezierPathView.configureShapeLayerBlock = ^(CAShapeLayer *layer, CGRect bounds) {
             layer.path = [UIBezierPath bezierPathWithRect:bounds].CGPath;

Signal/src/ViewControllers/ConversationView/Cells/OWSBubbleStrokeView.m

@@ -28,6 +28,9 @@ NS_ASSUME_NONNULL_BEGIN
     self.opaque = NO;
     self.backgroundColor = [UIColor clearColor];
 
+    self.shapeLayer = [CAShapeLayer new];
+    [self.layer addSublayer:self.shapeLayer];
+
     return self;
 }
 
@@ -47,7 +50,7 @@ NS_ASSUME_NONNULL_BEGIN
 
 - (void)setFrame:(CGRect)frame
 {
-    BOOL didChange = !CGSizeEqualToSize(self.frame.size, frame.size);
+    BOOL didChange = !CGRectEqualToRect(self.frame, frame);
 
     [super setFrame:frame];
 
@@ -58,7 +61,7 @@ NS_ASSUME_NONNULL_BEGIN
 
 - (void)setBounds:(CGRect)bounds
 {
-    BOOL didChange = !CGSizeEqualToSize(self.bounds.size, bounds.size);
+    BOOL didChange = !CGRectEqualToRect(self.bounds, bounds);
 
     [super setBounds:bounds];
 
@@ -76,10 +79,7 @@ NS_ASSUME_NONNULL_BEGIN
 
 - (void)updateLayers
 {
-    if (!self.shapeLayer) {
-        self.shapeLayer = [CAShapeLayer new];
-        [self.layer addSublayer:self.shapeLayer];
-    }
+    OWSAssert(self.shapeLayer);
 
     // Don't fill the shape layer; we just want a stroke around the border.
     self.shapeLayer.fillColor = [UIColor clearColor].CGColor;

Signal/src/ViewControllers/ConversationView/Cells/OWSBubbleView.m

@@ -8,7 +8,6 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
-// This approximates the curve of our message bubbles, which makes the animation feel a little smoother.
 const CGFloat kOWSMessageCellCornerRadius = 17;
 
 const CGFloat kBubbleVRounding = kOWSMessageCellCornerRadius;
@@ -29,6 +28,22 @@ const CGFloat kBubbleTextVInset = 10.f;
 
 @implementation OWSBubbleView
 
+- (instancetype)init
+{
+    self = [super init];
+    if (!self) {
+        return self;
+    }
+
+    self.shapeLayer = [CAShapeLayer new];
+    [self.layer addSublayer:self.shapeLayer];
+
+    self.maskLayer = [CAShapeLayer new];
+    self.layer.mask = self.maskLayer;
+
+    return self;
+}
+
 - (void)setIsOutgoing:(BOOL)isOutgoing
 {
     BOOL didChange = _isOutgoing != isOutgoing;
@@ -64,29 +79,35 @@ const CGFloat kBubbleTextVInset = 10.f;
 
 - (void)setFrame:(CGRect)frame
 {
-    BOOL didChange = !CGSizeEqualToSize(self.frame.size, frame.size);
+    // We only need to update our layers if the _size_ of this view
+    // changes since the contents of the layers are in local coordinates.
+    BOOL didChangeSize = !CGSizeEqualToSize(self.frame.size, frame.size);
 
     [super setFrame:frame];
 
-    if (didChange || !self.shapeLayer) {
+    if (didChangeSize || !self.shapeLayer) {
         [self updateLayers];
     }
 
-    // We need to inform the "bubble stroke view" (if any) any time our frame changes.
+    // We always need to inform the "bubble stroke view" (if any) if our
+    // frame/bounds/center changes. Its contents are not in local coordinates.
     [self.bubbleStrokeView updateLayers];
 }
 
 - (void)setBounds:(CGRect)bounds
 {
-    BOOL didChange = !CGSizeEqualToSize(self.bounds.size, bounds.size);
+    // We only need to update our layers if the _size_ of this view
+    // changes since the contents of the layers are in local coordinates.
+    BOOL didChangeSize = !CGSizeEqualToSize(self.bounds.size, bounds.size);
 
     [super setBounds:bounds];
 
-    if (didChange || !self.shapeLayer) {
+    if (didChangeSize || !self.shapeLayer) {
         [self updateLayers];
     }
 
-    // We need to inform the "bubble stroke view" (if any) any time our frame changes.
+    // We always need to inform the "bubble stroke view" (if any) if our
+    // frame/bounds/center changes. Its contents are not in local coordinates.
     [self.bubbleStrokeView updateLayers];
 }
 
@@ -94,7 +115,8 @@ const CGFloat kBubbleTextVInset = 10.f;
 {
     [super setCenter:center];
 
-    // We need to inform the "bubble stroke view" (if any) any time our frame changes.
+    // We always need to inform the "bubble stroke view" (if any) if our
+    // frame/bounds/center changes. Its contents are not in local coordinates.
     [self.bubbleStrokeView updateLayers];
 }
 
@@ -110,20 +132,13 @@ const CGFloat kBubbleTextVInset = 10.f;
 
 - (void)updateLayers
 {
-    if (!self.shapeLayer) {
-        self.shapeLayer = [CAShapeLayer new];
-        [self.layer addSublayer:self.shapeLayer];
-    }
-    if (!self.maskLayer) {
-        self.maskLayer = [CAShapeLayer new];
-        self.layer.mask = self.maskLayer;
-    }
+    OWSAssert(self.maskLayer);
+    OWSAssert(self.shapeLayer);
 
     UIBezierPath *bezierPath = [self maskPath];
 
     self.shapeLayer.fillColor = self.bubbleColor.CGColor;
     self.shapeLayer.path = bezierPath.CGPath;
-
     self.maskLayer.path = bezierPath.CGPath;
 }
 

Signal/src/ViewControllers/ConversationView/Cells/OWSMessageCell.m

@@ -42,7 +42,7 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
 @property (nonatomic) OWSBubbleView *bubbleView;
 
 @property (nonatomic) UILabel *dateHeaderLabel;
-@property (nonatomic) OWSMessageTextView *bodyTextViewCached;
+@property (nonatomic) OWSMessageTextView *bodyTextView;
 @property (nonatomic, nullable) UIImageView *failedSendBadgeView;
 @property (nonatomic) UIView *footerView;
 @property (nonatomic) UILabel *footerLabel;
@@ -75,7 +75,7 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
 
 - (void)commontInit
 {
-    OWSAssert(!self.bodyTextViewCached);
+    OWSAssert(!self.bodyTextView);
 
     _viewConstraints = [NSMutableArray new];
 
@@ -95,7 +95,7 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
     self.dateHeaderLabel.textColor = [UIColor lightGrayColor];
     [self.contentView addSubview:self.dateHeaderLabel];
 
-    self.bodyTextViewCached = [self newTextView];
+    self.bodyTextView = [self newTextView];
 
     self.footerLabel = [UILabel new];
     self.footerLabel.font = [UIFont ows_regularFontWithSize:12.f];
@@ -103,7 +103,7 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
     [self.footerView addSubview:self.footerLabel];
 
     // Hide these views by default.
-    self.bodyTextViewCached.hidden = YES;
+    self.bodyTextView.hidden = YES;
     self.dateHeaderLabel.hidden = YES;
     self.footerLabel.hidden = YES;
 
@@ -140,6 +140,8 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
     textView.contentInset = UIEdgeInsetsZero;
     textView.textContainer.lineFragmentPadding = 0;
     textView.scrollEnabled = NO;
+    // Setting dataDetectorTypes is expensive.  Do it just once.
+    textView.dataDetectorTypes = (UIDataDetectorTypeLink | UIDataDetectorTypeAddress | UIDataDetectorTypeCalendarEvent);
     return textView;
 }
 
@@ -401,6 +403,9 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
             [bodyMediaView autoPinLeadingToSuperviewWithMargin:0],
             [bodyMediaView autoPinTrailingToSuperviewWithMargin:0],
         ]];
+        // We need constraints to control the vertical sizing of media and text views, but we use
+        // lower priority so that when a message only contains media it uses the exact bounds of
+        // the message view.
         [NSLayoutConstraint
             autoSetPriority:UILayoutPriorityDefaultLow
              forConstraints:^{
@@ -446,6 +451,9 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
             [bodyTextView autoPinLeadingToSuperviewWithMargin:self.textLeadingMargin],
             [bodyTextView autoPinTrailingToSuperviewWithMargin:self.textTrailingMargin],
         ]];
+        // We need constraints to control the vertical sizing of media and text views, but we use
+        // lower priority so that when a message only contains media it uses the exact bounds of
+        // the message view.
         [NSLayoutConstraint
             autoSetPriority:UILayoutPriorityDefaultLow
              forConstraints:^{
@@ -523,7 +531,7 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
         DDLogError(@"%@ Failed to load cell media: %@", [self logTag], [self.attachmentStream mediaURL]);
         self.viewItem.didCellMediaFailToLoad = YES;
         // TODO: Do we need to hide/remove the media view?
-        [self showAttachmentErrorView:mediaView];
+        [self showAttachmentErrorViewWithMediaView:mediaView];
     }
     return cellMedia;
 }
@@ -721,12 +729,12 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
         TSOutgoingMessage *outgoingMessage = (TSOutgoingMessage *)self.viewItem.interaction;
         shouldIgnoreEvents = outgoingMessage.messageState != TSOutgoingMessageStateSentToService;
     }
-    [self.class loadForTextDisplay:self.bodyTextViewCached
+    [self.class loadForTextDisplay:self.bodyTextView
                               text:self.displayableBodyText.displayText
                          textColor:self.textColor
                               font:self.textMessageFont
                 shouldIgnoreEvents:shouldIgnoreEvents];
-    return self.bodyTextViewCached;
+    return self.bodyTextView;
 }
 
 + (void)loadForTextDisplay:(OWSMessageTextView *)textView
@@ -745,7 +753,6 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
         NSForegroundColorAttributeName : textColor,
         NSUnderlineStyleAttributeName : @(NSUnderlineStyleSingle | NSUnderlinePatternSolid)
     };
-    textView.dataDetectorTypes = (UIDataDetectorTypeLink | UIDataDetectorTypeAddress | UIDataDetectorTypeCalendarEvent);
     textView.shouldIgnoreEvents = shouldIgnoreEvents;
 }
 
@@ -797,10 +804,13 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
         if (!strongSelf) {
             return;
         }
+        OWSCAssert(strongSelf.lastBodyMediaView == stillImageView);
         if (stillImageView.image) {
             return;
         }
         // Don't cache large still images.
+        //
+        // TODO: Don't use full size images in the message cells.
         const NSUInteger kMaxCachableSize = 1024 * 1024;
         BOOL shouldSkipCache =
             [OWSFileSystem fileSizeOfPath:strongSelf.attachmentStream.filePath].unsignedIntegerValue < kMaxCachableSize;
@@ -813,6 +823,11 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
                                               shouldSkipCache:shouldSkipCache];
     };
     self.unloadCellContentBlock = ^{
+        OWSMessageCell *strongSelf = weakSelf;
+        if (!strongSelf) {
+            return;
+        }
+        OWSCAssert(strongSelf.lastBodyMediaView == stillImageView);
         stillImageView.image = nil;
     };
 
@@ -836,6 +851,7 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
         if (!strongSelf) {
             return;
         }
+        OWSCAssert(strongSelf.lastBodyMediaView == animatedImageView);
         if (animatedImageView.image) {
             return;
         }
@@ -854,6 +870,11 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
                                                  shouldSkipCache:NO];
     };
     self.unloadCellContentBlock = ^{
+        OWSMessageCell *strongSelf = weakSelf;
+        if (!strongSelf) {
+            return;
+        }
+        OWSCAssert(strongSelf.lastBodyMediaView == animatedImageView);
         animatedImageView.image = nil;
     };
 
@@ -911,6 +932,7 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
         if (!strongSelf) {
             return;
         }
+        OWSCAssert(strongSelf.lastBodyMediaView == stillImageView);
         if (stillImageView.image) {
             return;
         }
@@ -924,6 +946,11 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
                                               shouldSkipCache:NO];
     };
     self.unloadCellContentBlock = ^{
+        OWSMessageCell *strongSelf = weakSelf;
+        if (!strongSelf) {
+            return;
+        }
+        OWSCAssert(strongSelf.lastBodyMediaView == stillImageView);
         stillImageView.image = nil;
     };
 
@@ -999,26 +1026,25 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
 
     if (self.isOutgoing) {
         if (!self.attachmentStream.isUploaded) {
-            __unused AttachmentUploadView *attachmentUploadView =
-                //            self.attachmentUploadView =
-                // This view will be added to attachmentView which will retain a strong reference to it.
+            AttachmentUploadView *attachmentUploadView =
                 [[AttachmentUploadView alloc] initWithAttachment:self.attachmentStream
-                                                       superview:attachmentView
                                          attachmentStateCallback:attachmentStateCallback];
+            [attachmentView addSubview:attachmentUploadView];
+            [attachmentUploadView autoPinToSuperviewEdges];
         }
     }
 }
 
-- (void)showAttachmentErrorView:(UIView *)mediaView
+- (void)showAttachmentErrorViewWithMediaView:(UIView *)mediaView
 {
     OWSAssert(mediaView);
 
     // TODO: We could do a better job of indicating that the media could not be loaded.
-    UIView *customView = [UIView new];
-    customView.backgroundColor = [UIColor colorWithWhite:0.85f alpha:1.f];
-    customView.userInteractionEnabled = NO;
-    [mediaView addSubview:customView];
-    [customView autoPinEdgesToSuperviewEdges];
+    UIView *errorView = [UIView new];
+    errorView.backgroundColor = [UIColor colorWithWhite:0.85f alpha:1.f];
+    errorView.userInteractionEnabled = NO;
+    [mediaView addSubview:errorView];
+    [errorView autoPinEdgesToSuperviewEdges];
 }
 
 #pragma mark - Measurement
@@ -1037,10 +1063,10 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
     const int maxMessageWidth = [self maxMessageWidthForContentWidth:contentWidth];
     const int maxTextWidth = (int)floor(maxMessageWidth - (leftMargin + rightMargin));
 
-    self.bodyTextViewCached.text = self.displayableBodyText.displayText;
+    self.bodyTextView.text = self.displayableBodyText.displayText;
     // Honor dynamic type in the message bodies.
-    self.bodyTextViewCached.font = [self textMessageFont];
-    CGSize textSize = CGSizeCeil([self.bodyTextViewCached sizeThatFits:CGSizeMake(maxTextWidth, CGFLOAT_MAX)]);
+    self.bodyTextView.font = [self textMessageFont];
+    CGSize textSize = CGSizeCeil([self.bodyTextView sizeThatFits:CGSizeMake(maxTextWidth, CGFLOAT_MAX)]);
     CGSize textViewSize = textSize;
 
     if (includeMargins) {
@@ -1213,10 +1239,9 @@ CG_INLINE CGSize CGSizeCeil(CGSize size)
 
     self.dateHeaderLabel.text = nil;
     self.dateHeaderLabel.hidden = YES;
-    [self.bodyTextViewCached removeFromSuperview];
-    self.bodyTextViewCached.text = nil;
-    self.bodyTextViewCached.hidden = YES;
-    self.bodyTextViewCached.dataDetectorTypes = UIDataDetectorTypeNone;
+    [self.bodyTextView removeFromSuperview];
+    self.bodyTextView.text = nil;
+    self.bodyTextView.hidden = YES;
     [self.failedSendBadgeView removeFromSuperview];
     self.failedSendBadgeView = nil;
     self.footerLabel.text = nil;

SignalMessaging/utils/OWSMessagesBubbleImageFactory.swift

@@ -20,22 +20,22 @@ public class OWSMessagesBubbleImageFactory: NSObject {
     }()
 
     public lazy var incoming: JSQMessagesBubbleImage = {
-        let color = UIColor.jsq_messageBubbleLightGray()!
+        let color = bubbleColorIncoming
         return self.incoming(color: color)
     }()
 
     public lazy var outgoing: JSQMessagesBubbleImage = {
-        let color = UIColor.ows_materialBlue
+        let color = bubbleColorOutgoingSent
         return self.outgoing(color: color)
     }()
 
     public lazy var currentlyOutgoing: JSQMessagesBubbleImage = {
-        let color = UIColor.ows_fadedBlue
+        let color = bubbleColorOutgoingSending
         return self.outgoing(color: color)
     }()
 
     public lazy var outgoingFailed: JSQMessagesBubbleImage = {
-        let color = UIColor.gray
+        let color = bubbleColorOutgoingUnsent
         return self.outgoing(color: color)
     }()