From 60fdb8280e108fbf3a24a38041a0bfd004179695 Mon Sep 17 00:00:00 2001 From: Michelle Linington Date: Thu, 19 Aug 2021 14:38:07 -0700 Subject: [PATCH 1/7] Better handling of undownloaded media in quoted replies --- .../ConversationViewController+Scroll.swift | 7 +- .../ViewModels/OWSQuotedReplyModel.m | 46 +- .../Attachments/OWSAttachmentDownloads.swift | 13 +- .../src/Messages/Interactions/OWSContact.h | 1 - .../src/Messages/Interactions/TSMessage.h | 3 +- .../src/Messages/Interactions/TSMessage.m | 16 +- .../Messages/Interactions/TSOutgoingMessage.m | 40 +- .../Messages/Interactions/TSQuotedMessage.h | 71 +-- .../Messages/Interactions/TSQuotedMessage.m | 463 ++++++++++-------- SignalServiceKit/src/Messages/MessageSender.m | 15 +- 10 files changed, 316 insertions(+), 359 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController+Scroll.swift b/Signal/src/ViewControllers/ConversationView/ConversationViewController+Scroll.swift index 096a7e7cc2..a5bbeae525 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController+Scroll.swift +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController+Scroll.swift @@ -281,6 +281,7 @@ extension ConversationViewController { func scrollToQuotedMessage(_ quotedReply: OWSQuotedReplyModel, isAnimated: Bool) { if quotedReply.isRemotelySourced { + presentRemotelySourcedQuotedReplyToast() return } let quotedMessage = databaseStorage.read { transaction in @@ -289,10 +290,8 @@ extension ConversationViewController { author: quotedReply.authorAddress, transaction: transaction) } - guard let message = quotedMessage else { - return - } - guard !message.wasRemotelyDeleted else { + guard let message = quotedMessage, !message.wasRemotelyDeleted else { + presentMissingQuotedReplyToast() return } ensureInteractionLoadedThenScrollToInteraction(message.uniqueId, diff --git a/SignalMessaging/ViewModels/OWSQuotedReplyModel.m b/SignalMessaging/ViewModels/OWSQuotedReplyModel.m index d237bc83f0..9b3cf9a2cc 100644 --- a/SignalMessaging/ViewModels/OWSQuotedReplyModel.m +++ b/SignalMessaging/ViewModels/OWSQuotedReplyModel.m @@ -77,31 +77,21 @@ NS_ASSUME_NONNULL_BEGIN + (instancetype)quotedReplyWithQuotedMessage:(TSQuotedMessage *)quotedMessage transaction:(SDSAnyReadTransaction *)transaction { - OWSAssertDebug(quotedMessage.quotedAttachments.count <= 1); - OWSAttachmentInfo *attachmentInfo = quotedMessage.quotedAttachments.firstObject; - - BOOL thumbnailDownloadFailed = NO; UIImage *_Nullable thumbnailImage; - TSAttachmentPointer *attachmentPointer; - if (attachmentInfo.thumbnailAttachmentStreamId) { - TSAttachment *attachment = - [TSAttachment anyFetchWithUniqueId:attachmentInfo.thumbnailAttachmentStreamId transaction:transaction]; - - TSAttachmentStream *attachmentStream; - if ([attachment isKindOfClass:[TSAttachmentStream class]]) { - attachmentStream = (TSAttachmentStream *)attachment; - thumbnailImage = attachmentStream.thumbnailImageSmallSync; - } - } else if (attachmentInfo.thumbnailAttachmentPointerId) { - // download failed, or hasn't completed yet. - TSAttachment *attachment = - [TSAttachment anyFetchWithUniqueId:attachmentInfo.thumbnailAttachmentPointerId transaction:transaction]; + TSAttachment *_Nullable attachment = [quotedMessage fetchThumbnailWithTransaction:transaction]; + TSAttachmentPointer *_Nullable failedAttachmentPointer = nil; + if ([attachment isKindOfClass:[TSAttachmentStream class]]) { + thumbnailImage = [(TSAttachmentStream *)attachment thumbnailImageSmallSync]; + } else if (!quotedMessage.isThumbnailOwned) { + // If the quoted message isn't owning the thumbnail attachment, it's going to be referencing + // some other attachment (e.g. undownloaded media). In this case, let's just use the blur hash + thumbnailImage = attachment.blurHash ? [BlurHash imageForBlurHash:attachment.blurHash] : nil; + } else { + // If the quoted message has ownership of the thumbnail, but it hasn't been downloaded yet, + // we shoud surface this in the view. if ([attachment isKindOfClass:[TSAttachmentPointer class]]) { - attachmentPointer = (TSAttachmentPointer *)attachment; - if (attachmentPointer.state == TSAttachmentPointerStateFailed) { - thumbnailDownloadFailed = YES; - } + failedAttachmentPointer = (TSAttachmentPointer *)attachment; } } @@ -111,11 +101,11 @@ NS_ASSUME_NONNULL_BEGIN bodyRanges:quotedMessage.bodyRanges bodySource:quotedMessage.bodySource thumbnailImage:thumbnailImage - contentType:attachmentInfo.contentType - sourceFilename:attachmentInfo.sourceFilename + contentType:quotedMessage.contentType + sourceFilename:quotedMessage.sourceFilename attachmentStream:nil - thumbnailAttachmentPointer:attachmentPointer - thumbnailDownloadFailed:thumbnailDownloadFailed]; + thumbnailAttachmentPointer:failedAttachmentPointer + thumbnailDownloadFailed:(failedAttachmentPointer != nil)]; } + (nullable instancetype)quotedReplyForSendingWithItem:(id)item @@ -361,14 +351,12 @@ NS_ASSUME_NONNULL_BEGIN - (TSQuotedMessage *)buildQuotedMessageForSending { - NSArray *attachments = self.attachmentStream ? @[ self.attachmentStream ] : @[]; - // Legit usage of senderTimestamp to reference existing message return [[TSQuotedMessage alloc] initWithTimestamp:self.timestamp authorAddress:self.authorAddress body:self.body bodyRanges:self.bodyRanges - quotedAttachmentsForSending:attachments]; + quotedAttachmentForSending:self.attachmentStream]; } - (BOOL)isRemotelySourced diff --git a/SignalServiceKit/src/Messages/Attachments/OWSAttachmentDownloads.swift b/SignalServiceKit/src/Messages/Attachments/OWSAttachmentDownloads.swift index 1c43ca74ce..8b50e3463c 100644 --- a/SignalServiceKit/src/Messages/Attachments/OWSAttachmentDownloads.swift +++ b/SignalServiceKit/src/Messages/Attachments/OWSAttachmentDownloads.swift @@ -1040,13 +1040,8 @@ public extension OWSAttachmentDownloads { return jobRequests } - if let quotedMessage = message.quotedMessage { - for attachmentId in quotedMessage.thumbnailAttachmentStreamIds() { - addJobRequest(attachmentId: attachmentId, category: .quotedReplyThumbnail) - } - if let attachmentId = quotedMessage.thumbnailAttachmentPointerId() { - addJobRequest(attachmentId: attachmentId, category: .quotedReplyThumbnail) - } + if let quotedMessage = message.quotedMessage, quotedMessage.isThumbnailOwned { + addJobRequest(attachmentId: quotedMessage.thumbnailAttachmentId, category: .quotedReplyThumbnail) } if let attachmentId = message.contactShare?.avatarAttachmentId { @@ -1091,14 +1086,14 @@ public extension OWSAttachmentDownloads { Logger.warn("Missing message.") return } - guard let thumbnailAttachmentPointerId = message.quotedMessage?.thumbnailAttachmentPointerId(), + guard let thumbnailAttachmentPointerId = message.quotedMessage?.thumbnailAttachmentId, !thumbnailAttachmentPointerId.isEmpty else { return } guard let quotedMessageThumbnail = (attachmentStreams.filter { $0.uniqueId == thumbnailAttachmentPointerId }.first) else { return } - message.setQuotedMessageThumbnailAttachmentStream(quotedMessageThumbnail) + message.setQuotedMessageThumbnailAttachmentStream(quotedMessageThumbnail, transaction: transaction) } } diff --git a/SignalServiceKit/src/Messages/Interactions/OWSContact.h b/SignalServiceKit/src/Messages/Interactions/OWSContact.h index 1b3dac9e93..ece5b10f57 100644 --- a/SignalServiceKit/src/Messages/Interactions/OWSContact.h +++ b/SignalServiceKit/src/Messages/Interactions/OWSContact.h @@ -8,7 +8,6 @@ NS_ASSUME_NONNULL_BEGIN @class CNContact; -@class OWSAttachmentInfo; @class SDSAnyReadTransaction; @class SDSAnyWriteTransaction; @class SSKProtoDataMessage; diff --git a/SignalServiceKit/src/Messages/Interactions/TSMessage.h b/SignalServiceKit/src/Messages/Interactions/TSMessage.h index 5a53c24977..a277eb7f9d 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSMessage.h +++ b/SignalServiceKit/src/Messages/Interactions/TSMessage.h @@ -112,7 +112,8 @@ NS_DESIGNATED_INITIALIZER NS_SWIFT_NAME(init(grdbId:uniqueId:receivedAtTimestamp // quoted reply thumbnails, contact share avatars, link preview images, etc. - (NSArray *)allAttachmentIds; -- (void)setQuotedMessageThumbnailAttachmentStream:(TSAttachmentStream *)attachmentStream; +- (void)setQuotedMessageThumbnailAttachmentStream:(TSAttachmentStream *)attachmentStream + transaction:(SDSAnyWriteTransaction *)transaction; // The raw body contains placeholders for things like mentions and is not // user friendly. If you want a constant string representing the body of diff --git a/SignalServiceKit/src/Messages/Interactions/TSMessage.m b/SignalServiceKit/src/Messages/Interactions/TSMessage.m index b403d347aa..88fee13f0f 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSMessage.m @@ -312,12 +312,8 @@ static const NSUInteger OWSMessageSchemaVersion = 4; [result addObjectsFromArray:self.attachmentIds]; } - if (self.quotedMessage) { - [result addObjectsFromArray:self.quotedMessage.thumbnailAttachmentStreamIds]; - - if (self.quotedMessage.thumbnailAttachmentPointerId != nil) { - [result addObject:self.quotedMessage.thumbnailAttachmentPointerId]; - } + if (self.quotedMessage.thumbnailAttachmentId) { + [result addObject:self.quotedMessage.thumbnailAttachmentId]; } if (self.contactShare.avatarAttachmentId) { @@ -700,13 +696,13 @@ static const NSUInteger OWSMessageSchemaVersion = 4; return _body.filterStringForDisplay; } -- (void)setQuotedMessageThumbnailAttachmentStream:(TSAttachmentStream *)attachmentStream +- (void)setQuotedMessageThumbnailAttachmentStream:(TSAttachmentStream *)attachmentStream transaction:(SDSAnyWriteTransaction *)transaction { OWSAssertDebug([attachmentStream isKindOfClass:[TSAttachmentStream class]]); OWSAssertDebug(self.quotedMessage); - OWSAssertDebug(self.quotedMessage.quotedAttachments.count == 1); - - [self.quotedMessage setThumbnailAttachmentStream:attachmentStream]; + [self anyUpdateMessageWithTransaction:transaction block:^(TSMessage *message) { + [message.quotedMessage setThumbnailAttachmentStream:attachmentStream]; + }]; } #pragma mark - Update With... Methods diff --git a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m index 24a3e041e0..869e02e05e 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m @@ -1405,31 +1405,25 @@ NSUInteger const TSOutgoingMessageSchemaVersion = 1; } } - if (quotedMessage.quotedAttachments) { - for (OWSAttachmentInfo *attachment in quotedMessage.quotedAttachments) { - hasQuotedAttachment = YES; + if (quotedMessage.isThumbnailOwned && quotedMessage.thumbnailAttachmentId) { + NSString *attachmentId = quotedMessage.thumbnailAttachmentId; + SSKProtoDataMessageQuoteQuotedAttachmentBuilder *quotedAttachmentBuilder = [SSKProtoDataMessageQuoteQuotedAttachment builder]; + quotedAttachmentBuilder.contentType = quotedMessage.contentType; + quotedAttachmentBuilder.fileName = quotedMessage.sourceFilename; + quotedAttachmentBuilder.thumbnail = [TSAttachmentStream buildProtoForAttachmentId:attachmentId + transaction:transaction]; - SSKProtoDataMessageQuoteQuotedAttachmentBuilder *quotedAttachmentBuilder = - [SSKProtoDataMessageQuoteQuotedAttachment builder]; - - quotedAttachmentBuilder.contentType = attachment.contentType; - quotedAttachmentBuilder.fileName = attachment.sourceFilename; - if (attachment.thumbnailAttachmentStreamId) { - quotedAttachmentBuilder.thumbnail = - [TSAttachmentStream buildProtoForAttachmentId:attachment.thumbnailAttachmentStreamId - transaction:transaction]; - } - - NSError *error; - SSKProtoDataMessageQuoteQuotedAttachment *_Nullable quotedAttachmentMessage = - [quotedAttachmentBuilder buildAndReturnError:&error]; - if (error || !quotedAttachmentMessage) { - OWSFailDebug(@"could not build protobuf: %@", error); - return nil; - } - - [quoteBuilder addAttachments:quotedAttachmentMessage]; + NSError *error; + SSKProtoDataMessageQuoteQuotedAttachment *_Nullable quotedAttachmentMessage = [quotedAttachmentBuilder buildAndReturnError:&error]; + if (error || !quotedAttachmentMessage) { + OWSFailDebug(@"could not build protobuf: %@", error); + return nil; } + + [quoteBuilder addAttachments:quotedAttachmentMessage]; + hasQuotedAttachment = YES; + } else if (quotedMessage.thumbnailAttachmentId) { + OWSFailDebug(@"Referencing an attachment that isn't owned by the quoted message. Was this never thumbnailed?"); } if (hasQuotedText || hasQuotedAttachment) { diff --git a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h index 2622e25b74..aa7f661cc8 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h +++ b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h @@ -15,42 +15,6 @@ NS_ASSUME_NONNULL_BEGIN @class TSQuotedMessage; @class TSThread; -@interface OWSAttachmentInfo : MTLModel - -@property (nonatomic, readonly, nullable) NSString *contentType; -@property (nonatomic, readonly, nullable) NSString *sourceFilename; - -// This is only set when sending a new attachment so we have a way -// to reference the original attachment when generating a thumbnail. -// We don't want to do this until the message is saved, when the user sends -// the message so as not to end up with an orphaned file. -@property (nonatomic, readonly, nullable) NSString *attachmentId; - -// References a yet-to-be downloaded thumbnail file -@property (atomic, nullable) NSString *thumbnailAttachmentPointerId; - -// References an already downloaded or locally generated thumbnail file -@property (atomic, nullable) NSString *thumbnailAttachmentStreamId; - -+ (instancetype)new NS_UNAVAILABLE; -- (instancetype)init NS_UNAVAILABLE; - -- (instancetype)initWithAttachmentId:(nullable NSString *)attachmentId - contentType:(NSString *)contentType - sourceFilename:(NSString *)sourceFilename NS_DESIGNATED_INITIALIZER; - -- (instancetype)initWithAttachmentStream:(TSAttachmentStream *)attachmentStream; - -- (instancetype)initWithAttachmentId:(nullable NSString *)attachmentId - contentType:(NSString *)contentType - sourceFilename:(NSString *)sourceFilename - thumbnailAttachmentPointerId:(nullable NSString *)thumbnailAttachmentPointerId - thumbnailAttachmentStreamId:(nullable NSString *)thumbnailAttachmentStreamId NS_DESIGNATED_INITIALIZER; - -@end - -#pragma mark - - typedef NS_ENUM(NSUInteger, TSQuotedMessageContentSource) { TSQuotedMessageContentSourceUnknown, TSQuotedMessageContentSourceLocal, @@ -70,47 +34,30 @@ typedef NS_ENUM(NSUInteger, TSQuotedMessageContentSource) { #pragma mark - Attachments -// This is a MIME type. -// -// This property should be set IFF we are quoting an attachment message. -- (nullable NSString *)contentType; -- (nullable NSString *)sourceFilename; - -// References a yet-to-be downloaded thumbnail file -- (nullable NSString *)thumbnailAttachmentPointerId; +/// Is the quoted thumbnail currently owned by the quoted message model? Or is it referencing an existing attachment +@property (nonatomic, readonly) NSString *thumbnailAttachmentId; +@property (nonatomic, readonly) BOOL isThumbnailOwned; +@property (nonatomic, readonly) NSString *contentType; +@property (nonatomic, readonly) NSString *sourceFilename; +- (nullable TSAttachment *)fetchThumbnailWithTransaction:(SDSAnyReadTransaction *)transaction; // References an already downloaded or locally generated thumbnail file -- (nullable NSString *)thumbnailAttachmentStreamId; - (void)setThumbnailAttachmentStream:(TSAttachment *)thumbnailAttachmentStream; -// currently only used by orphan attachment cleaner -- (NSArray *)thumbnailAttachmentStreamIds; - -@property (atomic, readonly) NSArray *quotedAttachments; - // Before sending, persist a thumbnail attachment derived from the quoted attachment -- (NSArray *)createThumbnailAttachmentsIfNecessaryWithTransaction: - (SDSAnyWriteTransaction *)transaction; +- (nullable TSAttachmentStream *)createThumbnailIfNecessaryWithTransaction:(SDSAnyWriteTransaction *)transaction; + (instancetype)new NS_UNAVAILABLE; - (instancetype)init NS_UNAVAILABLE; -// used when receiving quoted messages -- (instancetype)initWithTimestamp:(uint64_t)timestamp - authorAddress:(SignalServiceAddress *)authorAddress - body:(nullable NSString *)body - bodyRanges:(nullable MessageBodyRanges *)bodyRanges - bodySource:(TSQuotedMessageContentSource)bodySource - receivedQuotedAttachmentInfos:(NSArray *)attachmentInfos; - // used when sending quoted messages - (instancetype)initWithTimestamp:(uint64_t)timestamp authorAddress:(SignalServiceAddress *)authorAddress body:(nullable NSString *)body bodyRanges:(nullable MessageBodyRanges *)bodyRanges - quotedAttachmentsForSending:(NSArray *)attachments; - + quotedAttachmentForSending:(nullable TSAttachment *)attachment; +// used when receiving quoted messages + (nullable instancetype)quotedMessageForDataMessage:(SSKProtoDataMessage *)dataMessage thread:(TSThread *)thread transaction:(SDSAnyWriteTransaction *)transaction; diff --git a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m index 23d320d13c..ced6c539ac 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m @@ -15,62 +15,109 @@ NS_ASSUME_NONNULL_BEGIN +/// Indicates the sort of attachment ID included in the attachment info +typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { + OWSAttachmentInfoReferenceUnset = 0, + + /// An attachment ID referencing original media + /// This should only be used prior to sending a new quoted message. After sending, a copy of the source media should be thumbnailed + /// and saved in its own attachment. + /// An attachment ID referencing original media + /// Indicates a valid handle to the original media, this may be a pointer to a pending download, or a stream. + /// If a pointer, we should show the blurhash. If it's a stream, we should thumbnail the original and save in a separate attachment. + /// An attachment ID referencing the quoted thumbnail + /// This thumbnail may have been generated locally, or it may have been fetched remotely from the sender of the quoted message. + /// An attachment ID referencing + + OWSAttachmentInfoReferenceOriginalForSend = 1, + OWSAttachmentInfoReferenceOriginal, + OWSAttachmentInfoReferenceThumbnail, + OWSAttachmentInfoReferenceUntrustedPointer, +}; + +@interface OWSAttachmentInfo : MTLModel +@property (class, nonatomic, readonly) NSUInteger currentSchemaVersion; +@property (nonatomic, readonly) NSUInteger schemaVersion; + +@property (nonatomic, readonly, nullable) NSString *contentType; +@property (nonatomic, readonly, nullable) NSString *sourceFilename; +@property (nonatomic) OWSAttachmentInfoReference attachmentType; +@property (nonatomic) NSString *rawAttachmentId; + ++ (instancetype)new NS_UNAVAILABLE; +- (instancetype)init NS_UNAVAILABLE; +@end + @implementation OWSAttachmentInfo -- (instancetype)initWithAttachmentStream:(TSAttachmentStream *)attachmentStream +- (instancetype)initWithOriginalAttachmentStream:(TSAttachmentStream *)attachmentStream { OWSAssertDebug([attachmentStream isKindOfClass:[TSAttachmentStream class]]); OWSAssertDebug(attachmentStream.uniqueId); OWSAssertDebug(attachmentStream.contentType); return [self initWithAttachmentId:attachmentStream.uniqueId + ofType:OWSAttachmentInfoReferenceOriginalForSend contentType:attachmentStream.contentType sourceFilename:attachmentStream.sourceFilename]; } -- (instancetype)initWithAttachmentId:(nullable NSString *)attachmentId +- (instancetype)initWithAttachmentId:(NSString *)attachmentId + ofType:(OWSAttachmentInfoReference)attachmentType contentType:(NSString *)contentType sourceFilename:(NSString *)sourceFilename { self = [super init]; - if (!self) { - return self; + if (self) { + _schemaVersion = self.class.currentSchemaVersion; + _rawAttachmentId = attachmentId; + _attachmentType = attachmentType; + _contentType = contentType; + _sourceFilename = sourceFilename; } - - _attachmentId = attachmentId; - _contentType = contentType; - _sourceFilename = sourceFilename; - return self; } -- (instancetype)initWithAttachmentId:(nullable NSString *)attachmentId - contentType:(NSString *)contentType - sourceFilename:(NSString *)sourceFilename - thumbnailAttachmentPointerId:(nullable NSString *)thumbnailAttachmentPointerId - thumbnailAttachmentStreamId:(nullable NSString *)thumbnailAttachmentStreamId ++ (NSUInteger)currentSchemaVersion { + return 1; +} + +- (nullable instancetype)initWithCoder:(NSCoder *)coder { - self = [super init]; + self = [super initWithCoder:coder]; if (!self) { return self; } - _attachmentId = attachmentId; - _contentType = contentType; - _sourceFilename = sourceFilename; - _thumbnailAttachmentPointerId = thumbnailAttachmentPointerId; - _thumbnailAttachmentStreamId = thumbnailAttachmentStreamId; + if (_schemaVersion == 0) { + NSString *_Nullable oldStreamId = [coder decodeObjectOfClass:[NSString class] forKey:@"thumbnailAttachmentStreamId"]; + NSString *_Nullable oldPointerId = [coder decodeObjectOfClass:[NSString class] forKey:@"thumbnailAttachmentPointerId"]; + NSString *_Nullable oldSourceAttachmentId = [coder decodeObjectOfClass:[NSString class] forKey:@"attachmentId"]; + // Before, we maintained each of these IDs in parallel, though in practice only one in use at a time. + // Migration codifies this behavior. + if (oldStreamId && [oldPointerId isEqualToString:oldStreamId]) { + _attachmentType = OWSAttachmentInfoReferenceThumbnail; + _rawAttachmentId = oldStreamId; + } else if (oldPointerId) { + _attachmentType = OWSAttachmentInfoReferenceUntrustedPointer; + _rawAttachmentId = oldPointerId; + } else if (oldStreamId) { + _attachmentType = OWSAttachmentInfoReferenceThumbnail; + _rawAttachmentId = oldStreamId; + } else if (oldSourceAttachmentId) { + _attachmentType = OWSAttachmentInfoReferenceOriginalForSend; + _rawAttachmentId = oldSourceAttachmentId; + } + } + _schemaVersion = self.class.currentSchemaVersion; return self; } @end @interface TSQuotedMessage () - -@property (atomic) NSArray *quotedAttachments; -@property (atomic) NSArray *quotedAttachmentsForSending; - +@property (nonatomic, nullable) OWSAttachmentInfo *quotedAttachment; @end @implementation TSQuotedMessage @@ -80,7 +127,7 @@ NS_ASSUME_NONNULL_BEGIN body:(nullable NSString *)body bodyRanges:(nullable MessageBodyRanges *)bodyRanges bodySource:(TSQuotedMessageContentSource)bodySource - receivedQuotedAttachmentInfos:(NSArray *)attachmentInfos + receivedQuotedAttachmentInfo:(nullable OWSAttachmentInfo *)attachmentInfo { OWSAssertDebug(timestamp > 0); OWSAssertDebug(authorAddress.isValid); @@ -95,7 +142,7 @@ NS_ASSUME_NONNULL_BEGIN _body = body; _bodyRanges = bodyRanges; _bodySource = bodySource; - _quotedAttachments = attachmentInfos; + _quotedAttachment = attachmentInfo; return self; } @@ -104,7 +151,7 @@ NS_ASSUME_NONNULL_BEGIN authorAddress:(SignalServiceAddress *)authorAddress body:(nullable NSString *)body bodyRanges:(nullable MessageBodyRanges *)bodyRanges - quotedAttachmentsForSending:(NSArray *)attachments + quotedAttachmentForSending:(nullable TSAttachmentStream *)attachment { OWSAssertDebug(timestamp > 0); OWSAssertDebug(authorAddress.isValid); @@ -119,12 +166,7 @@ NS_ASSUME_NONNULL_BEGIN _body = body; _bodyRanges = bodyRanges; _bodySource = TSQuotedMessageContentSourceLocal; - - NSMutableArray *attachmentInfos = [NSMutableArray new]; - for (TSAttachmentStream *attachmentStream in attachments) { - [attachmentInfos addObject:[[OWSAttachmentInfo alloc] initWithAttachmentStream:attachmentStream]]; - } - _quotedAttachments = [attachmentInfos copy]; + _quotedAttachment = attachment ? [[OWSAttachmentInfo alloc] initWithOriginalAttachmentStream:attachment] : nil; return self; } @@ -141,6 +183,17 @@ NS_ASSUME_NONNULL_BEGIN OWSAssertDebug(_authorAddress.isValid); } + if (_quotedAttachment == nil) { + NSSet *expectedClasses = [NSSet setWithArray:@[[NSArray class], [OWSAttachmentInfo class]]]; + NSArray *_Nullable attachmentInfos = [coder decodeObjectOfClasses:expectedClasses forKey:@"quotedAttachments"]; + + if ([attachmentInfos.firstObject isKindOfClass:[OWSAttachmentInfo class]]) { + // In practice, we only used the first item of this array + OWSAssertDebug(attachmentInfos.count <= 1); + _quotedAttachment = attachmentInfos.firstObject; + } + } + return self; } @@ -171,241 +224,233 @@ NS_ASSUME_NONNULL_BEGIN return nil; } - SignalServiceAddress *authorAddress = quoteProto.authorAddress; + TSQuotedMessage *_Nullable quotedMessage = nil; + TSMessage *_Nullable originalMessage = [InteractionFinder findMessageWithTimestamp:timestamp + threadId:thread.uniqueId + author:quoteProto.authorAddress + transaction:transaction]; + if (originalMessage) { + // Prefer to generate the quoted content locally if available. + quotedMessage = [self localQuotedMessageFromSourceMessage:originalMessage quoteProto:quoteProto transaction:transaction]; + } + if (!quotedMessage) { + // If we couldn't generate the quoted content from locally available info, we can generate it from the proto. + quotedMessage = [self remoteQuotedMessageFromQuoteProto:quoteProto transaction:transaction]; + } + + OWSAssertDebug(quotedMessage); + return quotedMessage; +} + ++ (nullable TSAttachment *)quotedAttachmentFromOriginalMessage:(TSMessage *)quotedMessage + transaction:(SDSAnyWriteTransaction *)transaction +{ + if (quotedMessage.attachmentIds.count > 0) { + return [quotedMessage bodyAttachmentsWithTransaction:transaction.unwrapGrdbRead].firstObject; + } else if (quotedMessage.linkPreview && quotedMessage.linkPreview.imageAttachmentId.length > 0) { + return [TSAttachment anyFetchWithUniqueId:quotedMessage.linkPreview.imageAttachmentId transaction:transaction]; + } else if (quotedMessage.messageSticker && quotedMessage.messageSticker.attachmentId.length > 0) { + return [TSAttachment anyFetchWithUniqueId:quotedMessage.messageSticker.attachmentId transaction:transaction]; + } else { + return nil; + } +} + +#pragma mark - Private + +/// Builds a quoted message from the original source message ++ (nullable TSQuotedMessage *)localQuotedMessageFromSourceMessage:(TSMessage *)quotedMessage + quoteProto:(SSKProtoDataMessageQuote *)proto + transaction:(SDSAnyWriteTransaction *)transaction +{ + if (quotedMessage.isViewOnceMessage) { + // We construct a quote that does not include any of the quoted message's renderable content. + NSString *body = NSLocalizedString(@"PER_MESSAGE_EXPIRATION_OUTGOING_MESSAGE", + @"Label for outgoing view-once messages."); + return [[TSQuotedMessage alloc] initWithTimestamp:quotedMessage.timestamp + authorAddress:proto.authorAddress + body:body + bodyRanges:nil + bodySource:TSQuotedMessageContentSourceLocal + receivedQuotedAttachmentInfo:nil]; + } NSString *_Nullable body = nil; MessageBodyRanges *_Nullable bodyRanges = nil; - BOOL hasAttachment = NO; - TSQuotedMessageContentSource contentSource = TSQuotedMessageContentSourceUnknown; + OWSAttachmentInfo *attachmentInfo = nil; - // Prefer to generate the text snippet locally if available. - TSMessage *_Nullable quotedMessage = [InteractionFinder findMessageWithTimestamp:timestamp - threadId:thread.uniqueId - author:authorAddress - transaction:transaction]; + if (quotedMessage.body.length > 0) { + body = quotedMessage.body; + bodyRanges = quotedMessage.bodyRanges; - if (quotedMessage) { - contentSource = TSQuotedMessageContentSourceLocal; - - if (quotedMessage.isViewOnceMessage) { - // We construct a quote that does not include any of the - // quoted message's renderable content. - body = NSLocalizedString( - @"PER_MESSAGE_EXPIRATION_OUTGOING_MESSAGE", @"Label for outgoing view-once messages."); - // Legit usage of senderTimestamp - this class references the message it is quoting by it's sender timestamp - return [[TSQuotedMessage alloc] initWithTimestamp:timestamp - authorAddress:authorAddress - body:body - bodyRanges:nil - bodySource:TSQuotedMessageContentSourceLocal - receivedQuotedAttachmentInfos:@[]]; - } - - if (quotedMessage.body.length > 0) { - body = quotedMessage.body; - bodyRanges = quotedMessage.bodyRanges; - - } else if (quotedMessage.contactShare.name.displayName.length > 0) { - // Contact share bodies are special-cased in OWSQuotedReplyModel - // We need to account for that here. - body = [@"👤 " stringByAppendingString:quotedMessage.contactShare.name.displayName]; - bodyRanges = nil; - } - } else { - OWSLogWarn(@"Could not find quoted message: %llu", timestamp); - contentSource = TSQuotedMessageContentSourceRemote; - if (quoteProto.text.length > 0) { - body = quoteProto.text; - } - if (quoteProto.bodyRanges.count > 0) { - bodyRanges = [[MessageBodyRanges alloc] initWithProtos:quoteProto.bodyRanges]; - } + } else if (quotedMessage.contactShare.name.displayName.length > 0) { + // Contact share bodies are special-cased in OWSQuotedReplyModel + // We need to account for that here. + body = [@"👤 " stringByAppendingString:quotedMessage.contactShare.name.displayName]; } - OWSAssertDebug(contentSource != TSQuotedMessageContentSourceUnknown); + SSKProtoDataMessageQuoteQuotedAttachment *_Nullable firstAttachmentProto = proto.attachments.firstObject; - NSMutableArray *attachmentInfos = [NSMutableArray new]; - for (SSKProtoDataMessageQuoteQuotedAttachment *quotedAttachment in quoteProto.attachments) { - hasAttachment = YES; - OWSAttachmentInfo *attachmentInfo = [[OWSAttachmentInfo alloc] initWithAttachmentId:nil - contentType:quotedAttachment.contentType - sourceFilename:quotedAttachment.fileName]; + // TODO: Why are we trusting the contentType in the proto? Why does the filename matter? + if (firstAttachmentProto && [TSAttachmentStream hasThumbnailForMimeType:firstAttachmentProto.contentType]) { + TSAttachment *toQuote = [self quotedAttachmentFromOriginalMessage:quotedMessage transaction:transaction]; - // We prefer deriving any thumbnail locally rather than fetching one from the network. - TSAttachmentStream *_Nullable localThumbnail = - [self tryToDeriveLocalThumbnailWithTimestamp:timestamp - threadId:thread.uniqueId - authorAddress:authorAddress - contentType:quotedAttachment.contentType - transaction:transaction]; + if ([toQuote isKindOfClass:[TSAttachmentStream class]]) { + // We found an attachment stream on the original message! Use it as our quoted attachment + TSAttachmentStream *thumbnail = [(TSAttachmentStream *)toQuote cloneAsThumbnail]; + [thumbnail anyInsertWithTransaction:transaction]; - if (localThumbnail) { - OWSLogDebug(@"Generated local thumbnail for quoted quoted message: %@:%llu", thread.uniqueId, timestamp); - // It would be surprising if we could derive a local thumbnail when - // the body had to be derived remotely. - OWSAssertDebug(contentSource == TSQuotedMessageContentSourceLocal); + attachmentInfo = [[OWSAttachmentInfo alloc] initWithAttachmentId:thumbnail.uniqueId + ofType:OWSAttachmentInfoReferenceThumbnail + contentType:firstAttachmentProto.contentType + sourceFilename:firstAttachmentProto.fileName]; - [localThumbnail anyInsertWithTransaction:transaction]; - - attachmentInfo.thumbnailAttachmentStreamId = localThumbnail.uniqueId; - } else if (quotedAttachment.thumbnail) { - OWSLogDebug(@"Saving reference for fetching remote thumbnail for quoted message: %@:%llu", - thread.uniqueId, - timestamp); - - contentSource = TSQuotedMessageContentSourceRemote; - SSKProtoAttachmentPointer *thumbnailAttachmentProto = quotedAttachment.thumbnail; - TSAttachmentPointer *_Nullable thumbnailPointer = - [TSAttachmentPointer attachmentPointerFromProto:thumbnailAttachmentProto albumMessage:nil]; - if (thumbnailPointer) { - [thumbnailPointer anyInsertWithTransaction:transaction]; - - attachmentInfo.thumbnailAttachmentPointerId = thumbnailPointer.uniqueId; - } else { - OWSFailDebug(@"Invalid thumbnail attachment."); - } + } else if ([toQuote isKindOfClass:[TSAttachmentPointer class]]) { + // No attachment stream, but we have a pointer. It's likely this media hasn't finished downloading yet. + attachmentInfo = [[OWSAttachmentInfo alloc] initWithAttachmentId:toQuote.uniqueId + ofType:OWSAttachmentInfoReferenceOriginal + contentType:firstAttachmentProto.contentType + sourceFilename:firstAttachmentProto.fileName]; } else { - OWSLogDebug(@"No thumbnail for quoted message: %@:%llu", thread.uniqueId, timestamp); + // This could happen if a sender spoofs their quoted message proto. + // Our quoted message will include no thumbnails. + OWSFailDebug(@"Sender sent %lu quoted attachments. Local copy has none.", proto.attachments.count); } - - [attachmentInfos addObject:attachmentInfo]; - - // For now, only support a single quoted attachment. - break; } - if (body.length == 0 && !hasAttachment) { + if (body.length == 0 && !attachmentInfo) { OWSFailDebug(@"quoted message has neither text nor attachment"); return nil; } - // Legit usage of senderTimestamp - this class references the message it is quoting by it's sender timestamp - return [[TSQuotedMessage alloc] initWithTimestamp:timestamp - authorAddress:authorAddress + return [[TSQuotedMessage alloc] initWithTimestamp:quotedMessage.timestamp + authorAddress:proto.authorAddress body:body bodyRanges:bodyRanges - bodySource:contentSource - receivedQuotedAttachmentInfos:attachmentInfos]; + bodySource:TSQuotedMessageContentSourceLocal + receivedQuotedAttachmentInfo:attachmentInfo]; } -+ (nullable TSAttachmentStream *)tryToDeriveLocalThumbnailWithTimestamp:(uint64_t)timestamp - threadId:(NSString *)threadId - authorAddress:(SignalServiceAddress *)authorAddress - contentType:(NSString *)contentType - transaction:(SDSAnyWriteTransaction *)transaction +/// Builds a remote message from the proto payload +/// @note Quoted messages constructed from proto material may not be representative of the original source content. This should be flagged +/// to the user. (See: -[OWSQuotedReplyModel isRemotelySourced]) ++ (nullable TSQuotedMessage *)remoteQuotedMessageFromQuoteProto:(SSKProtoDataMessageQuote *)proto + transaction:(SDSAnyWriteTransaction *)transaction { - TSMessage *_Nullable quotedMessage = [InteractionFinder findMessageWithTimestamp:timestamp - threadId:threadId - author:authorAddress - transaction:transaction]; + NSString *_Nullable body = nil; + MessageBodyRanges *_Nullable bodyRanges = nil; + OWSAttachmentInfo *attachmentInfo = nil; - if (!quotedMessage) { - OWSLogWarn(@"Could not find quoted message: %llu", timestamp); - return nil; + if (proto.text.length > 0) { + body = proto.text; + } + if (proto.bodyRanges.count > 0) { + bodyRanges = [[MessageBodyRanges alloc] initWithProtos:proto.bodyRanges]; + } + if (proto.attachments.count > 0) { + // We only look at the first attachment + SSKProtoAttachmentPointer *thumbnailProto = proto.attachments.firstObject.thumbnail; + TSAttachmentPointer *_Nullable attachment = [TSAttachmentPointer attachmentPointerFromProto:thumbnailProto + albumMessage:nil]; + if (attachment) { + [attachment anyInsertWithTransaction:transaction]; + attachmentInfo = [[OWSAttachmentInfo alloc] initWithAttachmentId:attachment.uniqueId + ofType:OWSAttachmentInfoReferenceUntrustedPointer + contentType:thumbnailProto.contentType + sourceFilename:thumbnailProto.fileName]; + } else { + OWSFailDebug(@"Invalid remote thumbnail attachment."); + } } - TSAttachment *_Nullable attachmentToQuote = nil; - if (quotedMessage.attachmentIds.count > 0) { - attachmentToQuote = [quotedMessage bodyAttachmentsWithTransaction:transaction.unwrapGrdbRead].firstObject; - } else if (quotedMessage.linkPreview && quotedMessage.linkPreview.imageAttachmentId.length > 0) { - attachmentToQuote = - [TSAttachment anyFetchWithUniqueId:quotedMessage.linkPreview.imageAttachmentId transaction:transaction]; - } else if (quotedMessage.messageSticker && quotedMessage.messageSticker.attachmentId.length > 0) { - attachmentToQuote = - [TSAttachment anyFetchWithUniqueId:quotedMessage.messageSticker.attachmentId transaction:transaction]; - } - if (![attachmentToQuote isKindOfClass:[TSAttachmentStream class]]) { + if (body.length > 0 || attachmentInfo) { + return [[TSQuotedMessage alloc] initWithTimestamp:proto.id + authorAddress:proto.authorAddress + body:body + bodyRanges:bodyRanges + bodySource:TSQuotedMessageContentSourceRemote + receivedQuotedAttachmentInfo:attachmentInfo]; + } else { + OWSFailDebug(@"Failed to construct a valid quoted message from remote proto content"); return nil; } - if (![TSAttachmentStream hasThumbnailForMimeType:contentType]) { - return nil; - } - TSAttachmentStream *sourceStream = (TSAttachmentStream *)attachmentToQuote; - return [sourceStream cloneAsThumbnail]; } #pragma mark - Attachment (not necessarily with a thumbnail) -- (nullable OWSAttachmentInfo *)firstAttachmentInfo +- (nullable TSAttachment *)fetchThumbnailWithTransaction:(SDSAnyReadTransaction *)transaction { - return self.quotedAttachments.firstObject; + NSString *attachmentId = self.quotedAttachment.rawAttachmentId; + TSAttachment *_Nullable attachment = nil; + if (attachmentId) { + attachment = [TSAttachment anyFetchWithUniqueId:self.quotedAttachment.rawAttachmentId transaction:transaction]; + } + + // If we have an attachment stream, we should've already thumbnailed the image. + // TODO: This will fail if we've downloaded the original source media. We need to add a hook to flag + // that we need to thumbnail the source to our own local copy +// +// BOOL needsThumbnailing = (attachmentType == OWSAttachmentInfoReferenceUntrustedPointer) || +// (attachmentType == OWSAttachmentInfoReferenceOriginal); +// OWSAssertDebug(![attachment isKindOfClass:[TSAttachmentStream class]] || !needsThumbnailing); + return attachment; } -- (nullable NSString *)contentType +- (BOOL)isThumbnailOwned { - OWSAttachmentInfo *firstAttachment = self.firstAttachmentInfo; - - return firstAttachment.contentType; + return (self.quotedAttachment.attachmentType == OWSAttachmentInfoReferenceUntrustedPointer || self.quotedAttachment.attachmentType == OWSAttachmentInfoReferenceThumbnail); } -- (nullable NSString *)sourceFilename +- (NSString *)contentType { - OWSAttachmentInfo *firstAttachment = self.firstAttachmentInfo; - - return firstAttachment.sourceFilename; + return self.quotedAttachment.contentType; } -- (nullable NSString *)thumbnailAttachmentPointerId +- (NSString *)sourceFilename { - OWSAttachmentInfo *firstAttachment = self.firstAttachmentInfo; - - return firstAttachment.thumbnailAttachmentPointerId; + return self.quotedAttachment.sourceFilename; } -- (nullable NSString *)thumbnailAttachmentStreamId +- (NSString *)thumbnailAttachmentId { - OWSAttachmentInfo *firstAttachment = self.firstAttachmentInfo; - - return firstAttachment.thumbnailAttachmentStreamId; + return self.quotedAttachment.rawAttachmentId; } - (void)setThumbnailAttachmentStream:(TSAttachmentStream *)attachmentStream { OWSAssertDebug([attachmentStream isKindOfClass:[TSAttachmentStream class]]); - OWSAssertDebug(self.quotedAttachments.count == 1); - OWSAttachmentInfo *firstAttachment = self.firstAttachmentInfo; - firstAttachment.thumbnailAttachmentStreamId = attachmentStream.uniqueId; -} - -- (NSArray *)thumbnailAttachmentStreamIds -{ - NSMutableArray *streamIds = [NSMutableArray new]; - for (OWSAttachmentInfo *info in self.quotedAttachments) { - if (info.thumbnailAttachmentStreamId) { - [streamIds addObject:info.thumbnailAttachmentStreamId]; - } + // If we're updating the attachmentId, it should be because we've downloaded the thumbnail from a remote source + if (self.quotedAttachment.rawAttachmentId != attachmentStream.uniqueId) { + OWSAssertDebug(self.quotedAttachment.attachmentType == OWSAttachmentInfoReferenceUntrustedPointer); } - return [streamIds copy]; + self.quotedAttachment.attachmentType = OWSAttachmentInfoReferenceThumbnail; + self.quotedAttachment.rawAttachmentId = attachmentStream.uniqueId; } -// Before sending, persist a thumbnail attachment derived from the quoted attachment -- (NSArray *)createThumbnailAttachmentsIfNecessaryWithTransaction: - (SDSAnyWriteTransaction *)transaction +- (nullable TSAttachmentStream *)createThumbnailIfNecessaryWithTransaction:(SDSAnyWriteTransaction *)transaction { - NSMutableArray *thumbnailAttachments = [NSMutableArray new]; - - for (OWSAttachmentInfo *info in self.quotedAttachments) { - - OWSAssertDebug(info.attachmentId); - TSAttachment *attachment = [TSAttachment anyFetchWithUniqueId:info.attachmentId transaction:transaction]; - if (![attachment isKindOfClass:[TSAttachmentStream class]]) { - continue; - } - TSAttachmentStream *sourceStream = (TSAttachmentStream *)attachment; - - TSAttachmentStream *_Nullable thumbnailStream = [sourceStream cloneAsThumbnail]; - if (!thumbnailStream) { - continue; - } - - [thumbnailStream anyInsertWithTransaction:transaction]; - info.thumbnailAttachmentStreamId = thumbnailStream.uniqueId; - [thumbnailAttachments addObject:thumbnailStream]; + if (!self.quotedAttachment.rawAttachmentId) { + return nil; } - return [thumbnailAttachments copy]; + OWSAssertDebug(self.quotedAttachment.attachmentType == OWSAttachmentInfoReferenceOriginalForSend); + NSString *attachmentId = self.quotedAttachment.rawAttachmentId; + TSAttachmentStream *attachment = [TSAttachmentStream anyFetchAttachmentStreamWithUniqueId:attachmentId + transaction:transaction]; + TSAttachmentStream *thumbnail = [attachment cloneAsThumbnail]; + [thumbnail anyInsertWithTransaction:transaction]; + + if (thumbnail) { + self.quotedAttachment.attachmentType = OWSAttachmentInfoReferenceThumbnail; + self.quotedAttachment.rawAttachmentId = thumbnail.uniqueId; + return thumbnail; + } else { + OWSFailDebug(@""); + self.quotedAttachment = nil; + return nil; + } } @end diff --git a/SignalServiceKit/src/Messages/MessageSender.m b/SignalServiceKit/src/Messages/MessageSender.m index 3f3d87fc73..b86411d1e3 100644 --- a/SignalServiceKit/src/Messages/MessageSender.m +++ b/SignalServiceKit/src/Messages/MessageSender.m @@ -1484,17 +1484,10 @@ NSString *const MessageSenderSpamChallengeResolvedException = @"SpamChallengeRes [message anyUpdateOutgoingMessageWithTransaction:transaction block:^(TSOutgoingMessage *message) { - // Though we currently only ever expect at most one thumbnail, the - // proto data model suggests this could change. The logic is intended - // to work with multiple, but if we ever actually want to send - // multiple, we should do more testing. - NSArray *quotedThumbnailAttachments = - [message.quotedMessage - createThumbnailAttachmentsIfNecessaryWithTransaction: - transaction]; - for (TSAttachmentStream *attachment in quotedThumbnailAttachments) { - [attachmentIds addObject:attachment.uniqueId]; - } + TSAttachmentStream *thumbnail = [message.quotedMessage createThumbnailIfNecessaryWithTransaction:transaction]; + if (thumbnail.uniqueId) { + [attachmentIds addObject:thumbnail.uniqueId]; + } }]; } From 19ac38edf4afb57678c29d8e1e840de629efb78b Mon Sep 17 00:00:00 2001 From: Michelle Linington Date: Thu, 19 Aug 2021 15:43:35 -0700 Subject: [PATCH 2/7] Lint --- .../src/Messages/Interactions/OWSContact.h | 2 +- .../src/Messages/Interactions/TSMessage.m | 10 ++-- .../Messages/Interactions/TSOutgoingMessage.m | 6 ++- .../Messages/Interactions/TSQuotedMessage.h | 2 +- .../Messages/Interactions/TSQuotedMessage.m | 50 ++++++++++--------- SignalServiceKit/src/Messages/MessageSender.m | 16 +++--- 6 files changed, 47 insertions(+), 39 deletions(-) diff --git a/SignalServiceKit/src/Messages/Interactions/OWSContact.h b/SignalServiceKit/src/Messages/Interactions/OWSContact.h index ece5b10f57..1316417708 100644 --- a/SignalServiceKit/src/Messages/Interactions/OWSContact.h +++ b/SignalServiceKit/src/Messages/Interactions/OWSContact.h @@ -1,5 +1,5 @@ // -// Copyright (c) 2020 Open Whisper Systems. All rights reserved. +// Copyright (c) 2021 Open Whisper Systems. All rights reserved. // #import diff --git a/SignalServiceKit/src/Messages/Interactions/TSMessage.m b/SignalServiceKit/src/Messages/Interactions/TSMessage.m index 88fee13f0f..2799c2529a 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSMessage.m @@ -696,13 +696,15 @@ static const NSUInteger OWSMessageSchemaVersion = 4; return _body.filterStringForDisplay; } -- (void)setQuotedMessageThumbnailAttachmentStream:(TSAttachmentStream *)attachmentStream transaction:(SDSAnyWriteTransaction *)transaction +- (void)setQuotedMessageThumbnailAttachmentStream:(TSAttachmentStream *)attachmentStream + transaction:(SDSAnyWriteTransaction *)transaction { OWSAssertDebug([attachmentStream isKindOfClass:[TSAttachmentStream class]]); OWSAssertDebug(self.quotedMessage); - [self anyUpdateMessageWithTransaction:transaction block:^(TSMessage *message) { - [message.quotedMessage setThumbnailAttachmentStream:attachmentStream]; - }]; + [self anyUpdateMessageWithTransaction:transaction + block:^(TSMessage *message) { + [message.quotedMessage setThumbnailAttachmentStream:attachmentStream]; + }]; } #pragma mark - Update With... Methods diff --git a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m index 869e02e05e..6a37dad608 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m @@ -1407,14 +1407,16 @@ NSUInteger const TSOutgoingMessageSchemaVersion = 1; if (quotedMessage.isThumbnailOwned && quotedMessage.thumbnailAttachmentId) { NSString *attachmentId = quotedMessage.thumbnailAttachmentId; - SSKProtoDataMessageQuoteQuotedAttachmentBuilder *quotedAttachmentBuilder = [SSKProtoDataMessageQuoteQuotedAttachment builder]; + SSKProtoDataMessageQuoteQuotedAttachmentBuilder *quotedAttachmentBuilder = + [SSKProtoDataMessageQuoteQuotedAttachment builder]; quotedAttachmentBuilder.contentType = quotedMessage.contentType; quotedAttachmentBuilder.fileName = quotedMessage.sourceFilename; quotedAttachmentBuilder.thumbnail = [TSAttachmentStream buildProtoForAttachmentId:attachmentId transaction:transaction]; NSError *error; - SSKProtoDataMessageQuoteQuotedAttachment *_Nullable quotedAttachmentMessage = [quotedAttachmentBuilder buildAndReturnError:&error]; + SSKProtoDataMessageQuoteQuotedAttachment *_Nullable quotedAttachmentMessage = + [quotedAttachmentBuilder buildAndReturnError:&error]; if (error || !quotedAttachmentMessage) { OWSFailDebug(@"could not build protobuf: %@", error); return nil; diff --git a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h index aa7f661cc8..f9c2ead47b 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h +++ b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h @@ -1,5 +1,5 @@ // -// Copyright (c) 2020 Open Whisper Systems. All rights reserved. +// Copyright (c) 2021 Open Whisper Systems. All rights reserved. // #import diff --git a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m index ced6c539ac..fcc22e4dee 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m @@ -2,6 +2,7 @@ // Copyright (c) 2021 Open Whisper Systems. All rights reserved. // +#import "TSQuotedMessage.h" #import #import #import @@ -10,7 +11,6 @@ #import #import #import -#import #import NS_ASSUME_NONNULL_BEGIN @@ -20,14 +20,12 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { OWSAttachmentInfoReferenceUnset = 0, /// An attachment ID referencing original media - /// This should only be used prior to sending a new quoted message. After sending, a copy of the source media should be thumbnailed - /// and saved in its own attachment. - /// An attachment ID referencing original media - /// Indicates a valid handle to the original media, this may be a pointer to a pending download, or a stream. - /// If a pointer, we should show the blurhash. If it's a stream, we should thumbnail the original and save in a separate attachment. - /// An attachment ID referencing the quoted thumbnail - /// This thumbnail may have been generated locally, or it may have been fetched remotely from the sender of the quoted message. - /// An attachment ID referencing + /// This should only be used prior to sending a new quoted message. After sending, a copy of the source media should + /// be thumbnailed and saved in its own attachment. An attachment ID referencing original media Indicates a valid + /// handle to the original media, this may be a pointer to a pending download, or a stream. If a pointer, we should + /// show the blurhash. If it's a stream, we should thumbnail the original and save in a separate attachment. An + /// attachment ID referencing the quoted thumbnail This thumbnail may have been generated locally, or it may have + /// been fetched remotely from the sender of the quoted message. An attachment ID referencing OWSAttachmentInfoReferenceOriginalForSend = 1, OWSAttachmentInfoReferenceOriginal, @@ -78,7 +76,8 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { return self; } -+ (NSUInteger)currentSchemaVersion { ++ (NSUInteger)currentSchemaVersion +{ return 1; } @@ -90,8 +89,10 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { } if (_schemaVersion == 0) { - NSString *_Nullable oldStreamId = [coder decodeObjectOfClass:[NSString class] forKey:@"thumbnailAttachmentStreamId"]; - NSString *_Nullable oldPointerId = [coder decodeObjectOfClass:[NSString class] forKey:@"thumbnailAttachmentPointerId"]; + NSString *_Nullable oldStreamId = [coder decodeObjectOfClass:[NSString class] + forKey:@"thumbnailAttachmentStreamId"]; + NSString *_Nullable oldPointerId = [coder decodeObjectOfClass:[NSString class] + forKey:@"thumbnailAttachmentPointerId"]; NSString *_Nullable oldSourceAttachmentId = [coder decodeObjectOfClass:[NSString class] forKey:@"attachmentId"]; // Before, we maintained each of these IDs in parallel, though in practice only one in use at a time. @@ -184,7 +185,7 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { } if (_quotedAttachment == nil) { - NSSet *expectedClasses = [NSSet setWithArray:@[[NSArray class], [OWSAttachmentInfo class]]]; + NSSet *expectedClasses = [NSSet setWithArray:@[ [NSArray class], [OWSAttachmentInfo class] ]]; NSArray *_Nullable attachmentInfos = [coder decodeObjectOfClasses:expectedClasses forKey:@"quotedAttachments"]; if ([attachmentInfos.firstObject isKindOfClass:[OWSAttachmentInfo class]]) { @@ -231,7 +232,9 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { transaction:transaction]; if (originalMessage) { // Prefer to generate the quoted content locally if available. - quotedMessage = [self localQuotedMessageFromSourceMessage:originalMessage quoteProto:quoteProto transaction:transaction]; + quotedMessage = [self localQuotedMessageFromSourceMessage:originalMessage + quoteProto:quoteProto + transaction:transaction]; } if (!quotedMessage) { // If we couldn't generate the quoted content from locally available info, we can generate it from the proto. @@ -265,8 +268,8 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { { if (quotedMessage.isViewOnceMessage) { // We construct a quote that does not include any of the quoted message's renderable content. - NSString *body = NSLocalizedString(@"PER_MESSAGE_EXPIRATION_OUTGOING_MESSAGE", - @"Label for outgoing view-once messages."); + NSString *body + = NSLocalizedString(@"PER_MESSAGE_EXPIRATION_OUTGOING_MESSAGE", @"Label for outgoing view-once messages."); return [[TSQuotedMessage alloc] initWithTimestamp:quotedMessage.timestamp authorAddress:proto.authorAddress body:body @@ -332,8 +335,8 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { } /// Builds a remote message from the proto payload -/// @note Quoted messages constructed from proto material may not be representative of the original source content. This should be flagged -/// to the user. (See: -[OWSQuotedReplyModel isRemotelySourced]) +/// @note Quoted messages constructed from proto material may not be representative of the original source content. This +/// should be flagged to the user. (See: -[OWSQuotedReplyModel isRemotelySourced]) + (nullable TSQuotedMessage *)remoteQuotedMessageFromQuoteProto:(SSKProtoDataMessageQuote *)proto transaction:(SDSAnyWriteTransaction *)transaction { @@ -389,16 +392,17 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { // If we have an attachment stream, we should've already thumbnailed the image. // TODO: This will fail if we've downloaded the original source media. We need to add a hook to flag // that we need to thumbnail the source to our own local copy -// -// BOOL needsThumbnailing = (attachmentType == OWSAttachmentInfoReferenceUntrustedPointer) || -// (attachmentType == OWSAttachmentInfoReferenceOriginal); -// OWSAssertDebug(![attachment isKindOfClass:[TSAttachmentStream class]] || !needsThumbnailing); + // + // BOOL needsThumbnailing = (attachmentType == OWSAttachmentInfoReferenceUntrustedPointer) || + // (attachmentType == OWSAttachmentInfoReferenceOriginal); + // OWSAssertDebug(![attachment isKindOfClass:[TSAttachmentStream class]] || !needsThumbnailing); return attachment; } - (BOOL)isThumbnailOwned { - return (self.quotedAttachment.attachmentType == OWSAttachmentInfoReferenceUntrustedPointer || self.quotedAttachment.attachmentType == OWSAttachmentInfoReferenceThumbnail); + return (self.quotedAttachment.attachmentType == OWSAttachmentInfoReferenceUntrustedPointer + || self.quotedAttachment.attachmentType == OWSAttachmentInfoReferenceThumbnail); } - (NSString *)contentType diff --git a/SignalServiceKit/src/Messages/MessageSender.m b/SignalServiceKit/src/Messages/MessageSender.m index b86411d1e3..afe97ec3f1 100644 --- a/SignalServiceKit/src/Messages/MessageSender.m +++ b/SignalServiceKit/src/Messages/MessageSender.m @@ -1481,14 +1481,14 @@ NSString *const MessageSenderSpamChallengeResolvedException = @"SpamChallengeRes if (message.quotedMessage) { // We need to update the message record here to reflect the new attachments we may create. - [message - anyUpdateOutgoingMessageWithTransaction:transaction - block:^(TSOutgoingMessage *message) { - TSAttachmentStream *thumbnail = [message.quotedMessage createThumbnailIfNecessaryWithTransaction:transaction]; - if (thumbnail.uniqueId) { - [attachmentIds addObject:thumbnail.uniqueId]; - } - }]; + [message anyUpdateOutgoingMessageWithTransaction:transaction + block:^(TSOutgoingMessage *message) { + TSAttachmentStream *thumbnail = [message.quotedMessage + createThumbnailIfNecessaryWithTransaction:transaction]; + if (thumbnail.uniqueId) { + [attachmentIds addObject:thumbnail.uniqueId]; + } + }]; } if (message.contactShare.avatarAttachmentId != nil) { From 0438388486b19519e96ac2175c1cca425891c717 Mon Sep 17 00:00:00 2001 From: Michelle Linington Date: Thu, 19 Aug 2021 16:14:53 -0700 Subject: [PATCH 3/7] Fix compilation error on buildbot Not sure why I didn't hit these locally --- .../src/Messages/Interactions/TSMessage.m | 17 +++++------------ .../src/Messages/Interactions/TSQuotedMessage.h | 1 + 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/SignalServiceKit/src/Messages/Interactions/TSMessage.m b/SignalServiceKit/src/Messages/Interactions/TSMessage.m index 2799c2529a..65cb8393f0 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSMessage.m @@ -884,19 +884,12 @@ static const NSUInteger OWSMessageSchemaVersion = 4; } } - NSMutableSet *quotedReplyAttachmentIds = [NSMutableSet new]; - if (self.quotedMessage.thumbnailAttachmentStreamIds != nil) { - [quotedReplyAttachmentIds addObjectsFromArray:self.quotedMessage.thumbnailAttachmentStreamIds]; + if (self.quotedMessage.thumbnailAttachmentId) { + [unknownAttachmentIds removeObject:self.quotedMessage.thumbnailAttachmentId]; } - if (self.quotedMessage.thumbnailAttachmentPointerId != nil) { - [quotedReplyAttachmentIds addObject:self.quotedMessage.thumbnailAttachmentPointerId]; - } - [unknownAttachmentIds minusSet:quotedReplyAttachmentIds]; - if (shouldRemoveQuotedReply) { - for (NSString *attachmentId in quotedReplyAttachmentIds) { - OWSLogVerbose(@"Removing quoted reply attachment."); - [self removeAttachmentWithId:attachmentId transaction:transaction]; - } + if (shouldRemoveQuotedReply && self.quotedMessage.thumbnailAttachmentId) { + OWSLogVerbose(@"Removing quoted reply attachment."); + [self removeAttachmentWithId:self.quotedMessage.thumbnailAttachmentId transaction:transaction]; } // Err on the side of cleaning up unknown attachments. diff --git a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h index f9c2ead47b..f8d6214fc7 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h +++ b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h @@ -7,6 +7,7 @@ NS_ASSUME_NONNULL_BEGIN @class MessageBodyRanges; +@class SDSAnyReadTransaction; @class SDSAnyWriteTransaction; @class SSKProtoDataMessage; @class SignalServiceAddress; From 4e7917524732bdfc3f2309c6cab90e4bfca94da2 Mon Sep 17 00:00:00 2001 From: Michelle Linington Date: Thu, 19 Aug 2021 17:48:15 -0700 Subject: [PATCH 4/7] Fix some comments --- .../src/Messages/Interactions/TSQuotedMessage.h | 3 ++- .../src/Messages/Interactions/TSQuotedMessage.m | 16 ++++++---------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h index f8d6214fc7..28f4eb830c 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h +++ b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h @@ -35,8 +35,9 @@ typedef NS_ENUM(NSUInteger, TSQuotedMessageContentSource) { #pragma mark - Attachments -/// Is the quoted thumbnail currently owned by the quoted message model? Or is it referencing an existing attachment @property (nonatomic, readonly) NSString *thumbnailAttachmentId; +/// Returns YES if the thumbnail is something maintained by the quoted reply itself (as opposed to to media in some +/// other message) @property (nonatomic, readonly) BOOL isThumbnailOwned; @property (nonatomic, readonly) NSString *contentType; @property (nonatomic, readonly) NSString *sourceFilename; diff --git a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m index fcc22e4dee..5fe6e0ea90 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m @@ -18,18 +18,14 @@ NS_ASSUME_NONNULL_BEGIN /// Indicates the sort of attachment ID included in the attachment info typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { OWSAttachmentInfoReferenceUnset = 0, - - /// An attachment ID referencing original media - /// This should only be used prior to sending a new quoted message. After sending, a copy of the source media should - /// be thumbnailed and saved in its own attachment. An attachment ID referencing original media Indicates a valid - /// handle to the original media, this may be a pointer to a pending download, or a stream. If a pointer, we should - /// show the blurhash. If it's a stream, we should thumbnail the original and save in a separate attachment. An - /// attachment ID referencing the quoted thumbnail This thumbnail may have been generated locally, or it may have - /// been fetched remotely from the sender of the quoted message. An attachment ID referencing - + /// An original attachment for a quoted reply draft. This needs to be thumbnailed before it is sent. OWSAttachmentInfoReferenceOriginalForSend = 1, + /// A reference to an original attachment in a quoted reply we've received. If this ever manifests as a stream + /// we should clone it as a private thumbnail OWSAttachmentInfoReferenceOriginal, + /// A private thumbnail that we (the quoted reply) have ownership of OWSAttachmentInfoReferenceThumbnail, + /// An untrusted pointer to a thumbnail. This was included in the proto of a message we've received. OWSAttachmentInfoReferenceUntrustedPointer, }; @@ -451,7 +447,7 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { self.quotedAttachment.rawAttachmentId = thumbnail.uniqueId; return thumbnail; } else { - OWSFailDebug(@""); + OWSFailDebug(@"Failed to clone a quoted reply thumbnail from original media"); self.quotedAttachment = nil; return nil; } From 9a49c3b64c491afaf377c2e03c102884b3e240db Mon Sep 17 00:00:00 2001 From: Michelle Linington Date: Thu, 19 Aug 2021 20:22:31 -0700 Subject: [PATCH 5/7] Bug fixes --- .../Cells/OWSQuotedMessageView.m | 1 + .../Cells/QuotedMessageView.swift | 1 + .../Messages/Interactions/TSQuotedMessage.m | 36 ++++++++++--------- 3 files changed, 21 insertions(+), 17 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/Cells/OWSQuotedMessageView.m b/Signal/src/ViewControllers/ConversationView/Cells/OWSQuotedMessageView.m index daecff8933..7ea36d1d99 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/OWSQuotedMessageView.m +++ b/Signal/src/ViewControllers/ConversationView/Cells/OWSQuotedMessageView.m @@ -270,6 +270,7 @@ const CGFloat kRemotelySourcedContentRowSpacing = 3; [quotedAttachmentView addGestureRecognizer:tapGesture]; quotedAttachmentView.userInteractionEnabled = YES; } else { + // TODO: Should we overlay the file extension like we do with CVComponentGenericAttachment UIImage *contentIcon = [UIImage imageNamed:@"generic-attachment"]; UIImageView *contentImageView = [self imageViewForImage:contentIcon]; contentImageView.contentMode = UIViewContentModeScaleAspectFit; diff --git a/Signal/src/ViewControllers/ConversationView/Cells/QuotedMessageView.swift b/Signal/src/ViewControllers/ConversationView/Cells/QuotedMessageView.swift index d2808e3255..4ef66ba097 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/QuotedMessageView.swift +++ b/Signal/src/ViewControllers/ConversationView/Cells/QuotedMessageView.swift @@ -452,6 +452,7 @@ public class QuotedMessageView: ManualStackViewWithLayer { return wrapper } else { + // TODO: Should we overlay the file extension like we do with CVComponentGenericAttachment quotedImageView.setTemplateImageName("generic-attachment", tintColor: .clear) quotedImageView.contentMode = .scaleAspectFit quotedImageView.clipsToBounds = false diff --git a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m index 5fe6e0ea90..5eb7c820b7 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m @@ -346,9 +346,10 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { if (proto.bodyRanges.count > 0) { bodyRanges = [[MessageBodyRanges alloc] initWithProtos:proto.bodyRanges]; } - if (proto.attachments.count > 0) { - // We only look at the first attachment - SSKProtoAttachmentPointer *thumbnailProto = proto.attachments.firstObject.thumbnail; + + // We're only interested in the first attachment + SSKProtoAttachmentPointer *thumbnailProto = proto.attachments.firstObject.thumbnail; + if (thumbnailProto) { TSAttachmentPointer *_Nullable attachment = [TSAttachmentPointer attachmentPointerFromProto:thumbnailProto albumMessage:nil]; if (attachment) { @@ -431,26 +432,27 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { - (nullable TSAttachmentStream *)createThumbnailIfNecessaryWithTransaction:(SDSAnyWriteTransaction *)transaction { - if (!self.quotedAttachment.rawAttachmentId) { + NSString *_Nullable attachmentId = self.quotedAttachment.rawAttachmentId; + if (!attachmentId) { return nil; } - OWSAssertDebug(self.quotedAttachment.attachmentType == OWSAttachmentInfoReferenceOriginalForSend); - NSString *attachmentId = self.quotedAttachment.rawAttachmentId; - TSAttachmentStream *attachment = [TSAttachmentStream anyFetchAttachmentStreamWithUniqueId:attachmentId - transaction:transaction]; - TSAttachmentStream *thumbnail = [attachment cloneAsThumbnail]; - [thumbnail anyInsertWithTransaction:transaction]; - - if (thumbnail) { - self.quotedAttachment.attachmentType = OWSAttachmentInfoReferenceThumbnail; + TSAttachmentStream *_Nullable thumbnail = nil; + TSAttachmentStream *_Nullable attachment = [TSAttachmentStream anyFetchAttachmentStreamWithUniqueId:attachmentId + transaction:transaction]; + if (attachment && self.quotedAttachment.attachmentType == OWSAttachmentInfoReferenceOriginalForSend) { + OWSLogInfo(@"Cloning source attachment to thumbnail for send."); + thumbnail = [attachment cloneAsThumbnail]; + [thumbnail anyInsertWithTransaction:transaction]; self.quotedAttachment.rawAttachmentId = thumbnail.uniqueId; - return thumbnail; + self.quotedAttachment.attachmentType = OWSAttachmentInfoReferenceThumbnail; + + } else if (attachment && self.quotedAttachment.attachmentType == OWSAttachmentInfoReferenceThumbnail) { + thumbnail = attachment; } else { - OWSFailDebug(@"Failed to clone a quoted reply thumbnail from original media"); - self.quotedAttachment = nil; - return nil; + OWSFailDebug(@"Unexpected attachment state. Current state: %lu. Fetched: %@", self.quotedAttachment.attachmentType, attachment.uniqueId);; } + return thumbnail; } @end From 436acf2694b4a2c36c1bdfdb9aa4ef99a575ba4f Mon Sep 17 00:00:00 2001 From: Michelle Linington Date: Thu, 19 Aug 2021 23:36:59 -0700 Subject: [PATCH 6/7] Various bug fixes and improvements Most significant here is better handling of quoted reply content without an attachment. Or with an attachment without a thumbnail --- .../CV/CVComponentState.swift | 3 +- .../Cells/OWSQuotedMessageView.m | 11 +- .../Cells/QuotedMessageView.swift | 9 +- .../ViewModels/OWSQuotedReplyModel.h | 7 +- .../ViewModels/OWSQuotedReplyModel.m | 35 +++--- .../Attachments/OWSAttachmentDownloads.swift | 9 +- .../src/Messages/Interactions/TSMessage.h | 1 + .../src/Messages/Interactions/TSMessage.m | 26 ++++ .../Messages/Interactions/TSOutgoingMessage.m | 20 +-- .../Messages/Interactions/TSQuotedMessage.h | 13 +- .../Messages/Interactions/TSQuotedMessage.m | 114 ++++++++++++------ SignalServiceKit/src/Messages/MessageSender.m | 2 +- 12 files changed, 158 insertions(+), 92 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/CV/CVComponentState.swift b/Signal/src/ViewControllers/ConversationView/CV/CVComponentState.swift index ab67fbfb66..f661f62d39 100644 --- a/Signal/src/ViewControllers/ConversationView/CV/CVComponentState.swift +++ b/Signal/src/ViewControllers/ConversationView/CV/CVComponentState.swift @@ -952,10 +952,9 @@ fileprivate extension CVComponentState.Builder { // TODO: Should we validate and throw errors? mutating func buildQuotedReply(message: TSMessage) { - guard let quotedMessage = message.quotedMessage else { + guard let quotedReplyModel = OWSQuotedReplyModel.quotedReply(from: message, transaction: transaction) else { return } - let quotedReplyModel = OWSQuotedReplyModel.quotedReply(with: quotedMessage, transaction: transaction) var displayableQuotedText: DisplayableText? if let quotedBody = quotedReplyModel.body, !quotedBody.isEmpty { diff --git a/Signal/src/ViewControllers/ConversationView/Cells/OWSQuotedMessageView.m b/Signal/src/ViewControllers/ConversationView/Cells/OWSQuotedMessageView.m index 7ea36d1d99..0c062d4e27 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/OWSQuotedMessageView.m +++ b/Signal/src/ViewControllers/ConversationView/Cells/OWSQuotedMessageView.m @@ -250,7 +250,7 @@ const CGFloat kRemotelySourcedContentRowSpacing = 3; [quotedAttachmentView addSubview:contentImageView]; [contentImageView autoCenterInSuperview]; } - } else if (self.quotedMessage.thumbnailDownloadFailed) { + } else if (self.quotedMessage.failedThumbnailAttachmentPointer) { // TODO design review icon and color UIImage *contentIcon = [[UIImage imageNamed:@"btnRefresh--white"] imageWithRenderingMode:UIImageRenderingModeAlwaysTemplate]; @@ -385,18 +385,13 @@ const CGFloat kRemotelySourcedContentRowSpacing = 3; { OWSLogDebug(@"in didTapFailedThumbnailDownload"); - if (!self.quotedMessage.thumbnailDownloadFailed) { - OWSFailDebug(@"thumbnailDownloadFailed was unexpectedly false"); - return; - } - - if (!self.quotedMessage.thumbnailAttachmentPointer) { + if (!self.quotedMessage.failedThumbnailAttachmentPointer) { OWSFailDebug(@"thumbnailAttachmentPointer was unexpectedly nil"); return; } [self.delegate didTapQuotedReply:self.quotedMessage - failedThumbnailDownloadAttachmentPointer:self.quotedMessage.thumbnailAttachmentPointer]; + failedThumbnailDownloadAttachmentPointer:self.quotedMessage.failedThumbnailAttachmentPointer]; } - (nullable UIImage *)tryToLoadThumbnailImage diff --git a/Signal/src/ViewControllers/ConversationView/Cells/QuotedMessageView.swift b/Signal/src/ViewControllers/ConversationView/Cells/QuotedMessageView.swift index 4ef66ba097..1429d18678 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/QuotedMessageView.swift +++ b/Signal/src/ViewControllers/ConversationView/Cells/QuotedMessageView.swift @@ -435,7 +435,7 @@ public class QuotedMessageView: ManualStackViewWithLayer { wrapper.addSubviewToCenterOnSuperviewWithDesiredSize(contentImageView) } return wrapper - } else if quotedReplyModel.thumbnailDownloadFailed { + } else if quotedReplyModel.failedThumbnailAttachmentPointer != nil { let wrapper = ManualLayoutViewWithLayer(name: "thumbnailDownloadFailedWrapper") wrapper.backgroundColor = configurator.highlightColor @@ -648,12 +648,7 @@ public class QuotedMessageView: ManualStackViewWithLayer { } let quotedReplyModel = state.quotedReplyModel - if !quotedReplyModel.thumbnailDownloadFailed { - owsFailDebug("thumbnailDownloadFailed was unexpectedly false") - return - } - - guard let thumbnailAttachmentPointer = quotedReplyModel.thumbnailAttachmentPointer else { + guard let thumbnailAttachmentPointer = quotedReplyModel.failedThumbnailAttachmentPointer else { owsFailDebug("thumbnailAttachmentPointer was unexpectedly nil") return } diff --git a/SignalMessaging/ViewModels/OWSQuotedReplyModel.h b/SignalMessaging/ViewModels/OWSQuotedReplyModel.h index ca0a3122e7..825b59c371 100644 --- a/SignalMessaging/ViewModels/OWSQuotedReplyModel.h +++ b/SignalMessaging/ViewModels/OWSQuotedReplyModel.h @@ -28,8 +28,7 @@ NS_ASSUME_NONNULL_BEGIN @property (nonatomic, readonly) uint64_t timestamp; @property (nonatomic, readonly) SignalServiceAddress *authorAddress; @property (nonatomic, readonly, nullable) TSAttachmentStream *attachmentStream; -@property (nonatomic, readonly, nullable) TSAttachmentPointer *thumbnailAttachmentPointer; -@property (nonatomic, readonly) BOOL thumbnailDownloadFailed; +@property (nonatomic, readonly, nullable) TSAttachmentPointer *failedThumbnailAttachmentPointer; // This property should be set IFF we are quoting a text message // or attachment with caption. @@ -50,8 +49,8 @@ NS_ASSUME_NONNULL_BEGIN - (instancetype)init NS_UNAVAILABLE; // Used for persisted quoted replies, both incoming and outgoing. -+ (instancetype)quotedReplyWithQuotedMessage:(TSQuotedMessage *)quotedMessage - transaction:(SDSAnyReadTransaction *)transaction; ++ (nullable instancetype)quotedReplyFromMessage:(TSMessage *)message + transaction:(SDSAnyReadTransaction *)transaction; // Builds a not-yet-sent QuotedReplyModel + (nullable instancetype)quotedReplyForSendingWithItem:(id)item diff --git a/SignalMessaging/ViewModels/OWSQuotedReplyModel.m b/SignalMessaging/ViewModels/OWSQuotedReplyModel.m index 9b3cf9a2cc..55c4e8f390 100644 --- a/SignalMessaging/ViewModels/OWSQuotedReplyModel.m +++ b/SignalMessaging/ViewModels/OWSQuotedReplyModel.m @@ -30,8 +30,7 @@ NS_ASSUME_NONNULL_BEGIN contentType:(nullable NSString *)contentType sourceFilename:(nullable NSString *)sourceFilename attachmentStream:(nullable TSAttachmentStream *)attachmentStream - thumbnailAttachmentPointer:(nullable TSAttachmentPointer *)thumbnailAttachmentPointer - thumbnailDownloadFailed:(BOOL)thumbnailDownloadFailed NS_DESIGNATED_INITIALIZER; + failedThumbnailAttachmentPointer:(nullable TSAttachmentPointer *)thumbnailAttachmentPointer NS_DESIGNATED_INITIALIZER; @end @@ -49,8 +48,7 @@ NS_ASSUME_NONNULL_BEGIN contentType:(nullable NSString *)contentType sourceFilename:(nullable NSString *)sourceFilename attachmentStream:(nullable TSAttachmentStream *)attachmentStream - thumbnailAttachmentPointer:(nullable TSAttachmentPointer *)thumbnailAttachmentPointer - thumbnailDownloadFailed:(BOOL)thumbnailDownloadFailed + failedThumbnailAttachmentPointer:(nullable TSAttachmentPointer *)failedThumbnailAttachmentPointer { self = [super init]; if (!self) { @@ -66,19 +64,23 @@ NS_ASSUME_NONNULL_BEGIN _contentType = contentType; _sourceFilename = sourceFilename; _attachmentStream = attachmentStream; - _thumbnailAttachmentPointer = thumbnailAttachmentPointer; - _thumbnailDownloadFailed = thumbnailDownloadFailed; + _failedThumbnailAttachmentPointer = failedThumbnailAttachmentPointer; return self; } #pragma mark - Factory Methods -+ (instancetype)quotedReplyWithQuotedMessage:(TSQuotedMessage *)quotedMessage - transaction:(SDSAnyReadTransaction *)transaction ++ (nullable instancetype)quotedReplyFromMessage:(TSMessage *)message + transaction:(SDSAnyReadTransaction *)transaction { + TSQuotedMessage *quotedMessage = message.quotedMessage; + if (!quotedMessage) { + return nil; + } + UIImage *_Nullable thumbnailImage; - TSAttachment *_Nullable attachment = [quotedMessage fetchThumbnailWithTransaction:transaction]; + TSAttachment *_Nullable attachment = [message fetchQuotedMessageThumbnailWithTransaction:transaction]; TSAttachmentPointer *_Nullable failedAttachmentPointer = nil; if ([attachment isKindOfClass:[TSAttachmentStream class]]) { @@ -104,8 +106,7 @@ NS_ASSUME_NONNULL_BEGIN contentType:quotedMessage.contentType sourceFilename:quotedMessage.sourceFilename attachmentStream:nil - thumbnailAttachmentPointer:failedAttachmentPointer - thumbnailDownloadFailed:(failedAttachmentPointer != nil)]; + failedThumbnailAttachmentPointer:failedAttachmentPointer]; } + (nullable instancetype)quotedReplyForSendingWithItem:(id)item @@ -151,8 +152,7 @@ NS_ASSUME_NONNULL_BEGIN contentType:nil sourceFilename:nil attachmentStream:nil - thumbnailAttachmentPointer:nil - thumbnailDownloadFailed:NO]; + failedThumbnailAttachmentPointer:nil]; } if (item.contactShare) { @@ -171,8 +171,7 @@ NS_ASSUME_NONNULL_BEGIN contentType:nil sourceFilename:nil attachmentStream:nil - thumbnailAttachmentPointer:nil - thumbnailDownloadFailed:NO]; + failedThumbnailAttachmentPointer:nil]; } if (item.stickerInfo || item.stickerAttachment || item.stickerMetadata) { @@ -260,8 +259,7 @@ NS_ASSUME_NONNULL_BEGIN contentType:contentType sourceFilename:quotedAttachment.sourceFilename attachmentStream:quotedAttachment - thumbnailAttachmentPointer:nil - thumbnailDownloadFailed:NO]; + failedThumbnailAttachmentPointer:nil]; } NSString *_Nullable quotedText = message.body; @@ -343,8 +341,7 @@ NS_ASSUME_NONNULL_BEGIN contentType:quotedAttachment.contentType sourceFilename:quotedAttachment.sourceFilename attachmentStream:quotedAttachment - thumbnailAttachmentPointer:nil - thumbnailDownloadFailed:NO]; + failedThumbnailAttachmentPointer:nil]; } #pragma mark - Instance Methods diff --git a/SignalServiceKit/src/Messages/Attachments/OWSAttachmentDownloads.swift b/SignalServiceKit/src/Messages/Attachments/OWSAttachmentDownloads.swift index 8b50e3463c..522f5f310c 100644 --- a/SignalServiceKit/src/Messages/Attachments/OWSAttachmentDownloads.swift +++ b/SignalServiceKit/src/Messages/Attachments/OWSAttachmentDownloads.swift @@ -1040,8 +1040,13 @@ public extension OWSAttachmentDownloads { return jobRequests } - if let quotedMessage = message.quotedMessage, quotedMessage.isThumbnailOwned { - addJobRequest(attachmentId: quotedMessage.thumbnailAttachmentId, category: .quotedReplyThumbnail) + // We only want to kick off a thumbnail fetching job if: + // - The thumbnail attachment is owned by the quoted message content (so it's solely responsible for fetching) + // - It's an unfetched pointer + if message.quotedMessage?.isThumbnailOwned == true, + let attachment = message.quotedMessage?.fetchThumbnail(with: transaction), + attachment is TSAttachmentPointer { + addJobRequest(attachment: attachment, category: .quotedReplyThumbnail) } if let attachmentId = message.contactShare?.avatarAttachmentId { diff --git a/SignalServiceKit/src/Messages/Interactions/TSMessage.h b/SignalServiceKit/src/Messages/Interactions/TSMessage.h index a277eb7f9d..b7ef385759 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSMessage.h +++ b/SignalServiceKit/src/Messages/Interactions/TSMessage.h @@ -112,6 +112,7 @@ NS_DESIGNATED_INITIALIZER NS_SWIFT_NAME(init(grdbId:uniqueId:receivedAtTimestamp // quoted reply thumbnails, contact share avatars, link preview images, etc. - (NSArray *)allAttachmentIds; +- (nullable TSAttachment *)fetchQuotedMessageThumbnailWithTransaction:(SDSAnyReadTransaction *)transaction; - (void)setQuotedMessageThumbnailAttachmentStream:(TSAttachmentStream *)attachmentStream transaction:(SDSAnyWriteTransaction *)transaction; diff --git a/SignalServiceKit/src/Messages/Interactions/TSMessage.m b/SignalServiceKit/src/Messages/Interactions/TSMessage.m index 65cb8393f0..7b9d9461cd 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSMessage.m @@ -696,6 +696,32 @@ static const NSUInteger OWSMessageSchemaVersion = 4; return _body.filterStringForDisplay; } +- (nullable TSAttachment *)fetchQuotedMessageThumbnailWithTransaction:(SDSAnyReadTransaction *)transaction +{ + TSAttachment *_Nullable attachment = [self.quotedMessage fetchThumbnailWithTransaction:transaction]; + + // We should clone the attachment if it's been downloaded but our quotedMessage doesn't have its own copy. + BOOL needsClone = [attachment isKindOfClass:[TSAttachmentStream class]] && !self.quotedMessage.isThumbnailOwned; + TSAttachment *(^saveUpdatedThumbnail)(SDSAnyWriteTransaction *) = ^TSAttachment *(SDSAnyWriteTransaction *writeTx) { + __block TSAttachment *_Nullable attachment = nil; + [self anyUpdateMessageWithTransaction:writeTx block:^(TSMessage *message) { + attachment = [message.quotedMessage createThumbnailIfNecessaryWithTransaction:writeTx]; + }]; + return attachment; + }; + + // If we happen to be handed a write transaction, we can perform the clone synchronously + // Otherwise, just hand the caller what we have. We'll clone it async. + if (needsClone && [transaction isKindOfClass:[SDSAnyWriteTransaction class]]) { + attachment = saveUpdatedThumbnail((SDSAnyWriteTransaction *)transaction); + } else if (needsClone) { + DatabaseStorageAsyncWrite(self.databaseStorage, ^(SDSAnyWriteTransaction *writeTx){ + saveUpdatedThumbnail(writeTx); + }); + } + return attachment; +} + - (void)setQuotedMessageThumbnailAttachmentStream:(TSAttachmentStream *)attachmentStream transaction:(SDSAnyWriteTransaction *)transaction { diff --git a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m index 6a37dad608..f9665525d5 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m @@ -1405,27 +1405,29 @@ NSUInteger const TSOutgoingMessageSchemaVersion = 1; } } - if (quotedMessage.isThumbnailOwned && quotedMessage.thumbnailAttachmentId) { - NSString *attachmentId = quotedMessage.thumbnailAttachmentId; + if (quotedMessage.hasAttachment) { SSKProtoDataMessageQuoteQuotedAttachmentBuilder *quotedAttachmentBuilder = - [SSKProtoDataMessageQuoteQuotedAttachment builder]; + [SSKProtoDataMessageQuoteQuotedAttachment builder]; quotedAttachmentBuilder.contentType = quotedMessage.contentType; quotedAttachmentBuilder.fileName = quotedMessage.sourceFilename; - quotedAttachmentBuilder.thumbnail = [TSAttachmentStream buildProtoForAttachmentId:attachmentId - transaction:transaction]; + + if (quotedMessage.thumbnailAttachmentId && quotedMessage.isThumbnailOwned) { + NSString *attachmentId = quotedMessage.thumbnailAttachmentId; + quotedAttachmentBuilder.thumbnail = [TSAttachmentStream buildProtoForAttachmentId:attachmentId + transaction:transaction]; + } else if (quotedMessage.thumbnailAttachmentId) { + OWSFailDebug(@"Referencing an attachment that isn't owned by the quote."); + } NSError *error; SSKProtoDataMessageQuoteQuotedAttachment *_Nullable quotedAttachmentMessage = - [quotedAttachmentBuilder buildAndReturnError:&error]; + [quotedAttachmentBuilder buildAndReturnError:&error]; if (error || !quotedAttachmentMessage) { OWSFailDebug(@"could not build protobuf: %@", error); return nil; } - [quoteBuilder addAttachments:quotedAttachmentMessage]; hasQuotedAttachment = YES; - } else if (quotedMessage.thumbnailAttachmentId) { - OWSFailDebug(@"Referencing an attachment that isn't owned by the quoted message. Was this never thumbnailed?"); } if (hasQuotedText || hasQuotedAttachment) { diff --git a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h index 28f4eb830c..6c1cbb6881 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h +++ b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h @@ -35,12 +35,15 @@ typedef NS_ENUM(NSUInteger, TSQuotedMessageContentSource) { #pragma mark - Attachments -@property (nonatomic, readonly) NSString *thumbnailAttachmentId; -/// Returns YES if the thumbnail is something maintained by the quoted reply itself (as opposed to to media in some -/// other message) +@property (nonatomic, readonly) BOOL hasAttachment; + +/// Returns YES if the thumbnail is something maintained by the quoted reply itself (as opposed to to media in some other message) @property (nonatomic, readonly) BOOL isThumbnailOwned; -@property (nonatomic, readonly) NSString *contentType; -@property (nonatomic, readonly) NSString *sourceFilename; +@property (nonatomic, readonly, nullable) NSString *thumbnailAttachmentId; +@property (nonatomic, readonly, nullable) NSString *contentType; +@property (nonatomic, readonly, nullable) NSString *sourceFilename; + +// Should only be called by TSMessage. May perform a sneaky write if necessary - (nullable TSAttachment *)fetchThumbnailWithTransaction:(SDSAnyReadTransaction *)transaction; // References an already downloaded or locally generated thumbnail file diff --git a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m index 5eb7c820b7..46924db82c 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m @@ -44,16 +44,35 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { @implementation OWSAttachmentInfo +- (instancetype)initWithoutThumbnailWithContentType:(NSString *)contentType + sourceFilename:(NSString *)sourceFilename +{ + self = [super init]; + if (self) { + _schemaVersion = self.class.currentSchemaVersion; + _rawAttachmentId = nil; + _attachmentType = OWSAttachmentInfoReferenceUnset; + _contentType = contentType; + _sourceFilename = sourceFilename; + } + return self; +} + - (instancetype)initWithOriginalAttachmentStream:(TSAttachmentStream *)attachmentStream { OWSAssertDebug([attachmentStream isKindOfClass:[TSAttachmentStream class]]); OWSAssertDebug(attachmentStream.uniqueId); OWSAssertDebug(attachmentStream.contentType); - return [self initWithAttachmentId:attachmentStream.uniqueId - ofType:OWSAttachmentInfoReferenceOriginalForSend - contentType:attachmentStream.contentType - sourceFilename:attachmentStream.sourceFilename]; + if ([TSAttachmentStream hasThumbnailForMimeType:attachmentStream.contentType]) { + return [self initWithAttachmentId:attachmentStream.uniqueId + ofType:OWSAttachmentInfoReferenceOriginalForSend + contentType:attachmentStream.contentType + sourceFilename:attachmentStream.sourceFilename]; + } else { + return [self initWithoutThumbnailWithContentType:attachmentStream.contentType + sourceFilename:attachmentStream.sourceFilename]; + } } - (instancetype)initWithAttachmentId:(NSString *)attachmentId @@ -105,6 +124,9 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { } else if (oldSourceAttachmentId) { _attachmentType = OWSAttachmentInfoReferenceOriginalForSend; _rawAttachmentId = oldSourceAttachmentId; + } else { + _attachmentType = OWSAttachmentInfoReferenceUnset; + _rawAttachmentId = nil; } } _schemaVersion = self.class.currentSchemaVersion; @@ -265,7 +287,7 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { if (quotedMessage.isViewOnceMessage) { // We construct a quote that does not include any of the quoted message's renderable content. NSString *body - = NSLocalizedString(@"PER_MESSAGE_EXPIRATION_OUTGOING_MESSAGE", @"Label for outgoing view-once messages."); + = NSLocalizedString(@"PER_MESSAGE_EXPIRATION_NOT_VIEWABLE", @"inbox cell and notification text for an already viewed view-once media message."); return [[TSQuotedMessage alloc] initWithTimestamp:quotedMessage.timestamp authorAddress:proto.authorAddress body:body @@ -290,26 +312,30 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { SSKProtoDataMessageQuoteQuotedAttachment *_Nullable firstAttachmentProto = proto.attachments.firstObject; - // TODO: Why are we trusting the contentType in the proto? Why does the filename matter? - if (firstAttachmentProto && [TSAttachmentStream hasThumbnailForMimeType:firstAttachmentProto.contentType]) { + if (firstAttachmentProto) { TSAttachment *toQuote = [self quotedAttachmentFromOriginalMessage:quotedMessage transaction:transaction]; + BOOL shouldThumbnail = [TSAttachmentStream hasThumbnailForMimeType:toQuote.contentType]; - if ([toQuote isKindOfClass:[TSAttachmentStream class]]) { + if ([toQuote isKindOfClass:[TSAttachmentStream class]] && shouldThumbnail) { // We found an attachment stream on the original message! Use it as our quoted attachment TSAttachmentStream *thumbnail = [(TSAttachmentStream *)toQuote cloneAsThumbnail]; [thumbnail anyInsertWithTransaction:transaction]; attachmentInfo = [[OWSAttachmentInfo alloc] initWithAttachmentId:thumbnail.uniqueId ofType:OWSAttachmentInfoReferenceThumbnail - contentType:firstAttachmentProto.contentType - sourceFilename:firstAttachmentProto.fileName]; + contentType:toQuote.contentType + sourceFilename:toQuote.sourceFilename]; - } else if ([toQuote isKindOfClass:[TSAttachmentPointer class]]) { + } else if ([toQuote isKindOfClass:[TSAttachmentPointer class]] && shouldThumbnail) { // No attachment stream, but we have a pointer. It's likely this media hasn't finished downloading yet. attachmentInfo = [[OWSAttachmentInfo alloc] initWithAttachmentId:toQuote.uniqueId ofType:OWSAttachmentInfoReferenceOriginal - contentType:firstAttachmentProto.contentType - sourceFilename:firstAttachmentProto.fileName]; + contentType:toQuote.contentType + sourceFilename:toQuote.sourceFilename]; + } else if (toQuote) { + // We have an attachment in the original message, but it doesn't support thumbnailing + attachmentInfo = [[OWSAttachmentInfo alloc] initWithoutThumbnailWithContentType:toQuote.contentType + sourceFilename:toQuote.sourceFilename]; } else { // This could happen if a sender spoofs their quoted message proto. // Our quoted message will include no thumbnails. @@ -322,8 +348,18 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { return nil; } + SignalServiceAddress *address = nil; + if ([quotedMessage isKindOfClass:[TSIncomingMessage class]]) { + address = ((TSIncomingMessage *)quotedMessage).authorAddress; + } else if ([quotedMessage isKindOfClass:[TSOutgoingMessage class]]) { + address = [TSAccountManager localAddress]; + } else { + OWSFailDebug(@"Received message of type: %@", NSStringFromClass(quotedMessage.class)); + return nil; + } + return [[TSQuotedMessage alloc] initWithTimestamp:quotedMessage.timestamp - authorAddress:proto.authorAddress + authorAddress:address body:body bodyRanges:bodyRanges bodySource:TSQuotedMessageContentSourceLocal @@ -359,7 +395,8 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { contentType:thumbnailProto.contentType sourceFilename:thumbnailProto.fileName]; } else { - OWSFailDebug(@"Invalid remote thumbnail attachment."); + attachmentInfo = [[OWSAttachmentInfo alloc] initWithoutThumbnailWithContentType:thumbnailProto.contentType + sourceFilename:thumbnailProto.fileName]; } } @@ -378,6 +415,11 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { #pragma mark - Attachment (not necessarily with a thumbnail) +- (BOOL)hasAttachment +{ + return (self.quotedAttachment != nil); +} + - (nullable TSAttachment *)fetchThumbnailWithTransaction:(SDSAnyReadTransaction *)transaction { NSString *attachmentId = self.quotedAttachment.rawAttachmentId; @@ -385,14 +427,6 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { if (attachmentId) { attachment = [TSAttachment anyFetchWithUniqueId:self.quotedAttachment.rawAttachmentId transaction:transaction]; } - - // If we have an attachment stream, we should've already thumbnailed the image. - // TODO: This will fail if we've downloaded the original source media. We need to add a hook to flag - // that we need to thumbnail the source to our own local copy - // - // BOOL needsThumbnailing = (attachmentType == OWSAttachmentInfoReferenceUntrustedPointer) || - // (attachmentType == OWSAttachmentInfoReferenceOriginal); - // OWSAssertDebug(![attachment isKindOfClass:[TSAttachmentStream class]] || !needsThumbnailing); return attachment; } @@ -402,17 +436,17 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { || self.quotedAttachment.attachmentType == OWSAttachmentInfoReferenceThumbnail); } -- (NSString *)contentType +- (nullable NSString *)contentType { return self.quotedAttachment.contentType; } -- (NSString *)sourceFilename +- (nullable NSString *)sourceFilename { return self.quotedAttachment.sourceFilename; } -- (NSString *)thumbnailAttachmentId +- (nullable NSString *)thumbnailAttachmentId { return self.quotedAttachment.rawAttachmentId; } @@ -432,25 +466,35 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { - (nullable TSAttachmentStream *)createThumbnailIfNecessaryWithTransaction:(SDSAnyWriteTransaction *)transaction { + // We want to clone the existing attachment to a new attachment if necessary. This means: + // - Fetching the attachment and making sure it's an attachment stream + // - If we already own the attachment, we've already cloned it! + // - Otherwise, we should copy the attachment stream to a new attachment + // - Updating our state to now point to the new attachment NSString *_Nullable attachmentId = self.quotedAttachment.rawAttachmentId; if (!attachmentId) { return nil; } + TSAttachment *_Nullable attachment = [TSAttachment anyFetchWithUniqueId:attachmentId transaction:transaction]; + if (![attachment isKindOfClass:[TSAttachmentStream class]]) { + // Nothing to clone + return nil; + } + TSAttachmentStream *attachmentStream = (TSAttachmentStream *)attachment; TSAttachmentStream *_Nullable thumbnail = nil; - TSAttachmentStream *_Nullable attachment = [TSAttachmentStream anyFetchAttachmentStreamWithUniqueId:attachmentId - transaction:transaction]; - if (attachment && self.quotedAttachment.attachmentType == OWSAttachmentInfoReferenceOriginalForSend) { - OWSLogInfo(@"Cloning source attachment to thumbnail for send."); - thumbnail = [attachment cloneAsThumbnail]; + + // We don't expect to be here in this state. Remote pointers are set via -setThumbnailAttachmentStream: + // If we have a stream and we're still in this state, something went wrong. + OWSAssertDebug(self.quotedAttachment.attachmentType != OWSAttachmentInfoReferenceUntrustedPointer); + if (!self.isThumbnailOwned) { + OWSLogInfo(@"Cloning attachment to thumbnail"); + thumbnail = [attachmentStream cloneAsThumbnail]; [thumbnail anyInsertWithTransaction:transaction]; self.quotedAttachment.rawAttachmentId = thumbnail.uniqueId; self.quotedAttachment.attachmentType = OWSAttachmentInfoReferenceThumbnail; - - } else if (attachment && self.quotedAttachment.attachmentType == OWSAttachmentInfoReferenceThumbnail) { - thumbnail = attachment; } else { - OWSFailDebug(@"Unexpected attachment state. Current state: %lu. Fetched: %@", self.quotedAttachment.attachmentType, attachment.uniqueId);; + thumbnail = attachmentStream; } return thumbnail; } diff --git a/SignalServiceKit/src/Messages/MessageSender.m b/SignalServiceKit/src/Messages/MessageSender.m index afe97ec3f1..b6e3b31de0 100644 --- a/SignalServiceKit/src/Messages/MessageSender.m +++ b/SignalServiceKit/src/Messages/MessageSender.m @@ -1479,7 +1479,7 @@ NSString *const MessageSenderSpamChallengeResolvedException = @"SpamChallengeRes [attachmentIds addObjectsFromArray:message.attachmentIds]; } - if (message.quotedMessage) { + if (message.quotedMessage.thumbnailAttachmentId) { // We need to update the message record here to reflect the new attachments we may create. [message anyUpdateOutgoingMessageWithTransaction:transaction block:^(TSOutgoingMessage *message) { From 19e5506dde0f3190de451da599e1c3a495ac4ce5 Mon Sep 17 00:00:00 2001 From: Michelle Linington Date: Thu, 19 Aug 2021 23:46:06 -0700 Subject: [PATCH 7/7] Lint --- .../ViewModels/OWSQuotedReplyModel.h | 5 +-- .../ViewModels/OWSQuotedReplyModel.m | 40 +++++++++---------- .../src/Messages/Interactions/TSMessage.m | 16 ++++---- .../Messages/Interactions/TSOutgoingMessage.m | 4 +- .../Messages/Interactions/TSQuotedMessage.h | 3 +- .../Messages/Interactions/TSQuotedMessage.m | 7 ++-- 6 files changed, 38 insertions(+), 37 deletions(-) diff --git a/SignalMessaging/ViewModels/OWSQuotedReplyModel.h b/SignalMessaging/ViewModels/OWSQuotedReplyModel.h index 825b59c371..729f90f745 100644 --- a/SignalMessaging/ViewModels/OWSQuotedReplyModel.h +++ b/SignalMessaging/ViewModels/OWSQuotedReplyModel.h @@ -1,5 +1,5 @@ // -// Copyright (c) 2020 Open Whisper Systems. All rights reserved. +// Copyright (c) 2021 Open Whisper Systems. All rights reserved. // #import "CVItemViewModel.h" @@ -49,8 +49,7 @@ NS_ASSUME_NONNULL_BEGIN - (instancetype)init NS_UNAVAILABLE; // Used for persisted quoted replies, both incoming and outgoing. -+ (nullable instancetype)quotedReplyFromMessage:(TSMessage *)message - transaction:(SDSAnyReadTransaction *)transaction; ++ (nullable instancetype)quotedReplyFromMessage:(TSMessage *)message transaction:(SDSAnyReadTransaction *)transaction; // Builds a not-yet-sent QuotedReplyModel + (nullable instancetype)quotedReplyForSendingWithItem:(id)item diff --git a/SignalMessaging/ViewModels/OWSQuotedReplyModel.m b/SignalMessaging/ViewModels/OWSQuotedReplyModel.m index 55c4e8f390..c585c95ea0 100644 --- a/SignalMessaging/ViewModels/OWSQuotedReplyModel.m +++ b/SignalMessaging/ViewModels/OWSQuotedReplyModel.m @@ -22,15 +22,16 @@ NS_ASSUME_NONNULL_BEGIN @property (nonatomic, readonly) TSQuotedMessageContentSource bodySource; - (instancetype)initWithTimestamp:(uint64_t)timestamp - authorAddress:(SignalServiceAddress *)authorAddress - body:(nullable NSString *)body - bodyRanges:(nullable MessageBodyRanges *)bodyRanges - bodySource:(TSQuotedMessageContentSource)bodySource - thumbnailImage:(nullable UIImage *)thumbnailImage - contentType:(nullable NSString *)contentType - sourceFilename:(nullable NSString *)sourceFilename - attachmentStream:(nullable TSAttachmentStream *)attachmentStream - failedThumbnailAttachmentPointer:(nullable TSAttachmentPointer *)thumbnailAttachmentPointer NS_DESIGNATED_INITIALIZER; + authorAddress:(SignalServiceAddress *)authorAddress + body:(nullable NSString *)body + bodyRanges:(nullable MessageBodyRanges *)bodyRanges + bodySource:(TSQuotedMessageContentSource)bodySource + thumbnailImage:(nullable UIImage *)thumbnailImage + contentType:(nullable NSString *)contentType + sourceFilename:(nullable NSString *)sourceFilename + attachmentStream:(nullable TSAttachmentStream *)attachmentStream + failedThumbnailAttachmentPointer:(nullable TSAttachmentPointer *)failedThumbnailAttachmentPointer + NS_DESIGNATED_INITIALIZER; @end @@ -40,15 +41,15 @@ NS_ASSUME_NONNULL_BEGIN #pragma mark - Initializers - (instancetype)initWithTimestamp:(uint64_t)timestamp - authorAddress:(SignalServiceAddress *)authorAddress - body:(nullable NSString *)body - bodyRanges:(nullable MessageBodyRanges *)bodyRanges - bodySource:(TSQuotedMessageContentSource)bodySource - thumbnailImage:(nullable UIImage *)thumbnailImage - contentType:(nullable NSString *)contentType - sourceFilename:(nullable NSString *)sourceFilename - attachmentStream:(nullable TSAttachmentStream *)attachmentStream - failedThumbnailAttachmentPointer:(nullable TSAttachmentPointer *)failedThumbnailAttachmentPointer + authorAddress:(SignalServiceAddress *)authorAddress + body:(nullable NSString *)body + bodyRanges:(nullable MessageBodyRanges *)bodyRanges + bodySource:(TSQuotedMessageContentSource)bodySource + thumbnailImage:(nullable UIImage *)thumbnailImage + contentType:(nullable NSString *)contentType + sourceFilename:(nullable NSString *)sourceFilename + attachmentStream:(nullable TSAttachmentStream *)attachmentStream + failedThumbnailAttachmentPointer:(nullable TSAttachmentPointer *)failedThumbnailAttachmentPointer { self = [super init]; if (!self) { @@ -71,8 +72,7 @@ NS_ASSUME_NONNULL_BEGIN #pragma mark - Factory Methods -+ (nullable instancetype)quotedReplyFromMessage:(TSMessage *)message - transaction:(SDSAnyReadTransaction *)transaction ++ (nullable instancetype)quotedReplyFromMessage:(TSMessage *)message transaction:(SDSAnyReadTransaction *)transaction { TSQuotedMessage *quotedMessage = message.quotedMessage; if (!quotedMessage) { diff --git a/SignalServiceKit/src/Messages/Interactions/TSMessage.m b/SignalServiceKit/src/Messages/Interactions/TSMessage.m index 7b9d9461cd..8b30e252a2 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSMessage.m @@ -702,11 +702,14 @@ static const NSUInteger OWSMessageSchemaVersion = 4; // We should clone the attachment if it's been downloaded but our quotedMessage doesn't have its own copy. BOOL needsClone = [attachment isKindOfClass:[TSAttachmentStream class]] && !self.quotedMessage.isThumbnailOwned; - TSAttachment *(^saveUpdatedThumbnail)(SDSAnyWriteTransaction *) = ^TSAttachment *(SDSAnyWriteTransaction *writeTx) { + TSAttachment * (^saveUpdatedThumbnail)(SDSAnyWriteTransaction *) = ^TSAttachment *(SDSAnyWriteTransaction *writeTx) + { __block TSAttachment *_Nullable attachment = nil; - [self anyUpdateMessageWithTransaction:writeTx block:^(TSMessage *message) { - attachment = [message.quotedMessage createThumbnailIfNecessaryWithTransaction:writeTx]; - }]; + [self anyUpdateMessageWithTransaction:writeTx + block:^(TSMessage *message) { + attachment = [message.quotedMessage + createThumbnailIfNecessaryWithTransaction:writeTx]; + }]; return attachment; }; @@ -715,9 +718,8 @@ static const NSUInteger OWSMessageSchemaVersion = 4; if (needsClone && [transaction isKindOfClass:[SDSAnyWriteTransaction class]]) { attachment = saveUpdatedThumbnail((SDSAnyWriteTransaction *)transaction); } else if (needsClone) { - DatabaseStorageAsyncWrite(self.databaseStorage, ^(SDSAnyWriteTransaction *writeTx){ - saveUpdatedThumbnail(writeTx); - }); + DatabaseStorageAsyncWrite( + self.databaseStorage, ^(SDSAnyWriteTransaction *writeTx) { saveUpdatedThumbnail(writeTx); }); } return attachment; } diff --git a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m index f9665525d5..95fa75d7f4 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m @@ -1407,7 +1407,7 @@ NSUInteger const TSOutgoingMessageSchemaVersion = 1; if (quotedMessage.hasAttachment) { SSKProtoDataMessageQuoteQuotedAttachmentBuilder *quotedAttachmentBuilder = - [SSKProtoDataMessageQuoteQuotedAttachment builder]; + [SSKProtoDataMessageQuoteQuotedAttachment builder]; quotedAttachmentBuilder.contentType = quotedMessage.contentType; quotedAttachmentBuilder.fileName = quotedMessage.sourceFilename; @@ -1421,7 +1421,7 @@ NSUInteger const TSOutgoingMessageSchemaVersion = 1; NSError *error; SSKProtoDataMessageQuoteQuotedAttachment *_Nullable quotedAttachmentMessage = - [quotedAttachmentBuilder buildAndReturnError:&error]; + [quotedAttachmentBuilder buildAndReturnError:&error]; if (error || !quotedAttachmentMessage) { OWSFailDebug(@"could not build protobuf: %@", error); return nil; diff --git a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h index 6c1cbb6881..14aca84453 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h +++ b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.h @@ -37,7 +37,8 @@ typedef NS_ENUM(NSUInteger, TSQuotedMessageContentSource) { @property (nonatomic, readonly) BOOL hasAttachment; -/// Returns YES if the thumbnail is something maintained by the quoted reply itself (as opposed to to media in some other message) +/// Returns YES if the thumbnail is something maintained by the quoted reply itself (as opposed to to media in some +/// other message) @property (nonatomic, readonly) BOOL isThumbnailOwned; @property (nonatomic, readonly, nullable) NSString *thumbnailAttachmentId; @property (nonatomic, readonly, nullable) NSString *contentType; diff --git a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m index 46924db82c..f1199d2b02 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSQuotedMessage.m @@ -44,8 +44,7 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { @implementation OWSAttachmentInfo -- (instancetype)initWithoutThumbnailWithContentType:(NSString *)contentType - sourceFilename:(NSString *)sourceFilename +- (instancetype)initWithoutThumbnailWithContentType:(NSString *)contentType sourceFilename:(NSString *)sourceFilename { self = [super init]; if (self) { @@ -286,8 +285,8 @@ typedef NS_ENUM(NSUInteger, OWSAttachmentInfoReference) { { if (quotedMessage.isViewOnceMessage) { // We construct a quote that does not include any of the quoted message's renderable content. - NSString *body - = NSLocalizedString(@"PER_MESSAGE_EXPIRATION_NOT_VIEWABLE", @"inbox cell and notification text for an already viewed view-once media message."); + NSString *body = NSLocalizedString(@"PER_MESSAGE_EXPIRATION_NOT_VIEWABLE", + @"inbox cell and notification text for an already viewed view-once media message."); return [[TSQuotedMessage alloc] initWithTimestamp:quotedMessage.timestamp authorAddress:proto.authorAddress body:body