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 savePhotoOnLongPress function #151

Merged
merged 10 commits into from Jan 13, 2020
Merged

added savePhotoOnLongPress function #151

merged 10 commits into from Jan 13, 2020

Conversation

alexookah
Copy link
Contributor

Created this function so that users can easily use a simple default UIAlertController with two options:
Save Photo and Cancel

using it in just one single line:
agrume.onLongPress = agrume.photoSavedToLibrary

I didnt add it as a default longPress function because some user's might not want this and also they might have not have NSPhotoLibraryUsageDescription and NSPhotoLibraryAddUsageDescription in their info.plist.

@JanGorman
Tell me what you think about this. If it needs some change or any suggestion how to improve.

I was thinking to add some documentation in the readme after this gets merged to master 👍

@alexookah
Copy link
Contributor Author

alexookah commented Jan 7, 2020

@JanGorman
maybe if this is okay to merge then i think it would be nice to add a right Button on navigationbar "..." to show users visually that they can also download an image or some other function callback users pass in agrume :)

@JanGorman
Copy link
Owner

I'll need some quiet time to go over this. I'll take a look on the weekend :)

@alexookah
Copy link
Contributor Author

@JanGorman did you have a chance to take a look?

Copy link
Owner

@JanGorman JanGorman left a comment

Choose a reason for hiding this comment

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

Overall it's nice to provide an easy way to add save functionality but what I don't quite like is that this is then so "hardcoded" in a more general purpose library. It's a leaky abstraction to have the save and cancel title properties as well as the completion handler. What I'd propose to do instead is the following:

  1. Change the onLongPress to the following: public var onLongPress: ((UIImage?, UIViewController) -> Void)? – this makes it more useful anyway because you can easily access the image and controller

  2. Put the save functionality into a completely separate class. I quickly tried this and what it could look like is this:

import UIKit

public final class AgrumePhotoLibraryHelper: NSObject {

  private let saveButtonTitle: String
  private let cancelButtonTitle: String
  private let saveToLibraryHandler: (_ error: Error?) -> Void

  public init(saveButtonTitle: String, cancelButtonTitle: String, saveToLibraryHandler: @escaping (_ error: Error?) -> Void) {
    self.saveButtonTitle = saveButtonTitle
    self.cancelButtonTitle = cancelButtonTitle
    self.saveToLibraryHandler = saveToLibraryHandler
  }

  public func makeLongPressGesture(for image: UIImage?, viewController: UIViewController) {
    guard let image = image else {
      return
    }
    let view = viewController.view!
    let alert = UIAlertController(title: nil, message: nil, preferredStyle: .actionSheet)
    alert.popoverPresentationController?.sourceView = view
    alert.popoverPresentationController?.permittedArrowDirections = .up
    let alertPosition = CGRect(x: view.bounds.midX, y: view.bounds.maxY - view.bounds.midY / 2, width: 0, height: 0)
    alert.popoverPresentationController?.sourceRect = alertPosition

    alert.addAction(UIAlertAction(title: saveButtonTitle, style: .default) { _ in
      UIImageWriteToSavedPhotosAlbum(image, self, #selector(self.image), nil)
    })
    alert.addAction(UIAlertAction(title: cancelButtonTitle, style: .cancel, handler: nil))

    viewController.present(alert, animated: true)
  }

  @objc
  private func image(_ image: UIImage, didFinishSavingWithError error: NSError?, contextInfo: UnsafeRawPointer) {
    saveToLibraryHandler(error)
  }

}

So we put the button titles into properties here, as well as the completion handler. Then, to invoke it:

private func makeHelper() -> AgrumePhotoLibraryHelper {
    let saveButtonTitle = NSLocalizedString("Save Photo", comment: "Save Photo")
    let cancelButtonTitle = NSLocalizedString("Cancel", comment: "Cancel")
    let helper = AgrumePhotoLibraryHelper(saveButtonTitle: saveButtonTitle, cancelButtonTitle: cancelButtonTitle) { error in
      guard error == nil else {
        print("Could not save your photo")
        return
      }
      print("Photo has been saved to your library")
    }
    return helper
  }

and when you want to add the gesture, you do

let helper = makeHelper()
agrume.onLongPress = helper.makeLongPressGesture

This is marginally more code on client side but a cleaner abstraction

Example/Agrume Example/SingleURLViewController.swift Outdated Show resolved Hide resolved
Agrume/Agrume.swift Outdated Show resolved Hide resolved
Agrume/Agrume.swift Outdated Show resolved Hide resolved
@alexookah
Copy link
Contributor Author

@JanGorman You were absolutely right. Thanks for your proposed changes.
I think i made all of them.

@alexookah
Copy link
Contributor Author

@JanGorman
Oh yes, missed that. Sorry.
How do you run the Example? it needs SwiftyGif and there is no PodFile to run podInstall.

@JanGorman
Copy link
Owner

JanGorman commented Jan 13, 2020

ah… good to know that it's not so evident. Need to document that. I don't like the CocoaPods default directory layout so for the example I opted to add this as git submodule. To run the example from the project root dir run git submodule update --init and then you can compile

@JanGorman
Copy link
Owner

SingleURLViewController still has a small bug

@alexookah
Copy link
Contributor Author

@JanGorman Finally i can run the example thank you. Yes this should be in the Documentation.

Are you sure? i did one more commit after my previous comment

@JanGorman
Copy link
Owner

Didn't have the latest state. Cool, thanks a lot for the addition

@alexookah
Copy link
Contributor Author

@JanGorman sorry for all these commits.
So Is this okay now for merge?

@JanGorman
Copy link
Owner

Got pulled into a meeting 😝. Yes, good to go

@JanGorman JanGorman merged commit 0971600 into JanGorman:master Jan 13, 2020
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.

None yet

2 participants