Add context menu entry to clean the Recycle Bin in databases by vsvyatski · Pull Request #520 · keepassxreboot/keepassxc

vsvyatski · 2017-04-21T13:18:33Z

Description

This implements the feature request (issue) #503, created by myself. If it's possible, could you include this functionality into the incoming version 2.2.0?

Motivation and context

In some way I missed the "Empty recycle bin" menu item from the original KeePass, so I felt up to implementing this functionality for KeePassXC. This pull request basically gets the job done for #503.

How has this been tested?

It was tested by running your unit tests (testcsvimport failed again) and manually by playing with the UI I had introduced. The stuff seems to be functioning properly. My testing environment is Windows 10 Creators Update.

Screenshots (if appropriate):

I have slightly modified the "Groups" so that it's more similar to the original KeePass. Below is the screenshot of KeePass 2.35

Here is the context menu of KeePassXC in my implementation (don't judge me too hard if it looks inappropriate to you)

The same stuff in the main menu

If the recycle bin is not selected, the "Empty recycle bin" is not shown, see below

Types of changes

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

Checklist:

✅ I have read the CONTRIBUTING document. [REQUIRED]
✅ My code follows the code style of this project. [REQUIRED]
✅ Almost all new and existing tests passed (except for # 18). [REQUIRED]
✅ No -DWITH_ASAN=ON, because I'm on Windows. [REQUIRED]
✅ User manual on this program should be updated (if it exists, of course) to describe this new feature.

This implements the feature request (issue) #503.

louib · 2017-04-21T14:09:53Z

src/gui/DatabaseWidget.cpp

+    return m_groupView->currentGroup() && m_groupView->currentGroup() == m_db->metadata()->recycleBin();
+}
+
+void DatabaseWidget::emptyTrash()


@vsvyatski for consistency we might want to name that emptyRecycleBin

louib · 2017-04-21T14:11:54Z

src/gui/DatabaseWidget.cpp

+        return;
+    }
+
+    if (currentGroup == m_db->metadata()->recycleBin()) {


@vsvyatski why not use isRecycleBinSelected?

louib · 2017-04-21T14:12:51Z

src/gui/DatabaseWidget.cpp

+    if (currentGroup == m_db->metadata()->recycleBin()) {
+        QMessageBox::StandardButton result = MessageBox::question(
+            this, tr("Empty recycle bin?"),
+            tr("Are you sure you want to permanently delete everytning from your recycle bin?"),


@vsvyatski everything**

louib · 2017-04-21T14:21:13Z

src/gui/DatabaseWidget.cpp

+
+        if (result == QMessageBox::Yes) {
+            // destroying direct entries of the recycle bin
+            QList<Entry*> subEntries = currentGroup->entries();


@vsvyatski I'd argue this could go in one of the core classes. In Database, there is already createRecycleBin, recycleEntry and recycleGroup. Maybe we could add emptyRecycleBin there. It would also be easier to unit test!

louib

@vsvyatski thanks for the PR! I added some comments on the changes.

droidmonkey · 2017-04-21T15:56:14Z

src/core/Database.cpp

+        // destroying direct entries of the recycle bin
+        QList<Entry*> subEntries = m_metadata->recycleBin()->entries();
+        for (Entry* entry : subEntries) {
+            delete entry;


Is this the appropriate way to get rid of an entry? I would imagine you would have to go through the db object to remove entries and groups officially.

@droidmonkey Well, it seems it is, or almost it is. Here is the code from Group destructor (src link):

Group::~Group() { // Destroy entries and children manually so DeletedObjects can be added // to database. const QList<Entry*> entries = m_entries; for (Entry* entry : entries) { delete entry; } const QList<Group*> children = m_children; for (Group* group : children) { delete group; } if (m_db && m_parent) { DeletedObject delGroup; delGroup.deletionTime = QDateTime::currentDateTimeUtc(); delGroup.uuid = m_uuid; m_db->addDeletedObject(delGroup); } cleanupParent(); }

As you can see, it does things similar to what I've done.

Here is the source of Entry destructor(src link)

Entry::~Entry() { if (m_group) { m_group->removeEntry(this); if (m_group->database()) { m_group->database()->addDeletedObject(m_uuid); } } qDeleteAll(m_history); }

It updates the DB and all that.

Some additional information. After cleaning the recycle bin and saving, a KDBX file is reduced in size and can be opened in KeePass 2.35 (the original .NET program). The latter doesn't report any errors and the recycle bin looks clean.

Cool, just making sure.

louib · 2017-04-21T21:01:03Z

@vsvyatski Do you think you could add a unit test for the new function in Database, now that it's well encapsulated?

As a side note, I could be nice at some point to have an empty recycle bin icon for when the recycle bin is empty!

vsvyatski · 2017-04-21T21:28:01Z

@louib I could try. I don't see any class TestDatabase in the tests folder though. What is the proper place to put that test? Should I create a new class?

Regarding your side note: perhaps later, I believe the original KeePass doesn't have that empty recycle bin icon either.

louib · 2017-04-21T22:27:57Z

@vsvyatski you could create a TestDatabase class in the tests directory (look at TestEntry.cpp for example).

I'll add an issue for the empty recycle bin.

…e "empty recycle bin" functionality

vsvyatski · 2017-04-22T16:30:55Z

@louib I've added 3 out of 4 test cases. I'm going to need to continue on another machine therefore I'm pushing the code. I will additionally notify you once I consider the tests implemented.

vsvyatski · 2017-04-22T22:01:13Z

@louib As promised, I'm done with the tests. You can now review.

louib · 2017-04-22T23:14:38Z

@vsvyatski awesome work, thanks for the contribution!

- Added YubiKey 2FA integration for unlocking databases [#127] - Added TOTP support [#519] - Added CSV import tool [#146, #490] - Added KeePassXC CLI tool [#254] - Added diceware password generator [#373] - Added support for entry references [#370, #378] - Added support for Twofish encryption [#167] - Enabled DEP and ASLR for in-memory protection [#371] - Enabled single instance mode [#510] - Enabled portable mode [#645] - Enabled database lock on screensaver and session lock [#545] - Redesigned welcome screen with common features and recent databases [#292] - Multiple updates to search behavior [#168, #213, #374, #471, #603, #654] - Added auto-type fields {CLEARFIELD}, {SPACE}, {{}, {}} [#267, #427, #480] - Fixed auto-type errors on Linux [#550] - Prompt user prior to executing a cmd:// URL [#235] - Entry attributes can be protected (hidden) [#220] - Added extended ascii to password generator [#538] - Added new database icon to toolbar [#289] - Added context menu entry to empty recycle bin in databases [#520] - Added "apply" button to entry and group edit windows [#624] - Added macOS tray icon and enabled minimize on close [#583] - Fixed issues with unclean shutdowns [#170, #580] - Changed keyboard shortcut to create new database to CTRL+SHIFT+N [#515] - Compare window title to entry URLs [#556] - Implemented inline error messages [#162] - Ignore group expansion and other minor changes when making database "dirty" [#464] - Updated license and copyright information on souce files [#632] - Added contributors list to about dialog [#629]

In these cases delete the group instead of trying to move it to the recycle bin. Closes keepassxreboot#520

Add context menu entry to clean the Recycle Bin in databases

dea65b6

This implements the feature request (issue) #503.

vsvyatski mentioned this pull request Apr 21, 2017

Add context menu entry to clean the Recycle Bin in databases #503

Closed

louib reviewed Apr 21, 2017

View reviewed changes

louib requested changes Apr 21, 2017

View reviewed changes

Add requested source code changes

75c16d1

droidmonkey reviewed Apr 21, 2017

View reviewed changes

droidmonkey approved these changes Apr 21, 2017

View reviewed changes

vsvyatski added 2 commits April 22, 2017 12:38

Add skeleton for TestDatabase.cpp and test data for unit tests for th…

5792bf1

…e "empty recycle bin" functionality

Add recycle bin test cases body

583f60a

Finish test cases for emptying recycle bin

c613f44

louib approved these changes Apr 22, 2017

View reviewed changes

louib merged commit ad79162 into keepassxreboot:develop Apr 22, 2017

phoerious added this to the v2.2.0 milestone May 7, 2017

phoerious added the user interface label May 7, 2017

supershoop pushed a commit to supershoop/keepassxc that referenced this pull request Oct 5, 2022

Fix crash when deleting parent group of recycle bin.

bf2fd63

In these cases delete the group instead of trying to move it to the recycle bin. Closes keepassxreboot#520

Uh oh!

Conversation

vsvyatski commented Apr 21, 2017

Description

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

louib Apr 21, 2017

Choose a reason for hiding this comment

Uh oh!

louib Apr 21, 2017

Choose a reason for hiding this comment

Uh oh!

louib Apr 21, 2017

Choose a reason for hiding this comment

Uh oh!

louib Apr 21, 2017

Choose a reason for hiding this comment

Uh oh!

louib left a comment

Choose a reason for hiding this comment

Uh oh!

droidmonkey Apr 21, 2017

Choose a reason for hiding this comment

Uh oh!

vsvyatski Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsvyatski Apr 21, 2017

Choose a reason for hiding this comment

Uh oh!

droidmonkey Apr 21, 2017

Choose a reason for hiding this comment

Uh oh!

louib commented Apr 21, 2017

Uh oh!

vsvyatski commented Apr 21, 2017

Uh oh!

louib commented Apr 21, 2017

Uh oh!

vsvyatski commented Apr 22, 2017

Uh oh!

vsvyatski commented Apr 22, 2017

Uh oh!

louib commented Apr 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vsvyatski Apr 21, 2017 •

edited

Loading