Add ImageSize #23929
Add ImageSize #23929
Conversation
Generated by 🚫 Danger |
|
| App Name |  WordPress Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr23929-3b036a0 | |
| Version | 25.6 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 3b036a0 | |
| App Center Build | WPiOS - One-Offs #11237 |
|
| App Name |  Jetpack Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr23929-3b036a0 | |
| Version | 25.6 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 3b036a0 | |
| App Center Build | jetpack-installable-builds #10275 |
| /// Image size in **pixels**. | ||
| public struct ImageSize: Hashable, Sendable { | ||
| public let width: CGFloat | ||
| public let height: CGFloat |
There was a problem hiding this comment.
Should these properties be Int? There is no decimal pixel, right?
There was a problem hiding this comment.
Updated in the upcoming PR.
| self.height = height | ||
| } | ||
|
|
||
| public init(_ size: CGSize) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That makes sense; updating in the upcoming PR.
| /// 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)) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
currentanymore - It enforces you to either provide a
view(always has a valid trait collection) or ascaleexclicitly - Add
max(1, scale)just in case - Add documentation (
**points**)
Adds
ImageSizeto clarify thatImageDownloaderuses pixels as a parameter and make sure you can't blindly pass something likeview.frame.sizeto in, which is incorrect.This PR also adds
ImageRequestsupport toAsyncImageView.Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR submission checklist:
RELEASE-NOTES.txtif necessary.Testing checklist: