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.
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?
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.
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.
22 | @@ -23,7 +23,8 @@ AskPassphraseDialog::AskPassphraseDialog(Mode _mode, QWidget *parent) : 23 | ui(new Ui::AskPassphraseDialog), 24 | mode(_mode), 25 | model(0), 26 | - fCapsLock(false) 27 | + fCapsLock(false), 28 | + fShowPass(false)
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.
Concept ACK.
In terms of UI QLineEdit could be extended to support this feature, but probably that's too much.
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?
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.
234 | @@ -234,6 +235,15 @@ bool AskPassphraseDialog::event(QEvent *event) 235 | return QWidget::event(event); 236 | } 237 | 238 | +void AskPassphraseDialog::setShowPass(bool showPass)
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.
70 | + <enum>QLineEdit::Password</enum> 71 | + </property> 72 | + </widget> 73 | + </item> 74 | + <item> 75 | + <widget class="QToolButton" name="passShow">
Nit, name="toggleShowPasswordButton".
234 | @@ -234,6 +235,15 @@ bool AskPassphraseDialog::event(QEvent *event) 235 | return QWidget::event(event); 236 | } 237 | 238 | +void AskPassphraseDialog::setShowPass(bool showPass) 239 | +{ 240 | + ui->passShow->setDown(showPass);
Why?
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.
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: <img src="https://github.com/bitcoin/bitcoin/raw/master/src/qt/res/icons/eye.png"> Would it be heretical to suggest re-using that?
Agree, use the existing eye icon.
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.
Concept ACK. Could you post a screenshot?
Screenshot:
I'd also consider putting it out to the right of the top-most text input, adding another narrow vertical column.
screenshot-looks-good-and-utACK 26234dbb77ef7de1dbf65728081d3b2ba9e15b0d
236 | @@ -234,6 +237,15 @@ bool AskPassphraseDialog::event(QEvent *event) 237 | return QWidget::event(event); 238 | } 239 | 240 | +void AskPassphraseDialog::toggleShowPassword(bool show) 241 | +{ 242 | + ui->toggleShowPasswordButton->setDown(show); 243 | + auto mode = show ? QLineEdit::Normal : QLineEdit::Password;
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.
Concept ACK. Test soon.
Concept-looks-nice ACK
236 | @@ -234,6 +237,15 @@ bool AskPassphraseDialog::event(QEvent *event) 237 | return QWidget::event(event); 238 | } 239 | 240 | +void AskPassphraseDialog::toggleShowPassword(bool show) 241 | +{ 242 | + ui->toggleShowPasswordButton->setDown(show);
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().
39 | @@ -40,6 +40,8 @@ AskPassphraseDialog::AskPassphraseDialog(Mode _mode, QWidget *parent) : 40 | ui->passEdit2->installEventFilter(this); 41 | ui->passEdit3->installEventFilter(this); 42 | 43 | + ui->toggleShowPasswordButton->setIcon(QIcon(":/icons/eye"));
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.
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): <img width="588" alt="bildschirmfoto 2017-10-14 um 21 01 59" src="https://user-images.githubusercontent.com/178464/31581524-03b2a0be-b123-11e7-92c1-1f1b80491763.png">
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?
Slight preference for the checkbox personally, it's clearer than the eye
:+1: checkbox, the layout is also cleaner.
Checkbox seems to be the group consensus, and it also makes the tabstop ordering clearer.
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.
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).
@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?