Permission handling in image picker by mithilesh-parmar · Pull Request #334 · mikaoj/BSImagePicker

mithilesh-parmar · 2021-06-01T17:17:13Z

Check for PHAuthorization status in onViewDidLoad() of AssetViewController
Show header for authorisation status with manage button

bivant · 2021-12-16T23:10:55Z

 			buildSettings = {
 				ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon;
 				CODE_SIGN_STYLE = Automatic;
-				DEVELOPMENT_TEAM = Y2NHS7RD78;


this should not be present in the PR

replaced with original

bivant · 2021-12-16T23:11:29Z

    var onDeselection: ((_ asset: PHAsset) -> Void)?
    var onCancel: ((_ assets: [PHAsset]) -> Void)?
    var onFinish: ((_ assets: [PHAsset]) -> Void)?
+    var onPermissionChange: ((_ status: PHAuthorizationStatus) -> Void)?


not used anywhere

Removed onPermissionChange

bivant · 2021-12-16T23:12:46Z

+
+    internal func checkAuthorizationStatus(){
+
+        if #available(iOS 14, *) {


requestAuthorization is done on controller presentation, does nothing here. Should be replaced with the authorizationStatus

Removed this method

bivant · 2021-12-16T23:14:51Z

+    func didTapManageButton(for status: PHAuthorizationStatus) {
+        switch status {
+        case .denied:
+            self.handleDeniedAccess()


controller is not shown if no access (full or limited), so .denied is never executed

removed handling of .denied access

bivant · 2021-12-16T23:15:43Z

+
+            manageButton.topAnchor.constraint(equalTo: self.topAnchor),
+            manageButton.bottomAnchor.constraint(equalTo: self.bottomAnchor),
+            manageButton.leadingAnchor.constraint(greaterThanOrEqualTo: self.titleLabel.trailingAnchor, constant: 8),


extra constraint, already created in 113 by `titleLabel.trailingAnchor.constraint(lessThanOrEqualTo: self.manageButton.leadingAnchor, constant: -8),

Removed duplicate constraint

mikaoj · 2021-12-19T12:31:21Z

I have yet to check this out and try it. I've only skimmed through it so far.

But this adds a feature where the image picker can show a header if it doesn't have been authorized to access a users photos?

I did see a lot of user facing strings that weren't localised though which would need to be fixed.

mithilesh-parmar · 2021-12-19T12:34:20Z

Yes, this mimics WhatsApp-like permission handling wherein if the app has limited access to photos it will show a header wherein the user can manage permission.

I'll look into strings localisations

mikaoj · 2021-12-19T12:36:44Z

I'll look into strings localisations

And by that I don't mean that you should translate them into 20 different languages.
Just wrap them in NSLocalizedString with a good english default string and a key so anyone wanting it in another language in their app could translate it :)

mithilesh-parmar · 2021-12-19T12:37:45Z

Cool, thanks for the heads up!

mithilesh-parmar · 2021-12-20T12:11:21Z

Added Localized strings @mikaoj

bivant · 2021-12-21T13:43:41Z

+
+        let openSettinsgLocalizedText = NSLocalizedString("bsimagepicker.restrictedAccess.alert.openSettings.title",
+                                                          value: "Open Settings",
+                                                          comment: "Primart button title")


Primart - typo

bivant · 2021-12-21T13:46:09Z

+            manageButton.topAnchor.constraint(equalTo: self.topAnchor),
+            manageButton.bottomAnchor.constraint(equalTo: self.bottomAnchor),
+            manageButton.trailingAnchor.constraint(equalTo: self.trailingAnchor, constant: -16),
+            manageButton.widthAnchor.constraint(greaterThanOrEqualToConstant: manageButton.runtimeSize().width)


is this runtimeSize needed + extension? Why not set hugging/compressing priority instead?

bivant · 2021-12-21T13:48:29Z

+            manageButton.bottomAnchor.constraint(equalTo: self.bottomAnchor),
+            manageButton.trailingAnchor.constraint(equalTo: self.trailingAnchor, constant: -16),
+            manageButton.widthAnchor.constraint(greaterThanOrEqualToConstant: manageButton.runtimeSize().width)
+


code style: extra empty lines across the PR after closing brackets.

bivant · 2021-12-21T13:49:18Z

 @objcMembers public class Settings : NSObject {
    public static let shared = Settings()
-
+    


code style: please try to change you indentation settings to match the default for the project so there no "empty" changes

bivant · 2021-12-21T16:35:23Z

+        let cancelAction = UIAlertAction(title: cancelLocalizedText, style: .cancel, handler: nil)
+        actionSheet.addAction(cancelAction)
+
+        present(actionSheet, animated: true, completion: nil)


I get crash on iPad because the popoverPresentationController?.sourceView is not set. Send here the "manage" button so sheet is displayed properly

mithilesh-parmar added 2 commits June 1, 2021 22:44

add permisison handling to assetview controller, similar to whatsapp

fcdc803

add textcolors for permission header view

ecca308

bivant reviewed Dec 16, 2021

View reviewed changes

mithilesh-parmar added 4 commits December 19, 2021 17:29

resolve: all comments mentioned on PR from @bivant

fa56af0

remove: Development Team from build settings

5887d42

replace: Development team credentials to default credentials

994b816

replace: bundle identifier of example with default identifier

8cacfff

mithilesh-parmar added 3 commits December 20, 2021 16:54

add: Localized strings for alerts

01220e0

add: default values for strings

dff1e7b

add: Localized strings keys as comments in settings class

8e9eb1c

bivant reviewed Dec 21, 2021

View reviewed changes


		internal func checkAuthorizationStatus(){

		if #available(iOS 14, *) {

		@objcMembers public class Settings : NSObject {
		public static let shared = Settings()

Conversation

mithilesh-parmar commented Jun 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mithilesh-parmar Dec 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikaoj commented Dec 19, 2021

Uh oh!

mithilesh-parmar commented Dec 19, 2021

Uh oh!

mikaoj commented Dec 19, 2021

Uh oh!

mithilesh-parmar commented Dec 19, 2021

Uh oh!

mithilesh-parmar commented Dec 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mithilesh-parmar Dec 19, 2021 •

edited

Loading