From 4d930dd5e4d4f70a233d5214bf92903c1b68db5e Mon Sep 17 00:00:00 2001 From: Brent Simmons Date: Fri, 9 May 2025 20:30:04 -0700 Subject: [PATCH] Restore changes reverted in previous beta. --- .../Timeline/TimelineViewController.swift | 2 +- .../Sources/RSCore/Shared/String+RSCore.swift | 17 ++- Modules/RSWeb/Sources/RSWeb/Downloader.swift | 110 ++++++++++++++++-- .../RSWeb/HTMLMetadataDownloader.swift | 42 +++---- .../ArticleThemeDownloader.swift | 4 + Shared/Favicons/SingleFaviconDownloader.swift | 30 ++--- Shared/Images/ImageDownloader.swift | 36 +++--- Shared/Timeline/ArticleArray.swift | 8 +- iOS/Article/ImageViewController.swift | 10 ++ iOS/Article/WebViewController.swift | 40 ++++--- iOS/MainFeed/MainFeedViewController.swift | 1 + .../MainTimelineViewController.swift | 1 + 12 files changed, 216 insertions(+), 85 deletions(-) diff --git a/Mac/MainWindow/Timeline/TimelineViewController.swift b/Mac/MainWindow/Timeline/TimelineViewController.swift index ce37ebc9f..f19276b9f 100644 --- a/Mac/MainWindow/Timeline/TimelineViewController.swift +++ b/Mac/MainWindow/Timeline/TimelineViewController.swift @@ -85,7 +85,7 @@ final class TimelineViewController: NSViewController, UndoableCommandRunner, Unr var showsSearchResults = false var selectedArticles: [Article] { - return Array(articles.articlesForIndexes(tableView.selectedRowIndexes)) + return articles.articlesForIndexes(tableView.selectedRowIndexes) } var hasAtLeastOneSelectedArticle: Bool { diff --git a/Modules/RSCore/Sources/RSCore/Shared/String+RSCore.swift b/Modules/RSCore/Sources/RSCore/Shared/String+RSCore.swift index 612f4fe60..237a268d0 100644 --- a/Modules/RSCore/Sources/RSCore/Shared/String+RSCore.swift +++ b/Modules/RSCore/Sources/RSCore/Shared/String+RSCore.swift @@ -189,13 +189,28 @@ public extension String { /// Removes an HTML tag and everything between its start and end tags. /// + /// The regex pattern `[\\s\\S]*?` explanation: + /// - `<` matches the literal `<` character. + /// - `tag` matches the literal parameter provided to the function, e.g., `style`. + /// - `>` matches the literal `>` character. + /// - `[\\s\\S]*?` + /// - `[\\s\\S]` matches _any_ character, including new lines. + /// - `*` will match zero or more of the preceeding character, in this case _any_ + /// character. + /// - `?` switches the matching mode to [lazy](https://javascript.info/regexp-greedy-and-lazy) + /// so it will match as few as characters as possible before satisfying the rest of the pattern. + /// - `<` matches the literal `<` character. + /// - `/` matches the literal `/` character. + /// - `tag` matches the literal parameter provided to the function, e.g., `style`. + /// - `>` matches the literal `>` character. + /// /// - Parameter tag: The tag to remove. /// /// - Returns: A new copy of `self` with the tag removed. /// /// - Note: Doesn't work correctly with nested tags of the same name. private func removingTagAndContents(_ tag: String) -> String { - return self.replacingOccurrences(of: "<\(tag).+?", with: "", options: [.regularExpression, .caseInsensitive]) + return self.replacingOccurrences(of: "<\(tag)>[\\s\\S]*?", with: "", options: [.regularExpression, .caseInsensitive]) } /// Strips HTML from a string. diff --git a/Modules/RSWeb/Sources/RSWeb/Downloader.swift b/Modules/RSWeb/Sources/RSWeb/Downloader.swift index 7b052a134..2588eaa9f 100755 --- a/Modules/RSWeb/Sources/RSWeb/Downloader.swift +++ b/Modules/RSWeb/Sources/RSWeb/Downloader.swift @@ -7,25 +7,34 @@ // import Foundation +import os +import RSCore -public typealias DownloadCallback = (Data?, URLResponse?, Error?) -> Swift.Void +public typealias DownloadCallback = @MainActor (Data?, URLResponse?, Error?) -> Swift.Void /// Simple downloader, for a one-shot download like an image /// or a web page. For a download-feeds session, see DownloadSession. -public final class Downloader { +/// Caches response for a short time for GET requests. May return cached response. +@MainActor public final class Downloader { public static let shared = Downloader() private let urlSession: URLSession + private var callbacks = [URL: [DownloadCallback]]() + + // Cache — short-lived + private let cache = Cache(timeToLive: 60 * 3, timeBetweenCleanups: 60 * 2) + + nonisolated private static let logger = Logger(subsystem: Bundle.main.bundleIdentifier!, category: "Downloader") + nonisolated private static let debugLoggingEnabled = false private init() { - let sessionConfiguration = URLSessionConfiguration.ephemeral sessionConfiguration.requestCachePolicy = .reloadIgnoringLocalCacheData sessionConfiguration.httpShouldSetCookies = false sessionConfiguration.httpCookieAcceptPolicy = .never sessionConfiguration.httpMaximumConnectionsPerHost = 1 sessionConfiguration.httpCookieStorage = nil - + if let userAgentHeaders = UserAgent.headers() { sessionConfiguration.httpAdditionalHeaders = userAgentHeaders } @@ -37,20 +46,103 @@ public final class Downloader { urlSession.invalidateAndCancel() } - public func download(_ url: URL, _ completion: DownloadCallback? = nil) { - download(URLRequest(url: url), completion) + public func download(_ url: URL, _ callback: @escaping DownloadCallback) { + assert(Thread.isMainThread) + download(URLRequest(url: url), callback) } - public func download(_ urlRequest: URLRequest, _ completion: DownloadCallback? = nil) { + public func download(_ urlRequest: URLRequest, _ callback: @escaping DownloadCallback) { + assert(Thread.isMainThread) + + guard let url = urlRequest.url else { + Self.logger.fault("Downloader: skipping download for URLRequest without a URL") + return + } + + let isCacheableRequest = urlRequest.httpMethod == HTTPMethod.get + + // Return cached record if available. + if isCacheableRequest { + if let cachedRecord = cache[url.absoluteString] { + if Self.debugLoggingEnabled { + Self.logger.debug("Downloader: returning cached record for \(url)") + } + callback(cachedRecord.data, cachedRecord.response, cachedRecord.error) + return + } + } + + // Add callback. If there is already a download in progress for this URL, return early. + if callbacks[url] == nil { + if Self.debugLoggingEnabled { + Self.logger.debug("Downloader: downloading \(url)") + } + callbacks[url] = [callback] + } else { + // A download is already be in progress for this URL. Don’t start a separate download. + // Add the callback to the callbacks array for this URL. + if Self.debugLoggingEnabled { + Self.logger.debug("Downloader: download in progress for \(url) — adding callback") + } + callbacks[url]?.append(callback) + return + } var urlRequestToUse = urlRequest urlRequestToUse.addSpecialCaseUserAgentIfNeeded() let task = urlSession.dataTask(with: urlRequestToUse) { (data, response, error) in - DispatchQueue.main.async() { - completion?(data, response, error) + + if isCacheableRequest { + if Self.debugLoggingEnabled { + Self.logger.debug("Downloader: caching record for \(url)") + } + let cachedRecord = DownloaderRecord(data: data, response: response, error: error) + self.cache[url.absoluteString] = cachedRecord + } + + Task { @MainActor in + self.callAndReleaseCallbacks(url, data, response, error) } } task.resume() } } + +private extension Downloader { + + func callAndReleaseCallbacks(_ url: URL, _ data: Data? = nil, _ response: URLResponse? = nil, _ error: Error? = nil) { + assert(Thread.isMainThread) + + defer { + callbacks[url] = nil + } + + guard let callbacksForURL = callbacks[url] else { + assertionFailure("Downloader: downloaded URL \(url) but no callbacks found") + Self.logger.fault("Downloader: downloaded URL \(url) but no callbacks found") + return + } + + if Self.debugLoggingEnabled { + let count = callbacksForURL.count + if count == 1 { + Self.logger.debug("Downloader: calling 1 callback for URL \(url)") + } else { + Self.logger.debug("Downloader: calling \(count) callbacks for URL \(url)") + } + } + + for callback in callbacksForURL { + callback(data, response, error) + } + } +} + +struct DownloaderRecord: CacheRecord, Sendable { + + let dateCreated = Date() + let data: Data? + let response: URLResponse? + let error: Error? +} diff --git a/Modules/RSWeb/Sources/RSWeb/HTMLMetadataDownloader.swift b/Modules/RSWeb/Sources/RSWeb/HTMLMetadataDownloader.swift index 96090e89d..6d8a0eea4 100644 --- a/Modules/RSWeb/Sources/RSWeb/HTMLMetadataDownloader.swift +++ b/Modules/RSWeb/Sources/RSWeb/HTMLMetadataDownloader.swift @@ -75,36 +75,38 @@ private extension HTMLMetadataDownloader { } func downloadMetadata(_ url: String) { - + guard let actualURL = URL(string: url) else { if Self.debugLoggingEnabled { Self.logger.debug("HTMLMetadataDownloader skipping download for \(url) because it couldn’t construct a URL.") } return } - + if Self.debugLoggingEnabled { Self.logger.debug("HTMLMetadataDownloader downloading for \(url)") } - - Downloader.shared.download(actualURL) { data, response, error in - if let data, !data.isEmpty, let response, response.statusIsOK { - let urlToUse = response.url ?? actualURL - let parserData = ParserData(url: urlToUse.absoluteString, data: data) - let htmlMetadata = RSHTMLMetadataParser.htmlMetadata(with: parserData) - if Self.debugLoggingEnabled { - Self.logger.debug("HTMLMetadataDownloader caching parsed metadata for \(url)") + + Task { @MainActor in + Downloader.shared.download(actualURL) { data, response, error in + if let data, !data.isEmpty, let response, response.statusIsOK { + let urlToUse = response.url ?? actualURL + let parserData = ParserData(url: urlToUse.absoluteString, data: data) + let htmlMetadata = RSHTMLMetadataParser.htmlMetadata(with: parserData) + if Self.debugLoggingEnabled { + Self.logger.debug("HTMLMetadataDownloader caching parsed metadata for \(url)") + } + self.cache[url] = htmlMetadata + return + } + + if let statusCode = response?.forcedStatusCode, (400...499).contains(statusCode) { + self.noteURLDidReturn4xx(url) + } + + if Self.debugLoggingEnabled { + Self.logger.debug("HTMLMetadataDownloader failed download for \(url)") } - self.cache[url] = htmlMetadata - return - } - - if let statusCode = response?.forcedStatusCode, (400...499).contains(statusCode) { - self.noteURLDidReturn4xx(url) - } - - if Self.debugLoggingEnabled { - Self.logger.debug("HTMLMetadataDownloader failed download for \(url)") } } } diff --git a/Shared/ArticleStyles/ArticleThemeDownloader.swift b/Shared/ArticleStyles/ArticleThemeDownloader.swift index d6714e169..43ee04567 100644 --- a/Shared/ArticleStyles/ArticleThemeDownloader.swift +++ b/Shared/ArticleStyles/ArticleThemeDownloader.swift @@ -75,6 +75,10 @@ public class ArticleThemeDownloader { private func findThemeFile(in searchPath: String) -> String? { if let directoryContents = FileManager.default.enumerator(atPath: searchPath) { while let file = directoryContents.nextObject() as? String { + if file.hasPrefix("__MACOSX/") { + //logger.debug("Ignoring theme file in __MACOSX folder.") + continue + } if file.hasSuffix(".nnwtheme") { return file } diff --git a/Shared/Favicons/SingleFaviconDownloader.swift b/Shared/Favicons/SingleFaviconDownloader.swift index 1d3a8e035..bc952bdf4 100644 --- a/Shared/Favicons/SingleFaviconDownloader.swift +++ b/Shared/Favicons/SingleFaviconDownloader.swift @@ -133,25 +133,27 @@ private extension SingleFaviconDownloader { } func downloadFavicon(_ completion: @escaping (RSImage?) -> Void) { - + guard let url = URL(string: faviconURL) else { completion(nil) return } - - Downloader.shared.download(url) { (data, response, error) in - - if let data = data, !data.isEmpty, let response = response, response.statusIsOK, error == nil { - self.saveToDisk(data) - RSImage.image(with: data, imageResultBlock: completion) - return + + Task { @MainActor in + Downloader.shared.download(url) { (data, response, error) in + + if let data = data, !data.isEmpty, let response = response, response.statusIsOK, error == nil { + self.saveToDisk(data) + RSImage.image(with: data, imageResultBlock: completion) + return + } + + if let error = error { + os_log(.info, log: self.log, "Error downloading image at %@: %@.", url.absoluteString, error.localizedDescription) + } + + completion(nil) } - - if let error = error { - os_log(.info, log: self.log, "Error downloading image at %@: %@.", url.absoluteString, error.localizedDescription) - } - - completion(nil) } } diff --git a/Shared/Images/ImageDownloader.swift b/Shared/Images/ImageDownloader.swift index f794e61a9..b5233ae55 100644 --- a/Shared/Images/ImageDownloader.swift +++ b/Shared/Images/ImageDownloader.swift @@ -99,28 +99,30 @@ private extension ImageDownloader { } func downloadImage(_ url: String, _ completion: @escaping (Data?) -> Void) { - + guard let imageURL = URL(string: url) else { completion(nil) return } - - Downloader.shared.download(imageURL) { (data, response, error) in - - if let data = data, !data.isEmpty, let response = response, response.statusIsOK, error == nil { - self.saveToDisk(url, data) - completion(data) - return + + Task { @MainActor in + Downloader.shared.download(imageURL) { (data, response, error) in + + if let data = data, !data.isEmpty, let response = response, response.statusIsOK, error == nil { + self.saveToDisk(url, data) + completion(data) + return + } + + if let response = response as? HTTPURLResponse, response.statusCode >= HTTPResponseCode.badRequest && response.statusCode <= HTTPResponseCode.notAcceptable { + self.badURLs.insert(url) + } + if let error = error { + os_log(.info, log: self.log, "Error downloading image at %@: %@.", url, error.localizedDescription) + } + + completion(nil) } - - if let response = response as? HTTPURLResponse, response.statusCode >= HTTPResponseCode.badRequest && response.statusCode <= HTTPResponseCode.notAcceptable { - self.badURLs.insert(url) - } - if let error = error { - os_log(.info, log: self.log, "Error downloading image at %@: %@.", url, error.localizedDescription) - } - - completion(nil) } } diff --git a/Shared/Timeline/ArticleArray.swift b/Shared/Timeline/ArticleArray.swift index 5ac8da789..2f8d69e06 100644 --- a/Shared/Timeline/ArticleArray.swift +++ b/Shared/Timeline/ArticleArray.swift @@ -44,12 +44,12 @@ extension Array where Element == Article { return nil } - func articlesForIndexes(_ indexes: IndexSet) -> Set
{ - return Set(indexes.compactMap{ (oneIndex) -> Article? in + func articlesForIndexes(_ indexes: IndexSet) -> [Article] { + return indexes.compactMap{ (oneIndex) -> Article? in return articleAtRow(oneIndex) - }) + } } - + func sortedByDate(_ sortDirection: ComparisonResult, groupByFeed: Bool = false) -> ArticleArray { return ArticleSorter.sortedByDate(articles: self, sortDirection: sortDirection, groupByFeed: groupByFeed) } diff --git a/iOS/Article/ImageViewController.swift b/iOS/Article/ImageViewController.swift index 91d8a17d9..fd1f17d79 100644 --- a/iOS/Article/ImageViewController.swift +++ b/iOS/Article/ImageViewController.swift @@ -25,6 +25,16 @@ class ImageViewController: UIViewController { return imageScrollView.zoomedFrame } + override var keyCommands: [UIKeyCommand]? { + return [ + UIKeyCommand( + title: NSLocalizedString("Close Image", comment: "Close Image"), + action: #selector(done(_:)), + input: " " + ) + ] + } + override func viewDidLoad() { super.viewDidLoad() diff --git a/iOS/Article/WebViewController.swift b/iOS/Article/WebViewController.swift index c5048bf09..5f6666edd 100644 --- a/iOS/Article/WebViewController.swift +++ b/iOS/Article/WebViewController.swift @@ -34,11 +34,12 @@ class WebViewController: UIViewController { private var webView: PreloadedWebView? { return view.subviews[0] as? PreloadedWebView } - + private lazy var contextMenuInteraction = UIContextMenuInteraction(delegate: self) private var isFullScreenAvailable: Bool { return AppDefaults.shared.articleFullscreenAvailable && traitCollection.userInterfaceIdiom == .phone && coordinator.isRootSplitCollapsed } + private lazy var articleIconSchemeHandler = ArticleIconSchemeHandler(coordinator: coordinator); private lazy var transition = ImageTransition(controller: self) private var clickedImageCompletion: (() -> Void)? @@ -358,15 +359,15 @@ extension WebViewController: WKNavigationDelegate { } } } - + func webView(_ webView: WKWebView, decidePolicyFor navigationAction: WKNavigationAction, decisionHandler: @escaping (WKNavigationActionPolicy) -> Void) { - + if navigationAction.navigationType == .linkActivated { guard let url = navigationAction.request.url else { decisionHandler(.allow) return } - + let components = URLComponents(url: url, resolvingAgainstBaseURL: false) if components?.scheme == "http" || components?.scheme == "https" { decisionHandler(.cancel) @@ -380,14 +381,14 @@ extension WebViewController: WKNavigationDelegate { self.openURLInSafariViewController(url) } } - + } else if components?.scheme == "mailto" { decisionHandler(.cancel) - + guard let emailAddress = url.percentEncodedEmailAddress else { return } - + if UIApplication.shared.canOpenURL(emailAddress) { UIApplication.shared.open(emailAddress, options: [.universalLinksOnly : false], completionHandler: nil) } else { @@ -397,11 +398,11 @@ extension WebViewController: WKNavigationDelegate { } } else if components?.scheme == "tel" { decisionHandler(.cancel) - + if UIApplication.shared.canOpenURL(url) { UIApplication.shared.open(url, options: [.universalLinksOnly : false], completionHandler: nil) } - + } else { decisionHandler(.allow) } @@ -413,7 +414,7 @@ extension WebViewController: WKNavigationDelegate { func webViewWebContentProcessDidTerminate(_ webView: WKWebView) { fullReload() } - + } // MARK: WKUIDelegate @@ -513,16 +514,16 @@ private extension WebViewController { func loadWebView(replaceExistingWebView: Bool = false) { guard isViewLoaded else { return } - + if !replaceExistingWebView, let webView = webView { self.renderPage(webView) return } - + coordinator.webViewProvider.dequeueWebView() { webView in - + webView.ready { - + // Add the webview webView.translatesAutoresizingMaskIntoConstraints = false self.view.insertSubview(webView, at: 0) @@ -532,7 +533,7 @@ private extension WebViewController { self.view.topAnchor.constraint(equalTo: webView.topAnchor), self.view.bottomAnchor.constraint(equalTo: webView.bottomAnchor) ]) - + // UISplitViewController reports the wrong size to WKWebView which can cause horizontal // rubberbanding on the iPad. This interferes with our UIPageViewController preventing // us from easily swiping between WKWebViews. This hack fixes that. @@ -554,11 +555,11 @@ private extension WebViewController { webView.configuration.userContentController.add(WrapperScriptMessageHandler(self), name: MessageName.showFeedInspector) self.renderPage(webView) - + } - + } - + } func renderPage(_ webView: PreloadedWebView?) { @@ -591,7 +592,8 @@ private extension WebViewController { "windowScrollY": String(windowScrollY) ] - let html = try! MacroProcessor.renderedText(withTemplate: ArticleRenderer.page.html, substitutions: substitutions) + var html = try! MacroProcessor.renderedText(withTemplate: ArticleRenderer.page.html, substitutions: substitutions) + html = ArticleRenderingSpecialCases.filterHTMLIfNeeded(baseURL: rendering.baseURL, html: html) webView.loadHTMLString(html, baseURL: ArticleRenderer.page.baseURL) } diff --git a/iOS/MainFeed/MainFeedViewController.swift b/iOS/MainFeed/MainFeedViewController.swift index db4af7b9e..4fc8afb16 100644 --- a/iOS/MainFeed/MainFeedViewController.swift +++ b/iOS/MainFeed/MainFeedViewController.swift @@ -7,6 +7,7 @@ // import UIKit +import WebKit import Account import Articles import RSCore diff --git a/iOS/MainTimeline/MainTimelineViewController.swift b/iOS/MainTimeline/MainTimelineViewController.swift index 1f190923b..ad7af3551 100644 --- a/iOS/MainTimeline/MainTimelineViewController.swift +++ b/iOS/MainTimeline/MainTimelineViewController.swift @@ -7,6 +7,7 @@ // import UIKit +import WebKit import RSCore import RSWeb import Account