search

venmo / VENTouchLock

A Touch ID and Passcode framework used in the Venmo app.
MIT License
965 stars 114 forks source link

Use window instead of pushing the lock onto the view controller #65

Open ayanonagon opened 8 years ago

eliperkins commented 8 years ago

Hm... Looks like Travis isn't too happy about Xcode 6.3... Any idea why we ended up with a box with 6.3 instead of 6.4?

eliperkins commented 8 years ago

Code looks good though!

@zinthemoney you wanna take a look at this change too, for a second set of :eyes:? I know you mentioned at DubDub this year that you're using this in Chime, so I just wanted to make sure these changes don't break too much for y'all!

zinthemoney commented 8 years ago

@ayanonagon, can you give a little background / problems on start using the window?

@eliperkins, by replacing VENTouchLock 1.11.0 with branch an/fix on my project, it doesn't work as before. I will take a deeper look later.

ayanonagon commented 8 years ago

@zinthemoney Sure! Two main reasons:

  1. It’s not cool that the current VENTouchLock mucks around with your app’s view controller hierarchy. By having a separate window, it’s a lot cleaner to maintain, and doesn’t need to care about stuff going on in the app’s main window.
  2. We can more reliably take a snapshot of the app when it goes into background mode, so we can cover sensitive data when the app is show in the app switcher.

Let me know if you have any more questions. Thanks for trying it out :D

eliperkins commented 8 years ago

@zinthemoney ah, certainly true. The idea spurned out of some information from this blog post: http://www.thecave.com/2015/09/28/how-to-present-an-alert-view-using-uialertcontroller-when-you-dont-have-a-view-controller/, specifically:

At WWDC I stopped in at one of the labs and asked an Apple Engineer this same question: “What was the best practice for displaying a UIAlertController?” And he said they had been getting this question a lot and we joked that they should have had a session on it. He said that internally Apple is creating a UIWindow with a transparent UIViewController and then presenting the UIAlertController on it.

I think setting the window level might help us mitigate some of those concerns: https://github.com/venmo/VENTouchLock/pull/65/files#r40910236 by at least placing our window over the rest. I'll have to check to see if this accomplishes what you're discussing, however, that touching our application's window will definitely have some interesting side-effects.

dasmer commented 8 years ago

@ayanonagon what's the status of this PR?

ghost commented 6 years ago

What's the status on this PR?

ericlewis commented 6 years ago

Not to speak for everyone, but as an active maintainer we haven’t been focused on implementing this. If you’d like to chat with me or test or suggest things we’d love to hear it. But as it stands this implementation doesn’t function for everyone.