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

Add password rating column to entry view #4797

Merged

Conversation

JJuiice
Copy link
Contributor

@JJuiice JJuiice commented May 30, 2020

Uses password health checker in order to retrieve password strength and color futher explained via tool-tip when hovering over column. Hidden by default

Closes #4216

Screenshots

Screenshot at 2020-05-30 18-18-03

Testing strategy

Unit and manual testing

Type of change

  • ✅ New feature (change that adds functionality)

@droidmonkey droidmonkey added this to the v2.7.0 milestone May 30, 2020
@droidmonkey
Copy link
Member

droidmonkey commented May 30, 2020

I would just use a three tier rating system: Red, Yellow, Green. The shades are too similar (although this is a ding on the health report colors really).

@JJuiice
Copy link
Contributor Author

JJuiice commented May 30, 2020

Unsure of failure on Windows testCli, will investigate

I would just use a three tier rating system: Red, Yellow, Green. The shades are too similar (although this is a ding on the health report colors really).

At the moment, I use the password health color palette. I utilize the same implementation as the password generator widget of "critical" coloring for the "Bad" and "Poor" category, "bad" coloring for "weak" passwords, and "good"/"excellent" for their respective ratings. I was thinking combining the "good" and "excellent" ratings to eliminate confusion with shading. Would that be appropriate?

@phoerious
Copy link
Member

There should also be no column reordering. Those lines are an artefact from before our settings refactor. 2.6 will simply reset users' column settings, no need to reorder anything.

@JJuiice JJuiice force-pushed the feature/password-rating-column branch from 398b845 to 35551b8 Compare August 28, 2020 01:55
src/gui/entry/EntryModel.cpp Outdated Show resolved Hide resolved
src/gui/entry/EntryModel.cpp Outdated Show resolved Hide resolved
src/gui/entry/EntryModel.cpp Outdated Show resolved Hide resolved
@networkException
Copy link

Would it be reasonable to add an option to show the health value directly as a number instead of a color indicator?
Even though the quality names ("Bad", "Good", etc) aren't bad of course, a user might want to have their own rating.
Additionally what value range is specified as what quality could also be made configurable.

@droidmonkey
Copy link
Member

Sweet!

@JJuiice
Copy link
Contributor Author

JJuiice commented Dec 31, 2020

Currently investigating the failure

@droidmonkey droidmonkey force-pushed the feature/password-rating-column branch from 904dfaf to a6a2c54 Compare February 6, 2021 21:16
@droidmonkey
Copy link
Member

Made major changes and also took this opportunity to clean up the code for reports exclusion:

image

@droidmonkey droidmonkey force-pushed the feature/password-rating-column branch from a6a2c54 to 2ee43eb Compare February 6, 2021 21:22
@phoerious
Copy link
Member

IMHO the column title should be an icon. It's ridiculously wide at the moment.

@droidmonkey
Copy link
Member

I was thinking the same thing, gotta find a good one

@droidmonkey
Copy link
Member

droidmonkey commented Feb 9, 2021

Made several changes:

  • Fixed handling of sort indicator icon. Phantom was using the common style size/position for dimensions and then applying their own drawing dimensions causing weird artifacts. I actually blame Qt for awkwardly splitting measurement from drawing.
  • Fixed size columns show full size icon instead of smaller icon due to poor handling by Phantom.
  • Fixed size columns resize to accommodate the sort indicator instead of overdrawing the icon.
  • Added icon for password strength column, removed centering code.

image

@droidmonkey droidmonkey force-pushed the feature/password-rating-column branch from 2ee43eb to aa2690c Compare February 9, 2021 03:50
@phoerious phoerious self-requested a review February 9, 2021 19:23
Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few things that need fixing.

Also, the new column is shown by default when you have a local keepassxc.ini and it is hidden when you don't. The default for whether this column is shown or hidden should be set explicitly. There may also be other columns which are missing an explicit default, which should be fixed along with it. I also believe the "Modified" column should be hidden by default.

src/gui/entry/EntryView.cpp Show resolved Hide resolved
src/gui/entry/EntryView.cpp Outdated Show resolved Hide resolved
src/gui/entry/EntryView.cpp Outdated Show resolved Hide resolved
src/gui/entry/EntryView.cpp Show resolved Hide resolved
src/gui/entry/EntryView.cpp Outdated Show resolved Hide resolved
src/gui/reports/ReportsWidgetStatistics.cpp Outdated Show resolved Hide resolved
@droidmonkey droidmonkey force-pushed the feature/password-rating-column branch from aa2690c to d187cad Compare February 12, 2021 04:13
@droidmonkey
Copy link
Member

Changes made, ready for final

JJuiice and others added 2 commits February 26, 2021 16:50
* Closes keepassxreboot#4216

Reduced to three-tiered rating system and fixed column implementation. Hide password strength indicator in entry view if excluded from reports.

Introduce password health caching to prevent unnecessary calculations.
@droidmonkey droidmonkey force-pushed the feature/password-rating-column branch from d187cad to 7149821 Compare February 26, 2021 22:18
@droidmonkey droidmonkey merged commit b9ea6fd into keepassxreboot:develop Feb 27, 2021
@phoerious phoerious added pr: new feature Pull request that adds a new feature and removed new feature labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: Reports pr: new feature Pull request that adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Password Rating column in entry table
4 participants