From bd370f1de497ccd790325b40bf09e5dd586ad5bc Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Thu, 3 Nov 2016 14:20:38 -0400 Subject: [PATCH] Fix cropped Chinese/Japanese messages The earlier fix for the broken ios10 emoji font ended up breaking messages for some users with a tall font. Here we have a lighter touch - ensuring we don't touch messages that don't use emoji. Also, introduce a different approach to the fix, rather than trying to compute the bounding rect of an appropriately attributed string, just add an extra bit of height per line. This approach isn't ideal for long messages with only one emoji line in them, but the previous approach was incompatible with Chinese messages that also contain emoji. See the new `MesssagesBubblesSizeCalculatorTest.swift` for test cases considered. // FREEBIE --- Signal.xcodeproj/project.pbxproj | 4 + .../Models/OWSMessagesBubblesSizeCalculator.h | 5 + .../Models/OWSMessagesBubblesSizeCalculator.m | 144 ++++++------------ .../view controllers/MessagesViewController.m | 29 ---- .../MesssagesBubblesSizeCalculatorTest.swift | 111 ++++++++++++++ Signal/test/SignalTests-Bridging-Header.h | 3 + 6 files changed, 170 insertions(+), 126 deletions(-) create mode 100644 Signal/test/Models/MesssagesBubblesSizeCalculatorTest.swift diff --git a/Signal.xcodeproj/project.pbxproj b/Signal.xcodeproj/project.pbxproj index 5e577d33d..07318fd74 100644 --- a/Signal.xcodeproj/project.pbxproj +++ b/Signal.xcodeproj/project.pbxproj @@ -18,6 +18,7 @@ 451DE9FD1DC1A28200810E42 /* SyncPushTokensJob.swift in Sources */ = {isa = PBXBuildFile; fileRef = 451DE9FC1DC1A28200810E42 /* SyncPushTokensJob.swift */; }; 451DE9FE1DC1A28200810E42 /* SyncPushTokensJob.swift in Sources */ = {isa = PBXBuildFile; fileRef = 451DE9FC1DC1A28200810E42 /* SyncPushTokensJob.swift */; }; 4520D8D51D417D8E00123472 /* Photos.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 4520D8D41D417D8E00123472 /* Photos.framework */; }; + 452D1EE81DCA90D100A57EC4 /* MesssagesBubblesSizeCalculatorTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 452D1EE71DCA90D100A57EC4 /* MesssagesBubblesSizeCalculatorTest.swift */; }; 452E3C8E1D935C77002A45B0 /* OWSConversationSettingsTableViewController.m in Sources */ = {isa = PBXBuildFile; fileRef = 452E3C8D1D935C77002A45B0 /* OWSConversationSettingsTableViewController.m */; }; 452E3C8F1D935C77002A45B0 /* OWSConversationSettingsTableViewController.m in Sources */ = {isa = PBXBuildFile; fileRef = 452E3C8D1D935C77002A45B0 /* OWSConversationSettingsTableViewController.m */; }; 453D28B71D32BA5F00D523F0 /* OWSDisplayedMessage.m in Sources */ = {isa = PBXBuildFile; fileRef = 453D28B61D32BA5F00D523F0 /* OWSDisplayedMessage.m */; }; @@ -544,6 +545,7 @@ 451DE9FC1DC1A28200810E42 /* SyncPushTokensJob.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = SyncPushTokensJob.swift; sourceTree = ""; }; 4520D8D41D417D8E00123472 /* Photos.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Photos.framework; path = System/Library/Frameworks/Photos.framework; sourceTree = SDKROOT; }; 4526BD481CA61C8D00166BC8 /* OWSMessageEditing.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = OWSMessageEditing.h; sourceTree = ""; }; + 452D1EE71DCA90D100A57EC4 /* MesssagesBubblesSizeCalculatorTest.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; name = MesssagesBubblesSizeCalculatorTest.swift; path = Models/MesssagesBubblesSizeCalculatorTest.swift; sourceTree = ""; }; 452E3C8C1D935C77002A45B0 /* OWSConversationSettingsTableViewController.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = OWSConversationSettingsTableViewController.h; sourceTree = ""; }; 452E3C8D1D935C77002A45B0 /* OWSConversationSettingsTableViewController.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; lineEnding = 0; path = OWSConversationSettingsTableViewController.m; sourceTree = ""; xcLanguageSpecificationIdentifier = xcode.lang.objc; }; 453CC0361D08E1A60040EBA3 /* sn */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = sn; path = translations/sn.lproj/Localizable.strings; sourceTree = ""; }; @@ -1226,6 +1228,7 @@ children = ( 458E38391D6699FA0094BD24 /* OWSDeviceProvisioningURLParserTest.m */, 458967101DC117CC00E9DD21 /* AccountManagerTest.swift */, + 452D1EE71DCA90D100A57EC4 /* MesssagesBubblesSizeCalculatorTest.swift */, ); name = Models; sourceTree = ""; @@ -3037,6 +3040,7 @@ B660F72E1C29988E00687D6E /* HttpManager.m in Sources */, 458E383A1D6699FA0094BD24 /* OWSDeviceProvisioningURLParserTest.m in Sources */, B660F72F1C29988E00687D6E /* HttpSocket.m in Sources */, + 452D1EE81DCA90D100A57EC4 /* MesssagesBubblesSizeCalculatorTest.swift in Sources */, B660F7301C29988E00687D6E /* IpAddress.m in Sources */, B660F7311C29988E00687D6E /* IpEndPoint.m in Sources */, B660F7321C29988E00687D6E /* PacketHandler.m in Sources */, diff --git a/Signal/src/Models/OWSMessagesBubblesSizeCalculator.h b/Signal/src/Models/OWSMessagesBubblesSizeCalculator.h index af1419212..bbce01dd6 100644 --- a/Signal/src/Models/OWSMessagesBubblesSizeCalculator.h +++ b/Signal/src/Models/OWSMessagesBubblesSizeCalculator.h @@ -2,6 +2,11 @@ #import +NS_SWIFT_NAME(MessagesBubblesSizeCalculator) @interface OWSMessagesBubblesSizeCalculator : JSQMessagesBubblesSizeCalculator +- (CGSize)messageBubbleSizeForMessageData:(id)messageData + atIndexPath:(NSIndexPath *)indexPath + withLayout:(JSQMessagesCollectionViewFlowLayout *)layout; + @end diff --git a/Signal/src/Models/OWSMessagesBubblesSizeCalculator.m b/Signal/src/Models/OWSMessagesBubblesSizeCalculator.m index dc34fb5cf..cabbcd87c 100644 --- a/Signal/src/Models/OWSMessagesBubblesSizeCalculator.m +++ b/Signal/src/Models/OWSMessagesBubblesSizeCalculator.m @@ -10,9 +10,10 @@ NS_ASSUME_NONNULL_BEGIN -// BEGIN HACK iOS10EmojiBug see: https://github.com/WhisperSystems/Signal-iOS/issues/1368 -// superclass protected methods we need in order to compute bubble size. -@interface OWSMessagesBubblesSizeCalculator (OWSiOS10EmojiBug) +/** + * We use some private method to size our info messages. + */ +@interface OWSMessagesBubblesSizeCalculator (JSQPrivateMethods) @property (strong, nonatomic, readonly) NSCache *cache; @property (assign, nonatomic, readonly) NSUInteger minimumBubbleWidth; @@ -24,7 +25,6 @@ NS_ASSUME_NONNULL_BEGIN withLayout:(JSQMessagesCollectionViewFlowLayout *)layout; - (CGFloat)textBubbleWidthForLayout:(JSQMessagesCollectionViewFlowLayout *)layout; @end -// END HACK iOS10EmojiBug see: https://github.com/WhisperSystems/Signal-iOS/issues/1368 @implementation OWSMessagesBubblesSizeCalculator @@ -56,9 +56,7 @@ NS_ASSUME_NONNULL_BEGIN CGSize size; // BEGIN HACK iOS10EmojiBug see: https://github.com/WhisperSystems/Signal-iOS/issues/1368 - BOOL isIOS10OrGreater = - [[NSProcessInfo processInfo] isOperatingSystemAtLeastVersion:(NSOperatingSystemVersion){.majorVersion = 10 }]; - if (isIOS10OrGreater) { + if ([self shouldApplyiOS10EmojiFixToString:messageData.text font:layout.messageBubbleFont]) { size = [self withiOS10EmojiFixSuperMessageBubbleSizeForMessageData:messageData atIndexPath:indexPath withLayout:layout]; @@ -67,108 +65,60 @@ NS_ASSUME_NONNULL_BEGIN } // END HACK iOS10EmojiBug see: https://github.com/WhisperSystems/Signal-iOS/issues/1368 + return CGSizeMake(size.width, size.height); +} +/** + * Emoji sizing bug only affects iOS10. Unfortunately the "fix" for emoji font breaks some other fonts, so it's + * important + * to only apply it when emoji is actually present. + */ +- (BOOL)shouldApplyiOS10EmojiFixToString:(NSString *)string font:(UIFont *)font +{ + BOOL isIOS10OrGreater = + [[NSProcessInfo processInfo] isOperatingSystemAtLeastVersion:(NSOperatingSystemVersion){.majorVersion = 10 }]; + if (!isIOS10OrGreater) { + return NO; + } - return size; + __block BOOL foundEmoji = NO; + NSDictionary *attributes = @{ NSFontAttributeName : font }; + + NSMutableAttributedString *attributedString = + [[NSMutableAttributedString alloc] initWithString:string attributes:attributes]; + [attributedString fixAttributesInRange:NSMakeRange(0, string.length)]; + [attributedString enumerateAttribute:NSFontAttributeName + inRange:NSMakeRange(0, string.length) + options:0 + usingBlock:^(id _Nullable value, NSRange range, BOOL *_Nonnull stop) { + UIFont *rangeFont = (UIFont *)value; + if ([rangeFont.fontName isEqualToString:@".AppleColorEmojiUI"]) { + DDLogVerbose(@"Detected Emoji at location: %lu, for length: %lu", + (unsigned long)range.location, + (unsigned long)range.length); + foundEmoji = YES; + *stop = YES; + } + }]; + + return foundEmoji; } /** * HACK iOS10EmojiBug see: https://github.com/WhisperSystems/Signal-iOS/issues/1368 - * iOS10 bug in rendering emoji requires to fudge some things in the middle of the super method. - * Copy/pasted the superclass method and inlined (and marked) our hacks inline. + * As of iOS10.0 the UIEmoji font doesn't present proper line heights. In some cases this causes the last line in a + * message to get cropped off. */ - (CGSize)withiOS10EmojiFixSuperMessageBubbleSizeForMessageData:(id)messageData atIndexPath:(NSIndexPath *)indexPath withLayout:(JSQMessagesCollectionViewFlowLayout *)layout { - NSValue *cachedSize = [self.cache objectForKey:@([messageData messageHash])]; - if (cachedSize != nil) { - return [cachedSize CGSizeValue]; - } - - CGSize finalSize = CGSizeZero; - - if ([messageData isMediaMessage]) { - finalSize = [[messageData media] mediaViewDisplaySize]; - } else { - CGSize avatarSize = [self jsq_avatarSizeForMessageData:messageData withLayout:layout]; + UIFont *emojiFont = [UIFont fontWithName:@".AppleColorEmojiUI" size:layout.messageBubbleFont.pointSize]; + CGSize superSize = [super messageBubbleSizeForMessageData:messageData atIndexPath:indexPath withLayout:layout]; + int lines = (int)floor(superSize.height / emojiFont.lineHeight); - // from the cell xibs, there is a 2 point space between avatar and bubble - CGFloat spacingBetweenAvatarAndBubble = 2.0f; - CGFloat horizontalContainerInsets = layout.messageBubbleTextViewTextContainerInsets.left - + layout.messageBubbleTextViewTextContainerInsets.right; - CGFloat horizontalFrameInsets - = layout.messageBubbleTextViewFrameInsets.left + layout.messageBubbleTextViewFrameInsets.right; - - CGFloat horizontalInsetsTotal - = horizontalContainerInsets + horizontalFrameInsets + spacingBetweenAvatarAndBubble; - CGFloat maximumTextWidth = [self textBubbleWidthForLayout:layout] - avatarSize.width - - layout.messageBubbleLeftRightMargin - horizontalInsetsTotal; - - /////////////////// - // BEGIN HACK iOS10EmojiBug see: https://github.com/WhisperSystems/Signal-iOS/issues/1368 - - // //stringRect doesn't give the correct size with the new emoji font. - // CGRect stringRect = [[messageData text] boundingRectWithSize:CGSizeMake(maximumTextWidth, CGFLOAT_MAX) - // options:(NSStringDrawingUsesLineFragmentOrigin | - // NSStringDrawingUsesFontLeading) - // attributes:@{ NSFontAttributeName : - // layout.messageBubbleFont } - // context:nil]; - - CGRect stringRect; - if (!messageData.text) { - stringRect = CGRectZero; - } else { - NSDictionary *attributes = @{ NSFontAttributeName : layout.messageBubbleFont }; - NSMutableAttributedString *string = - [[NSMutableAttributedString alloc] initWithString:[messageData text] attributes:attributes]; - [string fixAttributesInRange:NSMakeRange(0, string.length)]; - [string - enumerateAttribute:NSFontAttributeName - inRange:NSMakeRange(0, string.length) - options:0 - usingBlock:^(id _Nullable value, NSRange range, BOOL *_Nonnull stop) { - UIFont *font = (UIFont *)value; - if ([font.fontName isEqualToString:@".AppleColorEmojiUI"]) { - DDLogVerbose(@"Replacing new broken emoji font with old emoji font at location: %lu, " - @"for length: %lu", - (unsigned long)range.location, - (unsigned long)range.length); - [string addAttribute:NSFontAttributeName - value:[UIFont fontWithName:@"AppleColorEmoji" size:font.pointSize] - range:range]; - } - }]; - stringRect = - [string boundingRectWithSize:CGSizeMake(maximumTextWidth, CGFLOAT_MAX) - options:(NSStringDrawingUsesLineFragmentOrigin | NSStringDrawingUsesFontLeading) - context:nil]; - } - // END HACK iOS10EmojiBug see: https://github.com/WhisperSystems/Signal-iOS/issues/1368 - ///////////////////////// - - CGSize stringSize = CGRectIntegral(stringRect).size; - - CGFloat verticalContainerInsets = layout.messageBubbleTextViewTextContainerInsets.top - + layout.messageBubbleTextViewTextContainerInsets.bottom; - CGFloat verticalFrameInsets - = layout.messageBubbleTextViewFrameInsets.top + layout.messageBubbleTextViewFrameInsets.bottom; - - // add extra 2 points of space (`self.additionalInset`), because `boundingRectWithSize:` is slightly off - // not sure why. magix. (shrug) if you know, submit a PR - CGFloat verticalInsets = verticalContainerInsets + verticalFrameInsets + self.additionalInset; - - // same as above, an extra 2 points of magix - CGFloat finalWidth - = MAX(stringSize.width + horizontalInsetsTotal, self.minimumBubbleWidth) + self.additionalInset; - - finalSize = CGSizeMake(finalWidth, stringSize.height + verticalInsets); - } - - [self.cache setObject:[NSValue valueWithCGSize:finalSize] forKey:@([messageData messageHash])]; - - return finalSize; + // Add an extra pixel per line to fit the emoji. + return CGSizeMake(superSize.width, superSize.height + lines); } diff --git a/Signal/src/view controllers/MessagesViewController.m b/Signal/src/view controllers/MessagesViewController.m index 49d3012cc..d81605f04 100644 --- a/Signal/src/view controllers/MessagesViewController.m +++ b/Signal/src/view controllers/MessagesViewController.m @@ -794,9 +794,6 @@ typedef enum : NSUInteger { { JSQMessagesCollectionViewCell *cell = (JSQMessagesCollectionViewCell *)[super collectionView:self.collectionView cellForItemAtIndexPath:indexPath]; - // BEGIN HACK iOS10EmojiBug see: https://github.com/WhisperSystems/Signal-iOS/issues/1368 - [self fixupiOS10EmojiBugForTextView:cell.textView]; - // END HACK iOS10EmojiBug see: https://github.com/WhisperSystems/Signal-iOS/issues/1368 if (!message.isMediaMessage) { cell.textView.textColor = [UIColor ows_blackColor]; cell.textView.linkTextAttributes = @{ @@ -815,10 +812,6 @@ typedef enum : NSUInteger { = (OWSOutgoingMessageCollectionViewCell *)[super collectionView:self.collectionView cellForItemAtIndexPath:indexPath]; - // BEGIN HACK iOS10EmojiBug see: https://github.com/WhisperSystems/Signal-iOS/issues/1368 - [self fixupiOS10EmojiBugForTextView:cell.textView]; - // END HACK iOS10EmojiBug see: https://github.com/WhisperSystems/Signal-iOS/issues/1368 - if (message.isMediaMessage) { if (![message isKindOfClass:[TSMessageAdapter class]]) { DDLogError(@"%@ Unexpected media message:%@", self.tag, message.class); @@ -835,28 +828,6 @@ typedef enum : NSUInteger { return cell; } -// BEGIN HACK iOS10EmojiBug see: https://github.com/WhisperSystems/Signal-iOS/issues/1368 -- (void)fixupiOS10EmojiBugForTextView:(UITextView *)textView -{ - BOOL isIOS10OrGreater = - [[NSProcessInfo processInfo] isOperatingSystemAtLeastVersion:(NSOperatingSystemVersion){.majorVersion = 10 }]; - if (isIOS10OrGreater) { - [textView.textStorage enumerateAttribute:NSFontAttributeName - inRange:NSMakeRange(0, textView.textStorage.length) - options:0 - usingBlock:^(id _Nullable value, NSRange range, BOOL *_Nonnull stop) { - UIFont *font = (UIFont *)value; - if ([font.fontName isEqualToString:@".AppleColorEmojiUI"]) { - [textView.textStorage addAttribute:NSFontAttributeName - value:[UIFont fontWithName:@"AppleColorEmoji" - size:font.pointSize] - range:range]; - } - }]; - } -} -// END HACK iOS10EmojiBug see: https://github.com/WhisperSystems/Signal-iOS/issues/1368 - - (OWSCallCollectionViewCell *)loadCallCellForCall:(OWSCall *)call atIndexPath:(NSIndexPath *)indexPath { OWSCallCollectionViewCell *callCell = [self.collectionView dequeueReusableCellWithReuseIdentifier:[OWSCallCollectionViewCell cellReuseIdentifier] diff --git a/Signal/test/Models/MesssagesBubblesSizeCalculatorTest.swift b/Signal/test/Models/MesssagesBubblesSizeCalculatorTest.swift new file mode 100644 index 000000000..0faae288e --- /dev/null +++ b/Signal/test/Models/MesssagesBubblesSizeCalculatorTest.swift @@ -0,0 +1,111 @@ +// Created by Michael Kirk on 11/2/16. +// Copyright © 2016 Open Whisper Systems. All rights reserved. + +import XCTest + +class FakeMessageData: NSObject, JSQMessageData { + public func senderId() -> String! { + return "fake-sender-id" + } + + func senderDisplayName() -> String! { + return "fake-senderDisplayName" + } + + func date() -> Date! { + return Date() + } + + @objc func isMediaMessage() -> Bool { + return false + } + + @objc func messageHash() -> UInt { + return 1 + } + + var bodyText: String = "fake message data text" + func text() -> String { + return self.bodyText + } + + init(text: String) { + self.bodyText = text; + } +} + +class FakeiPhone6JSQMessagesCollectionViewFlowLayout: JSQMessagesCollectionViewFlowLayout { + // This value was nabbed by inspecting the super class layout.itemSize while debugging the `messageBubbleSizeForMessageData`. + // It requires the view to actually be rendered to get a proper size, so we're baking it in here. + // This will break if we change the layout. + override var itemWidth: CGFloat { return 367 } +} + +/** + * This is a brittle test, which will break if our layout changes. It serves mostly as documentation for cases to + * consider when changing the bubble size calculator. Primarly these test cases came out of a bug introduced in iOS10, + * which prevents us from computing proper boudning box for text that uses the UIEmoji font. + * + * If one of these tests breaks, it should be OK to update the expected value so long as you've tested the result renders + * correctly in the running app (the reference sizes ewre computed in the context of an iphone6 layour. + * @see `FakeiPhone6JSQMessagesCollectionViewFlowLayout` + */ +class MesssagesBubblesSizeCalculatorTest: XCTestCase { + + let indexPath = IndexPath() + let layout = FakeiPhone6JSQMessagesCollectionViewFlowLayout() + let calculator = MessagesBubblesSizeCalculator() + + func testHeightForShort1LineMessage() { + let messageData = FakeMessageData(text:"foo") + let actual = calculator.messageBubbleSize(for: messageData, at: indexPath, with: layout) + XCTAssertEqual(38, actual.height); + } + + func testHeightForLong1LineMessage() { + let messageData = FakeMessageData(text:"1 2 3 4 5 6 7 8 9 10 11 12 13 14 x") + let actual = calculator.messageBubbleSize(for: messageData, at: indexPath, with: layout) + XCTAssertEqual(38, actual.height); + } + + func testHeightForShort2LineMessage() { + let messageData = FakeMessageData(text:"1 2 3 4 5 6 7 8 9 10 11 12 13 14 x 1") + let actual = calculator.messageBubbleSize(for: messageData, at: indexPath, with: layout) + XCTAssertEqual(59, actual.height); + } + + func testHeightForLong2LineMessage() { + let messageData = FakeMessageData(text:"1 2 3 4 5 6 7 8 9 10 11 12 13 14 x 1 2 3 4 5 6 7 8 9 10 11 12 13 14 x") + let actual = calculator.messageBubbleSize(for: messageData, at: indexPath, with: layout) + XCTAssertEqual(59, actual.height); + } + + func testHeightForiOS10EmojiBug() { + let messageData = FakeMessageData(text:"Wunderschönen Guten Morgaaaahhhn 😝 - hast du gut geschlafen ☺️😘") + let actual = calculator.messageBubbleSize(for: messageData, at: indexPath, with: layout) + + XCTAssertEqual(84, actual.height); + } + + func testHeightForChineseWithEmojiBug() { + let messageData = FakeMessageData(text:"一二三四五六七八九十甲乙丙😝戊己庚辛壬圭咖啡牛奶餅乾水果蛋糕") + let actual = calculator.messageBubbleSize(for: messageData, at: indexPath, with: layout) + // erroneously seeing 69 with the emoji fix in place. + XCTAssertEqual(84, actual.height); + } + + func testHeightForChineseWithoutEmojiBug() { + let messageData = FakeMessageData(text:"一二三四五六七八九十甲乙丙丁戊己庚辛壬圭咖啡牛奶餅乾水果蛋糕") + let actual = calculator.messageBubbleSize(for: messageData, at: indexPath, with: layout) + // erroneously seeing 69 with the emoji fix in place. + XCTAssertEqual(81, actual.height); + } + + func testHeightForiOS10DoubleSpaceNumbersBug() { + let messageData = FakeMessageData(text:"１２３４５６７８９０１２３４５６７８９０") + let actual = calculator.messageBubbleSize(for: messageData, at: indexPath, with: layout) + // erroneously seeing 51 with emoji fix in place. It's the call to "fix string" + XCTAssertEqual(59, actual.height); + } + +} diff --git a/Signal/test/SignalTests-Bridging-Header.h b/Signal/test/SignalTests-Bridging-Header.h index f098150cf..b5222e94a 100644 --- a/Signal/test/SignalTests-Bridging-Header.h +++ b/Signal/test/SignalTests-Bridging-Header.h @@ -2,3 +2,6 @@ // Use this file to import your target's public headers that you would like to expose to Swift. // #import "Signal-Bridging-Header.h" +#import "OWSMessagesBubblesSizeCalculator.h" +#import +#import