Merge pull request #3307 from babbage/bugfix/3103-copy-repaired-URLs

Use repaired URLs for link, externalLink and imageLink where needed. Fixes #3103

Account/Sources/Account/CloudKit/CloudKitArticlesZone.swift

@@ -223,10 +223,10 @@ private extension CloudKitArticlesZone {
 		record[CloudKitArticle.Fields.title] = article.title
 		record[CloudKitArticle.Fields.contentHTML] = article.contentHTML
 		record[CloudKitArticle.Fields.contentText] = article.contentText
-		record[CloudKitArticle.Fields.url] = article.url
-		record[CloudKitArticle.Fields.externalURL] = article.externalURL
+		record[CloudKitArticle.Fields.url] = article.rawLink
+		record[CloudKitArticle.Fields.externalURL] = article.rawExternalLink
 		record[CloudKitArticle.Fields.summary] = article.summary
-		record[CloudKitArticle.Fields.imageURL] = article.imageURL
+		record[CloudKitArticle.Fields.imageURL] = article.rawImageLink
 		record[CloudKitArticle.Fields.datePublished] = article.datePublished
 		record[CloudKitArticle.Fields.dateModified] = article.dateModified
 		

Articles/Sources/Articles/Article.swift

@@ -19,10 +19,10 @@ public struct Article: Hashable {
 	public let title: String?
 	public let contentHTML: String?
 	public let contentText: String?
-	public let url: String?
-	public let externalURL: String?
+	public let rawLink: String? // We store raw source value, but use computed url or link other than where raw value required.
+    public let rawExternalLink: String? // We store raw source value, but use computed externalURL or externalLink other than where raw value required.
 	public let summary: String?
-	public let imageURL: String?
+	public let rawImageLink: String? // We store raw source value, but use computed imageURL or imageLink other than where raw value required.
 	public let datePublished: Date?
 	public let dateModified: Date?
 	public let authors: Set<Author>?
@@ -35,10 +35,10 @@ public struct Article: Hashable {
 		self.title = title
 		self.contentHTML = contentHTML
 		self.contentText = contentText
-		self.url = url
-		self.externalURL = externalURL
+		self.rawLink = url
+		self.rawExternalLink = externalURL
 		self.summary = summary
-		self.imageURL = imageURL
+		self.rawImageLink = imageURL
 		self.datePublished = datePublished
 		self.dateModified = dateModified
 		self.authors = authors
@@ -65,7 +65,7 @@ public struct Article: Hashable {
 	// MARK: - Equatable
 
 	static public func ==(lhs: Article, rhs: Article) -> Bool {
-		return lhs.articleID == rhs.articleID && lhs.accountID == rhs.accountID && lhs.webFeedID == rhs.webFeedID && lhs.uniqueID == rhs.uniqueID && lhs.title == rhs.title && lhs.contentHTML == rhs.contentHTML && lhs.contentText == rhs.contentText && lhs.url == rhs.url && lhs.externalURL == rhs.externalURL && lhs.summary == rhs.summary && lhs.imageURL == rhs.imageURL && lhs.datePublished == rhs.datePublished && lhs.dateModified == rhs.dateModified && lhs.authors == rhs.authors
+		return lhs.articleID == rhs.articleID && lhs.accountID == rhs.accountID && lhs.webFeedID == rhs.webFeedID && lhs.uniqueID == rhs.uniqueID && lhs.title == rhs.title && lhs.contentHTML == rhs.contentHTML && lhs.contentText == rhs.contentText && lhs.rawLink == rhs.rawLink && lhs.rawExternalLink == rhs.rawExternalLink && lhs.summary == rhs.summary && lhs.rawImageLink == rhs.rawImageLink && lhs.datePublished == rhs.datePublished && lhs.dateModified == rhs.dateModified && lhs.authors == rhs.authors
 	}
 }
 

ArticlesDatabase/Sources/ArticlesDatabase/Extensions/Article+Database.swift

@@ -71,7 +71,7 @@ extension Article {
 		if authors.isEmpty {
 			return self
 		}
-		return Article(accountID: self.accountID, articleID: self.articleID, webFeedID: self.webFeedID, uniqueID: self.uniqueID, title: self.title, contentHTML: self.contentHTML, contentText: self.contentText, url: self.url, externalURL: self.externalURL, summary: self.summary, imageURL: self.imageURL, datePublished: self.datePublished, dateModified: self.dateModified, authors: authors, status: self.status)
+		return Article(accountID: self.accountID, articleID: self.articleID, webFeedID: self.webFeedID, uniqueID: self.uniqueID, title: self.title, contentHTML: self.contentHTML, contentText: self.contentText, url: self.rawLink, externalURL: self.rawExternalLink, summary: self.summary, imageURL: self.rawImageLink, datePublished: self.datePublished, dateModified: self.dateModified, authors: authors, status: self.status)
 	}
 
 	func changesFrom(_ existingArticle: Article) -> DatabaseDictionary? {
@@ -87,10 +87,10 @@ extension Article {
 		addPossibleStringChangeWithKeyPath(\Article.title, existingArticle, DatabaseKey.title, &d)
 		addPossibleStringChangeWithKeyPath(\Article.contentHTML, existingArticle, DatabaseKey.contentHTML, &d)
 		addPossibleStringChangeWithKeyPath(\Article.contentText, existingArticle, DatabaseKey.contentText, &d)
-		addPossibleStringChangeWithKeyPath(\Article.url, existingArticle, DatabaseKey.url, &d)
-		addPossibleStringChangeWithKeyPath(\Article.externalURL, existingArticle, DatabaseKey.externalURL, &d)
+		addPossibleStringChangeWithKeyPath(\Article.rawLink, existingArticle, DatabaseKey.url, &d)
+		addPossibleStringChangeWithKeyPath(\Article.rawExternalLink, existingArticle, DatabaseKey.externalURL, &d)
 		addPossibleStringChangeWithKeyPath(\Article.summary, existingArticle, DatabaseKey.summary, &d)
-		addPossibleStringChangeWithKeyPath(\Article.imageURL, existingArticle, DatabaseKey.imageURL, &d)
+		addPossibleStringChangeWithKeyPath(\Article.rawImageLink, existingArticle, DatabaseKey.imageURL, &d)
 
 		// If updated versions of dates are nil, and we have existing dates, keep the existing dates.
 		// This is data that’s good to have, and it’s likely that a feed removing dates is doing so in error.
@@ -154,17 +154,17 @@ extension Article: DatabaseObject {
 		if let contentText = contentText {
 			d[DatabaseKey.contentText] = contentText
 		}
-		if let url = url {
-			d[DatabaseKey.url] = url
+		if let rawLink = rawLink {
+			d[DatabaseKey.url] = rawLink
 		}
-		if let externalURL = externalURL {
-			d[DatabaseKey.externalURL] = externalURL
+		if let rawExternalLink = rawExternalLink {
+			d[DatabaseKey.externalURL] = rawExternalLink
 		}
 		if let summary = summary {
 			d[DatabaseKey.summary] = summary
 		}
-		if let imageURL = imageURL {
-			d[DatabaseKey.imageURL] = imageURL
+		if let rawImageLink = rawImageLink {
+			d[DatabaseKey.imageURL] = rawImageLink
 		}
 		if let datePublished = datePublished {
 			d[DatabaseKey.datePublished] = datePublished

Mac/MainWindow/MainWindowController.swift

@@ -318,7 +318,7 @@ class MainWindowController : NSWindowController, NSUserInterfaceValidations {
 	}
 
 	@IBAction func copyExternalURL(_ sender: Any?) {
-		if let link = oneSelectedArticle?.externalURL {
+		if let link = oneSelectedArticle?.externalLink {
 			URLPasteboardWriter.write(urlString: link, to: .general)
 		}
 	}
@@ -1077,7 +1077,7 @@ private extension MainWindowController {
 	}
 	
 	func canCopyExternalURL() -> Bool {
-		return oneSelectedArticle?.externalURL != nil && oneSelectedArticle?.externalURL != currentLink
+		return oneSelectedArticle?.externalLink != nil && oneSelectedArticle?.externalLink != currentLink
 	}
 
 	func canGoToNextUnread(wrappingToTop wrapping: Bool = false) -> Bool {

Mac/MainWindow/Timeline/ArticlePasteboardWriter.swift

@@ -87,11 +87,11 @@ private extension ArticlePasteboardWriter {
 			s += "\(convertedHTML)\n\n"
 		}
 
-		if let url = article.url {
-			s += "URL: \(url)\n\n"
+		if let link = article.link {
+			s += "URL: \(rawLink)\n\n"
 		}
-		if let externalURL = article.externalURL {
-			s += "external URL: \(externalURL)\n\n"
+		if let externalLink = article.externalLink {
+			s += "external URL: \(externalLink)\n\n"
 		}
 
 		s += "Date: \(article.logicalDatePublished)\n\n"
@@ -151,10 +151,10 @@ private extension ArticlePasteboardWriter {
 		d[Key.title] = article.title ?? nil
 		d[Key.contentHTML] = article.contentHTML ?? nil
 		d[Key.contentText] = article.contentText ?? nil
-		d[Key.url] = article.url ?? nil
-		d[Key.externalURL] = article.externalURL ?? nil
+		d[Key.url] = article.rawLink ?? nil
+		d[Key.externalURL] = article.rawExternalLink ?? nil
 		d[Key.summary] = article.summary ?? nil
-		d[Key.imageURL] = article.imageURL ?? nil
+		d[Key.imageURL] = article.rawImageLink ?? nil
 		d[Key.datePublished] = article.datePublished ?? nil
 		d[Key.dateModified] = article.dateModified ?? nil
 		d[Key.dateArrived] = article.status.dateArrived

Mac/MainWindow/Timeline/TimelineViewController+ContextualMenus.swift

@@ -178,7 +178,7 @@ private extension TimelineViewController {
 			menu.addSeparatorIfNeeded()
 			menu.addItem(copyArticleURLMenuItem(link))
 			
-			if let externalLink = articles.first?.externalURL, externalLink != link {
+			if let externalLink = articles.first?.externalLink, externalLink != link {
 				menu.addItem(copyExternalURLMenuItem(externalLink))
 			}
 		}

Mac/Scriptability/Article+Scriptability.swift

@@ -57,17 +57,17 @@ class ScriptableArticle: NSObject, UniqueIdScriptingObject, ScriptingObjectConta
     
     @objc(url)
     var url:String?  {
-        return article.url ?? article.externalURL
+        return article.preferredLink
     }
 
     @objc(permalink)
     var permalink:String?  {
-        return article.url
+        return article.link
     }
 
     @objc(externalUrl)
     var externalUrl:String?  {
-        return article.externalURL
+        return article.externalLink
     }
     
     @objc(title)
@@ -132,7 +132,7 @@ class ScriptableArticle: NSObject, UniqueIdScriptingObject, ScriptingObjectConta
 
     @objc(imageURL)
     var imageURL:String  {
-        return article.imageURL ?? ""
+        return article.imageLink ?? ""
     }
     
     @objc(authors)

Shared/Article Rendering/ArticleRenderer.swift

@@ -209,7 +209,7 @@ private extension ArticleRenderer {
 		d["title"] = title
 		d["preferred_link"] = article.preferredLink ?? ""
 		
-		if let externalLink = article.externalURL, externalLink != article.preferredLink {
+		if let externalLink = article.externalLink, externalLink != article.preferredLink {
 			d["external_link_label"] = NSLocalizedString("Link:", comment: "Link")
 			d["external_link_stripped"] = externalLink.strippingHTTPOrHTTPSScheme
 			d["external_link"] = externalLink
@@ -331,7 +331,7 @@ private extension ArticleRenderer {
 private extension Article {
 
 	var baseURL: URL? {
-		var s = url
+		var s = link
 		if s == nil {
 			s = webFeed?.homePageURL
 		}

Shared/ExtensionPoints/SendToMarsEditCommand.swift

@@ -75,7 +75,7 @@ private extension SendToMarsEditCommand {
 		let body = article.contentHTML ?? article.contentText ?? article.summary
 		let authorName = article.authors?.first?.name
 
-		let sender = SendToBlogEditorApp(targetDescriptor: targetDescriptor, title: article.title, body: body, summary: article.summary, link: article.externalURL, permalink: article.url, subject: nil, creator: authorName, commentsURL: nil, guid: article.uniqueID, sourceName: article.webFeed?.nameForDisplay, sourceHomeURL: article.webFeed?.homePageURL, sourceFeedURL: article.webFeed?.url)
+		let sender = SendToBlogEditorApp(targetDescriptor: targetDescriptor, title: article.title, body: body, summary: article.summary, link: article.externalLink, permalink: article.link, subject: nil, creator: authorName, commentsURL: nil, guid: article.uniqueID, sourceName: article.webFeed?.nameForDisplay, sourceHomeURL: article.webFeed?.homePageURL, sourceFeedURL: article.webFeed?.url)
 		let _ = sender.send()
 	}
 	

Shared/Extensions/ArticleUtilities.swift

@@ -46,26 +46,48 @@ extension Article {
 		return account?.existingWebFeed(withWebFeedID: webFeedID)
 	}
 	
+	var url: URL? {
+		return URL.reparingIfRequired(rawLink)
+	}
+	
+	var externalURL: URL? {
+		return URL.reparingIfRequired(rawExternalLink)
+	}
+	
+	var imageURL: URL? {
+		return URL.reparingIfRequired(rawImageLink)
+	}
+	
+	var link: String? {
+		// Prefer link from URL, if one can be created, as these are repaired if required.
+		// Provide the raw link if URL creation fails.
+		return url?.absoluteString ?? rawLink
+	}
+	
+	var externalLink: String? {
+		// Prefer link from externalURL, if one can be created, as these are repaired if required.
+		// Provide the raw link if URL creation fails.
+		return externalURL?.absoluteString ?? rawExternalLink
+	}
+
+	var imageLink: String? {
+		// Prefer link from imageURL, if one can be created, as these are repaired if required.
+		// Provide the raw link if URL creation fails.
+		return imageURL?.absoluteString ?? rawImageLink
+	}
+
 	var preferredLink: String? {
-		if let url = url, !url.isEmpty {
-			return url
+		if let link = link, !link.isEmpty {
+			return link
 		}
-		if let externalURL = externalURL, !externalURL.isEmpty {
-			return externalURL
+		if let externalLink = externalLink, !externalLink.isEmpty {
+			return externalLink
 		}
 		return nil
 	}
 	
 	var preferredURL: URL? {
-		guard let link = preferredLink else { return nil }
-		// If required, we replace any space characters to handle malformed links that are otherwise percent
-		// encoded but contain spaces. For performance reasons, only try this if initial URL init fails.
-		if let url = URL(string: link) {
-			return url
-		} else if let url = URL(string: link.replacingOccurrences(of: " ", with: "%20")) {
-			return url
-		}
-		return nil
+		return url ?? externalURL
 	}
 	
 	var body: String? {

Shared/Extensions/URL-Extensions.swift

@@ -42,4 +42,16 @@ extension URL {
 		return value
 		
 	}
+	
+	static func reparingIfRequired(_ link: String?) -> URL? {
+		// If required, we replace any space characters to handle malformed links that are otherwise percent
+		// encoded but contain spaces. For performance reasons, only try this if initial URL init fails.
+		guard let link = link, !link.isEmpty else { return nil }
+		if let url = URL(string: link) {
+			return url
+		} else {
+			return URL(string: link.replacingOccurrences(of: " ", with: "%20"))
+		}
+	}
+
 }

iOS/MasterTimeline/MasterTimelineViewController.swift

@@ -545,7 +545,7 @@ class MasterTimelineViewController: UITableViewController, UndoableCommandRunner
 		
 		let prototypeID = "prototype"
 		let status = ArticleStatus(articleID: prototypeID, read: false, starred: false, dateArrived: Date())
-		let prototypeArticle = Article(accountID: prototypeID, articleID: prototypeID, webFeedID: prototypeID, uniqueID: prototypeID, title: longTitle, contentHTML: nil, contentText: nil, url: nil, externalURL: nil, summary: nil, imageURL: nil, datePublished: nil, dateModified: nil, authors: nil, status: status)
+		let prototypeArticle = Article(accountID: prototypeID, articleID: prototypeID, webFeedID: prototypeID, uniqueID: prototypeID, title: longTitle, contentHTML: nil, contentText: nil, link: nil, externalLink: nil, summary: nil, imageLink: nil, datePublished: nil, dateModified: nil, authors: nil, status: status)
 		
 		let prototypeCellData = MasterTimelineCellData(article: prototypeArticle, showFeedName: .feed, feedName: "Prototype Feed Name", byline: nil, iconImage: nil, showIcon: false, featuredImage: nil, numberOfLines: numberOfTextLines, iconSize: iconSize)
 		
@@ -739,7 +739,7 @@ private extension MasterTimelineViewController {
 	}
 	
 	func featuredImageFor(_ article: Article) -> UIImage? {
-		if let url = article.imageURL, let data = appDelegate.imageDownloader.image(for: url) {
+		if let link = article.imageLink, let data = appDelegate.imageDownloader.image(for: link) {
 			return RSImage(data: data)
 		}
 		return nil
@@ -924,7 +924,7 @@ private extension MasterTimelineViewController {
 	}
 	
 	func copyExternalURLAction(_ article: Article) -> UIAction? {
-		guard let externalURL = article.externalURL, externalURL != article.preferredLink, let url = URL(string: externalURL) else { return nil }
+		guard let externalLink = article.externalLink, externalLink != article.preferredLink, let url = URL(string: externalLink) else { return nil }
 		let title = NSLocalizedString("Copy External URL", comment: "Copy External URL")
 		let action = UIAction(title: title, image: AppAssets.copyImage) { action in
 			UIPasteboard.general.url = url

iOS/Settings/TimelinePreviewTableViewController.swift

@@ -67,7 +67,7 @@ private extension TimelinePreviewTableViewController {
 		
 		let prototypeID = "prototype"
 		let status = ArticleStatus(articleID: prototypeID, read: false, starred: false, dateArrived: Date())
-		let prototypeArticle = Article(accountID: prototypeID, articleID: prototypeID, webFeedID: prototypeID, uniqueID: prototypeID, title: longTitle, contentHTML: nil, contentText: nil, url: nil, externalURL: nil, summary: nil, imageURL: nil, datePublished: nil, dateModified: nil, authors: nil, status: status)
+		let prototypeArticle = Article(accountID: prototypeID, articleID: prototypeID, webFeedID: prototypeID, uniqueID: prototypeID, title: longTitle, contentHTML: nil, contentText: nil, link: nil, externalLink: nil, summary: nil, imageLink: nil, datePublished: nil, dateModified: nil, authors: nil, status: status)
 		
 		let iconImage = IconImage(AppAssets.faviconTemplateImage.withTintColor(AppAssets.secondaryAccentColor))