search

velikanov / SwiftPasscodeLock

An iOS passcode lock with TouchID authentication written in Swift.
MIT License
199 stars 52 forks source link

Updates to accommodate FaceID and cancellable locks #53

Closed mjmarathon closed 6 years ago

mjmarathon commented 6 years ago

This pull request encompasses two issues.

  1. With the release of iOS 11.0 Apple released FaceID. Fortunately all of the calls are identical to using TouchId so there weren't any changes needed there. However, the current property names and text on the view controller do not reflect that. They were changed to a more generic "Biometric Authentication" to better reflect functionality. A potential improvement for this is that as of iOS 11.0 the device can differentiate between which functionality the device supports and we could change the text of the button on the controller to reflect this.

  2. Currently the only cancellable lock type is .remove which is done via the declaration of the type. This really doesn't make any sense. We should allow users to decide which locks are and are not cancellable. I have added a property to the PasscodeLockConfigurationType class which the view controller will key off of to show/hide the cancel button. Personally I think this makes more sense as the cancellation seems more like a configuration thing rather than a lock type thing.

ziogaschr commented 6 years ago

@mjmarathon nice work man, I will try to review it locally soon. I don't see any changes to the strings file and also the image for TouchID (fingerprint) in the UI remains the same, do you think it will be a good idea to change those too? Can we somehow check if the supported method is TouchID or FaceID?

mjmarathon commented 6 years ago

@ziogaschr In the order of things you asked.

  1. Looking at the strings file I can see that we're using "PasscodeLockTouchIDReason" as a default when we're presenting the TouchID prompt. FaceID will use this in the same way so no change is needed other than to make the naming scheme more accurate.
  2. I am not seeing which images you're referring to, if you mean on the prompt for face/touchid itself those are handled by the OS. It that's not what you're talking about lemme know where these images are referenced and i'll take a look.
  3. FaceID is only supported by iOS 11 and up. With that OS release we can indeed check which method is supported. The way apple handled this code makes it seem like a device is either going to have face OR touch and not both. Personally I've found handling this to be a bit of a pain in the ass to deal with all the @avaliables needed for UI to reflect which is supported by the device but I can take another look at it if you'd like.
ziogaschr commented 6 years ago

Hey @mjmarathon, sorry for the long wait. I am checking the code now.

Can you explain why have you replaced isCancellableAction from PasscodeLockStateType with PasscodeLockConfigurationType .allowsCancellation. I don't think this is bad, but for this to work for all use cases we will have to make the PasscodeLockConfigurationType copyable by implementing the NSCopying, this way it might be easier for someone to change this or any value for a specific PIN display.

Although despite what we think, a lot of people are using this project already and we have to keep it backwards compatible, so this change will break all these apps. Such change, if needed, might be fine to be added in a next major release (v2.0).

For the rest methods that you've renamed TouchID to Biometrics, I liked them, but we can keep the old ones pointing to new ones, at least for v1.0 with a deprecation warning.

What do you think? Are you fine if I make changes to your PR or will you like to make them your own?

ziogaschr commented 6 years ago

@mjmarathon Please check #54 for FaceID (based on your PR 👍 ). For the cancelable actions, please make a new PR explaining the reasons for it. :)