Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When passing in nil to clear the animated image, remove anything sitting in super.image #250

Merged

Conversation

michafaw
Copy link
Contributor

@michafaw michafaw commented Jul 8, 2021

We're already covering the case of clearing out super.nil when setAnimatedImage is not nil. This adds a small tweak to handle when you pass in nil.

@raphaelschaad
Copy link
Collaborator

Looks safe -- what bug does this fix?

@michafaw
Copy link
Contributor Author

@raphaelschaad If you have already set an animated image, then sometimes you'll have an image in super.image. The bug is that it is not always being updated (removed). There's already code in place (in the if (super.image) block above my change) to cover changing animated images, but not when going from animated image to no image.
Let me know and I'll create an issue + example project to link to.

@raphaelschaad raphaelschaad merged commit 07c0893 into Flipboard:master Aug 9, 2021
@raphaelschaad
Copy link
Collaborator

CHANGES 0723343

@freezy7
Copy link

freezy7 commented Mar 4, 2022

this cause this method sd_set normal image reset image = nil

if (!isGIF) {
                               strongSelf.image = image;
                               strongSelf.animatedImage = nil;
                               return;
                           }
FLAnimatedImageView+WebCache.h

- (void)sd_setImageWithURL:(nullable NSURL *)url
          placeholderImage:(nullable UIImage *)placeholder
                   options:(SDWebImageOptions)options
                  progress:(nullable SDWebImageDownloaderProgressBlock)progressBlock
                 completed:(nullable SDExternalCompletionBlock)completedBlock {
    __weak typeof(self)weakSelf = self;
    [self sd_internalSetImageWithURL:url
                    placeholderImage:placeholder
                             options:options
                        operationKey:nil
                       setImageBlock:^(UIImage *image, NSData *imageData) {
                           __strong typeof(weakSelf)strongSelf = weakSelf;
                           if (!strongSelf) {
                               return;
                           }
                           // Step 1. Check memory cache (associate object)
                           FLAnimatedImage *associatedAnimatedImage = image.sd_FLAnimatedImage;
                           if (associatedAnimatedImage) {
                               // Asscociated animated image exist
                               strongSelf.animatedImage = associatedAnimatedImage;
                               strongSelf.image = nil;
                               return;
                           }
                           // Step 2. Check if original compressed image data is "GIF"
                           BOOL isGIF = (image.sd_imageFormat == SDImageFormatGIF || [NSData sd_imageFormatForImageData:imageData] == SDImageFormatGIF);
                           if (!isGIF) {
                               strongSelf.image = image;
                               strongSelf.animatedImage = nil;
                               return;
                           }
                           // Step 3. Check if data exist or query disk cache
                           if (!imageData) {
                               NSString *key = [[SDWebImageManager sharedManager] cacheKeyForURL:url];
                               imageData = [[SDImageCache sharedImageCache] diskImageDataForKey:key];
                           }
                           // Step 4. Create FLAnimatedImage
                           FLAnimatedImage *animatedImage = SDWebImageCreateFLAnimatedImage(strongSelf, imageData);
                           // Step 5. Set animatedImage or normal image
                           if (animatedImage) {
                               if (strongSelf.sd_cacheFLAnimatedImage) {
                                   image.sd_FLAnimatedImage = animatedImage;
                               }
                               strongSelf.animatedImage = animatedImage;
                               strongSelf.image = nil;
                           } else {
                               strongSelf.animatedImage = nil;
                               strongSelf.image = image;
                           }
                       }
                            progress:progressBlock
                           completed:completedBlock];
}

@matrush
Copy link
Collaborator

matrush commented Apr 5, 2022

If you have already set an animated image, then sometimes you'll have an image in super.image.

This is not possible, as when we are setting the animated image we always check if there is super.image, if so, it will be cleared.

I feel this change itself doesn't make sense and causes the extra issues mentioned in the above. I will revert the changes for now.

@michafaw
Copy link
Contributor Author

michafaw commented Apr 5, 2022

@matrush Seems the cure is worse than the problem here. I agree this small change should be reverted.

The side effect (as seen in freezy7's quoted code) is caused by the order of setting animatedImage to nil after setting image to something valid. That code has no issue without this PR's change. (And honestly, those same = nil lines are unnecessary. As you pointed out, we clear when we set, so there is no need for the extra line in each of those 5 steps.)

Additional thoughts: I find it confusing that iv.image = something wipes out animatedImage and iv.animatedImage = something wipes out image, but setting either of those to nil does not. With the exception of the way setting an animatedImage gives you access to its currentFrame via iv.image, the general analogy is that this is a UIImageView that shows one image, not two. The internals for resetting "the image" do not fully reflect that same analogy.

itaen pushed a commit to DSAppTeam/FLAnimatedImage that referenced this pull request Apr 8, 2022
…ed_image_nil_fix"

This reverts commit 07c0893, reversing
changes made to d7558c8.

Flipboard#250 fix this error problem
matrush added a commit that referenced this pull request May 6, 2022
The image could be set before we are setting a `nil` `animatedImage`. We shouldn't clear it here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants