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

Added autoplay control property to AnimatedImageView (autoPlayAnimatedImage) #3003

Merged
merged 7 commits into from May 10, 2020

Conversation

liulichao123
Copy link
Contributor

@liulichao123 liulichao123 commented May 7, 2020

New Pull Request Checklist

  • I have read and understood the CONTRIBUTING guide

  • I have read the Documentation

  • I have searched for a similar pull request in the project and found none

  • I have updated this branch with the latest master to avoid conflicts (via merge from master or rebase)

  • I have added the required tests to prove the fix/feature I am adding

  • I have updated the documentation (if necessary)

  • I have run the tests and they pass

  • I have run the lint and it passes (pod lib lint)

This merge request fixes / refers to the following issues: ...

Pull Request Description

Sometimes we set up a GIF and don't want it to play right away, so we need to add a control
Added autoplay control property to AnimatedImageView (autoPlayAnimatedImage)
(reference YYAnimatedImageView)

@dreampiggy
Copy link
Contributor

dreampiggy commented May 8, 2020

@liulichao123 Sounds a interesting and useful feature.

Can you add a test case for this as well ? To ensure it always work. You can check other's animated image testing code, check the isAnimating status for the image view with new property set to NO.

@dreampiggy dreampiggy changed the title Added autoplay control property to AnimatedImageView (autoPlayAnimate… Added autoplay control property to AnimatedImageView (autoPlayAnimatedImage) May 8, 2020
@liulichao123
Copy link
Contributor Author

@liulichao123 Sounds a interesting and useful feature.

Can you add a test case for this as well ? To ensure it always work. You can check other's animated image testing code, check the isAnimating status for the image view with new property set to NO.

Test cases have been added

@dreampiggy
Copy link
Contributor

dreampiggy commented May 9, 2020

❌ /Users/travis/build/SDWebImage/SDWebImage/Tests/Tests/SDAnimatedImageTest.m:501:26: property 'animating' not found on object of type 'SDAnimatedImageView *'; did you mean 'animations'?

@liulichao123 Test case failed to compile on macOS.

macOS AppKit use animates property from NSImageView, not isAnimating which is UIKit's UIImageView. Fix it using the macro, see other test cases.

@dreampiggy dreampiggy added this to the 5.8.0 milestone May 9, 2020
@codecov
Copy link

codecov bot commented May 9, 2020

Codecov Report

Merging #3003 into master will increase coverage by 0.16%.
The diff coverage is 75.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3003      +/-   ##
==========================================
+ Coverage   83.78%   83.95%   +0.16%     
==========================================
  Files          69       69              
  Lines        7408     7534     +126     
==========================================
+ Hits         6207     6325     +118     
- Misses       1201     1209       +8     
Flag Coverage Δ
#ios 84.08% <86.15%> (+0.14%) ⬆️
#macos 83.58% <75.52%> (+0.08%) ⬆️
#tvos 83.95% <86.15%> (-0.01%) ⬇️
Impacted Files Coverage Δ
SDWebImage/Core/SDWebImageDefine.m 86.76% <ø> (ø)
...DWebImageMapKit/MapKit/MKAnnotationView+WebCache.m 65.51% <ø> (ø)
SDWebImage/Core/SDWebImageTransition.m 70.52% <41.66%> (+10.29%) ⬆️
...bImage/Core/SDWebImageDownloaderResponseModifier.m 80.64% <73.68%> (-11.03%) ⬇️
...ebImage/Core/SDWebImageDownloaderRequestModifier.m 83.87% <78.94%> (-7.80%) ⬇️
SDWebImage/Core/SDWebImageManager.m 86.85% <85.71%> (-3.37%) ⬇️
SDWebImage/Core/SDAnimatedImageView.m 84.93% <88.23%> (+2.26%) ⬆️
SDWebImage/Core/SDImageIOAnimatedCoder.m 85.64% <91.66%> (+0.07%) ⬆️
SDWebImage/Core/SDImageIOCoder.m 88.94% <100.00%> (+0.22%) ⬆️
SDWebImage/Core/UIView+WebCache.m 90.57% <100.00%> (+0.48%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a430b2...3e87488. Read the comment docs.

[self stopAnimating];
#else
Copy link
Contributor

@dreampiggy dreampiggy May 9, 2020

Choose a reason for hiding this comment

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

This code actually don't need. Because SDAnimatedImageView provide the startAnimating and stopAnimating for macOS (not public API, implementation only)

The only thing not exist is the isAnimting property.

You can actually check the code again...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean to call [self stopAnimating] directly here without distinguishing between iOS and Mac?
However, if we do not call [self setAnimates: NO] in the Mac environment, the animates property will not change.

Copy link
Contributor

@dreampiggy dreampiggy May 9, 2020

Choose a reason for hiding this comment

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

However, if we do not call [self setAnimates: NO] in the Mac environment, the animates property will not change.

Previous code does not call that. I just review the code based on changes diff.

@liulichao123 If you think this is a bug, maybe we can have a fix at the same time :)

Copy link
Contributor

@dreampiggy dreampiggy May 9, 2020

Choose a reason for hiding this comment

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

like:

- (void)startAnimating
{
    if (self.player) {
        [self updateShouldAnimate];
        if (self.shouldAnimate) {
            [self.player startPlaying];
        }
    } else {
#if SD_UIKIT
        [super startAnimating];
#else
        [super setAnimates:YES];
#endif
    }
}

then just keep the exist [self startAnimating]; caller code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did some tweaking, and I overwrote the - (BOOL)animates function

@dreampiggy
Copy link
Contributor

LGTM now. Thanks for your contribution @liulichao123 👍

@dreampiggy dreampiggy merged commit a31ba17 into SDWebImage:master May 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants