-
Notifications
You must be signed in to change notification settings - Fork 112
Reconsider the quick password access support #574
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
base: master
Are you sure you want to change the base?
Conversation
66fd1e3
to
f09f7ef
Compare
Thanks, it looks better but for portability, it should be behind a feature gate. |
I will add a feature gate |
You could consider making a Message enum in the password_manager.rs file and then embedding it in a Password message at the higher level. That would make it more modular. |
Then, something like this would be all that is needed in the update function, and any added messages would not all need feature gates.
|
This didn't really work, since we need access to the pane_model to get the terminal to paste to it. But it still got a lot better, since it is now all in one block |
src/password_manager.rs
Outdated
)), | ||
) | ||
.push( | ||
widget::button::text("-") |
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.
Please use the icon button used for deleting profiles.
The delete button is the last thing I saw, but UX may have other changes. |
Thanks for the mockup! I wil try to implement this during the weekend |
57ca52e
to
486d88f
Compare
I have rewritten the UI to resemble the mocup. A minor difference is that I used a secured text box for the password, and that have a show/hide password icon instead of the copy icon. |
486d88f
to
e2018a6
Compare
I also changed the title from "Manage Passwords" to just "Passwords" since the main use of the page is to access and paste passwords, the management is still possible but that is not what it is mainly used for. |
Thanks, I will review on Monday |
One thing that I'm not really happy with is the Length::Fixed(290.0) .... if I replace the button with text, then Length::Fill works as expected, but if I use a button (which I need since the user must be able to click it to auto fill the password) then Length::Fill will cause the button to push all the other element of the visible area of the row. Not sure why a text widget and a button widget behaves differently here. So, for now a Fixed since of 290 is the only way I could get it to render properly. |
e2018a6
to
afa779d
Compare
Hi
I have been maintaining this "password manger" feature in my own branch, but it would be good for things like i18n and general discoverability to get it merged.
Last time it was said that a password management feature is not in line with the product and that users can use the regular password manager (like seahorse) instead. To this, I would like to point out that it is not really much of a manager, that is not the feature. The feature is to provide quick and easy access to insert passwords for ssh, vpn and similar. The passwords are already stored in your keyring, so this just provides quick access to them. Much like the auto-fill feature in iOS and android.
So, the workflow for the user to start a cmdline based vpn will be:
Instead of:
If you work with many VPNs or other tasks tasks that requires a lot of passwords, this feature will make your life a lot easier.
The downside to include this is mainly more code to maintain. But since I have been maintaining it in it's own branch, I have structured it to be easy to maintain so that should not be a very big problem.