diff --git a/Signal/src/ViewControllers/ConversationView/ConversationInputTextView.h b/Signal/src/ViewControllers/ConversationView/ConversationInputTextView.h index 726ac1627..1b499ec73 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationInputTextView.h +++ b/Signal/src/ViewControllers/ConversationView/ConversationInputTextView.h @@ -1,5 +1,5 @@ // -// Copyright (c) 2018 Open Whisper Systems. All rights reserved. +// Copyright (c) 2019 Open Whisper Systems. All rights reserved. // #import @@ -23,6 +23,7 @@ NS_ASSUME_NONNULL_BEGIN @protocol ConversationTextViewToolbarDelegate - (void)textViewDidChange:(UITextView *)textView; +- (void)textViewDidChangeSelection:(UITextView *)textView; @end diff --git a/Signal/src/ViewControllers/ConversationView/ConversationInputTextView.m b/Signal/src/ViewControllers/ConversationView/ConversationInputTextView.m index a23327590..be67f543d 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationInputTextView.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationInputTextView.m @@ -189,6 +189,11 @@ NS_ASSUME_NONNULL_BEGIN [self.textViewToolbarDelegate textViewDidChange:self]; } +- (void)textViewDidChangeSelection:(UITextView *)textView +{ + [self.textViewToolbarDelegate textViewDidChangeSelection:self]; +} + #pragma mark - Key Commands - (nullable NSArray *)keyCommands diff --git a/Signal/src/ViewControllers/ConversationView/ConversationInputToolbar.m b/Signal/src/ViewControllers/ConversationView/ConversationInputToolbar.m index 791564043..b351b4d54 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationInputToolbar.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationInputToolbar.m @@ -869,6 +869,11 @@ const CGFloat kMaxTextViewHeight = 98; [self updateInputLinkPreview]; } +- (void)textViewDidChangeSelection:(UITextView *)textView +{ + [self updateInputLinkPreview]; +} + - (void)updateHeightWithTextView:(UITextView *)textView { // compute new height assuming width is unchanged @@ -922,7 +927,10 @@ const CGFloat kMaxTextViewHeight = 98; return; } - NSString *_Nullable previewUrl = [OWSLinkPreview previewUrlForMessageBodyText:body]; + // It's key that we use the *raw/unstripped* text, so we can reconcile cursor position with the + // selectedRange. + NSString *_Nullable previewUrl = [OWSLinkPreview previewUrlForRawBodyText:self.inputTextView.text + selectedRange:self.inputTextView.selectedRange]; if (previewUrl.length < 1) { [self clearLinkPreviewStateAndView]; return; diff --git a/Signal/test/util/ProtoParsingTest.m b/Signal/test/util/ProtoParsingTest.m index 6bb0e75d0..6e06aebbc 100644 --- a/Signal/test/util/ProtoParsingTest.m +++ b/Signal/test/util/ProtoParsingTest.m @@ -1,5 +1,5 @@ // -// Copyright (c) 2018 Open Whisper Systems. All rights reserved. +// Copyright (c) 2019 Open Whisper Systems. All rights reserved. // #import "SignalBaseTest.h" @@ -63,6 +63,10 @@ NS_ASSUME_NONNULL_BEGIN return nil; } +- (nonnull NSString *)displayNameForPhoneIdentifier:(NSString * _Nullable)recipientId transaction:(nonnull YapDatabaseReadTransaction *)transaction { + return nil; +} + @end #pragma mark - diff --git a/SignalServiceKit/src/Messages/Interactions/OWSLinkPreview.swift b/SignalServiceKit/src/Messages/Interactions/OWSLinkPreview.swift index 26df0bedb..5663017dd 100644 --- a/SignalServiceKit/src/Messages/Interactions/OWSLinkPreview.swift +++ b/SignalServiceKit/src/Messages/Interactions/OWSLinkPreview.swift @@ -429,10 +429,14 @@ public class OWSLinkPreview: MTLModel { // MARK: - Text Parsing // This cache should only be accessed on main thread. - private static var previewUrlCache: NSCache = NSCache() + private static var previewUrlCache: NSCache = NSCache() @objc - public class func previewUrl(forMessageBodyText body: String?) -> String? { + public class func previewUrl(forRawBodyText body: String?, selectedRange: NSRange) -> String? { + return previewUrl(forMessageBodyText: body, selectedRange: selectedRange) + } + + public class func previewUrl(forMessageBodyText body: String?, selectedRange: NSRange?) -> String? { AssertIsOnMainThread() // Exit early if link previews are not enabled in order to avoid @@ -440,38 +444,62 @@ public class OWSLinkPreview: MTLModel { guard OWSLinkPreview.featureEnabled else { return nil } + guard SSKPreferences.areLinkPreviewsEnabled() else { return nil } - if let cachedUrl = previewUrlCache.object(forKey: body as AnyObject) as? String { + guard let body = body else { + return nil + } + + if let cachedUrl = previewUrlCache.object(forKey: body as NSString) as String? { Logger.verbose("URL parsing cache hit.") guard cachedUrl.count > 0 else { return nil } return cachedUrl } - let previewUrls = allPreviewUrls(forMessageBodyText: body) - guard let previewUrl = previewUrls.first else { + let previewUrlMatches = allPreviewUrlMatches(forMessageBodyText: body) + guard let urlMatch = previewUrlMatches.first else { // Use empty string to indicate "no preview URL" in the cache. - previewUrlCache.setObject("" as AnyObject, forKey: body as AnyObject) + previewUrlCache.setObject("", forKey: body as NSString) return nil } - previewUrlCache.setObject(previewUrl as AnyObject, forKey: body as AnyObject) - return previewUrl + if let selectedRange = selectedRange { + Logger.verbose("match: urlString: \(urlMatch.urlString) range: \(urlMatch.matchRange) selectedRange: \(selectedRange)") + let cursorAtEndOfMatch = urlMatch.matchRange.location + urlMatch.matchRange.length == selectedRange.location + if selectedRange.location != body.count, + (urlMatch.matchRange.intersection(selectedRange) != nil || cursorAtEndOfMatch) { + Logger.debug("ignoring URL, since the user is currently editing it.") + // we don't want to cache the result here, as we want to fetch the link preview + // if the user moves the cursor. + return nil + } + Logger.debug("considering URL, since the user is not currently editing it.") + } + + previewUrlCache.setObject(urlMatch.urlString as NSString, forKey: body as NSString) + return urlMatch.urlString } - class func allPreviewUrls(forMessageBodyText body: String?) -> [String] { + struct URLMatchResult { + let urlString: String + let matchRange: NSRange + } + + class func allPreviewUrls(forMessageBodyText body: String) -> [String] { + return allPreviewUrlMatches(forMessageBodyText: body).map { $0.urlString } + } + + class func allPreviewUrlMatches(forMessageBodyText body: String) -> [URLMatchResult] { guard OWSLinkPreview.featureEnabled else { return [] } guard SSKPreferences.areLinkPreviewsEnabled() else { return [] } - guard let body = body else { - return [] - } let detector: NSDataDetector do { @@ -481,7 +509,7 @@ public class OWSLinkPreview: MTLModel { return [] } - var previewUrls = [String]() + var urlMatches: [URLMatchResult] = [] let matches = detector.matches(in: body, options: [], range: NSRange(location: 0, length: body.count)) for match in matches { guard let matchURL = match.url else { @@ -490,21 +518,22 @@ public class OWSLinkPreview: MTLModel { } let urlString = matchURL.absoluteString if isValidLinkUrl(urlString) { - previewUrls.append(urlString) + let matchResult = URLMatchResult(urlString: urlString, matchRange: match.range) + urlMatches.append(matchResult) } } - return previewUrls + return urlMatches } // MARK: - Preview Construction // This cache should only be accessed on serialQueue. - private static var linkPreviewDraftCache: NSCache = NSCache() + private static var linkPreviewDraftCache: NSCache = NSCache() private class func cachedLinkPreview(forPreviewUrl previewUrl: String) -> OWSLinkPreviewDraft? { var result: OWSLinkPreviewDraft? serialQueue.sync { - result = linkPreviewDraftCache.object(forKey: previewUrl as AnyObject) + result = linkPreviewDraftCache.object(forKey: previewUrl as NSString) } return result } @@ -522,7 +551,7 @@ public class OWSLinkPreview: MTLModel { } serialQueue.sync { - previewUrlCache.setObject(linkPreviewDraft, forKey: previewUrl as AnyObject) + linkPreviewDraftCache.setObject(linkPreviewDraft, forKey: previewUrl as NSString) } } diff --git a/SignalServiceKit/tests/Messages/OWSLinkPreviewTest.swift b/SignalServiceKit/tests/Messages/OWSLinkPreviewTest.swift index db5a1743c..41ec45fe2 100644 --- a/SignalServiceKit/tests/Messages/OWSLinkPreviewTest.swift +++ b/SignalServiceKit/tests/Messages/OWSLinkPreviewTest.swift @@ -169,19 +169,20 @@ class OWSLinkPreviewTest: SSKBaseTestSwift { } func testPreviewUrlForMessageBodyText() { - XCTAssertNil(OWSLinkPreview.previewUrl(forMessageBodyText: "")) - XCTAssertNil(OWSLinkPreview.previewUrl(forMessageBodyText: "alice bob jim")) - XCTAssertNil(OWSLinkPreview.previewUrl(forMessageBodyText: "alice bob jim http://")) - XCTAssertNil(OWSLinkPreview.previewUrl(forMessageBodyText: "alice bob jim http://a.com")) + Assert(bodyText: "", extractsLink: nil) + Assert(bodyText: "alice bob jim", extractsLink: nil) + Assert(bodyText: "alice bob jim http://", extractsLink: nil) + Assert(bodyText: "alice bob jim http://a.com", extractsLink: nil) - XCTAssertEqual(OWSLinkPreview.previewUrl(forMessageBodyText: "https://www.youtube.com/watch?v=tP-Ipsat90c"), - "https://www.youtube.com/watch?v=tP-Ipsat90c") - XCTAssertEqual(OWSLinkPreview.previewUrl(forMessageBodyText: "alice bob https://www.youtube.com/watch?v=tP-Ipsat90c jim"), - "https://www.youtube.com/watch?v=tP-Ipsat90c") + Assert(bodyText: "https://www.youtube.com/watch?v=tP-Ipsat90c", + extractsLink: "https://www.youtube.com/watch?v=tP-Ipsat90c") + + Assert(bodyText: "alice bob https://www.youtube.com/watch?v=tP-Ipsat90c jim", + extractsLink: "https://www.youtube.com/watch?v=tP-Ipsat90c") // If there are more than one, take the first. - XCTAssertEqual(OWSLinkPreview.previewUrl(forMessageBodyText: "alice bob https://www.youtube.com/watch?v=tP-Ipsat90c jim https://www.youtube.com/watch?v=other-url carol"), - "https://www.youtube.com/watch?v=tP-Ipsat90c") + Assert(bodyText: "alice bob https://www.youtube.com/watch?v=tP-Ipsat90c jim https://www.youtube.com/watch?v=other-url carol", + extractsLink: "https://www.youtube.com/watch?v=tP-Ipsat90c") } func testUtils() { @@ -454,4 +455,87 @@ class OWSLinkPreviewTest: SSKBaseTestSwift { options: [], range: NSRange(location: 0, length: text.utf16.count))) } + + func testCursorPositions() { + // sanity check + Assert(bodyText: "https://www.youtube.com/watch?v=testCursorPositionsa", + extractsLink: "https://www.youtube.com/watch?v=testCursorPositionsa", + selectedRange: nil) + + // Don't extract link if cursor is touching text + let text2 = "https://www.youtube.com/watch?v=testCursorPositionsb" + XCTAssertEqual(text2.count, 52) + Assert(bodyText: text2, + extractsLink: nil, + selectedRange: NSRange(location: 51, length: 0)) + + Assert(bodyText: text2, + extractsLink: nil, + selectedRange: NSRange(location: 51, length: 10)) + + Assert(bodyText: text2, + extractsLink: nil, + selectedRange: NSRange(location: 0, length: 0)) + + // Unless the cursor is at the end of the text + Assert(bodyText: text2, + extractsLink: "https://www.youtube.com/watch?v=testCursorPositionsb", + selectedRange: NSRange(location: 52, length: 0)) + + // Once extracted, keep the existing link preview, even if the cursor moves back. + Assert(bodyText: text2, + extractsLink: "https://www.youtube.com/watch?v=testCursorPositionsb", + selectedRange: NSRange(location: 51, length: 0)) + + let text3 = "foo https://www.youtube.com/watch?v=testCursorPositionsc bar" + XCTAssertEqual(text3.count, 60) + + // front edge + Assert(bodyText: text3, + extractsLink: nil, + selectedRange: NSRange(location: 4, length: 0)) + + // middle + Assert(bodyText: text3, + extractsLink: nil, + selectedRange: NSRange(location: 4, length: 0)) + + // rear edge + Assert(bodyText: text3, + extractsLink: nil, + selectedRange: NSRange(location: 56, length: 0)) + + // extract link if selecting after link + Assert(bodyText: text3, + extractsLink: "https://www.youtube.com/watch?v=testCursorPositionsc", + selectedRange: NSRange(location: 57, length: 0)) + + let text4 = "bar https://www.youtube.com/watch?v=testCursorPositionsd foo" + XCTAssertEqual(text4.count, 60) + + // front edge + Assert(bodyText: text4, + extractsLink: nil, + selectedRange: NSRange(location: 4, length: 0)) + + // middle + Assert(bodyText: text4, + extractsLink: nil, + selectedRange: NSRange(location: 20, length: 0)) + + // rear edge + Assert(bodyText: text4, + extractsLink: nil, + selectedRange: NSRange(location: 56, length: 0)) + + // extract link if selecting before link + Assert(bodyText: text4, + extractsLink: "https://www.youtube.com/watch?v=testCursorPositionsd", + selectedRange: NSRange(location: 3, length: 0)) + } + + private func Assert(bodyText: String, extractsLink link: String?, selectedRange: NSRange? = nil, file: StaticString = #file, line: UInt = #line) { + let actual = OWSLinkPreview.previewUrl(forMessageBodyText: bodyText, selectedRange: selectedRange) + XCTAssertEqual(actual, link, file: file, line: line) + } }