Closed sonique6784 closed 5 years ago
Thanks for the PR! I would like to wait for the next beta before fully reviewing this, as the available APIs may change. In any event, it seems like the proper replacement for the init
call is to create an instance and then set the memory and disk capacities directly, so that would need to be done as part of this PR. There doesn't appear to be a way to set the path, which is why I'm hoping additional APIs are coming.
According to the header in the new build, the replacement method is init(memoryCapacity:diskCapacity:directory:)
.
Yeah the way I patched this for my app as of beta3 was:
open class func defaultURLCache() -> URLCache {
#if !targetEnvironment(UIKitForMac)
return URLCache(
memoryCapacity: 20 * 1024 * 1024, // 20 MB
diskCapacity: 150 * 1024 * 1024, // 150 MB
diskPath: "org.alamofire.imagedownloader"
)
#else
return URLCache(
memoryCapacity: 20 * 1024 * 1024, // 20 MB,
diskCapacity: 150 * 1024 * 1024, // 150 MB,
directory: FileManager.default.temporaryDirectory.appendingPathComponent("org.alamofire.imagedownloader")
)
#endif
}
Any chance this is going to be merged in to a beta soon?
@sonique6784 Looks like this approach is now out of date. There is a new initializer for all platforms: nit(memoryCapacity:diskCapacity:directory:)
. It should be implemented with a runtime check, not just a check for Catalyst.
if #available(OSX 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) {
return URLCache(
memoryCapacity: 20 * 1024 * 1024, // 20 MB
diskCapacity: 150 * 1024 * 1024, // 150 MB
diskPath: "org.alamofire.imagedownloader"
)
} else {
return URLCache(
memoryCapacity: 20 * 1024 * 1024, // 20 MB,
diskCapacity: 150 * 1024 * 1024, // 150 MB,
directory: FileManager.default.temporaryDirectory.appendingPathComponent("org.alamofire.imagedownloader")
)
}
Once that's updated we can merge.
@jshier, unfortunately, the Multiplatform initializer is not backward compatible, and doesn't exists on previous iOS version. therefore we have two options:
#available
, down side we can't set the directory as the shared signature only allow to construct a "empty" URLCache()
then we can set only memory and disk capacitytargetEnvironment(macCatalyst)
down side we can only target Mac Catalyst, which mean we use the old URLCache() constructor for any other platform (iOS, watchOS, tvOS)
open class func defaultURLCache() -> URLCache {
let memoryCapacity = 20 * 1024 * 1024 // 20 MB
let diskCapacity = 150 * 1024 * 1024 // 150 MB,
let storageName = "org.alamofire.imagedownloader"
// approach 1
if #available(OSX 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *) {
return URLCache(
memoryCapacity: memoryCapacity,
diskCapacity: diskCapacity,
directory: URL(string: storageName)
)
} else {
let urlCache = URLCache()
urlCache.memoryCapacity = memoryCapacity
urlCache.diskCapacity = diskCapacity
return urlCache
}
// approach 2
#if targetEnvironment(macCatalyst)
return URLCache(
memoryCapacity: memoryCapacity,
diskCapacity: diskCapacity,
directory: URL(string: storageName)
)
#else
return URLCache(memoryCapacity: memoryCapacity,
diskCapacity: diskCapacity,
diskPath: FileManager.default.temporaryDirectory.appendingPathComponent(storageName))
#endif
}
My personal preference is for approach 2 but let me know which approach you prefer.
Also, just something to be aware of per the docs on the old initializer (and is probably why they deprecated it):
path
In macOS, path is the location at which to store the on-disk cache.
In iOS, path is the name of a subdirectory of the application’s default cache directory in which to store the on-disk cache (the subdirectory is created if it does not exist).
@msmollin, I believe iOS apps are sandboxed and macCatalyst apps as well. So this is not really a worry. In addition, both approaches will be using the new initilizer for macCatalyst, so we are safe with the path I believe.
Valid point. Unrelated to that, does URL(string: storageName)
actually work? I would've figured you needed URL(fileURLWithPath: storageName, relativeTo: FileManager.default.temporaryDirectory)
to avoid escaping issues.
It does work, but I think you are right we could use : FileManager.default.temporaryDirectory.appendingPathComponent(storageName)
for both macCatalyst and iOS, watchOS, tvOS.
The current code (stable) is diskPath: "org.alamofire.imagedownloader"
so that's why I'm not sure if we need to set the path to temporary, but it seems cleaner.
Yeah unsure. @jshier might know legacy reasons for it. My guess is there hasn't been much use of AlamofireImage by non-sandboxed mac apps which would have to deal with real paths. We can leave for now if it's working.
@sonique6784 I think this is close, but I'd like to fix the apparent cache path bug on macOS as well. It's also unfortunate that using targetEnvironment(macCatalyst)
is a permanent warning on older versions of Xcode. Here's what I came up with:
open class func defaultURLCache() -> URLCache {
let memoryCapacity = 50 * 1024 * 1024 // 50 MB
let diskCapacity = 150 * 1024 * 1024 // 150 MB
let cacheDirectory = FileManager.default.urls(for: .cachesDirectory, in: .userDomainMask).first
let imageDownloaderPath = "org.alamofire.imageDownloader"
#if targetEnvironment(macCatalyst)
return URLCache(memoryCapacity: memoryCapacity,
diskCapacity: diskCapacity,
directory: cacheDirectory?.appendingPathComponent(imageDownloaderPath))
#else
#if os(macOS)
return URLCache(memoryCapacity: memoryCapacity,
diskCapacity: diskCapacity,
diskPath: cacheDirectory?.appendingPathComponent(imageDownloaderPath).absoluteString)
#else
return URLCache(memoryCapacity: memoryCapacity, diskCapacity: diskCapacity, diskPath: imageDownloaderPath)
#endif
#endif
}
It's unfortunate the new initializer isn't available on all platforms.
Goals :soccer:
Bring support for macOS (project catalyst) iPad apps can run on macOS Catalina, this PR bring support to that.
Implementation Details :construction:
simply implement targetEnvironment around default cache, so it works with URLCache constructor provided on macOS / UIKit
Testing Details :mag:
No test added