migrate to preloadAllAnimationData #664

ShaharHD · 2017-05-08T03:57:21Z

Rename all instances of preloadAllGIFData to preloadAllAnimationData with proper @available notices.

References #663

`pod install` resulted in automated project updates

@available

All instances of `preloadAllGIFData` are now `preloadAllAnimationData` with proper @available notices

ShaharHD · 2017-05-08T04:06:27Z

@onevcat Please note that the initial pod install changed all the files I've put in the 1st commit (didn't touch anything)
All my changes afterwards are in the 2nd commit.

Also, when trying to run the tests on Xcode I'm getting errors on Kingfisher/Kingfisher-TestImages/Modified/Overlay/onevcat-overlay-red-07-mac.jpg: No such file or directory Are some files missing from the git repo?
Found git clone https://github.com/onevcat/Kingfisher-TestImages.git Kingfisher-TestImages

All Tests on my machine completes with no errors.
Even the one travis-ci reported as failed:

Test Suite 'KingfisherTests-macOS.xctest' passed at 2017-05-08 07:17:35.353.
	 Executed 92 tests, with 0 failures (0 unexpected) in 7.738 (8.656) seconds

onevcat · 2017-05-08T04:29:58Z

Thanks!
It was a travis issue and I'll restart the tests.

ShaharHD · 2017-05-08T04:35:47Z

Worked.
Two FYIs:

Version 3.7.0 was never pushed to cocoapods
I would appreciate if you can push 3.7.1 also with my fix after you approve it so I can base KingfisherWebP on the latest while I'm implementing the proper animation (right now I have it pointed to my branch for development)

onevcat

LGTM. Added several comments, please review them. We could merge it once things get done & CI passed.

Thanks!

onevcat · 2017-05-08T04:35:41Z

Sources/KingfisherOptionsInfo.swift

    public var preloadAllGIFData: Bool {
-        return contains { $0 <== .preloadAllGIFData }
+        return contains { $0 <== .preloadAllAnimationData }


Maybe we could just return preloadAllAnimationData?

onevcat · 2017-05-08T04:42:52Z

Sources/Image.swift

        return image
    }
+
+    @available(*, deprecated, message: "preloadAllGIFData is now preloadAllAnimationData")


Since it is an internal API, we could just remove it instead of marking it as deprecated.

onevcat · 2017-05-08T04:44:46Z

Sources/KingfisherOptionsInfo.swift

+    case preloadAllAnimationData
+
+    /// Back compatibility for deprecated preloadAllGIFData
+    @available(*, deprecated, renamed: "preloadAllAnimation")


Could you move the deprecated statements to an extra extension at the end of this file (like what is done here)?

The difference here it's a enum and not functions. But I'll fix the other ones as requested.

It should be fine to have this static property in an extension (as long as the extension is public, it should also supply good back compatibility).

I don't understand how you suggest having the enum value deprecated outside of the enum declaration (Swift AFAIK does not support adding to an enum through inheritance or any other extension mechanism)

Here you are using a static property to "simulate" the syntax of an enum case:

@available(*, deprecated, renamed: "preloadAllAnimation") static let preloadAllGIFData = KingfisherOptionsInfoItem.preloadAllAnimationData

So it is possible to move it to extension (it is not an enum member anymore). Sure you could keep it as an enum member (in which case you cannot move it to an extension), but I think it is a clever way to make it a static property here.

In there, this should also compile:

public extension KingfisherOptionsInfoItem { @available(*, deprecated, renamed: "preloadAllAnimation") static let preloadAllGIFData = KingfisherOptionsInfoItem.preloadAllAnimationData }

And users could get a rename warning if it is used.

Tried the same, but with incorrect syntax is seems... Thanks 👍

onevcat · 2017-05-08T04:47:45Z

Version 3.7.0 was never pushed to cocoapods

Oops, didn't notice it. It should since the release script runs without issue. I guess it might be a CocoaPods issue.

I would appreciate if you can push 3.7.1 also with my fix after you approve it so I can base KingfisherWebP on the latest while I'm implementing the proper animation (right now I have it pointed to my branch for development)

Sure. I'll release 3.7.1 once it prepared.

onevcat · 2017-05-08T05:41:00Z

Sources/KingfisherOptionsInfo.swift

+    @available(*, deprecated, renamed: "preloadAllAnimation")
+    public var preloadAllGIFData: KingfisherOptionsInfoItem {
+        return .preloadAllAnimationData
+    }


This should return a Bool with implementation like this:

@available(*, deprecated, renamed: "preloadAllAnimation") public var preloadAllGIFData: Bool { return preloadAllAnimationData }

My bad a typo of copy & paste.
Fixed

onevcat · 2017-05-08T05:42:19Z

@ShaharHD

Great. I added another comment. Please fix that too before we could finally merge it.

migrate to preloadAllAnimationData

ShaharHD added 2 commits May 8, 2017 06:01

Initial project load

c7b6fc5

`pod install` resulted in automated project updates

Rename GIF to Animation

f9e975b

All instances of `preloadAllGIFData` are now `preloadAllAnimationData` with proper @available notices

onevcat reviewed May 8, 2017

View reviewed changes

ShaharHD force-pushed the fix/preloadAllAnimationData branch from 48aa054 to 9dbbab3 Compare May 8, 2017 05:35

onevcat reviewed May 8, 2017

View reviewed changes

Fix PR review comments

63c4bac

ShaharHD force-pushed the fix/preloadAllAnimationData branch from 9dbbab3 to 63c4bac Compare May 8, 2017 05:47

onevcat merged commit 7e892b0 into onevcat:master May 8, 2017

ShaharHD deleted the fix/preloadAllAnimationData branch May 8, 2017 05:55

This was referenced May 8, 2017

Rename KingfisherOptionsInfoItem preloadAllGIFData #663

Closed

Add support for animated webp yeatse/KingfisherWebP#3

Merged

onevcat mentioned this pull request Aug 30, 2017

Pod Install gives me this error: Value of type 'KingfisherOptionsInfo' (aka 'Array<KingfisherOptionsInfoItem>') has no member 'preloadAllAnimationData' #758

Closed

3 tasks

petarlazarov pushed a commit to dowjones-mobile/Kingfisher that referenced this pull request Oct 6, 2021

Merge pull request onevcat#664 from ShaharHD/fix/preloadAllAnimationData

9ec16de

migrate to preloadAllAnimationData

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate to preloadAllAnimationData #664

migrate to preloadAllAnimationData #664

ShaharHD commented May 8, 2017

ShaharHD commented May 8, 2017 •

edited

Loading

onevcat commented May 8, 2017

ShaharHD commented May 8, 2017

onevcat left a comment

onevcat May 8, 2017

onevcat May 8, 2017

onevcat May 8, 2017

ShaharHD May 8, 2017

onevcat May 8, 2017

ShaharHD May 8, 2017

onevcat May 8, 2017 •

edited

Loading

ShaharHD May 8, 2017

onevcat commented May 8, 2017

onevcat May 8, 2017

ShaharHD May 8, 2017

onevcat commented May 8, 2017

migrate to preloadAllAnimationData #664

migrate to preloadAllAnimationData #664

Conversation

ShaharHD commented May 8, 2017

ShaharHD commented May 8, 2017 • edited Loading

onevcat commented May 8, 2017

ShaharHD commented May 8, 2017

onevcat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onevcat May 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onevcat commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onevcat commented May 8, 2017

ShaharHD commented May 8, 2017 •

edited

Loading

onevcat May 8, 2017 •

edited

Loading