-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 preview panel for entries and groups, closes #454 and #558 #879
Conversation
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.
I don't like the password being in an input field. Makes the whole layout rather inconsistent and visually overwhelming / confusing.
Also you may want to adjust the spacing a little. The second and third column are way closer to each other than the first and second and the third and fourth. The "Notes" label should also be consistent with the other labels (i.e., bold and top-aligned). |
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.
When navigating between entries with the up/down arrow keys the preview pane does not update.
@phoerious the spacing is due to the PasswordEdit and the visibility button, there isn't much I can do. Using a label for the password will bring the same problem as the URL since most of my password are >40 char and such length will broke the layout on small window size. @droidmonkey like I said in the first message, enabling that will bring some problems with guitest that I can't reproduce manually |
Show the password as three asterisks or bullets, don't use the password edit. Add an extra button to reveal it. Maybe add some sort of context pop-up on hover for all entries to show a non-elided full-length version. |
6e9f0ab
to
51bf314
Compare
I've fixed the navigating issue, the notes label and the password field. Should be good now 😉 |
I'll take a look tomorrow but there is still considerable whitespace issues (as of the last screenshot) and questionable need to display creation/access times. |
Looks great! Found an issue though: The new hi-res icons get cut-off and enlarge the view. Also, do we want to repeat the root group's folder in the path? It's technically not a subfolder and could be confusing. The password field is also unmasked. Suggestion: It might be nice to add an auto-type tab that lists the currently assigned auto-type window/sequence combos for the entry. |
Woops, I didn't tested with hi-res icon
You mean removing the first
There should be an option in Settings > Security >
Nice, I will work on this |
I meant removing the "NewDatabase" label |
Oh ok got it. I will work on this in the next few days |
88216d9
to
5d6f399
Compare
12f166b
to
603aeb4
Compare
b37bde9
to
c0f9d54
Compare
Rebased on develop, ready for review |
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.
Found a few more issues that need fixing.
src/gui/DetailsWidget.cpp
Outdated
for (const QString& key : customAttributes) { | ||
QString value = m_currentEntry->attributes()->value(key); | ||
if (m_currentEntry->attributes()->isProtected(key)) { | ||
value = "<i>[PROTECTED]</i>"; |
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.
Not translated. Re-use locale string from edit entry screen (without the i
tags).
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.
In the edit entry it's tr("[PROTECTED] Press reveal to view or edit")
I will separate the 2 string so we can re-use [PROTECTED]
src/gui/DetailsWidget.cpp
Outdated
|
||
m_ui->autotypeTree->clear(); | ||
AutoTypeAssociations* autotypeAssociations = m_currentEntry->autoTypeAssociations(); | ||
QList<QTreeWidgetItem *> items; |
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.
Remove space before *.
src/gui/DetailsWidget.cpp
Outdated
QString newurl = ""; | ||
if (url.length() > 60) { | ||
newurl.append(url.left(20)); | ||
newurl.append("..."); |
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.
This should be an ellipsis character (U+2026).
src/gui/DetailsWidget.cpp
Outdated
QString newpassword = ""; | ||
if (password.length() > 60) { | ||
newpassword.append(password.left(50)); | ||
newpassword.append("..."); |
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.
Same here.
src/gui/entry/EditEntryWidget.cpp
Outdated
@@ -632,7 +632,7 @@ void EditEntryWidget::displayAttribute(QModelIndex index, bool showProtected) | |||
if (index.isValid()) { | |||
QString key = m_attributesModel->keyByIndex(index); | |||
if (showProtected) { | |||
m_advancedUi->attributesEdit->setPlainText(tr("[PROTECTED] Press reveal to view or edit")); | |||
m_advancedUi->attributesEdit->setPlainText(tr("[PROTECTED]")+" "+tr("Press reveal to view or edit")); |
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 add spaces between operators or use .append().
e2ce318
to
e3e3053
Compare
bedf5b2
to
275bd38
Compare
What is missing before merge? |
Let's merge this, we can make tweaks later as we use it more. |
04f4624
to
f3d85ae
Compare
I know this is closed, but I've got some feedback on the layout of this. In the old keepass2, I really liked and used a lot the notes fields for storing bits and pieces. Being able to view the notes in the preview panel with 1 click is a really nice feature and something that stops me moving from the old keepass2 to keepassxc. With this implementation, the notes field is hidden by default. Would it not be more usable to display everything in one, like the old keepass2? |
Notes are visible in a tab, you can select that tab and it will persist between entry selections. |
- Add support for KDBX 4.0, Argon2 and ChaCha20 [#148, #1179, #1230, #1494] - Add SSH Agent feature [#1098, #1450, #1463] - Add preview panel with details of the selected entry [#879, #1338] - Add more and configurable columns to entry table and allow copying of values by double click [#1305] - Add KeePassXC-Browser API as a replacement for KeePassHTTP [#608] - Deprecate KeePassHTTP [#1392] - Add support for Steam one-time passwords [#1206] - Add support for multiple Auto-Type sequences for a single entry [#1390] - Adjust YubiKey HMAC-SHA1 challenge-response key generation for KDBX 4.0 [#1060] - Replace qHttp with cURL for website icon downloads [#1460] - Remove lock file [#1231] - Add option to create backup file before saving [#1385] - Ask to save a generated password before closing the entry password generator [#1499] - Resolve placeholders recursively [#1078] - Add Auto-Type button to the toolbar [#1056] - Improve window focus handling for Auto-Type dialogs [#1204, #1490] - Auto-Type dialog and password generator can now be exited with ESC [#1252, #1412] - Add optional dark tray icon [#1154] - Add new "Unsafe saving" option to work around saving problems with file sync services [#1385] - Add IBus support to AppImage and additional image formats to Windows builds [#1534, #1537] - Add diceware password generator to CLI [#1406] - Add --key-file option to CLI [#816, #824] - Add DBus interface for opening and closing KeePassXC databases [#283] - Add KDBX compression options to database settings [#1419] - Discourage use of old fixed-length key files in favor of arbitrary files [#1326, #1327] - Correct reference resolution in entry fields [#1486] - Fix window state and recent databases not being remembered on exit [#1453] - Correct history item generation when configuring TOTP for an entry [#1446] - Correct multiple TOTP bugs [#1414] - Automatic saving after every change is now a default [#279] - Allow creation of new entries during search [#1398] - Correct menu issues on macOS [#1335] - Allow compilation on OpenBSD [#1328] - Improve entry attachments view [#1139, #1298] - Fix auto lock for Gnome and Xfce [#910, #1249] - Don't remember key files in file dialogs when the setting is disabled [#1188] - Improve database merging and conflict resolution [#807, #1165] - Fix macOS pasteboard issues [#1202] - Improve startup times on some platforms [#1205] - Hide the notes field by default [#1124] - Toggle main window by clicking tray icon with the middle mouse button [#992] - Fix custom icons not copied over when databases are merged [#1008] - Allow use of DEL key to delete entries [#914] - Correct intermittent crash due to stale history items [#1527] - Sanitize newline characters in title, username and URL fields [#1502] - Reopen previously opened databases in correct order [#774] - Use system's zxcvbn library if available [#701] - Implement various i18n improvements [#690, #875, #1436]
Description
This PR add a preview panel for entires and groups and add/fix some minor things:
[PROTECTED]
Some info on the preview panel:
The preview panel can be disabled (and hidden) in the settings, it can also be hidden using the spacer above it.
It can be temporary hidden using the X icon, the panel will be visible again when a new Entry/Group is clicked.
You must click on an Entry/Group to display its info on the panel, this is due to a bug when using both click and change signals (currently the change signals are commented, line 196-198
src/gui/DatabaseWidget.cpp
)You can decide to disable/hide passwords in the preview panel from the settings, in this case the visualized password will be 4 dot.
Motivation and context
Closes #454 and #558
How has this been tested?
Manual gui testing
Screenshots (if appropriate):
Screenshots here #454 (comment)
Types of changes
Checklist:
-DWITH_ASAN=ON
. [REQUIRED]