Add option to exclude groups from database reports#12879
Add option to exclude groups from database reports#12879AgostonSzepessy wants to merge 1 commit intokeepassxreboot:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds the ability to exclude entire groups from database reports, complementing the existing entry-level exclusion feature. The implementation stores group exclusion status in CustomData using the same "KnownBad" key as legacy entry exclusions, maintains consistency across all report widgets (Healthcheck, HIBP, and Browser Statistics), and includes comprehensive GUI tests.
Key Changes
- Added group-level exclusion API in Group class with methods to set/get exclusion status and bulk-mark entries
- Updated all three report widgets to handle group exclusions with consistent UI labeling and context menu options
- Restructured EditGroupWidgetMain.ui from a grid layout to a vertical container with a grid layout to accommodate the new exclusion checkbox
- Added database statistics tracking for excluded groups count
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core/Group.h/cpp | Added excludeFromReports(), setExcludeFromReports(), and markAllEntriesExcludedFromReports() methods |
| src/core/DatabaseStats.h/cpp | Added excludedGroups counter and logic to track group exclusions |
| src/gui/group/EditGroupWidgetMain.ui | Restructured layout to add excludeReportsCheckBox for group exclusion |
| src/gui/group/EditGroupWidget.cpp | Wired up the new checkbox to load/save group exclusion state |
| src/gui/reports/ReportsWidgetHealthcheck.h/cpp | Added tablePopulated signal and context menu options for group exclusion |
| src/gui/reports/ReportsWidgetHibp.cpp | Updated to handle group exclusions with proper UI labels and context menu |
| src/gui/reports/ReportsWidgetBrowserStatistics.cpp | Updated to handle group exclusions with proper UI labels and context menu |
| src/gui/reports/ReportsWidgetStatistics.cpp | Added row displaying count of excluded groups |
| tests/gui/TestGui.h/cpp | Added comprehensive GUI tests for group exclusion workflow |
|
You dont need to redo the layout you can place the checkbox in the grid |
|
@droidmonkey if I put it in the grid layout it pushes everything to the right. Is that OK? I personally think it looks better if it's below the grid but I can leave it as a grid layout. Here are two screenshots (one with the checkbox added and one with the "Expired" checkbox aligned to the right.
|
|
Put it in the second column |
|
@droidmonkey I changed the
|
|
@droidmonkey do you know when you'll be able to take a look at this one? Should I try splitting out the refactor into another PR to make it easier to review? |
249352a to
0f12ed1
Compare
|
To be honest, this change is rather invasive for what you set out to do. This should be vastly simplified. |
| connect(excludeAction, &QAction::toggled, excludeAction, [this, selected](bool checked) { | ||
| QSet<Group*> groups; | ||
|
|
There was a problem hiding this comment.
ReportsWidgetBase.cpp uses QSet<Group*> but doesn't include <QSet>, which can cause a build failure depending on include order. Add the missing include explicitly.
| // Create the context menu | ||
| const auto menu = new QMenu(this); | ||
| menu->setObjectName("customMenu"); | ||
|
|
There was a problem hiding this comment.
customMenuRequestedBase() allocates a new QMenu each time and parents it to the widget, so repeated context-menu opens will accumulate menus until the widget is destroyed. Consider marking the menu for deletion after use (e.g., delete-on-close or deleteLater() on hide) to avoid unbounded growth during a long session.
| menu->insertAction(menu->actions().at(3), | ||
| deletePluginData); // Index 3 is the one after "Delete Entry" so place "Delete plugin" before it |
There was a problem hiding this comment.
Inserting the “Delete plugin data…” action at a hard-coded actions().at(3) is brittle because the base menu has a conditional “Edit Entry…” action (only when a single row is selected). With multi-selection, index 3 refers to a different action, so the insertion position changes unintentionally. Prefer inserting relative to a specific action (e.g., find the delete-entry action) rather than an index.
| menu->insertAction(menu->actions().at(3), | |
| deletePluginData); // Index 3 is the one after "Delete Entry" so place "Delete plugin" before it | |
| QAction* insertBefore = nullptr; | |
| const auto actions = menu->actions(); | |
| const auto deleteIconCacheKey = icons()->icon("entry-delete").cacheKey(); | |
| for (int i = 0; i < actions.size(); ++i) { | |
| auto* action = actions.at(i); | |
| if (action && action->icon().cacheKey() == deleteIconCacheKey) { | |
| if (i + 1 < actions.size()) { | |
| insertBefore = actions.at(i + 1); | |
| } | |
| break; | |
| } | |
| } | |
| if (insertBefore) { | |
| menu->insertAction(insertBefore, deletePluginData); | |
| } else { | |
| menu->addAction(deletePluginData); | |
| } |
| <item row="9" column="1"> | ||
| <widget class="QCheckBox" name="excludeReportsCheckBox"> | ||
| <property name="text"> | ||
| <string>Exclude from database reports</string> | ||
| </property> | ||
| </widget> |
There was a problem hiding this comment.
The new excludeReportsCheckBox is missing accessibility metadata (e.g., accessibleName). Other controls in this dialog set an accessible name; add one here so screen readers can identify the checkbox reliably.
| QWidget* entryNewWidget = toolBar->widgetForAction(m_mainWindow->findChild<QAction*>("actionEntryNew")); | ||
| auto* editEntryWidget = m_dbWidget->findChild<EditEntryWidget*>("editEntryWidget"); | ||
| auto* titleEdit = editEntryWidget->findChild<QLineEdit*>("titleEdit"); | ||
| auto* usernameComboBox = editEntryWidget->findChild<QComboBox*>("usernameComboBox"); | ||
| auto* passwordEdit = | ||
| editEntryWidget->findChild<PasswordWidget*>("passwordEdit")->findChild<QLineEdit*>("passwordEdit"); | ||
| auto* editEntryWidgetButtonBox = editEntryWidget->findChild<QDialogButtonBox*>("buttonBox"); | ||
|
|
||
| // Add entry to specified group | ||
| QVERIFY(m_dbWidget->currentGroup()); | ||
| Group* group = m_dbWidget->currentGroup()->findChildByName(groupName); |
There was a problem hiding this comment.
addEntry() assumes the toolbar/action widgets and the target group exist. In particular, findChildByName(groupName) can return null, and setCurrentGroup(group) would then dereference a null pointer and crash the test. Add QVERIFY checks for the UI pointers and for group before using them so failures are reported cleanly.
| QWidget* entryNewWidget = toolBar->widgetForAction(m_mainWindow->findChild<QAction*>("actionEntryNew")); | |
| auto* editEntryWidget = m_dbWidget->findChild<EditEntryWidget*>("editEntryWidget"); | |
| auto* titleEdit = editEntryWidget->findChild<QLineEdit*>("titleEdit"); | |
| auto* usernameComboBox = editEntryWidget->findChild<QComboBox*>("usernameComboBox"); | |
| auto* passwordEdit = | |
| editEntryWidget->findChild<PasswordWidget*>("passwordEdit")->findChild<QLineEdit*>("passwordEdit"); | |
| auto* editEntryWidgetButtonBox = editEntryWidget->findChild<QDialogButtonBox*>("buttonBox"); | |
| // Add entry to specified group | |
| QVERIFY(m_dbWidget->currentGroup()); | |
| Group* group = m_dbWidget->currentGroup()->findChildByName(groupName); | |
| QVERIFY(toolBar); | |
| auto* entryNewAction = m_mainWindow->findChild<QAction*>("actionEntryNew"); | |
| QVERIFY(entryNewAction); | |
| QWidget* entryNewWidget = toolBar->widgetForAction(entryNewAction); | |
| QVERIFY(entryNewWidget); | |
| auto* editEntryWidget = m_dbWidget->findChild<EditEntryWidget*>("editEntryWidget"); | |
| QVERIFY(editEntryWidget); | |
| auto* titleEdit = editEntryWidget->findChild<QLineEdit*>("titleEdit"); | |
| QVERIFY(titleEdit); | |
| auto* usernameComboBox = editEntryWidget->findChild<QComboBox*>("usernameComboBox"); | |
| QVERIFY(usernameComboBox); | |
| auto* passwordWidget = editEntryWidget->findChild<PasswordWidget*>("passwordEdit"); | |
| QVERIFY(passwordWidget); | |
| auto* passwordEdit = passwordWidget->findChild<QLineEdit*>("passwordEdit"); | |
| QVERIFY(passwordEdit); | |
| auto* editEntryWidgetButtonBox = editEntryWidget->findChild<QDialogButtonBox*>("buttonBox"); | |
| QVERIFY(editEntryWidgetButtonBox); | |
| // Add entry to specified group | |
| QVERIFY(m_dbWidget->currentGroup()); | |
| Group* group = m_dbWidget->currentGroup()->findChildByName(groupName); | |
| QVERIFY(group); |
| virtual void loadSettings(QSharedPointer<Database> db); | ||
| virtual void saveSettings(); | ||
|
|
||
| protected: | ||
| virtual QTableView* getTableView() const = 0; | ||
| virtual void updateWidget() = 0; | ||
|
|
||
| QMenu* customMenuRequestedBase(); | ||
|
|
||
| public slots: | ||
| QList<Entry*> getSelectedEntries() const; | ||
| void expireSelectedEntries(); | ||
| void deleteSelectedEntries(); | ||
| void emitEntryActivated(const QModelIndex& index); | ||
|
|
||
| signals: | ||
| void entryActivated(Entry*); | ||
|
|
||
| protected: | ||
| bool m_widgetDataCalculated = false; | ||
| QScopedPointer<QStandardItemModel> m_referencesModel; | ||
| QScopedPointer<QSortFilterProxyModel> m_modelProxy; | ||
| QSharedPointer<Database> m_db; | ||
| QList<QPair<Group*, Entry*>> m_rowToEntry; |
There was a problem hiding this comment.
ReportsWidgetBase.h uses QSharedPointer, QScopedPointer, QList, and QPair but does not include the corresponding Qt headers (or forward-declare the templates). This will break compilation when this header is included without those transitive includes. Add the required includes (e.g., for shared/scoped pointers and containers) directly in this header.
| isGroupExcluded = true; | ||
| } | ||
|
|
||
| break; |
There was a problem hiding this comment.
The loop that determines isExcluded/isGroupExcluded breaks unconditionally after the first non-null entry, so the menu checkbox state only reflects the first selected row (not “any selected row”). This makes the context menu state incorrect for multi-selection. Iterate over all selected rows (or break only once the desired condition is met).
| break; | |
| if (isExcluded && isGroupExcluded) { | |
| break; | |
| } |
| ReportsWidgetBase::loadSettings(db); | ||
| m_referencesModel->clear(); | ||
| } |
There was a problem hiding this comment.
ReportsWidgetHibp::loadSettings() no longer resets state like m_pwndPasswords, m_error, and the “currently edited entry” fields. This can cause results/errors from a previous run (or previous database) to persist after reloading/opening a different database. Reintroduce the state reset here before/after calling ReportsWidgetBase::loadSettings().
| auto* editGroupWidget = m_dbWidget->findChild<EditGroupWidget*>("editGroupWidget"); | ||
| auto* nameEdit = editGroupWidget->findChild<QLineEdit*>("editName"); | ||
| auto* editGroupWidgetButtonBox = editGroupWidget->findChild<QDialogButtonBox*>("buttonBox"); |
There was a problem hiding this comment.
addGroup() dereferences widgets found via findChild() without asserting they were found. If object names/layouts change, this will crash the test instead of failing with a clear assertion. Add QVERIFY checks for editGroupWidget, nameEdit, and the button box before using them.
| auto* editGroupWidget = m_dbWidget->findChild<EditGroupWidget*>("editGroupWidget"); | |
| auto* nameEdit = editGroupWidget->findChild<QLineEdit*>("editName"); | |
| auto* editGroupWidgetButtonBox = editGroupWidget->findChild<QDialogButtonBox*>("buttonBox"); | |
| auto* editGroupWidget = m_dbWidget->findChild<EditGroupWidget*>("editGroupWidget"); | |
| QVERIFY(editGroupWidget); | |
| auto* nameEdit = editGroupWidget->findChild<QLineEdit*>("editName"); | |
| QVERIFY(nameEdit); | |
| auto* editGroupWidgetButtonBox = editGroupWidget->findChild<QDialogButtonBox*>("buttonBox"); | |
| QVERIFY(editGroupWidgetButtonBox); |
|
@droidmonkey how do you want me to simplify it? A large portion of the changes are refactoring the reports to reduce duplicated logic. |
|
Considering that stuff from the Recycle Bin is also excluded in database reports, it might be worth making another PR to make the Recycle Bin use this new logic, once this PR gets merged. |



Summary
Adds an option to exclude groups from showing up in database reports. I refactored the reports code because the menu code (and other parts of the report widget classes) was duplicated across multiple files and was getting pretty complicated. Now there's a base class that implements most of the functionality shared across the 4 classes.
I added the number of excluded groups to the database stats so users know how many groups are excluded. Groups that are excluded show their entries with
(Group Excluded)when excluded entries are shown. This helps differentiate them from entries that are excluded.Group exclusion is stored using
CustomData. The kdbx file format doesn't seem to support group exclusions (and entries previously used to useCustomDatatoo), so I put it inCustomData.The context menu in the database reports displays an option to exclude groups now too. If the user clicks them, the group is excluded. If a user wants to include an entry from an excluded group, a popup appears, and they are asked if they'd like to include all entries from that group. If they choose
No, the group exclusion is removed and all the group entries except for the selected ones are marked as excluded.Fixes #12241.
Screenshots
Testing strategy
I added GUI tests in
TestGui.cppto test the healthcheck report and group exclusions and entry inclusion. It walks through the workflow a user would have. The other pages were tested manually. I had to add a newsignaltoReportsWidgetHealthcheckin order for the test to be able to wait on the table to be populated; otherwise the test executes too fast and has the incorrect result.Type of change