From df0f6008fc82a8a8887fe2f9797ad52ccd1e185b Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Mon, 24 Aug 2020 16:23:51 +0800 Subject: [PATCH 1/4] Refactory the current behavior to use transition. Introduce `SDWebImageForceTransitionAsync` (compared to old one `SDWebImageForceTransitionAlways`) --- SDWebImage/Core/SDImageCache.m | 4 +--- SDWebImage/Core/SDWebImageDefine.h | 8 ++++++++ SDWebImage/Core/UIView+WebCache.m | 27 ++++++++++++++++++++++++++- 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/SDWebImage/Core/SDImageCache.m b/SDWebImage/Core/SDImageCache.m index 25b39e97..f197f596 100644 --- a/SDWebImage/Core/SDImageCache.m +++ b/SDWebImage/Core/SDImageCache.m @@ -528,13 +528,10 @@ static NSString * _defaultDiskCacheDirectory; @autoreleasepool { NSData *diskData = [self diskImageDataBySearchingAllPathsForKey:key]; UIImage *diskImage; - SDImageCacheType cacheType = SDImageCacheTypeNone; if (image) { // the image is from in-memory cache, but need image data diskImage = image; - cacheType = SDImageCacheTypeMemory; } else if (diskData) { - cacheType = SDImageCacheTypeDisk; // decode image data only if in-memory cache missed diskImage = [self diskImageForKey:key data:diskData options:options context:context]; if (diskImage && self.config.shouldCacheImagesInMemory) { @@ -542,6 +539,7 @@ static NSString * _defaultDiskCacheDirectory; [self.memoryCache setObject:diskImage forKey:key cost:cost]; } } + SDImageCacheType cacheType = diskImage ? SDImageCacheTypeDisk : SDImageCacheTypeNone; if (doneBlock) { if (shouldQueryDiskSync) { diff --git a/SDWebImage/Core/SDWebImageDefine.h b/SDWebImage/Core/SDWebImageDefine.h index 90ee605f..ec8bfd08 100644 --- a/SDWebImage/Core/SDWebImageDefine.h +++ b/SDWebImage/Core/SDWebImageDefine.h @@ -161,6 +161,7 @@ typedef NS_OPTIONS(NSUInteger, SDWebImageOptions) { /** * By default, when you use `SDWebImageTransition` to do some view transition after the image load finished, this transition is only applied for image download from the network. This mask can force to apply view transition for memory and disk cache as well. + * @note This options naming may be `SDWebImageForceTransitionAlways` in the furture. Which does not check any condition, just do transition even we query the cache immediately from memory. See related `SDWebImageForceTransitionAsync`. */ SDWebImageForceTransition = 1 << 17, @@ -201,6 +202,13 @@ typedef NS_OPTIONS(NSUInteger, SDWebImageOptions) { * Use this flag to transform them anyway. */ SDWebImageTransformVectorImage = 1 << 23, + + /** + * By default, when you use `SDWebImageTransition` to do some view transition after the image load finished, this transition is only applied for image download from the network. This mask can force to apply view transition for condition when the callback from manager is asynchronous. + * For example, when memory cache hit, or disk cache who using `queryDiskDataSync`, this will trigger transition. The default behavior (without any options) only do transition when network query successed. + * @note This is used for UI rendering which relay the same runloop to avoid flashing, suitable for common use case cases. Which means, if user can see any waiting, do transition. else not. + */ + SDWebImageForceTransitionAsync = 1 << 24 }; diff --git a/SDWebImage/Core/UIView+WebCache.m b/SDWebImage/Core/UIView+WebCache.m index 160a77aa..c309b5e6 100644 --- a/SDWebImage/Core/UIView+WebCache.m +++ b/SDWebImage/Core/UIView+WebCache.m @@ -174,7 +174,32 @@ const int64_t SDWebImageProgressUnitCountUnknown = 1LL; #if SD_UIKIT || SD_MAC // check whether we should use the image transition SDWebImageTransition *transition = nil; - if (finished && (options & SDWebImageForceTransition || cacheType == SDImageCacheTypeNone)) { + BOOL shouldUseTransition = NO; + if (options & SDWebImageForceTransition) { + // Always + shouldUseTransition = YES; + } else if (cacheType == SDImageCacheTypeNone) { + // Default, from network + shouldUseTransition = YES; + } else { + // Async, from disk (and, user don't use sync query) + if (options & SDWebImageForceTransitionAsync) { + if (cacheType == SDImageCacheTypeMemory) { + shouldUseTransition = NO; + } else if (cacheType == SDImageCacheTypeDisk) { + if (options & SDWebImageQueryMemoryDataSync || options & SDWebImageQueryDiskDataSync) { + shouldUseTransition = NO; + } else { + shouldUseTransition = YES; + } + } else { + shouldUseTransition = NO; + } + } else { + shouldUseTransition = NO; + } + } + if (finished && shouldUseTransition) { transition = self.sd_imageTransition; } #endif From 6f61b7e37eb9dc13a2fbaebf028a1cfbddf9043d Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Mon, 24 Aug 2020 16:44:01 +0800 Subject: [PATCH 2/4] Added the test case about the doing transition when async callback --- SDWebImage/Core/SDWebImageDefine.h | 4 +-- Tests/Tests/SDWebCacheCategoriesTests.m | 44 +++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/SDWebImage/Core/SDWebImageDefine.h b/SDWebImage/Core/SDWebImageDefine.h index ec8bfd08..d0ab2ec2 100644 --- a/SDWebImage/Core/SDWebImageDefine.h +++ b/SDWebImage/Core/SDWebImageDefine.h @@ -205,8 +205,8 @@ typedef NS_OPTIONS(NSUInteger, SDWebImageOptions) { /** * By default, when you use `SDWebImageTransition` to do some view transition after the image load finished, this transition is only applied for image download from the network. This mask can force to apply view transition for condition when the callback from manager is asynchronous. - * For example, when memory cache hit, or disk cache who using `queryDiskDataSync`, this will trigger transition. The default behavior (without any options) only do transition when network query successed. - * @note This is used for UI rendering which relay the same runloop to avoid flashing, suitable for common use case cases. Which means, if user can see any waiting, do transition. else not. + * For example, when disk cache hit (and you don't use `queryDiskDataSync`), this will trigger transition because disk query is asynchronous. The default behavior (without any options) only do transition when network query successed. + * @note This is best used for UI rendering common cases, because even the cache is from disk, we may see a quick flashing of placeholder. In summary, if user can see any waiting (whether it takes 10 milliseconds or 10 minutes), do transition. else not. */ SDWebImageForceTransitionAsync = 1 << 24 }; diff --git a/Tests/Tests/SDWebCacheCategoriesTests.m b/Tests/Tests/SDWebCacheCategoriesTests.m index 666037dd..f7213437 100644 --- a/Tests/Tests/SDWebCacheCategoriesTests.m +++ b/Tests/Tests/SDWebCacheCategoriesTests.m @@ -296,6 +296,50 @@ [self waitForExpectationsWithCommonTimeout]; } +- (void)testUIViewTransitionAsyncWork { + XCTestExpectation *expectation = [self expectationWithDescription:@"UIView transition async does not work"]; + + // Attach a window, or CALayer will not submit drawing + UIImageView *imageView = [[UIImageView alloc] initWithFrame:CGRectMake(0, 0, 50, 50)]; + imageView.sd_imageTransition = SDWebImageTransition.fadeTransition; + imageView.sd_imageTransition.duration = 1; + +#if SD_UIKIT + [self.window addSubview:imageView]; +#else + imageView.wantsLayer = YES; + [self.window.contentView addSubview:imageView]; +#endif + + NSData *imageData = [NSData dataWithContentsOfFile:[self testJPEGPath]]; + UIImage *placeholder = [[UIImage alloc] initWithData:imageData]; + + // Ensure the image is cached in disk but not memory + [SDImageCache.sharedImageCache removeImageFromMemoryForKey:kTestJPEGURL]; + [SDImageCache.sharedImageCache removeImageFromDiskForKey:kTestJPEGURL]; + [SDImageCache.sharedImageCache storeImageDataToDisk:imageData forKey:kTestJPEGURL]; + + NSURL *originalImageURL = [NSURL URLWithString:kTestJPEGURL]; + __weak typeof(imageView) wimageView = imageView; + [imageView sd_setImageWithURL:originalImageURL + placeholderImage:placeholder + options:SDWebImageForceTransitionAsync | SDWebImageFromCacheOnly // Ensure we queired from disk cache + completed:^(UIImage * _Nullable image, NSError * _Nullable error, SDImageCacheType cacheType, NSURL * _Nullable imageURL) { + [SDImageCache.sharedImageCache removeImageFromMemoryForKey:kTestJPEGURL]; + [SDImageCache.sharedImageCache removeImageFromDiskForKey:kTestJPEGURL]; + __strong typeof(wimageView) simageView = imageView; + // Delay to let CALayer commit the transition in next runloop + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, kMinDelayNanosecond), dispatch_get_main_queue(), ^{ + // Check current view contains layer animation + NSArray *animationKeys = simageView.layer.animationKeys; + expect(animationKeys.count).beGreaterThan(0); + [expectation fulfill]; + }); + }]; + + [self waitForExpectationsWithCommonTimeout]; +} + - (void)testUIViewIndicatorWork { XCTestExpectation *expectation = [self expectationWithDescription:@"UIView indicator does not work"]; From 2c9eaccf237a247ab94e4cec2c20d82f5903c648 Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Thu, 27 Aug 2020 10:49:57 +0800 Subject: [PATCH 3/4] Remove the `SDWebImageForceTransitionAsync`, change the default behavior because it's most suitable for UI rendering and common use case. No need this hack. If user want, we may produce a `setTransitionBlock` in the future --- Examples/SDWebImage OSX Demo/ViewController.m | 2 +- SDWebImage/Core/SDWebImageDefine.h | 13 +++---------- SDWebImage/Core/UIView+WebCache.m | 19 ++++++++----------- Tests/Tests/SDWebCacheCategoriesTests.m | 10 +++++----- 4 files changed, 17 insertions(+), 27 deletions(-) diff --git a/Examples/SDWebImage OSX Demo/ViewController.m b/Examples/SDWebImage OSX Demo/ViewController.m index b59f236f..652a61a3 100644 --- a/Examples/SDWebImage OSX Demo/ViewController.m +++ b/Examples/SDWebImage OSX Demo/ViewController.m @@ -47,7 +47,7 @@ self.imageView4.sd_imageTransition = SDWebImageTransition.fadeTransition; self.imageView4.imageScaling = NSImageScaleProportionallyUpOrDown; self.imageView4.imageAlignment = NSImageAlignLeft; // supports NSImageView's layout properties - [self.imageView4 sd_setImageWithURL:[NSURL URLWithString:@"http://littlesvr.ca/apng/images/SteamEngine.webp"] placeholderImage:nil options:SDWebImageForceTransition]; + [self.imageView4 sd_setImageWithURL:[NSURL URLWithString:@"http://littlesvr.ca/apng/images/SteamEngine.webp"]]; NSMenu *menu2 = [[NSMenu alloc] initWithTitle:@"Toggle Animation"]; NSMenuItem *item2 = [menu2 addItemWithTitle:@"Toggle Animation" action:@selector(toggleAnimation:) keyEquivalent:@""]; item2.tag = 2; diff --git a/SDWebImage/Core/SDWebImageDefine.h b/SDWebImage/Core/SDWebImageDefine.h index d0ab2ec2..43df0464 100644 --- a/SDWebImage/Core/SDWebImageDefine.h +++ b/SDWebImage/Core/SDWebImageDefine.h @@ -160,8 +160,8 @@ typedef NS_OPTIONS(NSUInteger, SDWebImageOptions) { SDWebImageFromLoaderOnly = 1 << 16, /** - * By default, when you use `SDWebImageTransition` to do some view transition after the image load finished, this transition is only applied for image download from the network. This mask can force to apply view transition for memory and disk cache as well. - * @note This options naming may be `SDWebImageForceTransitionAlways` in the furture. Which does not check any condition, just do transition even we query the cache immediately from memory. See related `SDWebImageForceTransitionAsync`. + * By default, when you use `SDWebImageTransition` to do some view transition after the image load finished, this transition is only applied for image when the callback from manager is asynchronous (from network, or disk cache query) + * This mask can force to apply view transition for any cases, like memory cache query, or sync disk cache query. */ SDWebImageForceTransition = 1 << 17, @@ -201,14 +201,7 @@ typedef NS_OPTIONS(NSUInteger, SDWebImageOptions) { * We usually don't apply transform on vector images, because vector images supports dynamically changing to any size, rasterize to a fixed size will loss details. To modify vector images, you can process the vector data at runtime (such as modifying PDF tag / SVG element). * Use this flag to transform them anyway. */ - SDWebImageTransformVectorImage = 1 << 23, - - /** - * By default, when you use `SDWebImageTransition` to do some view transition after the image load finished, this transition is only applied for image download from the network. This mask can force to apply view transition for condition when the callback from manager is asynchronous. - * For example, when disk cache hit (and you don't use `queryDiskDataSync`), this will trigger transition because disk query is asynchronous. The default behavior (without any options) only do transition when network query successed. - * @note This is best used for UI rendering common cases, because even the cache is from disk, we may see a quick flashing of placeholder. In summary, if user can see any waiting (whether it takes 10 milliseconds or 10 minutes), do transition. else not. - */ - SDWebImageForceTransitionAsync = 1 << 24 + SDWebImageTransformVectorImage = 1 << 23 }; diff --git a/SDWebImage/Core/UIView+WebCache.m b/SDWebImage/Core/UIView+WebCache.m index c309b5e6..befff482 100644 --- a/SDWebImage/Core/UIView+WebCache.m +++ b/SDWebImage/Core/UIView+WebCache.m @@ -179,23 +179,20 @@ const int64_t SDWebImageProgressUnitCountUnknown = 1LL; // Always shouldUseTransition = YES; } else if (cacheType == SDImageCacheTypeNone) { - // Default, from network + // From network shouldUseTransition = YES; } else { - // Async, from disk (and, user don't use sync query) - if (options & SDWebImageForceTransitionAsync) { - if (cacheType == SDImageCacheTypeMemory) { + // From disk (and, user don't use sync query) + if (cacheType == SDImageCacheTypeMemory) { + shouldUseTransition = NO; + } else if (cacheType == SDImageCacheTypeDisk) { + if (options & SDWebImageQueryMemoryDataSync || options & SDWebImageQueryDiskDataSync) { shouldUseTransition = NO; - } else if (cacheType == SDImageCacheTypeDisk) { - if (options & SDWebImageQueryMemoryDataSync || options & SDWebImageQueryDiskDataSync) { - shouldUseTransition = NO; - } else { - shouldUseTransition = YES; - } } else { - shouldUseTransition = NO; + shouldUseTransition = YES; } } else { + // Not valid cache type, fallback shouldUseTransition = NO; } } diff --git a/Tests/Tests/SDWebCacheCategoriesTests.m b/Tests/Tests/SDWebCacheCategoriesTests.m index f7213437..f27af474 100644 --- a/Tests/Tests/SDWebCacheCategoriesTests.m +++ b/Tests/Tests/SDWebCacheCategoriesTests.m @@ -253,8 +253,8 @@ [self waitForExpectationsWithCommonTimeout]; } -- (void)testUIViewTransitionWork { - XCTestExpectation *expectation = [self expectationWithDescription:@"UIView transition does not work"]; +- (void)testUIViewTransitionFromNetworkWork { + XCTestExpectation *expectation = [self expectationWithDescription:@"UIView transition from network does not work"]; // Attach a window, or CALayer will not submit drawing UIImageView *imageView = [[UIImageView alloc] initWithFrame:CGRectMake(0, 0, 50, 50)]; @@ -296,8 +296,8 @@ [self waitForExpectationsWithCommonTimeout]; } -- (void)testUIViewTransitionAsyncWork { - XCTestExpectation *expectation = [self expectationWithDescription:@"UIView transition async does not work"]; +- (void)testUIViewTransitionFromDiskWork { + XCTestExpectation *expectation = [self expectationWithDescription:@"UIView transition from disk does not work"]; // Attach a window, or CALayer will not submit drawing UIImageView *imageView = [[UIImageView alloc] initWithFrame:CGRectMake(0, 0, 50, 50)]; @@ -323,7 +323,7 @@ __weak typeof(imageView) wimageView = imageView; [imageView sd_setImageWithURL:originalImageURL placeholderImage:placeholder - options:SDWebImageForceTransitionAsync | SDWebImageFromCacheOnly // Ensure we queired from disk cache + options:SDWebImageFromCacheOnly // Ensure we queired from disk cache completed:^(UIImage * _Nullable image, NSError * _Nullable error, SDImageCacheType cacheType, NSURL * _Nullable imageURL) { [SDImageCache.sharedImageCache removeImageFromMemoryForKey:kTestJPEGURL]; [SDImageCache.sharedImageCache removeImageFromDiskForKey:kTestJPEGURL]; From bcda3f5000adc378773784c7808a2d8a874250b8 Mon Sep 17 00:00:00 2001 From: DreamPiggy Date: Thu, 27 Aug 2020 11:08:12 +0800 Subject: [PATCH 4/4] Change to always return SDImageCacheTypeDisk cache type when querying memory cache (unless user cancel the query) --- SDWebImage/Core/SDImageCache.m | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/SDWebImage/Core/SDImageCache.m b/SDWebImage/Core/SDImageCache.m index f197f596..c2f18a9d 100644 --- a/SDWebImage/Core/SDImageCache.m +++ b/SDWebImage/Core/SDImageCache.m @@ -539,14 +539,13 @@ static NSString * _defaultDiskCacheDirectory; [self.memoryCache setObject:diskImage forKey:key cost:cost]; } } - SDImageCacheType cacheType = diskImage ? SDImageCacheTypeDisk : SDImageCacheTypeNone; if (doneBlock) { if (shouldQueryDiskSync) { - doneBlock(diskImage, diskData, cacheType); + doneBlock(diskImage, diskData, SDImageCacheTypeDisk); } else { dispatch_async(dispatch_get_main_queue(), ^{ - doneBlock(diskImage, diskData, cacheType); + doneBlock(diskImage, diskData, SDImageCacheTypeDisk); }); } }