SelectionLimitReach callback added with few important params now expo… #286

nayaks2019 · 2020-07-16T17:54:14Z

…sed for ObjectiveC

1.Exposed few params to ObjectiveC
2.Added callback for selectionLimitReach as the delegate never worked.

…sed for ObjectiveC 1.Exposed few params to ObjectiveC 2.Added callback for selectionLimitReach as the delegate never worked.

mikaoj · 2020-07-17T09:18:44Z

Thanks for taking the time to help out, but I don't think I'll be able to merge this PR.

The idea behind the delegate was to have the closure api simple and covering the most common use cases and let user who wanted more 'advanced' features use the delegate.
Otherwise I'll end up having a bunch of different closures in the common api that most people will never use.
It might be a bad idea, but it's what I've got right now :)

Would you mind just fixing the bug that the delegate is never called on reaching max instead of adding more closures?

…erControllerDelegate and few Settings parameters to objectiveC mikaoj#1.didReachSelectionLimit was not getting called-Fixed mikaoj#2.Exposed some parameters of settings to ObjectiveC. mikaoj#3.Exposed ImagePickerControllerDelegate to objective C

nayaks2019 · 2020-07-17T15:11:16Z

@mikaoj -Found out why the delegate method was not getting called and fixed and pushed the changes in the above pull request.
Please review and approve.

nayaks2019 · 2020-07-20T21:00:44Z

@mikaoj -Did you get a chance to look at this pull request?.I made the required changes.Please review and Let me know

mikaoj

Thanks for taking the time to add this. But the PR could probably do with a little cleaning :)

mikaoj · 2020-07-21T12:25:00Z

Example/ViewController.swift

@@ -31,21 +31,22 @@ class ViewController: UIViewController {
        imagePicker.settings.selection.max = 5
        imagePicker.settings.theme.selectionStyle = .numbered
        imagePicker.settings.fetch.assets.supportedMediaTypes = [.image, .video]
-        imagePicker.settings.selection.unselectOnReachingMax = true
-
+       // imagePicker.settings.selection.unselectOnReachingMax = true


Don't comment out code. Maybe create a new example that uses the delegate instead of closures.

mikaoj · 2020-07-21T12:26:41Z

Example/ViewController.swift

        let start = Date()
-        self.presentImagePicker(imagePicker, select: { (asset) in


Most of the changes here seem to be whitespace/indentation? Don't commit stuff like that.

mikaoj · 2020-07-21T12:29:17Z

Sources/Controller/ImagePickerController+Assets.swift

@@ -31,6 +31,11 @@ extension ImagePickerController: AssetsViewControllerDelegate {
                imagePickerDelegate?.imagePicker(self, didDeselectAsset: first)
            }
        }
+        else if settings.selection.notifyReachingMaxLimit && assetStore.count > settings.selection.max {


The logic here seems wrong. Firstly it should probably be an if instead of an else if as a user can have both notify and unselect set to true.
And I don't think we should do any asset removal or unselection in this case?

Changed else if to if.
Remove and unselect has to be there.Because-
Suppose maxselection is 2
When didSelectAsset gets called when we are on 3rd selection, then only this loop will get executed.so that asset has to be removed.If we are not removing that asset.then we will have 3 assets selected instead of 2 assets.

mikaoj · 2020-07-21T12:30:27Z

Sources/Controller/ImagePickerController+Closure.swift

@@ -37,14 +37,13 @@ import Photos
    ///   - cancel: Cancel callback
    ///   - finish: Finish callback
    ///   - completion: Presentation completion callback
-    public func presentImagePicker(_ imagePicker: ImagePickerController, animated: Bool = true, select: ((_ asset: PHAsset) -> Void)?, deselect: ((_ asset: PHAsset) -> Void)?, cancel: (([PHAsset]) -> Void)?, finish: (([PHAsset]) -> Void)?, completion: (() -> Void)? = nil) {
+   public func presentImagePicker(_ imagePicker: ImagePickerController, animated: Bool = true, select: ((_ asset: PHAsset) -> Void)?, deselect: ((_ asset: PHAsset) -> Void)?, cancel: (([PHAsset]) -> Void)?, finish: (([PHAsset]) -> Void)?,completion: (() -> Void)? = nil) {


Don't commit whitespace/indentation changes. The log will be horrible if every committer does that.

mikaoj · 2020-07-21T12:30:42Z

Sources/Controller/ImagePickerController+Closure.swift

@@ -64,6 +63,7 @@ import Photos
        }
    }
 }
+//selectionLimitReach: ((_ count:Int) -> Void)?,


mikaoj · 2020-07-21T12:31:24Z

Sources/Controller/ImagePickerController+Closure.swift

@@ -90,6 +90,6 @@ extension ImagePickerController: ImagePickerControllerDelegate {
    }

    public func imagePicker(_ imagePicker: ImagePickerController, didReachSelectionLimit count: Int) {
-
+         onReachMaxSelectionLimit?(count)


remove what?

mikaoj · 2020-07-21T12:31:29Z

Sources/Controller/ImagePickerController.swift

@@ -49,7 +49,7 @@ import Photos
    var onDeselection: ((_ asset: PHAsset) -> Void)?
    var onCancel: ((_ assets: [PHAsset]) -> Void)?
    var onFinish: ((_ assets: [PHAsset]) -> Void)?
-
+    var onReachMaxSelectionLimit: ((_ count: Int) -> Void)?


remove what?

mikaoj · 2020-07-21T12:34:35Z

Sources/Model/Settings.swift

+       @objc public lazy var unselectOnReachingMax : Bool = false
+
+       /// If it reaches the max limit, notify
+       @objc public lazy var notifyReachingMaxLimit : Bool = false


To follow the naming of the previous property...maybe change to notifyOnReachingMax

mikaoj#1.Removed all white spaces mikaoj#2.Changed the name from notifyReachingMaxLimit to onReachMaxSelectionLimit

nayaks2019 · 2020-09-28T20:58:29Z

@mikaoj hey I am wondering why it is not merged?is there any concern?

mikaoj · 2020-09-29T08:05:42Z

Something might have got lost in translation, but I tried to explain that I didn't want the closures but only the delegate.

Thanks for taking the time to help out, but I don't think I'll be able to merge this PR.

The idea behind the delegate was to have the closure api simple and covering the most common use cases and let user who wanted more 'advanced' features use the delegate.
Otherwise I'll end up having a bunch of different closures in the common api that most people will never use.
It might be a bad idea, but it's what I've got right now :)

Would you mind just fixing the bug that the delegate is never called on reaching max instead of adding more closures?

But you kept the closures. :)

SelectionLimitReach callback added with few important params now expo…

f85875f

…sed for ObjectiveC 1.Exposed few params to ObjectiveC 2.Added callback for selectionLimitReach as the delegate never worked.

mikaoj requested changes Jul 21, 2020

View reviewed changes

nayaks2019 added 2 commits July 21, 2020 10:42

Code review changes

00bed42

mikaoj#1.Removed all white spaces mikaoj#2.Changed the name from notifyReachingMaxLimit to onReachMaxSelectionLimit

Code review changes

b9f271e

mikaoj closed this Sep 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SelectionLimitReach callback added with few important params now expo… #286

SelectionLimitReach callback added with few important params now expo… #286

nayaks2019 commented Jul 16, 2020

mikaoj commented Jul 17, 2020

nayaks2019 commented Jul 17, 2020

nayaks2019 commented Jul 20, 2020

mikaoj left a comment

mikaoj Jul 21, 2020

nayaks2019 Jul 21, 2020

mikaoj Jul 21, 2020

nayaks2019 Jul 21, 2020

mikaoj Jul 21, 2020

nayaks2019 Jul 21, 2020

mikaoj Jul 21, 2020

nayaks2019 Jul 21, 2020

mikaoj Jul 21, 2020

nayaks2019 Jul 21, 2020

mikaoj Jul 21, 2020

nayaks2019 Jul 21, 2020

mikaoj Jul 21, 2020

nayaks2019 Jul 21, 2020

mikaoj Jul 21, 2020

nayaks2019 Jul 21, 2020

nayaks2019 commented Sep 28, 2020

mikaoj commented Sep 29, 2020

		let start = Date()
		self.presentImagePicker(imagePicker, select: { (asset) in

SelectionLimitReach callback added with few important params now expo… #286

SelectionLimitReach callback added with few important params now expo… #286

Conversation

nayaks2019 commented Jul 16, 2020

mikaoj commented Jul 17, 2020

nayaks2019 commented Jul 17, 2020

nayaks2019 commented Jul 20, 2020

mikaoj 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nayaks2019 commented Sep 28, 2020

mikaoj commented Sep 29, 2020