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

Image Queue #18

Merged
merged 2 commits into from
Feb 22, 2020
Merged

Image Queue #18

merged 2 commits into from
Feb 22, 2020

Conversation

jusbar23
Copy link
Contributor

Allow users to queue multiple images locally or remote

Copy link
Contributor

@tsheaff tsheaff left a comment

Choose a reason for hiding this comment

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

I'd like to approve this, looks really strong. But I think you need to add the pod dependency, which is the only blocker. You'll also want to bump the version (major bump seems appropriate?)

I also and left a few nits on the code.

KenBurns/Classes/KenBurns.swift Show resolved Hide resolved
KenBurns/Classes/KenBurns.swift Show resolved Hide resolved
KenBurns/Classes/KenBurns.swift Show resolved Hide resolved

self.fetchImage(self.imageURLs!.read()!, placeholder: nil)

for _ in 0..<10 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand where this magic number. 10 is coming from. Can you please explain, and also drop a comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my testing, I found that when the nextImage function is used there is a glitch that remote images might not be downloaded in time. 10 is an arbitrary number. Of course there are optimizations we could do to avoid this such as image compression, preloading, caching, etc. This is more of a temporary solution

KenBurns/Classes/KenBurns.swift Outdated Show resolved Hide resolved
KenBurns/Classes/RingBuffer.swift Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@jusbar23
Copy link
Contributor Author

Hi Tyler, thanks for taking the time to look at my PR. Going ahead to make the changes requested

Copy link
Contributor

@tsheaff tsheaff left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.

Can you bump the version in the podspec to 0.4.0?

One final question before approving: I noticed from your gif that they're not cross-fading over one another. I haven't tried and run this myself. Does this ruin the cross-fade effect? That's an important part of this, and I would have expected, for example, Image 1 to gently fade out to reveal Image 2 behind, then Image 3 etc in a loop.

@jusbar23
Copy link
Contributor Author

Yes, thanks for pointing that out. Fixing that right now, give me a few!

Add ring buffer for smoothness

Removed Copyright added GIF and removed SD Image

GIF was too large

Added demo, cleaned up code, updated gif, removed ringbuffer

Increase GIF Size

Reduced Size of GIF
@jusbar23
Copy link
Contributor Author

Changes made:

  1. Updated GIF
  2. Removed Ring Buffer for Image Views
  3. Cleaned up code
  4. Added Demo Project
  5. Rebased

@tsheaff tsheaff merged commit 4c166ac into calm:master Feb 22, 2020
@tsheaff
Copy link
Contributor

tsheaff commented Feb 22, 2020

@justinbarnett1 merged to master and published to Cocoapods trunk:

➜  KenBurns git:(master) pod trunk push KenBurns.podspec
Updating spec repo `trunk`

CocoaPods 1.9.0.beta.3 is available.
To update use: `sudo gem install cocoapods --pre`
[!] This is a test version we'd love you to try.

For more information, see https://blog.cocoapods.org and the CHANGELOG for this version at https://github.com/CocoaPods/CocoaPods/releases/tag/1.9.0.beta.3

Validating podspec
 -> KenBurns (0.4.0)
    - NOTE  | xcodebuild:  note: Using new build system
    - NOTE  | xcodebuild:  note: Planning build
    - NOTE  | xcodebuild:  note: Constructing build description
    - NOTE  | xcodebuild:  warning: Skipping code signing because the target does not have an Info.plist file and one is not being generated automatically. (in target 'App' from project 'App')
    - NOTE  | xcodebuild:  note: Execution policy exception registration failed and was skipped: Error Domain=NSPOSIXErrorDomain Code=1 "Operation not permitted" (in target 'CalmParametricAnimations' from project 'Pods')
    - NOTE  | xcodebuild:  note: Execution policy exception registration failed and was skipped: Error Domain=NSPOSIXErrorDomain Code=1 "Operation not permitted" (in target 'Kingfisher' from project 'Pods')
    - NOTE  | xcodebuild:  note: Execution policy exception registration failed and was skipped: Error Domain=NSPOSIXErrorDomain Code=1 "Operation not permitted" (in target 'KenBurns' from project 'Pods')
    - NOTE  | xcodebuild:  note: Execution policy exception registration failed and was skipped: Error Domain=NSPOSIXErrorDomain Code=1 "Operation not permitted" (in target 'Pods-App' from project 'Pods')
    - NOTE  | xcodebuild:  note: Execution policy exception registration failed and was skipped: Error Domain=NSPOSIXErrorDomain Code=1 "Operation not permitted" (in target 'App' from project 'App')

Updating spec repo `trunk`

CocoaPods 1.9.0.beta.3 is available.
To update use: `sudo gem install cocoapods --pre`
[!] This is a test version we'd love you to try.

For more information, see https://blog.cocoapods.org and the CHANGELOG for this version at https://github.com/CocoaPods/CocoaPods/releases/tag/1.9.0.beta.3


--------------------------------------------------------------------------------
 🎉  Congrats

 🚀  KenBurns (0.4.0) successfully published
 📅  February 22nd, 11:31
 🌎  https://cocoapods.org/pods/KenBurns
 👍  Tell your friends!
--------------------------------------------------------------------------------

@jusbar23
Copy link
Contributor Author

jusbar23 commented Feb 22, 2020

Thank you! Just wondering are you guys hiring for PM? I’m based in FL but willing to relocate

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.

2 participants