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

[ui] Add toggle for unblinding password fields #11480

Merged
merged 1 commit into from Nov 7, 2017

Conversation

Projects
None yet
8 participants
@tjps
Copy link
Contributor

tjps commented Oct 11, 2017

Proposed change for adding the ability to toggle password visibility in the password dialog. This is similar to functionality in most password managers and is specifically added with the use case of password managers in mind - the password in that case is likely pasted twice into both the new password and confirm password fields.

If this is a welcome change, I am open to suggestions on rearranging the layout.

@fanquake fanquake added the GUI label Oct 11, 2017

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 11, 2017

This is similar to functionality in most password managers and is specifically added with the use case of password managers in mind

I don't entirely follow how this helps with password managers. As far as I know, you already paste into both the new password and confirm password fields?

@tjps

This comment has been minimized.

Copy link
Contributor Author

tjps commented Oct 11, 2017

The idea is that say you miss the actual password copy, or you copy the wrong entry, or you use the middle-click paste (Linux distros) instead of system clipboard paste, etc. - any disconnect between what you think you are putting in that field and what is actually put in that field.

I realize this may be a rarer use case, but an option to unblind the password field is an increasingly common UI element and would certainly add to peace of mind for my own use case when setting a password generated by a password manager.

As it is now, the flow to verify is to create a new wallet, generate a password, encrypt the wallet with it, then attempt to sign a message to verify that unlock works. This simplifies that process and potentially has other use cases as well, without reducing security.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 11, 2017

any disconnect between what you think you are putting in that field and what is actually put in that field.

Yes, that seems like a valid argument to me, I just didn't follow before, concept ACK then.

@promag
Copy link
Member

promag left a comment

Concept ACK.

In terms of UI QLineEdit could be extended to support this feature, but probably that's too much.

src/qt/askpassphrasedialog.cpp Outdated
@@ -23,7 +23,8 @@ AskPassphraseDialog::AskPassphraseDialog(Mode _mode, QWidget *parent) :
ui(new Ui::AskPassphraseDialog),
mode(_mode),
model(0),
fCapsLock(false)
fCapsLock(false),
fShowPass(false)

This comment has been minimized.

@promag

promag Oct 11, 2017

Member

Remove this variable. Use toggled(bool checked) signal (http://doc.qt.io/qt-5/qabstractbutton.html#toggled) which gives the state needed.

This comment has been minimized.

@tjps

tjps Oct 11, 2017

Author Contributor

I had initially tried that, but toggled(bool) was not firing. It turns out I was setting the 'checkable' property incorrectly. Fixed now.

@promag

This comment has been minimized.

Copy link
Member

promag commented Oct 11, 2017

BTW, IIRC in windows the password is only visible while the button is being pressed, it's like a sneak peek. Just saying that there are other alternatives of achieving this.

I also wonder if this button is a candidate for a new icon?

@tjps tjps force-pushed the tjps:tjps_wallet_dialog branch Oct 11, 2017

@tjps

This comment has been minimized.

Copy link
Contributor Author

tjps commented Oct 11, 2017

Re: extending QLineEdit to support this, I had thought it would be neat to add a small "eye" icon (such as in KeyPassX) to right border of the control to support this, but didn't want to scope creep this PR.

I also see that there are very few icon graphics in the repo and didn't want to start down the path of adding one just for this, but using an icon would remove the need for any translation work for this new element.

@promag
Copy link
Member

promag left a comment

@tjps yes I think this is fine without icon for the moment.

src/qt/askpassphrasedialog.cpp Outdated
@@ -234,6 +235,15 @@ bool AskPassphraseDialog::event(QEvent *event)
return QWidget::event(event);
}

void AskPassphraseDialog::setShowPass(bool showPass)

This comment has been minimized.

@promag

promag Oct 11, 2017

Member

Nit, this is not a setter, just void showPassword(bool show).

Edit: or void toggleShowPassword(bool show). The idea is that this slot is an action, not a property change.

This comment has been minimized.

@tjps

tjps Oct 11, 2017

Author Contributor

Changed. Thanks for the pointer, I'm new to Qt.

src/qt/forms/askpassphrasedialog.ui Outdated
</widget>
</item>
<item>
<widget class="QToolButton" name="passShow">

This comment has been minimized.

@promag

promag Oct 11, 2017

Member

Nit, name="toggleShowPasswordButton".

src/qt/askpassphrasedialog.cpp Outdated
@@ -234,6 +235,15 @@ bool AskPassphraseDialog::event(QEvent *event)
return QWidget::event(event);
}

void AskPassphraseDialog::setShowPass(bool showPass)
{
ui->passShow->setDown(showPass);

This comment has been minimized.

@promag

This comment has been minimized.

@tjps

tjps Oct 11, 2017

Author Contributor

You mean why setDown()? That is to show that it is in the 'active' state when the passwords are visible. When you toggle back to masking the passwords, the button returns to the original up state.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 11, 2017

I also see that there are very few icon graphics in the repo and didn't want to start down the path of adding one just for this, but using an icon would remove the need for any translation work for this new element.

Yes, agree, an eye icon would be nice.
Edit: We have an eye icon in the repository, used in the transaction list:
Would it be heretical to suggest re-using that?

@promag

This comment has been minimized.

Copy link
Member

promag commented Oct 11, 2017

Agree, use the existing eye icon.

@tjps tjps force-pushed the tjps:tjps_wallet_dialog branch Oct 12, 2017

@tjps

This comment has been minimized.

Copy link
Contributor Author

tjps commented Oct 12, 2017

Addressed comments, and switched to using the eye icon.

The only remaining question in my mind is placement of the button on the form - currently it is right-aligned above the topmost text input, but it might possibly make sense to the right of the top-most text input.

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Oct 12, 2017

Concept ACK. Could you post a screenshot?

@tjps

This comment has been minimized.

Copy link
Contributor Author

tjps commented Oct 12, 2017

Screenshot:
screenshot at 2017-10-12 11 46 27

I'd also consider putting it out to the right of the top-most text input, adding another narrow vertical column.

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Oct 12, 2017

screenshot-looks-good-and-utACK 26234dbb77ef7de1dbf65728081d3b2ba9e15b0d

src/qt/askpassphrasedialog.cpp Outdated
void AskPassphraseDialog::toggleShowPassword(bool show)
{
ui->toggleShowPasswordButton->setDown(show);
auto mode = show ? QLineEdit::Normal : QLineEdit::Password;

This comment has been minimized.

@jonasschnelli

jonasschnelli Oct 12, 2017

Member

nit: does the auto here make sense? IMO this is a way of using auto that leads to confusion reading the code.

This comment has been minimized.

@tjps

tjps Oct 12, 2017

Author Contributor

auto is one of the rare items in C++11 I go back and forth on. My general heuristic is that if the thing declared as auto is used in at most the next five lines, and the type of the variable doesn't add anything meaningful to the understanding of the code, then auto is fine (and maybe slightly preferable).

In this case, mode is used just in the following three lines as an argument to a descriptively named setter. I don't think explicitly declaring it as QSomeEnumType adds anything here.

This comment has been minimized.

@promag

promag Oct 12, 2017

Member

IMO const auto echo_mode = ... and the RHS is descriptive enough.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Oct 12, 2017

Concept ACK. Test soon.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Oct 12, 2017

Concept-looks-nice ACK

@@ -234,6 +237,15 @@ bool AskPassphraseDialog::event(QEvent *event)
return QWidget::event(event);
}

void AskPassphraseDialog::toggleShowPassword(bool show)
{
ui->toggleShowPasswordButton->setDown(show);

This comment has been minimized.

@promag

promag Oct 12, 2017

Member

I haven't tested, but since the button is checkable this shouldn't be necessary right?

This comment has been minimized.

@tjps

tjps Oct 13, 2017

Author Contributor

It did not visually switch to being 'down' when toggled until I added the setDown() call. It seems to require both the checkable = true and the call to setDown().

src/qt/askpassphrasedialog.cpp Outdated
@@ -40,6 +40,8 @@ AskPassphraseDialog::AskPassphraseDialog(Mode _mode, QWidget *parent) :
ui->passEdit2->installEventFilter(this);
ui->passEdit3->installEventFilter(this);

ui->toggleShowPasswordButton->setIcon(QIcon(":/icons/eye"));

This comment has been minimized.

@promag

promag Oct 12, 2017

Member

This can be set in the .ui?

This comment has been minimized.

@tjps

tjps Oct 13, 2017

Author Contributor

It turns out it can. Wasn't immediately apparent from the docs, but then I discovered QtCreator which made it much simpler to play around with.

@tjps tjps force-pushed the tjps:tjps_wallet_dialog branch Oct 13, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Oct 15, 2017

Tested a bit. Works as intended.
UX wise it feels uncommon/unknown. Usually for a such show-passphrase features you have a checkbox "show password.
The mysterious eye-icon may lead to confusion.

I would recommend something like this (not 1:1 applicable):
bildschirmfoto 2017-10-14 um 21 01 59

@tjps

This comment has been minimized.

Copy link
Contributor Author

tjps commented Oct 18, 2017

So what do people prefer, the all-seeing eye on the top-right (pictured above), or a normal checkbox underneath like this:

screenshot at 2017-10-18 11 37 32

I have a slight preference for the eye, but can go either way. What's the consensus?

@MeshCollider

This comment has been minimized.

Copy link
Member

MeshCollider commented Oct 18, 2017

Slight preference for the checkbox personally, it's clearer than the eye

@promag

This comment has been minimized.

Copy link
Member

promag commented Oct 18, 2017

👍 checkbox, the layout is also cleaner.

@tjps tjps force-pushed the tjps:tjps_wallet_dialog branch to ff35de8 Oct 18, 2017

@tjps

This comment has been minimized.

Copy link
Contributor Author

tjps commented Oct 18, 2017

Checkbox seems to be the group consensus, and it also makes the tabstop ordering clearer.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 19, 2017

I'd also consider putting it out to the right of the top-most text input, adding another narrow vertical column.

Yes, if you stick with the icon, definitely line it up to the right of the input field, currently the association is not clear.

No strong opinion with regard to checkbox versus eye icon, though I slightly prefer the eye from a practical point of view because it doesn't add a translation message.

@promag

This comment has been minimized.

Copy link
Member

promag commented Oct 19, 2017

Yes, if you stick with the icon, definitely line it up to the right of the input field, currently the association is not clear.

@laanwj IMO the checkbox alternative is more clear regarding what it does, which is to show all passwords. Until you press the icon button you don't know if only the first password is shown or all.

Edit: maybe the checkbox text should be "Show passwords" (plural).

@tjps

This comment has been minimized.

Copy link
Contributor Author

tjps commented Oct 25, 2017

@promag - what's the process for reaching consensus here? I'm eager to get this into mainline so it is included in the next release so I don't have to run a patched version locally.

Should it be 'Show password' or 'Show passwords'? Should it change based on the number of edit fields visible on the form?

@tjps

This comment has been minimized.

Copy link
Contributor Author

tjps commented Nov 6, 2017

@promag @laanwj - 2 week check-in. I feel like consensus has been reached on what form the input should take, any chance to get this merged? Thanks.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 7, 2017

@tjps sorry, lost track of this a bit because of the focus on 0.15.1. Back on earth now.

Tested ACK ff35de8

@laanwj laanwj merged commit ff35de8 into bitcoin:master Nov 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 7, 2017

Merge #11480: [ui] Add toggle for unblinding password fields
ff35de8 [ui] Add toggle for unblinding password fields (Thomas Snider)

Pull request description:

  Proposed change for adding the ability to toggle password visibility in the password dialog.  This is similar to functionality in most password managers and is specifically added with the use case of password managers in mind - the password in that case is likely pasted twice into both the new password and confirm password fields.

  If this is a welcome change, I am open to suggestions on rearranging the layout.

Tree-SHA512: 1823f356f8f941cc584c44de264433e9a573cb8a358efa300a412c4458b5564d8d193969be40859195cf9c8d6768eee895ee22440d51db4f09175f9b4e28bced
@tjps

This comment has been minimized.

Copy link
Contributor Author

tjps commented Nov 7, 2017

@laanwj no worries, I appreciate that there is a lot more important things going on in the space recently. Thanks!

@tjps tjps deleted the tjps:tjps_wallet_dialog branch Nov 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.