Skip to content
This repository was archived by the owner on Aug 3, 2022. It is now read-only.

Refactor custom animated image class #3

Merged
merged 3 commits into from
Aug 28, 2018

Conversation

dreampiggy
Copy link
Collaborator

@dreampiggy dreampiggy commented Aug 20, 2018

Changes

The previous version (0.2.0), use a SDWebImageFLCoder custom coder, to create custom FLAnimatedImage for category. However, since it need extra config after import this plugin (not so out-of-box), and the associated object way of creating UIImage which represent FLAnimatedImage is not so convient. So this PR use another solution, by using Animated Image Customization.

Solution

This PR create a subclass of UIImage called SDFLAnimatedImage, and it conforms to SDAnimatedImage protocol. So our FLAnimatedImage+WebCache does not need any extra config, the wrapper class should be used to create and represent a actual FLAnimatedImage.

And it's now more easy to use, because you know that SDFLAnimatedImage is always GIF representation. So that it can be easy to detect and debug.

@dreampiggy dreampiggy added the refactory Code refactory label Aug 20, 2018
@dreampiggy dreampiggy requested a review from bpoplauschi August 21, 2018 06:14
@dreampiggy
Copy link
Collaborator Author

dreampiggy commented Aug 21, 2018

This changes need to wait SDWebImage/SDWebImage#2453 to be merged in. So that we can keep the SDWebImageContextOptimalFrameCacheSize && SDWebImageContextPredrawingEnabled this usage. Or that this options will be un-available because the context options is not avaible for animated image class.

However, since this option are actually rare for most of usage, even that PR is not merged in, I think this solution is still better than the previous custom coder solution. Because now it don't need a extra coder, and make the SDFLAnimatedImage wrapper easy to use and determine than using a associated object. And it allows you to use it with the SDAnimatedImageView (Though it's not encouraged).

Example/Podfile Outdated
@@ -3,6 +3,7 @@ inhibit_all_warnings!

target 'SDWebImageFLPlugin_Example' do
pod 'SDWebImageFLPlugin', :path => '../'
pod 'SDWebImage/Core', :path => '../../SDWebImage'
Copy link
Member

Choose a reason for hiding this comment

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

we don't want to add this, it will not work. Also breaks the CI

Copy link
Collaborator Author

@dreampiggy dreampiggy Aug 28, 2018

Choose a reason for hiding this comment

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

@bpoplauschi It's local podfile because I test it locally to ensure it works. But that SDWebImage/SDWebImage#2453 (comment) was not been merged when this PR was created.

Now I can specify the branch dependency and update podfile.lock

Copy link
Member

Choose a reason for hiding this comment

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

Got it :)

Copy link
Member

@bpoplauschi bpoplauschi left a comment

Choose a reason for hiding this comment

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

Other than the Podfile change, looks good. Waiting for SDWebImage/SDWebImage#2453 to be merged

Supports custom extra initializer args for FLAnimatedImage
@dreampiggy dreampiggy force-pushed the refactor_custom_animated_image_class branch from a04be4a to 74692f2 Compare August 28, 2018 09:42
@bpoplauschi
Copy link
Member

Waiting for CI

@dreampiggy
Copy link
Collaborator Author

dreampiggy commented Aug 28, 2018

Opps...The CI have a pod lib lint. Which will resolve the dependency and download the 5.0.0-beta2 code. Maybe I have to disable it temporally, only check the demo project build.

Later I will also move the test case into this repo.

@bpoplauschi
Copy link
Member

Never mind. I can merge it as it is, it will work when we release a new version.

@bpoplauschi bpoplauschi merged commit c82d8ba into master Aug 28, 2018
@bpoplauschi bpoplauschi deleted the refactor_custom_animated_image_class branch August 28, 2018 09:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
refactory Code refactory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants