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

Custom view instead of Image as placeholder? #746

Closed
teameh opened this issue Aug 4, 2017 · 10 comments
Closed

Custom view instead of Image as placeholder? #746

teameh opened this issue Aug 4, 2017 · 10 comments

Comments

@teameh
Copy link
Contributor

teameh commented Aug 4, 2017

Check List

✔️ Read and checked all 3 items

Feature Request

Option to add a custom view instead of a Image as a placeholder. I've tried to abuse the indicatorType for this but I couldn't make it work.

I can think of some number of use cases where this is handy, for example:

  • Showing a list of users, users without a photo are shown with a letter of their name:
let placeholder = UILabel()
placeholder.color = ... etc
placeholder.text = user.name.characters.first!
carImage.kf.setImage(with: user.image, placeholder: placeholder)
  • Showing some text when image failed to load:
myImage.kf.setImage(with: imageUrl, completionHandler: {
    (image, error, cacheType, imageUrl) in
    let placeholder = UILabel()
    placeholder.text = "Image could not be downloaded"
    myImage.kf.setImage(nil, placeholder: placeholder)
}

I'm not fully up to date with swift 3 yet, but I could work on a PR if you think this is a good idea.

@onevcat
Copy link
Owner

onevcat commented Aug 5, 2017

Good idea! And I think the case 1 will be quite useful since it is very common. I'd be glad to see a PR if you could contribute your time to it!

I have several consideration on this:

  1. We need to keep it back compatible since it is a very common API. I suggest to add a protocol like Placeholder and make Image conform to this protocol.

  2. To avoid introduce runtime overhead, we'd better not do any type cast when setting the placeholder. A possible way IMO is defining the Placeholder protocol and implementing it like this:

    protocol Placeholder {
        func add(to imageView: ImageView)
        func remove(from imageView: ImageView)
    }
  3. And now we could give some default implementation by protocol extension.

    extension Image: Placeholder {
        func add(to imageView: ImageView) { imageView.image = self }
        func remove(from imageView: ImageView) { imageVIew.image = nil }
    }
    
    extension Placeholder where Self:UIView {
        func add(to imageView: ImageView) { 
            imageView.addSubView(self)
            // also set constraints
        }
        func remove(from imageView: ImageView) {
            removeFromSuperview()
        }
    }
  4. Finally, we could replace the placeholder Image in the original API to Placeholder. And also update it when we setting/resetting the image view.

@teameh How do you think about it? Tell me if you are going to try it or if you have any good idea on this! And never mind if it is not quite possible for you to implement it, I could also do it late. :]

@teameh
Copy link
Contributor Author

teameh commented Aug 5, 2017

Thanks for the quick reply. I'll definitely have a go at it. Thanks a lot for the pointers, that will really help. You'll probably need to rework the PR a bit but we'll see. I'll try to submit it somewhere in August. Cheers!

@onevcat
Copy link
Owner

onevcat commented Aug 5, 2017

@teameh Sure! +1

@teameh
Copy link
Contributor Author

teameh commented Aug 28, 2017

Hi @onevcat I finally had a chance to work on this, see master...teameh:custom-placeholder.

kf

(I basically copy and pasted your code 😝 )

But I ran into a couple of problems. While the protocol works fine for UIImageView the protocol should be different for UIButton or NSButton. Or well.. it would be better if it would be the same of course but that would make the protocol implementation look something like:

extension Image: Placeholder {
    public func add(to imageViewOrButton: Any) {
        if let imageView = imageViewOrButton as? ImageView {
            imageView.image = self
        } else if let uiButton = imageViewOrButton as? UIButton {
            uiButton.setImage(self, for: /* hmm we need the state here as well */ )
        } else if let nsButton = imageViewOrButton as? NSButton {
            nsButton.image = self
        }
    }

    public func remove(from imageViewOrButton: Any) {
        if let imageView = imageViewOrButton as? ImageView {
            imageView.image = nil
        } else if let uiButton = imageViewOrButton as? UIButton {
            uiButton.setImage(nil, for: /* hmm we need the state here as well */ )
        } else if let nsButton = imageViewOrButton as? NSButton {
            nsButton.image = nil
        }
    }
}

Not ideal.

I was also wondering why you use extension Placeholder where Self:UIView {...} in your example code btw, why not extension UIView: Placeholder {...}

I'm not sure if I have time to finish this PR, any time soon, go ahead with implementing it if you feel like it.

@onevcat
Copy link
Owner

onevcat commented Aug 29, 2017

@teameh Wow, great. I'd check it soon.

I will take a look at how to make it generic and definitely we do not want to do Any conversion like this.

The extension Placeholder where Self:UIView {...} is also a consideration on type inference. And we might not want all UIViews to be compatible with Placeholder, unless the users set them explicitly too. Don't worry I will try to see how to handle it!

Thanks for your contribution!

@teameh
Copy link
Contributor Author

teameh commented Aug 29, 2017

Cool!

You don't need me to create a PR for this right? Here's my remote [email protected]:teameh/Kingfisher.git so you don't have to look it up if you want to continue on my custom-placeholder branch.

Good luck, cheers!

@onevcat
Copy link
Owner

onevcat commented Aug 29, 2017

Yes, I created a p-r and did some additional work here. It will be merged once everything goes fine. (I think it should be fine to just support Image View now, but I am still considering a better interface.)

@onevcat
Copy link
Owner

onevcat commented Aug 30, 2017

Implemented in 3.12.0.

@onevcat onevcat closed this as completed Aug 30, 2017
@teameh
Copy link
Contributor Author

teameh commented Aug 30, 2017

Wow, you move fast! Kudo's!

@sinbadbd
Copy link

 func getImage(imageUrl: String,title:String?=nil){

        let someTitle = UILabel()
        view.addSubview(someTitle)
        someTitle.text = title
        someTitle.isHidden = true
       
        someTitle.topAnchor.constraint(equalTo: self.view.topAnchor).isActive = true
        someTitle.bottomAnchor.constraint(equalTo: self.view.bottomAnchor).isActive = true
        someTitle.leadingAnchor.constraint(equalTo: self.view.leadingAnchor).isActive = true
        someTitle.trailingAnchor.constraint(equalTo: self.view.trailingAnchor).isActive = true

        DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) { [self] in
            let url = URL(string: imageUrl )
            yourImage.kf.setImage(
                with: url,
                placeholder:  (someTitle as? Placeholder),
                options: [
                    .loadDiskFileSynchronously,
                    .cacheOriginalImage,
                    .transition(.fade(0.25))
                ],
                completionHandler: { result in
                    // Done
                    switch result {
                    case .success(let value):
                         self.yourImage.image = value.image
                    case .failure(let error):
                        someTitle.isHidden = false
                        print("Error-image: \(error)")
                    }
                }
            )
                        
        }
    }

Screenshot 2021-07-29 at 10 32 49 PM

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

No branches or pull requests

3 participants