[ui] Add toggle for unblinding password fields by tjps · Pull Request #11480 · bitcoin/bitcoin

tjps · 2017-10-11T05:01:55Z

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.

laanwj · 2017-10-11T06:54:53Z

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 · 2017-10-11T07:22:15Z

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 · 2017-10-11T08:12:09Z

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

Concept ACK.

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

promag · 2017-10-11T08:25:58Z

src/qt/askpassphrasedialog.cpp

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

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

promag · 2017-10-11T08:31:25Z

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 · 2017-10-11T09:07:13Z

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

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

promag · 2017-10-11T09:13:10Z

src/qt/askpassphrasedialog.cpp

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.

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

promag · 2017-10-11T09:13:41Z

src/qt/forms/askpassphrasedialog.ui

Nit, name="toggleShowPasswordButton".

promag · 2017-10-11T09:14:19Z

src/qt/askpassphrasedialog.cpp

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 · 2017-10-11T09:28:17Z

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 · 2017-10-11T09:48:17Z

Agree, use the existing eye icon.

tjps · 2017-10-12T01:04:14Z

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 · 2017-10-12T10:50:23Z

Concept ACK. Could you post a screenshot?

tjps · 2017-10-12T18:48:41Z

Screenshot:

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

achow101 · 2017-10-12T19:01:58Z

screenshot-looks-good-and-utACK 26234dbb77ef7de1dbf65728081d3b2ba9e15b0d

jonasschnelli · 2017-10-12T19:29:31Z

src/qt/askpassphrasedialog.cpp

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

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.

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

jonasschnelli · 2017-10-12T19:29:56Z

Concept ACK. Test soon.

sipa · 2017-10-12T23:09:01Z

Concept-looks-nice ACK

promag · 2017-10-12T23:28:13Z

src/qt/askpassphrasedialog.cpp

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

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().

promag · 2017-10-12T23:28:44Z

src/qt/askpassphrasedialog.cpp

This can be set in the .ui?

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.

jonasschnelli · 2017-10-15T04:03:03Z

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):

tjps · 2017-10-18T18:39:13Z

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

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

meshcollider · 2017-10-18T19:30:44Z

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

promag · 2017-10-18T19:39:18Z

👍 checkbox, the layout is also cleaner.

tjps · 2017-10-18T20:58:57Z

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

laanwj · 2017-10-19T12:46:34Z

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 · 2017-10-19T22:55:29Z

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 · 2017-10-25T01:03:26Z

@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 · 2017-11-06T20:48:58Z

@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 · 2017-11-07T07:26:06Z

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

Tested ACK ff35de8

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 · 2017-11-07T07:37:38Z

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

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

fanquake added the GUI label Oct 11, 2017

promag reviewed Oct 11, 2017

View reviewed changes

jonasschnelli reviewed Oct 12, 2017

View reviewed changes

promag reviewed Oct 12, 2017

View reviewed changes

[ui] Add toggle for unblinding password fields

ff35de8

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

tjps deleted the tjps_wallet_dialog branch November 17, 2017 00:55

meshcollider mentioned this pull request Jan 17, 2018

TODO for release notes 0.16.0 #11054

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

tjps commented Oct 11, 2017

Uh oh!

laanwj commented Oct 11, 2017

Uh oh!

tjps commented Oct 11, 2017

Uh oh!

laanwj commented Oct 11, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Oct 11, 2017

Uh oh!

tjps commented Oct 11, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Oct 11, 2017

Uh oh!

tjps commented Oct 12, 2017

Uh oh!

meshcollider commented Oct 12, 2017

Uh oh!

tjps commented Oct 12, 2017

Uh oh!

achow101 commented Oct 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Oct 12, 2017

Uh oh!

sipa commented Oct 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Oct 15, 2017

Uh oh!

tjps commented Oct 18, 2017

Uh oh!

meshcollider commented Oct 18, 2017

Uh oh!

promag commented Oct 18, 2017

Uh oh!

tjps commented Oct 18, 2017

Uh oh!

laanwj commented Oct 19, 2017

Uh oh!

promag Oct 11, 2017 •

edited

Loading

laanwj commented Oct 11, 2017 •

edited

Loading

promag Oct 12, 2017 •

edited

Loading

promag commented Oct 19, 2017 •

edited

Loading