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

Colored passwords in the preview pane #7097

Merged

Conversation

wolframroesler
Copy link
Contributor

In the entry preview pane, the password is now displayed with in multiple colours (digits in blue, upper-case letters in red, etc). The idea and the color scheme are taken from the iOS app "Strongbox".

A checkbox was added to section "General -> User Interface" of the settings dialog to turn color display off.

Fixes #4099

Screenshots

Screenshot from 2021-10-26 16-56-18

Screenshot from 2021-10-26 16-57-46

Testing strategy

Tested manually.

Type of change

  • ✅ New feature (change that adds functionality)

@codecov-commenter
Copy link

codecov-commenter commented Oct 30, 2021

Codecov Report

Base: 64.39% // Head: 64.42% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (91bfe3a) compared to base (54f9b25).
Patch coverage: 25.00% of modified lines in pull request are covered.

❗ Current head 91bfe3a differs from pull request most recent head 4faed7a. Consider uploading reports for the commit 4faed7a to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7097      +/-   ##
===========================================
+ Coverage    64.39%   64.42%   +0.03%     
===========================================
  Files          339      339              
  Lines        43990    43968      -22     
===========================================
- Hits         28324    28322       -2     
+ Misses       15666    15646      -20     
Impacted Files Coverage Δ
src/core/Config.cpp 89.76% <ø> (ø)
src/core/Config.h 100.00% <ø> (ø)
src/gui/EntryPreviewWidget.cpp 85.52% <17.65%> (-2.53%) ⬇️
src/gui/ApplicationSettingsWidget.cpp 51.42% <50.00%> (-0.01%) ⬇️
src/gui/Application.cpp 31.97% <100.00%> (+0.41%) ⬆️
src/core/FileWatcher.cpp 85.54% <0.00%> (-1.20%) ⬇️
src/core/Entry.cpp 82.55% <0.00%> (-0.16%) ⬇️
src/gui/MainWindow.cpp 71.45% <0.00%> (-0.04%) ⬇️
src/gui/osutils/nixutils/NixUtils.h 50.00% <0.00%> (ø)
src/gui/osutils/nixutils/NixUtils.cpp 43.00% <0.00%> (+1.56%) ⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@droidmonkey droidmonkey marked this pull request as draft November 25, 2021 03:38
@wolframroesler
Copy link
Contributor Author

Not really possible to resolve the conflict in demo.kdbx. You may keep or undo my changes, doesn't really matter, I didn't change anything significant.

@wolframroesler wolframroesler marked this pull request as ready for review March 4, 2022 13:38
@wolframroesler
Copy link
Contributor Author

Didn't realise this was in draft state. Un-drafted so it can be reviewed and eventually merged. Any chance to get it into the upcoming 2.7?

@droidmonkey
Copy link
Member

This will be looked at for 2.7.1

@wolframroesler wolframroesler force-pushed the feature/colour-passwds branch from 6030fbe to 246530f Compare March 4, 2022 15:18
@wolframroesler
Copy link
Contributor Author

Thanks @droidmonkey. Rebased it onto current develop and resolved the conflict in demo.kdbx so all should be good now.

@sky4055
Copy link

sky4055 commented Apr 7, 2022

This will be looked at for 2.7.1

version 2.7.1 does not include this evolution, is it correct?

@droidmonkey
Copy link
Member

Still open and unmerged

@droidmonkey
Copy link
Member

I am really just not a fan of colored passwords at all. KeePassDX implemented it on Android and it looks terrible, immediately disabled. I am, however, in favor of a popup widget that separately shows the password in large text with color and maybe even the position numbers below the letters. This could also be a way to perform "pickchars" auto-type directly from this widget.

Thoughts @wolframroesler @phoerious

@wolframroesler
Copy link
Contributor Author

The pop-idea sounds good, but should go into a different issue.

About the colours, some like them and some don't, the feedback I got when asking about it on Twitter (https://twitter.com/WolframRoesler/status/1452928493549981696) was mostly positive. Some people would definitely benefit from it (myself included) so the feature should be available IMHO, especially because there's a configuration option to turn it off; perhaps we could turn it off by default if it's too distracting for the majority of users.

Strongbox has been having it for a long time. What feedback did you receive, @strongbox-mark?

@strongbox-mark
Copy link

Strongbox has been having it for a long time. What feedback did you receive, @strongbox-mark?

Yes, entirely positive for both the Large Text popup feature and the colorized passwords. Not a single complaint, all positive comments, but I can understand why some don't like it. :)

BTW, on a completely separate topic, I can't respond to the other issue #863 as it's limited to collaborators - I meant to get in touch / reply to @varkolintu there, as this is something I'm considering adding support to Strongbox for (Credit Card fields or some kind of standardization for them). Was going to drop you guys a mail, but since we're here, can you add me as a collaborator there so that I can respond/chat, or is there a better place to do it?

@sky4055
Copy link

sky4055 commented Jun 20, 2022

Strongbox has been having it for a long time. What feedback did you receive, @strongbox-mark?

Yes, entirely positive for both the Large Text popup feature and the colorized passwords. Not a single complaint, all positive comments, but I can understand why some don't like it. :)

BTW, on a completely separate topic, I can't respond to the other issue #863 as it's limited to collaborators - I meant to get in touch / reply to @varkolintu there, as this is something I'm considering adding support to Strongbox for (Credit Card fields or some kind of standardization for them). Was going to drop you guys a mail, but since we're here, can you add me as a collaborator there so that I can respond/chat, or is there a better place to do it?

and on Stongbox I can't do without this feature. I'm looking forward to it on KeepassXC!

@droidmonkey
Copy link
Member

droidmonkey commented Jun 20, 2022

@strongbox-mark unlocked the issue! Although that thread is super long, we could also start a new discussion thread about the api / implementation. I too am very interested in it, just a lot of gui work.

I'll merge this feature in after another review of the code. A similar comment I have from the other colorized text one... it would be good to extract the code that actually creates the html so we can use it across multiple widgets (like the password generator) if we so desire. Although Qt annoyingly does not allow html in qlineedit.

@strongbox-mark
Copy link

Nice one, thank you @droidmonkey - Yes, it's such a big feature it's daunting, I wonder if we can some how trim the scope down very heavily and go for some kind of simplified "Entry Type" system with a fairly fixed set of well-known or conventional fields doing the heavy lifting... Of course, GUI work on top will be arduous for all of us :/

@J-Jamet
Copy link

J-Jamet commented Jun 21, 2022

I am really just not a fan of colored passwords at all. KeePassDX implemented it on Android and it looks terrible, immediately disabled.

Concerning the password colors, I also have only positive feedback, except yours. It allows users who manually copy elements to find their way around better. Of course, if you have suggestions to improve visibility by harmonizing colors between applications, I am open to proposals to update KeePassDX.

@mariuszdb
Copy link

I'd love to see this feature in KeePassXC. Is this being worked on/considered to be added soon? I took a look at the milestones for v2.7.2 and v2.8.0 and they don't contain this. After reading the conversation I get the impression that the work has been done and it just needs a review.

@wolframroesler
Copy link
Contributor Author

Hi @mariuszdb, you're right, the implementation is finished (cf. the screenshots above) and ready to be reviewed and merged.

@droidmonkey droidmonkey force-pushed the feature/colour-passwds branch from 246530f to 2abb634 Compare October 2, 2022 13:44
@droidmonkey
Copy link
Member

Made a few changes to the code and actually found a bug in our theme switching as well! This also fixes password overflow UI by showing a horizontal scroll bar on long passwords.

image

@droidmonkey droidmonkey added this to the v2.7.2 milestone Oct 2, 2022
@droidmonkey droidmonkey added the pr: backport pending Pull request yet to be backported to a previous release label Oct 2, 2022
@droidmonkey droidmonkey force-pushed the feature/colour-passwds branch 3 times, most recently from ecccd55 to ef49c7f Compare October 3, 2022 11:47
Closes keepassxreboot#4099

* Fixed bug in Application that did not set the dark theme flag when the theme was changed from dark to light.
@droidmonkey droidmonkey force-pushed the feature/colour-passwds branch from ef49c7f to 4faed7a Compare October 4, 2022 01:19
@droidmonkey droidmonkey merged commit b1e7c34 into keepassxreboot:develop Oct 4, 2022
@droidmonkey droidmonkey added pr: backported Pull request backported to previous release and removed pr: backport pending Pull request yet to be backported to a previous release labels Oct 5, 2022
@haarp
Copy link

haarp commented Jan 21, 2023

This is a great improvement that I just discovered, thanks for the merge! I would like to request different colors however. Green and blue can not always be well distinguished.

Take this screenshot for example. At 1:1 zoom I can not easily tell apart blue from green. Both sort-of look black-ish (move your head back from the screen a bit if it's still easy for you :) )

Screenshot_2023-01-21_23-41-40

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: backported Pull request backported to previous release user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Colored Passwords
8 participants