Improve and extend entry view table #1305

fonic · 2017-12-19T09:52:17Z

Description

This improves and extends the entry view table,

Adding additional colums Password, Notes, Expires, Created, Modified, Accessed and Attachments (addresses Add sortable last-modified column to entry list view #129)
Adding settings to display usernames/passwords hidden/visible
Adding a context menu to customize view (toggle column visibility, resize and reset columns) (addresses Enable configurable columns in the entries grid #17, Add sortable last-modified column to entry list view #129, A way to edit list of attributes in the upper part #397)
Adding a 'copy-on-doubleclick' feature for certain columns (addresses Copy column field value on double-click #360)
Syncing view states when switching between multiple tabs
Reading/writing last used view settings from/to config

bringing the entry view table in par with KeePassX 0.4.x and even beyond.

NOTE:

KeePassXC's user configuration should be reset prior to testing this
This is replaces my original pull request Improve entry table display/functionality in GUI #849, providing a clean rewrite with clean commits.

Motivation

In my opinion, this addresses a major disadvantage of KeePassXC as the time of writing. The additional columns, the ability to customize the views and the 'copy-on-doubleclick' feature improve KeePassXC's overall usability and user experience.

DONE

Add additional columns to entry view table
Add code for correct sorting of entries for any given column
Add settings to display usernames/passwords hidden/visible
Add context menu to entry view table header for view customization
Add 'copy-on-doubleclick' feature for columns where applicable
Add code to sync view states when switching between multiple tabs
Add code to make view settings persistent using configuration file
Finalize and clean up code

Possible follow-ups

Double-clicking columns 'Attachments' and 'Notes' should open corresponding tab in details view/pane
Make header context menu available as 'View' in menu bar of main window
Optionally add header context menu as submenu 'View' to already existing entry list context menu
Copy-on-doubleclick should give feedback, e.g. progress bar that displays how long the copied element will stay in the clipboard (requested by @droidmonkey)
Update/modify search mode to work without column 'Group' to simplify state syncing (see Redesign database search #1317)
Add entropy column displaying password quality (see this comment)

How has this been tested?

locally

Screenshots

Types of changes

✅ New feature (non-breaking change which adds functionality)

Checklist (in progress)

✅ I have read the CONTRIBUTING document. [REQUIRED]
✅ My code follows the code style of this project. [REQUIRED]
✅ All new and existing tests passed. [REQUIRED]
✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
❎ My change requires a change to the documentation and I have updated it accordingly.
❎ I have added tests to cover my changes.

fonic · 2017-12-23T07:54:33Z

So, @droidmonkey, @TheZ3ro, @louib, @cjsheets, @frostasm, this is it, done for now. Just in time to serve as a nice present ;)

Please test & discuss the issue mentioned below.

Merry Christmas to everyone!

fonic · 2017-12-23T08:33:13Z

A note on state syncing (short answer at the bottom):

I spent hours trying to find ways for proper state syncing. Now I'm confident to say that, with the current/original implementation, it is not possible and never will be to achieve a clean solution. The problem is the search mode, which, by showing/hiding column 'Group', effectively taints the view state.

Thus, I'd like to suggest changing the search mode to work without this column. This would greatly reduce code complexity - I'd say we're talking about 250-500 lines of code. One solution I can think of is implementing an E-Mail-client-like search, where the number of search results for each group is displayed in the group tree view.

Here's a quick mock-up:

Please also note that I currently crafted the state syncing to be as close to the original behavior as possible. But, as it turns out, the original state syncing never worked as it was designed due to a bug in class DatabaseWidgetStateSync:

void DatabaseWidgetStateSync::updateColumnSizes()
{
    if (m_blockUpdates) {
        return;
    }

    if (m_activeDbWidget->isGroupSelected()) {
        m_columnSizesList = m_activeDbWidget->entryHeaderViewSizes();
    }
    else {
        m_columnSizesSearch = m_activeDbWidget->entryHeaderViewSizes();
    }
}

doesn't make sense, it should be

void DatabaseWidgetStateSync::updateColumnSizes()
{
    if (m_blockUpdates) {
        return;
    }

    if (m_activeDbWidget->isInSearchMode()) {
        m_columnSizesSearch = m_activeDbWidget->entryHeaderViewSizes();
    }
    else {
        m_columnSizesList = m_activeDbWidget->entryHeaderViewSizes();
    }
}

It seems to me that this is either a remnant from previous implementations or a copy-and-paste error. This bug is not inherent to the develop branch, it is also present in release 2.2.4.

To cut a long story short: In my implementation, this bug is fixed, which results in a somewhat different behavior. But, as mentioned before, this is the behavior that was originally intended, and the original intent was: the first time the search is used, its view is initialized using the current list view. After that, both views are kept separate for all times (i.e. until config file is reset).

droidmonkey · 2017-12-23T13:15:35Z

I recommend an entirely different approach. Search results should appear in their own entry view with fixed columns like it does currently. The original entry view would be hidden or sorted below the stack. When search is over, the main entry view is shown again as usual. This would eliminate the need for the state sync, although you'll need to replumb some signals and slots depending on which view is visible. This widget's display abd behavior could be encapsulated into a simple "SearchResultsWidget" owned by the database widget.

My ultimate goal is to move the search display above all tabs. We should be searching across all open databases. The current implementation is a stop gap after moving the search bar to the toolbar.

I would accept your original proposal, but without adding the count to group names.

fonic · 2017-12-23T14:04:05Z

I recommend an entirely different approach. Search results should appear in their own entry view with fixed columns like it does currently. The original entry view would be hidden or sorted below the stack. When search is over, the main entry view is shown again as usual. This would eliminate the need for the state sync, although you'll need to replumb some signals and slots depending on which view is visible. This widget's display abd behavior could be encapsulated into a simple "SearchResultsWidget" owned by the database widget.

My ultimate goal is to move the search display above all tabs. We should be searching across all open databases. The current implementation is a stop gap after moving the search bar to the toolbar.

Personally, I think the current search is quite good, except for the fact that it is always global and clicking on groups when in search mode does nothing. I'd also prefer searching in only the current database. Just my opinion of course, other users might feel different. In the end, I think something like that should be configurable as you'll probably never find a solution that fits all use cases.

By the way: syncing would still be required even with your solution, as it syncs list/search mode + tabs, at least in the current implementation.

I would accept your original proposal, but without adding the count to group names.

Without that and with the group column gone, you wouldn't know where exactly the listed entries are in your database (and I think since you are searching you want to know). That's why I came up with the count idea in the first place.

droidmonkey · 2017-12-23T15:31:02Z

The count still doesn't tell you and adds complexity

fonic · 2017-12-23T17:55:10Z

The count still doesn't tell you and adds complexity

I agree, but it gives you a valuable hint, and that might do the trick in most situations. As mentioned before, it all comes down to use cases.

Therefore, I'd like to suggest opening an issue for this where we try to collect/construct all imaginable use cases, i.e. what would one search for and what would one expect of the search to help you reach your objective. Once we got the use cases, we can come up with ideas for implementations that would cover those.

Whatever the outcome may be, this is outside of the scope of this pull request, so it would be great if you could test and review in the meantime.

fonic · 2017-12-24T08:10:10Z

Ok, I'll create an issue for that.

Personally, I think the current search is quite good, except for the fact that it is always global and clicking on groups when in search mode does nothing.

Just found out that this feature is actually already implemented. If one adds this to the config

[General]
SearchLimitGroup=true

the behavior is exactly as described. It seems there's currently no way to toggle this within the GUI, or am I missing something?

phoerious · 2017-12-24T08:33:41Z

There is. Click on the little arrow next to the search icon.

fonic · 2017-12-24T08:58:05Z

Hm, just wanted to say 'what arrow?', but it seems that it just isn't visible on my system, probably related to the dark KDE plasma theme (is the arrow dark grey?) -> next issue to create ;)

phoerious · 2017-12-24T11:26:56Z

I don't quite remember if there actually is one or if we only talked about having one there. If there is one, it's not only invisible to you. ;-)

About this whole search issue: my dream search function would work like this: you type in a search term and while you type, groups are being reduced to groups with matching entries only (and their parents of course). The entry table view doesn't modify its columns, but shows all search results in the selected group and its children (you can change the scope, by clicking on a group in the left-hand pane).
Inside the table view, groups are visually separated by column-spanning rows containing the group name.

I quickly photoshopped your mockup to visualize my ideas:

I think this customized table view could even be an optional standard view for groups (instead of only showing direct descendants for each group).

To tackle the problem of global search: I think that one should be optional and if we do that, a similar view should open in a new tab with a merged results view. We need to make sure to update it as soon as one of the databases is closed, though.

fonic · 2017-12-24T12:04:51Z

I don't quite remember if there actually is one or if we only talked about having one there. If there is one, it's not only invisible to you. ;-)

Thanks for clarifying!

About this whole search issue:
[...]
I quickly photoshopped your mockup to visualize my ideas:
[...]

Wow, this is great! Now that would/could be something I really like, I bet other users as well!

I created issue #1317 to further discuss this as suggested above.

fonic · 2017-12-24T12:08:33Z

@droidmonkey, @phoerious: could you repost your ideas in issue #1317?

I'll repost my ideas there, too.

phoerious · 2017-12-24T13:58:34Z

Yeah, sure.

TheZ3ro · 2017-12-24T19:13:52Z

@fonic @phoerious

There is. Click on the little arrow next to the search icon.

It's not an arrow, it's directly the search icon (the magnifying glass)

That option anyway doesn't implement multi-database search, the goal @droidmonkey is trying to achieve.

phoerious · 2017-12-25T00:35:58Z

That option anyway doesn't implement multi-database search, the goal @droidmonkey is trying to achieve.

Read my last paragraph. I think that should be optional and can be done in the same way in a separate tab.

TheChiefMeat · 2017-12-29T02:47:39Z

Can I suggest a entropy column so you can see password quality/length at a glance?

fonic · 2017-12-29T09:39:05Z

@TheChiefMeat
Can I suggest a entropy column so you can see password quality/length at a glance?

I added this to the list of possible follow-ups in the description of this PR.

fonic · 2018-01-11T06:48:38Z

Closed this by accident, reopening.

Is anyone reviewing? Would be great, this was lots of work and I'd like to finish it while I still remember everything.

phoerious · 2018-01-12T10:04:04Z

I'll see what I can do in the evening and over the weekend.

fonic · 2018-01-12T10:07:08Z

@phoerious: no rush. It's lots of code. As I said, would just be great to finish this while the memory is still fresh.

TheZ3ro · 2018-01-17T16:14:44Z

@fonic can you rebase on top of develop branch? Anyway, very good work 🎉

@phoerious @droidmonkey please review your requested changes

phoerious

Awesome, thanks!

fonic · 2018-01-17T16:32:49Z

One little thing slipped through, changing to bullets in DetailsWidget.cpp. Done.

fonic · 2018-01-17T16:37:18Z

Rebased.

droidmonkey

Great work!

fonic · 2018-01-19T08:05:00Z

There are two things I'd like to discuss before this gets merged:

EntryModel.cpp

// Paperclip symbol
const QString EntryModel::PaperClipDisplay(QString("\U0001f4ce"));

seems to produce memory leaks:

=================================================================
==11525==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 6656 byte(s) in 13 object(s) allocated from:
    #0 0x7f583a45ead0 in realloc (/usr/lib/gcc/x86_64-pc-linux-gnu/6.4.0/libasan.so.3+0xc7ad0)
    #1 0x7f582bc12e4a  (/usr/lib64/libfontconfig.so.1+0x1de4a)

Indirect leak of 9408 byte(s) in 294 object(s) allocated from:
    #0 0x7f583a45e8e0 in calloc (/usr/lib/gcc/x86_64-pc-linux-gnu/6.4.0/libasan.so.3+0xc78e0)
    #1 0x7f582bc12ad7  (/usr/lib64/libfontconfig.so.1+0x1dad7)

Indirect leak of 6112 byte(s) in 191 object(s) allocated from:
    #0 0x7f583a45e8e0 in calloc (/usr/lib/gcc/x86_64-pc-linux-gnu/6.4.0/libasan.so.3+0xc78e0)
    #1 0x7f582bc12a7c  (/usr/lib64/libfontconfig.so.1+0x1da7c)

Indirect leak of 3258 byte(s) in 329 object(s) allocated from:
    #0 0x7f583a45e720 in malloc (/usr/lib/gcc/x86_64-pc-linux-gnu/6.4.0/libasan.so.3+0xc7720)
    #1 0x7f5838236399 in __strdup (/lib64/libc.so.6+0x89399)

Indirect leak of 624 byte(s) in 13 object(s) allocated from:
    #0 0x7f583a45e720 in malloc (/usr/lib/gcc/x86_64-pc-linux-gnu/6.4.0/libasan.so.3+0xc7720)
    #1 0x7f582bc0d15d in FcLangSetCreate (/usr/lib64/libfontconfig.so.1+0x1815d)

Indirect leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x7f583a45e8e0 in calloc (/usr/lib/gcc/x86_64-pc-linux-gnu/6.4.0/libasan.so.3+0xc78e0)
    #1 0x7f582bc13608  (/usr/lib64/libfontconfig.so.1+0x1e608)

SUMMARY: AddressSanitizer: 26090 byte(s) leaked in 841 allocation(s).

If changed to a simple string like 'x', they disappear. If fail to understand what this is about. Could simply be a bug in libfontconfig, though.

EntryModel.cpp

// String being displayed when hiding content
const QString EntryModel::HiddenContentDisplay(QString(QChar(0x25CF)).repeated(6));

Should we move this to some global header file, so that it can be used throughout the application, e.g. for the details pane?

TheZ3ro · 2018-01-19T12:39:43Z

@fonic I would place both the paperclip and the bullet in a global file, for example in core/Entry.cpp or something like that.

Yes, seems a memory leak in libfontconfig AFAICS

phoerious · 2018-01-19T17:45:36Z

Memory leaks in libfontconfig are very common. KeePassXC throws tons of them at exit. Nothing we can do about that.

phoerious · 2018-01-20T00:15:37Z

Could you rebase your branch please, so I can merge?

Add additional columns 'Password', 'Notes', 'Expires', 'Created', 'Modified', 'Accessed' and 'Attachments' to entry view table: - add columns themselves - add display role data providers - introduce/apply sort role - add sort role data providers - add settings to display usernames/passwords visible/hidden - minor addition to EntryModel::columnCount() as advised by Qt documentation

Update comparison values of modelProxy->columnCount() to account for additional columns 'Password', 'Notes', 'Expires', 'Created', 'Modified', 'Accessed' and 'Attachments'

Add header context menu to entry view table (accessible using right click on header), providing: - Actions to toggle 'Hide Usernames' / 'Hide Passwords' - Actions to toggle column visibility - Actions to resize columns - Action to reset view to defaults

Update clicks within entry view referencing column indices to account for changed column indices due to new way of showing/hiding column Entry Model::ParentGroup. This column now has fixed index 0 wether it's shown or hidden, thus all indices need to be shifted by +1 when not in search mode

Add 'copy-on-doubleclick' feature to entry view table by extending already existing DatabaseWidget::entryActivationSignalReceived(). Currently, username, password and notes are copyied on doubleclick, while doubleclicking URL still opens browser as before. Can easily be extended to account for other/additional columns (switch-case).

Update state syncer (class DatabaseWidgetStateSync) to account for new features: - properly sync view state when switching tabs - properly read/write view state from/to config Update classes EntryModel and EntryView to consistenly name list/search modes. Before, both classes defined list mode as 'group mode' and search mode as 'entry list mode', which differed from naming in other classes such as DatabaseWidget.

Add additional column 'Paperclip' to entry view table: - add column itself - add display role data provider - add sort role data provider - update total column count

Update comparison values of modelProxy->columnCount() to account for additional column 'Paperclip'

Comprehensive code cleanup: - formatting - comments - obsolete code

fonic · 2018-01-21T14:20:19Z

@TheZ3ro, @phoerious regarding libfontconfig memory leak:
Alright, thought so. I removed the corresponding TODO from EntryModel.cpp.

@TheZ3ro:
core/Entry.cpp is not GUI related, should I really put it there? Well, we could also leave it as is, the paperclip symbol is only used in EntryModel.cpp, the hidden content string only in two places.

@phoerious:
Rebased! Third time's a charm :)

phoerious · 2018-01-21T14:34:43Z

Amazing, thanks. I think leaving the paper clip in EntryModel.cpp is fine.

fonic · 2018-01-21T14:41:56Z

Ok, good. Looking forward to creating further contributions ;)

fonic · 2018-01-21T17:41:38Z

@phoerious: you should be able to close several issues listed in the PR's description now that this got merged

jab416171 · 2018-01-21T23:17:05Z

This is fantastic, thank you so much. How soon can we expect this to make it into a release?

phoerious · 2018-01-22T07:19:56Z

It will be in 2.3, which is the next feature release. Not too long.

frostasm · 2018-01-22T20:35:31Z

great work @fonic 👍

- 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]

fonic changed the title ~~Add additional columns to entry view table~~ Improve entry view table Dec 19, 2017

fonic mentioned this pull request Dec 19, 2017

Improve entry table display/functionality in GUI #849

Closed

fonic changed the title ~~Improve entry view table~~ Improve and extend entry view table Dec 19, 2017

fonic mentioned this pull request Dec 24, 2017

Redesign database search #1317

Closed

njfox mentioned this pull request Jan 5, 2018

[Feature Request] CSV import with created and modifed dates #1208

Closed

weslly mentioned this pull request Jan 10, 2018

new feature: copy password and user name button for each entry #1369

Closed

fonic closed this Jan 11, 2018

fonic reopened this Jan 11, 2018

fonic mentioned this pull request Jan 12, 2018

Improve password generator, add option to minimize recurrences #1337

Closed

phoerious added this to the v2.3.0 milestone Jan 13, 2018

phoerious approved these changes Jan 17, 2018

View reviewed changes

droidmonkey approved these changes Jan 19, 2018

View reviewed changes

fonic added 9 commits January 21, 2018 15:17

Update entry model test to account for additional columns

268035f

Update comparison values of modelProxy->columnCount() to account for additional columns 'Password', 'Notes', 'Expires', 'Created', 'Modified', 'Accessed' and 'Attachments'

Add column 'Paperclip' to entry view table

161d0ea

Add additional column 'Paperclip' to entry view table: - add column itself - add display role data provider - add sort role data provider - update total column count

Update entry model test to account for additional column 'Paperclip'

9ff6481

Update comparison values of modelProxy->columnCount() to account for additional column 'Paperclip'

Code cleanup

8c78aca

Comprehensive code cleanup: - formatting - comments - obsolete code

phoerious merged commit 0a876c8 into keepassxreboot:develop Jan 21, 2018

hllmtl mentioned this pull request Mar 1, 2018

Right click context menu doesn't show keyboard shortcuts anymore #1588

Closed

phoerious added pr: new feature Pull request that adds a new feature and removed new feature labels Nov 22, 2024

Improve and extend entry view table #1305

Improve and extend entry view table #1305

Conversation

fonic commented Dec 19, 2017 • edited Loading

Description

Motivation

DONE

Possible follow-ups

How has this been tested?

Screenshots

Types of changes

Checklist (in progress)

fonic commented Dec 23, 2017 • edited Loading

fonic commented Dec 23, 2017 • edited Loading

droidmonkey commented Dec 23, 2017 • edited Loading

fonic commented Dec 23, 2017 • edited Loading

droidmonkey commented Dec 23, 2017 • edited Loading

fonic commented Dec 23, 2017 • edited Loading

fonic commented Dec 24, 2017 • edited Loading

phoerious commented Dec 24, 2017

fonic commented Dec 24, 2017 • edited Loading

phoerious commented Dec 24, 2017

fonic commented Dec 24, 2017 • edited Loading

fonic commented Dec 24, 2017 • edited Loading

phoerious commented Dec 24, 2017

TheZ3ro commented Dec 24, 2017 • edited Loading

phoerious commented Dec 25, 2017 • edited Loading

TheChiefMeat commented Dec 29, 2017

fonic commented Dec 29, 2017

fonic commented Jan 11, 2018 • edited Loading

phoerious commented Jan 12, 2018

fonic commented Jan 12, 2018

TheZ3ro commented Jan 17, 2018 • edited Loading

phoerious left a comment

Choose a reason for hiding this comment

fonic commented Jan 17, 2018

fonic commented Jan 17, 2018

droidmonkey left a comment

Choose a reason for hiding this comment

fonic commented Jan 19, 2018 • edited Loading

TheZ3ro commented Jan 19, 2018

phoerious commented Jan 19, 2018

phoerious commented Jan 20, 2018

fonic commented Jan 21, 2018 • edited Loading

phoerious commented Jan 21, 2018

fonic commented Jan 21, 2018 • edited Loading

fonic commented Jan 21, 2018

jab416171 commented Jan 21, 2018

phoerious commented Jan 22, 2018

frostasm commented Jan 22, 2018

fonic commented Dec 19, 2017 •

edited

Loading

fonic commented Dec 23, 2017 •

edited

Loading

fonic commented Dec 23, 2017 •

edited

Loading

droidmonkey commented Dec 23, 2017 •

edited

Loading

fonic commented Dec 23, 2017 •

edited

Loading

droidmonkey commented Dec 23, 2017 •

edited

Loading

fonic commented Dec 23, 2017 •

edited

Loading

fonic commented Dec 24, 2017 •

edited

Loading

fonic commented Dec 24, 2017 •

edited

Loading

fonic commented Dec 24, 2017 •

edited

Loading

fonic commented Dec 24, 2017 •

edited

Loading

TheZ3ro commented Dec 24, 2017 •

edited

Loading

phoerious commented Dec 25, 2017 •

edited

Loading

fonic commented Jan 11, 2018 •

edited

Loading

TheZ3ro commented Jan 17, 2018 •

edited

Loading

fonic commented Jan 19, 2018 •

edited

Loading

fonic commented Jan 21, 2018 •

edited

Loading

fonic commented Jan 21, 2018 •

edited

Loading