From 96b5f22799853e5391c894fac0414e010f99a607 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 15 Feb 2018 11:02:26 -0500 Subject: [PATCH 1/4] Improve handling of attachments with captions. --- .../src/Messages/Interactions/TSMessage.m | 22 ++++++++++++++----- .../src/Messages/OWSMessageSender.m | 5 +++-- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/SignalServiceKit/src/Messages/Interactions/TSMessage.m b/SignalServiceKit/src/Messages/Interactions/TSMessage.m index 13f5512f7..cc6af66cc 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSMessage.m @@ -227,7 +227,11 @@ static const NSUInteger OWSMessageSchemaVersion = 4; - (NSString *)debugDescription { - if ([self hasAttachments]) { + if ([self hasAttachments] && self.body.length > 0) { + NSString *attachmentId = self.attachmentIds[0]; + return [NSString + stringWithFormat:@"Media Message with attachmentId: %@ and caption: '%@'", attachmentId, self.body]; + } else if ([self hasAttachments]) { NSString *attachmentId = self.attachmentIds[0]; return [NSString stringWithFormat:@"Media Message with attachmentId:%@", attachmentId]; } else { @@ -237,7 +241,10 @@ static const NSUInteger OWSMessageSchemaVersion = 4; - (NSString *)previewTextWithTransaction:(YapDatabaseReadTransaction *)transaction { - if ([self hasAttachments]) { + if (self.body.length > 0) { + // Use the message text/caption, if any. + return self.body; + } else if ([self hasAttachments]) { NSString *attachmentId = self.attachmentIds[0]; TSAttachment *attachment = [TSAttachment fetchObjectWithUniqueID:attachmentId transaction:transaction]; if (attachment) { @@ -246,14 +253,18 @@ static const NSUInteger OWSMessageSchemaVersion = 4; return NSLocalizedString(@"UNKNOWN_ATTACHMENT_LABEL", @"In Inbox view, last message label for thread with corrupted attachment."); } } else { - return self.body; + // TODO: We should do better here. + return @""; } } // TODO deprecate this and implement something like previewTextWithTransaction: for all TSInteractions - (NSString *)description { - if ([self hasAttachments]) { + if (self.body.length > 0) { + // Use the message text/caption, if any. + return self.body; + } else if ([self hasAttachments]) { NSString *attachmentId = self.attachmentIds[0]; TSAttachment *attachment = [TSAttachment fetchObjectWithUniqueID:attachmentId]; if (attachment) { @@ -262,7 +273,8 @@ static const NSUInteger OWSMessageSchemaVersion = 4; return NSLocalizedString(@"UNKNOWN_ATTACHMENT_LABEL", @"In Inbox view, last message label for thread with corrupted attachment."); } } else { - return self.body; + // TODO: We should do better here. + return @""; } } diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index cb1da141c..0d0d181a9 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -1153,8 +1153,9 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; [self handleMessageSentLocally:outgoingMessage]; if (!(outgoingMessage.body || outgoingMessage.hasAttachments)) { - DDLogDebug( - @"%@ Refusing to make incoming copy of non-standard message sent to self:%@", self.logTag, outgoingMessage); + DDLogDebug(@"%@ Refusing to make incoming copy of non-standard message sent to self: %@", + self.logTag, + outgoingMessage); return; } From 8576da791c4b8b420eba08271c5d53aa107481b4 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 15 Feb 2018 15:15:23 -0500 Subject: [PATCH 2/4] Improve handling of attachments with captions. --- .../src/Messages/Interactions/TSMessage.m | 33 +++++++++++++++---- .../src/Messages/OWSMessageSender.m | 6 +++- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/SignalServiceKit/src/Messages/Interactions/TSMessage.m b/SignalServiceKit/src/Messages/Interactions/TSMessage.m index cc6af66cc..8d2f09126 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSMessage.m @@ -3,6 +3,7 @@ // #import "TSMessage.h" +#import "AppContext.h" #import "NSDate+OWS.h" #import "NSString+SSK.h" #import "TSAttachment.h" @@ -241,18 +242,37 @@ static const NSUInteger OWSMessageSchemaVersion = 4; - (NSString *)previewTextWithTransaction:(YapDatabaseReadTransaction *)transaction { - if (self.body.length > 0) { - // Use the message text/caption, if any. - return self.body; - } else if ([self hasAttachments]) { + NSString *_Nullable attachmentDescription = nil; + if ([self hasAttachments]) { NSString *attachmentId = self.attachmentIds[0]; TSAttachment *attachment = [TSAttachment fetchObjectWithUniqueID:attachmentId transaction:transaction]; if (attachment) { - return attachment.description; + attachmentDescription = attachment.description; } else { - return NSLocalizedString(@"UNKNOWN_ATTACHMENT_LABEL", @"In Inbox view, last message label for thread with corrupted attachment."); + attachmentDescription = NSLocalizedString(@"UNKNOWN_ATTACHMENT_LABEL", + @"In Inbox view, last message label for thread with corrupted attachment."); + } + } + + NSString *_Nullable bodyDescription = nil; + if (self.body.length > 0) { + // TODO: Filter this text using something like DisplayableText. + bodyDescription = self.body; + } + + if (attachmentDescription.length > 0 && bodyDescription.length > 0) { + // Attachment with caption. + if ([CurrentAppContext() isRTL]) { + return [[bodyDescription stringByAppendingString:@": "] stringByAppendingString:attachmentDescription]; + } else { + return [[attachmentDescription stringByAppendingString:@": "] stringByAppendingString:bodyDescription]; } + } else if (bodyDescription.length > 0) { + return bodyDescription; + } else if (attachmentDescription.length > 0) { + return attachmentDescription; } else { + OWSFail(@"%@ message has neither body nor attachment.", self.logTag); // TODO: We should do better here. return @""; } @@ -273,6 +293,7 @@ static const NSUInteger OWSMessageSchemaVersion = 4; return NSLocalizedString(@"UNKNOWN_ATTACHMENT_LABEL", @"In Inbox view, last message label for thread with corrupted attachment."); } } else { + OWSFail(@"%@ message has neither body nor attachment.", self.logTag); // TODO: We should do better here. return @""; } diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index 0d0d181a9..bbdb82ccf 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -1153,7 +1153,11 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; [self handleMessageSentLocally:outgoingMessage]; if (!(outgoingMessage.body || outgoingMessage.hasAttachments)) { - DDLogDebug(@"%@ Refusing to make incoming copy of non-standard message sent to self: %@", + // We only want to "clone" text and attachment messages. + // + // This method shouldn't be called for sync messages, so this + // probably represents a bug. + OWSFail(@"%@ Refusing to make incoming copy of non-standard message sent to self: %@", self.logTag, outgoingMessage); return; From 6006d22870d17bd9300befcf7b7e7f54f0ea3882 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 15 Feb 2018 15:16:37 -0500 Subject: [PATCH 3/4] Improve handling of attachments with captions. --- SignalServiceKit/src/Messages/Interactions/TSMessage.m | 1 + 1 file changed, 1 insertion(+) diff --git a/SignalServiceKit/src/Messages/Interactions/TSMessage.m b/SignalServiceKit/src/Messages/Interactions/TSMessage.m index 8d2f09126..8c91b1682 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSMessage.m @@ -240,6 +240,7 @@ static const NSUInteger OWSMessageSchemaVersion = 4; } } +// TODO: This method contains view-specific logic and probably belongs in NotificationsManager, not in SSK. - (NSString *)previewTextWithTransaction:(YapDatabaseReadTransaction *)transaction { NSString *_Nullable attachmentDescription = nil; From 10ca369da8aef09cfbd4e1c3c488fe6bfa4de33c Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 22 Feb 2018 16:12:39 -0500 Subject: [PATCH 4/4] Respond to CR. --- .../src/Messages/Interactions/TSMessage.m | 21 +++++-------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/SignalServiceKit/src/Messages/Interactions/TSMessage.m b/SignalServiceKit/src/Messages/Interactions/TSMessage.m index 8c91b1682..8b8f425ab 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSMessage.m @@ -282,22 +282,11 @@ static const NSUInteger OWSMessageSchemaVersion = 4; // TODO deprecate this and implement something like previewTextWithTransaction: for all TSInteractions - (NSString *)description { - if (self.body.length > 0) { - // Use the message text/caption, if any. - return self.body; - } else if ([self hasAttachments]) { - NSString *attachmentId = self.attachmentIds[0]; - TSAttachment *attachment = [TSAttachment fetchObjectWithUniqueID:attachmentId]; - if (attachment) { - return attachment.description; - } else { - return NSLocalizedString(@"UNKNOWN_ATTACHMENT_LABEL", @"In Inbox view, last message label for thread with corrupted attachment."); - } - } else { - OWSFail(@"%@ message has neither body nor attachment.", self.logTag); - // TODO: We should do better here. - return @""; - } + __block NSString *result; + [self.dbReadConnection readWithBlock:^(YapDatabaseReadTransaction *transaction) { + result = [self previewTextWithTransaction:transaction]; + }]; + return result; } - (void)removeWithTransaction:(YapDatabaseReadWriteTransaction *)transaction