-
Notifications
You must be signed in to change notification settings - Fork 0
Password protected entry point for debug menu #12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- View modifier to bring up debug password alert - Password protection functionality
fb3ead4 to
a9fbf4b
Compare
nbrooke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks pretty reasonable to me. A few thing (mostly interface tweaks) I'd like to see changed:
- Right not it seems like (maybe I've misread the code, didn't actually try it) that it asks for the password every time you trigger the debug menu gesture. I think ideally you should only need to authorize once, and subsequent times you do the gesture it it just goes directly to the menu without needing to enter the password again.
- I think that probably too many of the extensions and things related to the alerts are public. Adding an extension with the super-generic name of "alert" to all Views seems like pretty bad form. Ideally, the only API from this that should be publicly visible is the "debugEntry" modifier.
- The sha256 extension on String: same deal, should probably be private to avoid polluting the global namespace.
- Speaking of the debugEntry modifier, I don't love the name of that, though don't have like a slam dunk better candidate in mind. I think the problem is it reads to me more like "this in a entry in some sort of table of debug options" rather than "this is the entry point for launching the debug menu" Maybe something like "debugMenuNavigation"? I think I like that better, though still don't love it.
Some other thoughts on future directions, these don't need to be addressed in this change, just some things to think about:
- A lot of times the current interface we actually use for unlocking the debug menu is that the long press / multi-tap gesture doesn't GO to the debug menu, but shows a hidden menu item in the settings view that can now just go directly to the settings view. That's always going to require a little more involvement from the app itself then this approach (need to add a view to the List for your menu or whatever), but it's be nice if it was structured so that you can at least use the password checking logic in that case (Maybe you can pass the debugEntry modifier a custom binding to toggle instead of actually showing the menu, and then put a regular list item that does a drill in to the debug menu, either one we provide or custom to the app, that's shown or hidden based on that binding in your settings view).
- It might be nice to have some way to set a global default for the password (so if you have multiple entry points, they can just automatically use the same password).
- It would also be nice to have some select-ability in what gesture to use. In particular, I think it could be useful to have two entry points with different complexity of gestures. Like one relatively easy one that's a little hidden (long press on this specific view in the settings menu) and one more global one with a more complicated gesture (three finger quadruple tap on the root view to bring the debug menu up from ANYWHERE). Maybe debugEntry should take an enum of what gesture to use rather than just a long press duration?
- Speaking of that, we should try and verify that this modifier CAN be used to put in a global gesture that can be triggered from anywhere (obviously do need an option for a more complicated gesture to make that at all feasible). It seems like it should (just put the modifier on the root view of the scene), but it'd be nice to verify that works.
|
I don't have a ton to add to Nigel's review, other than to echo his sentiment that we should probably err on the side of things being private unless they have to be public, and then being deliberate to name then things that are unlikely to collide with other namespaces. Also, it's not really a blocking problem, but IMO it's a little more Swifty to use [computed properties](Not a blocking change, but IMO it's a little more Swifty to have simple accessors like |
- Update Example project
|
@nbrooke will add your points for future improvements to a separate ticket |
nbrooke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Two more comments on the public / private stuff (don't think I needs a re-review after addressing, so just gonna approve anyway).
- With this change it looks like all the unnecessarily public interfaces HAVE been privatized, but there are still a few cases where the access level appears inconsistent (there are some
internaltypes, but that still havepublicmembers). That doesn't actually affect the semantics at all, members only have the visibility of their enclosing type so apublicmember of aninternaltype is actually also internal, HOWEVER it might be a little confusing to people to have a bunch of methods that are marked as public but aren't actually public. We should remove thepublicfrom those methods - Technically, you never need to specify
internalaccess level, since it's the default. I think there is some argument to be made than for a library, where you do have a mix ofpublicandinternaltypes there is some value in redundantly specifying it (at least for top level declarations, individual properties seems like overkill) to make the public / internal split clearer. However, if we are going to do that, we problaby need to do it consistently, at least within a given library, possibly globally in all our projects, and I don't think we DO generally do that in other libraries. So I'd be inclined to remove the current uses of internal, or if someone does feel strongly about using them, we should ADD internal to all the other internal types that don't specify it.
This PR introduces a custom view modifier that lets us access the debug menu from any view by providing a datasource, sha256 encrypted password and a min long press time to activate the alert.
The code to define the entry point is now:
I made the longPressDuration short just for testing purposes, ideally it should be like 5 seconds. The gesture we want to use is TBD doesn't have to be long press.
Details:
RPReplay_Final1638815571.MP4