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.

Sign up for GitHub

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

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

Closed
wants to merge 4 commits into from
+30 −22
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f85875f
SelectionLimitReach callback added with few important params now expo…
nayaks2019 Jul 16, 2020
8506475
didReachSelectionLimit not getting called-Fixed and Exposed ImagePick…
nayaks2019 Jul 17, 2020
00bed42
Code review changes
nayaks2019 Jul 21, 2020
b9f271e
Code review changes
nayaks2019 Jul 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 14 additions & 13 deletions Example/ViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

resolved

imagePicker.settings.selection.notifyReachingMaxLimit=true;
let start = Date()
self.presentImagePicker(imagePicker, select: { (asset) in
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

resolved

print("Selected: \(asset)")
}, deselect: { (asset) in
print("Deselected: \(asset)")
}, cancel: { (assets) in
print("Canceled with selections: \(assets)")
}, finish: { (assets) in
print("Finished with selections: \(assets)")
}, completion: {
self.presentImagePicker(imagePicker, select: { (asset) in
print("Selected: \(asset)")
}, deselect: { (asset) in
print("Deselected: \(asset)")
}, cancel: { (assets) in
print("Canceled with selections: \(assets)")
}, finish: { (assets) in
let finish = Date()
print(finish.timeIntervalSince(start))
})
print(finish.timeIntervalSince(start))
},completion: {
let finish = Date()
print(finish.timeIntervalSince(start))
})
}

@IBAction func showCustomImagePicker(_ sender: UIButton) {
Expand Down
5 changes: 5 additions & 0 deletions Sources/Controller/ImagePickerController+Assets.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ extension ImagePickerController: AssetsViewControllerDelegate {
imagePickerDelegate?.imagePicker(self, didDeselectAsset: first)
}
}
else if settings.selection.notifyReachingMaxLimit && assetStore.count > settings.selection.max {
Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

assetStore.remove(asset)
assetsViewController.unselect(asset:asset)
imagePickerDelegate?.imagePicker(self, didReachSelectionLimit: settings.selection.max)
}
updatedDoneButton()
imagePickerDelegate?.imagePicker(self, didSelectAsset: asset)
}
Expand Down
6 changes: 3 additions & 3 deletions Sources/Controller/ImagePickerController+Closure.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

resolved

authorize {
// Set closures
imagePicker.onSelection = select
imagePicker.onDeselection = deselect
imagePicker.onCancel = cancel
imagePicker.onFinish = finish

// And since we are using the blocks api. Set ourselfs as delegate
imagePicker.imagePickerDelegate = imagePicker

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

Choose a reason for hiding this comment

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

Remove

Copy link
Author

Choose a reason for hiding this comment

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

resolved


extension ImagePickerController {
public static var currentAuthorization : PHAuthorizationStatus {
Expand All @@ -90,6 +90,6 @@ extension ImagePickerController: ImagePickerControllerDelegate {
}

public func imagePicker(_ imagePicker: ImagePickerController, didReachSelectionLimit count: Int) {

onReachMaxSelectionLimit?(count)
Copy link
Owner

Choose a reason for hiding this comment

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

remove

Copy link
Author

Choose a reason for hiding this comment

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

remove what?

}
}
2 changes: 1 addition & 1 deletion Sources/Controller/ImagePickerController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)?
Copy link
Owner

Choose a reason for hiding this comment

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

remove

Copy link
Author

Choose a reason for hiding this comment

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

remove what?

let assetsViewController: AssetsViewController
let albumsViewController = AlbumsViewController()
let dropdownTransitionDelegate = DropdownTransitionDelegate()
Expand Down
2 changes: 1 addition & 1 deletion Sources/Controller/ImagePickerControllerDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import Foundation
import Photos

/// Delegate of the image picker
public protocol ImagePickerControllerDelegate: class {
@objc public protocol ImagePickerControllerDelegate: class {
/// An asset was selected
/// - Parameter imagePicker: The image picker that asset was selected in
/// - Parameter asset: selected asset
Expand Down
39 changes: 21 additions & 18 deletions Sources/Model/Settings.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,18 @@ import Photos
]
}

public class Selection : NSObject {
@objc public class Selection : NSObject {
/// Max number of selections allowed
public lazy var max: Int = Int.max
@objc public lazy var max: Int = Int.max

/// Min number of selections you have to make
public lazy var min: Int = 1
@objc public lazy var min: Int = 1

/// If it reaches the max limit, unselect the first selection, and allow the new selection
public lazy var unselectOnReachingMax : Bool = false
@objc public lazy var unselectOnReachingMax : Bool = false

/// If it reaches the max limit, notify
@objc public lazy var notifyReachingMaxLimit : Bool = false
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

resolved

}

public class List : NSObject {
Expand All @@ -100,10 +103,10 @@ import Photos
public lazy var enabled: Bool = true
}

public class Fetch : NSObject {
public class Album : NSObject {
@objc public class Fetch : NSObject {
@objc public class Album : NSObject {
/// Fetch options for albums/collections
public lazy var options: PHFetchOptions = {
@objc public lazy var options: PHFetchOptions = {
let fetchOptions = PHFetchOptions()
return fetchOptions
}()
Expand All @@ -115,12 +118,12 @@ import Photos
/// PHAssetCollection.fetchAssetCollections(with: .smartAlbum, subtype: .smartAlbumSelfPortraits, options: options),
/// PHAssetCollection.fetchAssetCollections(with: .smartAlbum, subtype: .smartAlbumPanoramas, options: options),
/// PHAssetCollection.fetchAssetCollections(with: .smartAlbum, subtype: .smartAlbumVideos, options: options),
public lazy var fetchResults: [PHFetchResult<PHAssetCollection>] = [
@objc public lazy var fetchResults: [PHFetchResult<PHAssetCollection>] = [
PHAssetCollection.fetchAssetCollections(with: .smartAlbum, subtype: .smartAlbumUserLibrary, options: options),
]
}

public class Assets : NSObject {
@objc public class Assets : NSObject {
/// Fetch options for assets

/// Simple wrapper around PHAssetMediaType to ensure we only expose the supported types.
Expand Down Expand Up @@ -175,13 +178,13 @@ import Photos
}

/// Album fetch settings
public lazy var album = Album()
@objc public lazy var album = Album()

/// Asset fetch settings
public lazy var assets = Assets()
@objc public lazy var assets = Assets()

/// Preview fetch settings
public lazy var preview = Preview()
@objc public lazy var preview = Preview()
}

public class Dismiss : NSObject {
Expand All @@ -193,20 +196,20 @@ import Photos
}

/// Theme settings
public lazy var theme = Theme()
@objc public lazy var theme = Theme()

/// Selection settings
public lazy var selection = Selection()
@objc public lazy var selection = Selection()

/// List settings
public lazy var list = List()
@objc public lazy var list = List()

/// Fetch settings
public lazy var fetch = Fetch()
@objc public lazy var fetch = Fetch()

/// Dismiss settings
public lazy var dismiss = Dismiss()
@objc public lazy var dismiss = Dismiss()

/// Preview options
public lazy var preview = Preview()
@objc public lazy var preview = Preview()
}
2 changes: 1 addition & 1 deletion Sources/Scene/Assets/AssetsViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ extension AssetsViewController: UICollectionViewDelegate {
}

func collectionView(_ collectionView: UICollectionView, shouldSelectItemAt indexPath: IndexPath) -> Bool {
guard store.count < settings.selection.max || settings.selection.unselectOnReachingMax else { return false }
guard store.count < settings.selection.max || settings.selection.unselectOnReachingMax || settings.selection.notifyReachingMaxLimit else { return false }
selectionFeedback.prepare()

return true
Expand Down