search

velikanov / SwiftPasscodeLock

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

Made the value of reason in authenticateWithBiometrics injectable #21

Closed phampton24 closed 8 years ago

phampton24 commented 8 years ago

I've coded a potential solution to issue #20. This solution leverages PasscodeLockConfigurationType to optionally pass a value leveraged by authenticateWithBiometrics. If the value is not set in the implementation of the Configuration Type then the code falls back to the existing logic that uses the strings resource file.

ziogaschr commented 8 years ago

Looks ok, and I agree that more people might need this. I didn't have the time to test it though. @velikanov what do you think? I will try to test it during the weekend

velikanov commented 8 years ago

@ziogaschr, unfortunately, I just don't have enough time even to test it :( too busy with my main job feel free to make anything that will bring us to success :) cheers!

ziogaschr commented 8 years ago

I have tested this and it is great. Although it will break current Apps that are using this framework, so I vote for keeping it for the next major release v2.0.0.

What do you think?

phampton24 commented 8 years ago

@ziogaschr I'm curious about your statement that the changes will break current Apps that are using the framework. My intention was to make the changes fully backwards compatible for existing user's of the framework, which I thought I did by making the touchIdReason optional on PasscodeLockConfigurationType. If there's something I overlooked, though, I'd appreciate your feedback so that I can file it away for future reference.

ziogaschr commented 8 years ago

You are tottaly right @phampton24. I thought that var touchIdReason: String? = nil will have to be implemented in the Apps code, otherwise it will threw an error. I just tested it and it works ok.

ziogaschr commented 8 years ago

Grrrr, I had the same issue #30 at the beginning. Although, it wasn't a problem last time I tested it, before doing the merge.

ziogaschr commented 8 years ago

Shall we revert this one and has already caused trouble to people