Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Modules/Sources/WordPressMedia/ImageDecoder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ private extension Data {
}
}

private extension CGSize {
extension CGSize {
func scaled(by scale: CGFloat) -> CGSize {
CGSize(width: width * scale, height: height * scale)
}
Expand Down
16 changes: 11 additions & 5 deletions Modules/Sources/WordPressMedia/ImageDownloader.swift
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public final class ImageDownloader {
return image
}
let data = try await data(for: request)
let image = try await ImageDecoder.makeImage(from: data, size: options.size)
let image = try await ImageDecoder.makeImage(from: data, size: options.size.map(CGSize.init))
if options.isMemoryCacheEnabled {
cache[key] = image
}
Expand Down Expand Up @@ -69,24 +69,30 @@ public final class ImageDownloader {

// MARK: - Caching

/// Returns an image from the memory cache.
nonisolated public func cachedImage(for request: ImageRequest) -> UIImage? {
guard let imageURL = request.source.url else { return nil }
return cachedImage(for: imageURL, size: request.options.size)
}

/// Returns an image from the memory cache.
///
/// - note: Use it to retrieve the image synchronously, which is no not possible
/// with the async functions.
nonisolated public func cachedImage(for imageURL: URL, size: CGSize? = nil) -> UIImage? {
nonisolated public func cachedImage(for imageURL: URL, size: ImageSize? = nil) -> UIImage? {
cache[makeKey(for: imageURL, size: size)]
}

nonisolated public func setCachedImage(_ image: UIImage?, for imageURL: URL, size: CGSize? = nil) {
nonisolated public func setCachedImage(_ image: UIImage?, for imageURL: URL, size: ImageSize? = nil) {
cache[makeKey(for: imageURL, size: size)] = image
}

private nonisolated func makeKey(for imageURL: URL?, size: CGSize?) -> String {
private nonisolated func makeKey(for imageURL: URL?, size: ImageSize?) -> String {
guard let imageURL else {
assertionFailure("The request.url was nil") // This should never happen
return ""
}
return imageURL.absoluteString + (size.map { "?size=\($0)" } ?? "")
return imageURL.absoluteString + (size.map { "?w=\($0.width),h=\($0.height)" } ?? "")
}

public func clearURLSessionCache() {
Expand Down
40 changes: 37 additions & 3 deletions Modules/Sources/WordPressMedia/ImageRequest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ public final class ImageRequest: Sendable {
}

public struct ImageRequestOptions: Hashable, Sendable {
/// Resize the thumbnail to the given size (in pixels). By default, `nil`.
public var size: CGSize?
/// Resize the thumbnail to the given size. By default, `nil`.
public var size: ImageSize?

/// If enabled, uses ``MemoryCache`` for caching decompressed images.
public var isMemoryCacheEnabled = true
Expand All @@ -39,7 +39,7 @@ public struct ImageRequestOptions: Hashable, Sendable {
public var isDiskCacheEnabled = true

public init(
size: CGSize? = nil,
size: ImageSize? = nil,
isMemoryCacheEnabled: Bool = true,
isDiskCacheEnabled: Bool = true
) {
Expand All @@ -48,3 +48,37 @@ public struct ImageRequestOptions: Hashable, Sendable {
self.isDiskCacheEnabled = isDiskCacheEnabled
}
}

/// Image size in **pixels**.
public struct ImageSize: Hashable, Sendable {
public let width: CGFloat
public let height: CGFloat
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these properties be Int? There is no decimal pixel, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in the upcoming PR.


public init(width: CGFloat, height: CGFloat) {
self.width = width
self.height = height
}

public init(_ size: CGSize) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you thinking changing this API to init(pixels:)? IMHO, the name init(pixels:) matches the init(scaling...) ones to make it a bit clearer that this API accepts pixels sizes, and the "scaling" ones accept points sizes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense; updating in the upcoming PR.

self.width = size.width
self.height = size.height
}

/// Initializes `ImageSize` with the given size scaled for the given view.
@MainActor
public init(scaling size: CGSize, in view: UIView) {
self.init(size.scaled(by: view.traitCollection.displayScale))
}

/// Initializes `ImageSize` with the given size scaled for the current trait
/// collection display scale.
public init(scaling size: CGSize) {
self.init(size.scaled(by: UITraitCollection.current.displayScale))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using UITraitCollection.current.displayScale as the default scale (even though that may be the correct value 90% of the time) may lead to misuse. What do you think about adding a non-optional scale argument here? Maybe, even include "points" in the name: ImageSize(points:scale:)?

Of course, you can add some convenient methods on top of it, like ImageSize(points: CGSize, in: UITraitCollection), to make it easier to use the app wide scale easier: ImageSize(points: ..., in: .current)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you can misuse current. I'm not entirely sure I understand how it works. I'm chasing it to the following:

    /// A convenience initializer that creates `ImageSize` with the given size
    /// in **points** scaled for the given view.
    @MainActor
    public init(scaling size: CGSize, in view: UIView) {
        self.init(scaling: size, scale: view.traitCollection.displayScale)
    }

    /// Initializes `ImageSize` with the given size in **points** scaled for the
    /// current trait collection display scale.
    public init(scaling size: CGSize, scale: CGFloat) {
        self.init(pixels: size.scaled(by: max(1, scale)))
    }
  • It doesn't use current anymore
  • It enforces you to either provide a view (always has a valid trait collection) or a scale exclicitly
  • Add max(1, scale) just in case
  • Add documentation (**points**)

}
}

extension CGSize {
init(_ size: ImageSize) {
self.init(width: size.width, height: size.height)
}
}
6 changes: 3 additions & 3 deletions Modules/Tests/WordPressMediaTests/ImageDownloaderTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import OHHTTPStubsSwift

// WHEN
let options = ImageRequestOptions(
size: CGSize(width: 256, height: 256),
size: ImageSize(width: 256, height: 256),
isMemoryCacheEnabled: false,
isDiskCacheEnabled: false
)
Expand All @@ -46,7 +46,7 @@ import OHHTTPStubsSwift

// WHEN
let options = ImageRequestOptions(
size: CGSize(width: 256, height: 256),
size: ImageSize(width: 256, height: 256),
isMemoryCacheEnabled: false,
isDiskCacheEnabled: false
)
Expand All @@ -72,7 +72,7 @@ import OHHTTPStubsSwift
let imageURL = try #require(URL(string: "https://example.files.wordpress.com/2023/09/image.jpg"))
try mockResponse(withResource: "test-image", fileExtension: "jpg")

let size = CGSize(width: 256, height: 256)
let size = ImageSize(width: 256, height: 256)
let options = ImageRequestOptions(
size: size,
isMemoryCacheEnabled: true,
Expand Down
10 changes: 7 additions & 3 deletions WordPress/Classes/Utility/Media/AsyncImageView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,14 @@ final class AsyncImageView: UIView {
func setImage(
with imageURL: URL,
host: MediaHost? = nil,
size: CGSize? = nil,
completion: (@MainActor (Result<UIImage, Error>) -> Void)? = nil
size: ImageSize? = nil
) {
controller.setImage(with: imageURL, host: host, size: size, completion: completion)
let request = ImageRequest(url: imageURL, host: host, options: ImageRequestOptions(size: size))
controller.setImage(with: request)
}

func setImage(with request: ImageRequest, completion: (@MainActor (Result<UIImage, Error>) -> Void)? = nil) {
controller.setImage(with: request, completion: completion)
}

private func setState(_ state: ImageLoadingController.State) {
Expand Down
22 changes: 4 additions & 18 deletions WordPress/Classes/Utility/Media/ImageLoadingController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -27,28 +27,17 @@ final class ImageLoadingController {
}

/// - parameter completion: Gets called on completion _after_ `onStateChanged`.
func setImage(
with imageURL: URL,
host: MediaHost? = nil,
size: CGSize? = nil,
completion: (@MainActor (Result<UIImage, Error>) -> Void)? = nil
) {
func setImage(with request: ImageRequest, completion: (@MainActor (Result<UIImage, Error>) -> Void)? = nil) {
task?.cancel()

if let image = downloader.cachedImage(for: imageURL, size: size) {
if let image = downloader.cachedImage(for: request) {
onStateChanged(.success(image))
completion?(.success(image))
} else {
onStateChanged(.loading)
task = Task { @MainActor [downloader, weak self] in
do {
let options = ImageRequestOptions(size: size)
let image: UIImage
if let host {
image = try await downloader.image(from: imageURL, host: host, options: options)
} else {
image = try await downloader.image(from: imageURL, options: options)
}
let image = try await downloader.image(for: request)
// This line guarantees that if you cancel on the main thread,
// none of the `onStateChanged` callbacks get called.
guard !Task.isCancelled else { return }
Expand All @@ -63,10 +52,7 @@ final class ImageLoadingController {
}
}

func setImage(
with media: Media,
size: MediaImageService.ImageSize
) {
func setImage(with media: Media, size: MediaImageService.ImageSize) {
task?.cancel()

if let image = service.getCachedThumbnail(for: .init(media), size: size) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,12 @@ struct ImageViewExtensions {
}
}

func setImage(
with imageURL: URL,
host: MediaHost? = nil,
size: CGSize? = nil,
completion: (@MainActor (Result<UIImage, Error>) -> Void)? = nil
) {
controller.setImage(with: imageURL, host: host, size: size, completion: completion)
func setImage(with imageURL: URL, host: MediaHost? = nil, size: ImageSize? = nil) {
setImage(with: ImageRequest(url: imageURL, host: host, options: ImageRequestOptions(size: size)))
}

func setImage(with request: ImageRequest, completion: (@MainActor (Result<UIImage, Error>) -> Void)? = nil) {
controller.setImage(with: request, completion: completion)
}

var controller: ImageLoadingController {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,7 @@ final class BlazeCampaignTableViewCell: UITableViewCell, Reusable {
let host = MediaHost(with: blog, failure: { error in
WordPressAppDelegate.crashLogging?.logError(error)
})
let preferredSize = CGSize(width: Metrics.featuredImageSize, height: Metrics.featuredImageSize)
.scaled(by: UITraitCollection.current.displayScale)
let preferredSize = ImageSize(scaling: CGSize(width: Metrics.featuredImageSize, height: Metrics.featuredImageSize), in: self)
featuredImageView.setImage(with: imageURL, host: host, size: preferredSize)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,8 @@ final class BlazePostPreviewView: UIView {

if let url = post.featuredImageURL {
featuredImageView.isHidden = false
let preferredSize = CGSize(width: featuredImageView.frame.width, height: featuredImageView.frame.height)
.scaled(by: UITraitCollection.current.displayScale)
featuredImageView.setImage(with: url, host: MediaHost(post), size: preferredSize)
let targetSize = ImageSize(scaling: featuredImageView.frame.size, in: self)
featuredImageView.setImage(with: url, host: MediaHost(post), size: targetSize)

} else {
featuredImageView.isHidden = true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,7 @@ final class DashboardBlazeCampaignView: UIView {
let host = MediaHost(with: blog, failure: { error in
WordPressAppDelegate.crashLogging?.logError(error)
})
let targetSize = Constants.imageSize
.scaled(by: UITraitCollection.current.displayScale)
let targetSize = ImageSize(scaling: Constants.imageSize, in: self)
imageView.setImage(with: imageURL, host: host, size: targetSize)
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import UIKit
import WordPressMedia

final class ExternalMediaPickerCollectionCell: UICollectionViewCell {
private let imageView = AsyncImageView()
Expand All @@ -22,7 +23,7 @@ final class ExternalMediaPickerCollectionCell: UICollectionViewCell {
imageView.prepareForReuse()
}

func configure(imageURL: URL, size: CGSize) {
func configure(imageURL: URL, size: ImageSize) {
imageView.setImage(with: imageURL, size: size)
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import UIKit
import WordPressMedia

protocol ExternalMediaPickerViewDelegate: AnyObject {
/// If the user cancels the flow, the selection is empty.
Expand Down Expand Up @@ -235,7 +236,7 @@ final class ExternalMediaPickerViewController: UIViewController, UICollectionVie
func collectionView(_ collectionView: UICollectionView, cellForItemAt indexPath: IndexPath) -> UICollectionViewCell {
let cell = collectionView.dequeueReusableCell(withReuseIdentifier: Self.cellReuseID, for: indexPath) as! ExternalMediaPickerCollectionCell
let item = dataSource.assets[indexPath.item]
cell.configure(imageURL: item.thumbnailURL, size: flowLayout.itemSize.scaled(by: UIScreen.main.scale))
cell.configure(imageURL: item.thumbnailURL, size: ImageSize(scaling: flowLayout.itemSize))
return cell
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import UIKit
import WordPressUI
import WordPressMedia

final class LightboxImagePageViewController: UIViewController {
private(set) var scrollView = LightboxImageScrollView()
Expand Down Expand Up @@ -51,7 +52,7 @@ final class LightboxImagePageViewController: UIViewController {
case .image(let image):
setState(.success(image))
case .asset(let asset):
controller.setImage(with: asset.sourceURL, host: asset.host)
controller.setImage(with: ImageRequest(url: asset.sourceURL, host: asset.host))
case .media(let media):
controller.setImage(with: media, size: .original)
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import Foundation
import WordPressShared
import WordPressUI
import WordPressMedia
import Gravatar

// MARK: - NoteBlockHeaderTableViewCell
Expand Down Expand Up @@ -70,7 +71,7 @@ class NoteBlockHeaderTableViewCell: NoteBlockTableViewCell {
if let gravatar = AvatarURL(url: url) {
authorAvatarImageView.downloadGravatar(gravatar, placeholder: .gravatarPlaceholderImage, animate: true)
} else {
authorAvatarImageView.wp.setImage(with: url, size: SiteIconViewModel.Size.regular.size)
authorAvatarImageView.wp.setImage(with: url, size: ImageSize(scaling: SiteIconViewModel.Size.regular.size))
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ final class PostCompactCell: UITableViewCell, Reusable {
featuredImageView.isHidden = false

let host = MediaHost(post)
let targetSize = Constants.imageSize.scaled(by: traitCollection.displayScale)
let targetSize = ImageSize(scaling: Constants.imageSize, in: self)
featuredImageView.setImage(with: url, host: host, size: targetSize)
} else {
featuredImageView.isHidden = true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import Foundation
import AutomatticTracks
import WordPressShared
import WordPressUI
import WordPressMedia

final class ReaderCrossPostCell: ReaderStreamBaseCell {
private let view = ReaderCrossPostView()
Expand Down Expand Up @@ -132,8 +133,7 @@ private final class ReaderCrossPostView: UIView {

avatarView.setPlaceholder(UIImage(named: "post-blavatar-placeholder"))
if let avatarURL = post.avatarURLForDisplay() {
let avatarSize = CGSize(width: avatarSize, height: avatarSize)
.scaled(by: UITraitCollection.current.displayScale)
let avatarSize = ImageSize(scaling: CGSize(width: avatarSize, height: avatarSize))
avatarView.setImage(with: avatarURL, size: avatarSize)
}
}
Expand Down
11 changes: 5 additions & 6 deletions WordPress/Classes/ViewRelated/Reader/Cards/ReaderPostCell.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import SwiftUI
import UIKit
import Combine
import WordPressShared
import WordPressMedia

final class ReaderPostCell: ReaderStreamBaseCell {
private let view = ReaderPostCellView()
Expand Down Expand Up @@ -51,13 +52,12 @@ final class ReaderPostCell: ReaderStreamBaseCell {
super.updateConstraints()
}

static func preferredCoverSize(in window: UIWindow, isCompact: Bool) -> CGSize {
static func preferredCoverSize(in window: UIWindow, isCompact: Bool) -> ImageSize {
var coverWidth = ReaderPostCell.regularCoverWidth
if isCompact {
coverWidth = min(window.bounds.width, window.bounds.height) - ReaderStreamBaseCell.insets.left * 2
}
return CGSize(width: coverWidth, height: coverWidth)
.scaled(by: min(2, window.traitCollection.displayScale))
return ImageSize(scaling: CGSize(width: coverWidth, height: coverWidth), in: window)
}
}

Expand Down Expand Up @@ -314,7 +314,7 @@ private final class ReaderPostCellView: UIView {
}
}

private var preferredCoverSize: CGSize? {
private var preferredCoverSize: ImageSize? {
guard let window = window ?? UIApplication.shared.mainWindow else { return nil }
return ReaderPostCell.preferredCoverSize(in: window, isCompact: isCompact)
}
Expand Down Expand Up @@ -345,8 +345,7 @@ private final class ReaderPostCellView: UIView {

private func setAvatar(with viewModel: ReaderPostCellViewModel) {
avatarView.setPlaceholder(UIImage(named: "post-blavatar-placeholder"))
let avatarSize = CGSize(width: ReaderPostCell.avatarSize, height: ReaderPostCell.avatarSize)
.scaled(by: UITraitCollection.current.displayScale)
let avatarSize = ImageSize(scaling: CGSize(width: ReaderPostCell.avatarSize, height: ReaderPostCell.avatarSize))
if let avatarURL = viewModel.avatarURL {
avatarView.setImage(with: avatarURL, size: avatarSize)
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ class ReaderDetailFeaturedImageView: UIView, NibLoadable {
completionHandler(CGSize(width: 1000, height: 1000 * ReaderPostCell.coverAspectRatio))
}

imageView.setImage(with: imageURL, host: MediaHost(post)) { [weak self] result in
imageView.setImage(with: ImageRequest(url: imageURL, host: MediaHost(post))) { [weak self] result in
guard let self else { return }
switch result {
case .success:
Expand Down
Loading