Permission handling in image picker #334

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

Example.xcodeproj/project.pbxproj

@@ -432,14 +432,14 @@
 			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

Sources/Controller/ImagePickerController.swift

@@ -47,6 +48,7 @@ import Photos
    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

Sources/Extension/PHAuthorization+BSImagePicker.swift

+
+    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

Sources/Scene/Assets/AssetsViewController.swift

+    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

Sources/Scene/Assets/AutorizationStatusHeaderView.swift

+
+            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

Sources/Extension/Alert+BSImagePicker.swift

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


Primart - typo

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

Sources/Scene/Assets/AutorizationStatusHeaderView.swift

+            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

Sources/Scene/Assets/AutorizationStatusHeaderView.swift

+            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

Sources/Model/Settings.swift

@@ -26,7 +26,7 @@ import Photos
 @objc(BSImagePickerSettings) // Fix for ObjC header name conflicting.
 @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

Sources/Extension/Alert+BSImagePicker.swift

+        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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permission handling in image picker #334

Permission handling in image picker #334

mithilesh-parmar commented Jun 1, 2021

bivant Dec 16, 2021

mithilesh-parmar Dec 19, 2021 •

edited

Loading

bivant Dec 16, 2021

mithilesh-parmar Dec 19, 2021

bivant Dec 16, 2021

mithilesh-parmar Dec 19, 2021

bivant Dec 16, 2021

mithilesh-parmar Dec 19, 2021

bivant Dec 16, 2021

mithilesh-parmar Dec 19, 2021

mikaoj commented Dec 19, 2021

mithilesh-parmar commented Dec 19, 2021

mikaoj commented Dec 19, 2021

mithilesh-parmar commented Dec 19, 2021

mithilesh-parmar commented Dec 20, 2021

bivant Dec 21, 2021

bivant Dec 21, 2021

bivant Dec 21, 2021

bivant Dec 21, 2021

bivant Dec 21, 2021


		internal func checkAuthorizationStatus(){

		if #available(iOS 14, *) {

Permission handling in image picker #334

Are you sure you want to change the base?

Permission handling in image picker #334

Conversation

mithilesh-parmar commented Jun 1, 2021

Choose a reason for hiding this comment

mithilesh-parmar Dec 19, 2021 • edited Loading

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

mikaoj commented Dec 19, 2021

mithilesh-parmar commented Dec 19, 2021

mikaoj commented Dec 19, 2021

mithilesh-parmar commented Dec 19, 2021

mithilesh-parmar commented Dec 20, 2021

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

mithilesh-parmar Dec 19, 2021 •

edited

Loading