Skip to content
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

Settings screen for password digit grouping #290

Merged
merged 28 commits into from
Sep 26, 2018
Merged

Conversation

mattrubin
Copy link
Owner

@mattrubin mattrubin commented Sep 25, 2018

TL;DR: This PR adds a settings screen that allows the user to choose whether they prefer passwords rendered in groups of two digits or groups of three.


This PR is the result of two separate issues I've been thinking about for a long time:

  1. Version 2.0 of the app changed how passwords are displayed: instead of showing the whole password as a single string of digits, the passwords are chunked into two-digit groups to make them easier to read and remember. Since then, several users of the app have reported that they have an easier time remembering six-digit passwords when they are chunked into groups of three digits. A pull request from @beaucollins proposed to change the chunk size dynamically based on the length of the password, but despite the cleverness of this approach, it never quite felt like the right solution to me.
    I avoided making any changes on this for almost a year, for a very simple reason – I personally find it much easier to read and remember passwords chunked into groups of two, and the handful of users I had originally asked for input felt the same way. (At least one user wrote in an App Store review that the two-digit grouping is the specific reason they use Authenticator over other two-factor apps.) I have been reluctant to make any changes when the current two-digit chunking strategy has been working for (anecdotally) most users.
  2. As long as I have been developing Authenticator, I have tried to keep the app as simple as possible. I want the app to be clear and straightforward for users, and I didn't want to add a settings screen with a bunch of potentially-confusing options. In addition to complexity for users, adding user settings increases the number of different states in which the app operates, and can make testing and debugging significantly more complicated. For these reasons, I've avoided adding a settings screen to the app.

After thinking over the best way to present passwords that are readable and memorable for all users, I've concluded that the solution is to overcome my reluctance to add a settings screen to the app, and to add a toggle allowing the user to pick how they prefer for passwords to be grouped. Currently, the options are groups of two digits or groups of three digits. If I receive further user feedback on the issue, I will consider adding more options, but for now I want to keep things as simple as possible. While I think the approach of changing the chunk size based on password length is aesthetically pleasing, I have found in my conversations with users that preferred chunk size for ease of memorization seems constant for a given person, regardless of password length.

I would love to hear what people think of this approach, and in particular to know whether this is is a satisfactory solution for those who prefer passwords in groups of three digits.

Screenshot with passwords in groups of two digits Screenshot of settings screen with toggle for password grouping size Screenshot with passwords in groups of three digits

This copies the TokenFormViewController implementation to implement DisplayOptionsViewController.
Move the TableViewModel-related complexity into DisplayOptionsViewController.
@mide
Copy link

mide commented Sep 25, 2018

I'm not familiar with Swift, but this looks great! I'm very impressed by the options menu and how clear the two options are. Well done!

@codecov
Copy link

codecov bot commented Sep 25, 2018

Codecov Report

Merging #290 into develop will decrease coverage by 2.42%.
The diff coverage is 21.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #290      +/-   ##
===========================================
- Coverage    40.91%   38.48%   -2.43%     
===========================================
  Files           36       40       +4     
  Lines         1662     1863     +201     
===========================================
+ Hits           680      717      +37     
- Misses         982     1146     +164
Impacted Files Coverage Δ
Authenticator/Source/InfoListViewController.swift 0% <ø> (ø) ⬆️
Authenticator/Source/RootViewController.swift 21.05% <0%> (-3.69%) ⬇️
Authenticator/Source/DisplayOptions.swift 0% <0%> (ø)
...nticator/Source/DisplayOptionsViewController.swift 0% <0%> (ø)
Authenticator/Source/TokenFormViewController.swift 0% <0%> (ø) ⬆️
Authenticator/Source/TokenList.swift 100% <100%> (ø) ⬆️
Authenticator/Source/TokenRowModel.swift 94.44% <100%> (ø) ⬆️
Authenticator/Source/InfoList.swift 100% <100%> (ø) ⬆️
Authenticator/Source/AppController.swift 26.66% <40%> (+0.61%) ⬆️
Authenticator/Source/Settings.swift 50% <50%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 713a80f...a8a7e33. Read the comment docs.

@mattrubin
Copy link
Owner Author

mattrubin commented Sep 25, 2018

To clarify: all passwords will be chunked into the same size digit groups, regardless of password length, even if that results in the password ending with an incomplete group.

Screenshot with passwords in groups of two digits. A seven-digit password has three groups of two digits followed by a one-digit group. Screenshot with passwords in groups of three digits. A seven-digit password has two groups of three digits followed by a one-digit group. An eight-digit password has two groups of three digits followed by a two-digit group.

@beaucollins
Copy link
Collaborator

beaucollins commented Sep 26, 2018

I understand the desire to not have a settings screen. I feel a trade off could be hiding the toggle behind the toolbar then revealing it by dragging down.

First launch sets the scroll offset to to show the option but future launches set it to the first password.

If you plan to have more options then the settings screen makes the most sense.

@mattrubin
Copy link
Owner Author

@beaucollins Putting the toggle at the top of the list and hiding it by default after first launch is clever, but adding the toggle to the main screen is probably too prominent a place for a control most users will only ever use one time (or zero times).

And, as you pointed out, a settings screen would eventually be made necessary by plenty of other potential features (TouchID, encrypted backups, etc.), so adding it now makes sense.

@mattrubin mattrubin added this to the 2.next milestone Sep 26, 2018
@mattrubin
Copy link
Owner Author

@mide @beaucollins Thanks for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants