Refactor Database and Database widgets by phoerious · Pull Request #2491 · keepassxreboot/keepassxc

phoerious · 2018-11-18T21:04:57Z

Description

This is the long-overdue refactoring of the Database internals.

The core changes are:

Move loading and saving logic from widgets into the Database class
Let database objects keep track of modifications and dirty/clean state instead of handing this to external widgets
Move GUI interactions for loading and saving from the DatabaseTabWidget into the DatabaseWidget (resolves Prevent accidentally closing database with new password in open password generator #2494 as a side-effect)
Heavily clean up DatabaseTabWidget and degrade it to a slightly glorified QTabWidget
Use QSharedPointers for all Database objects
Remove the confusing modifiedImmediate signal (rejoice!)
Get rid of the pesky DatabaseManagerStruct (wack out!)
Implement proper tabName() method instead of reading back titles from GUI widgets (resolves Ampersand in database name not sanitized for tab #1389 and its duplicates Window title bar shows ampersand when switching between multiple databases (Linux) #2146 Bug regarding ALT key modifiers / window title with multiple tabs in GUI #855)
Fix unwanted AES-KDF downgrade if database uses Argon2 and has CustomData
Make ugly code less ugly

This patch is also the first major step towards solving issues #476 and #2322.

How has this been tested?

Tests pass, behaviour is the same as before (only slightly less buggy).

Types of changes

✅ Bug fix (non-breaking change which fixes an issue)
✅ 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]
✅ All new and existing tests passed. [REQUIRED]
✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

…file not opened

droidmonkey

Round 1: Got up to Database.h

src/browser/BrowserService.cpp

src/cli/Add.cpp

src/cli/Edit.cpp

src/cli/Merge.cpp

src/core/Database.h

droidmonkey

Some general comments:
Group - The database should be held as a QWeakPointer instead of QPointer.

Still need to review DatabaseWidget and DatabaseTabWidget

src/core/Metadata.cpp

src/gui/DatabaseOpenWidget.cpp

src/gui/entry/EditEntryWidget.h

phoerious · 2018-11-20T11:53:25Z

I don't know if we should really use a QWeakPointer instead of a QPointer. Yes, it does guarantee that the object is not deleted while you are using it (which is unlikely, because everything runs on the same thread), but it has the overhead of requiring you to promote it to a QSharedPointer before you can use it. QPointer is much simpler to use. It does not guarantee that the object goes out of scope while you are using it, but it still guarantees that the pointer is reset to nullptr if the object is deleted, so no dangling pointers.

droidmonkey · 2018-11-20T12:23:59Z

Ah, I forgot QPointer sets to nullptr. We are good to go on that front then, please ignore my comment.

phoerious · 2018-11-20T12:58:14Z

Which is why I am converting all raw pointer member variables of a QObject type to QPointers when I see them. ;-)

droidmonkey

Final batch of comments

src/gui/DatabaseTabWidget.cpp

src/gui/DatabaseWidget.h

droidmonkey

Ready for merging!

* Fix SSHAgent identity removal on database lock * Refactor storage and manipulation of SSHAgent keys to streamline process with multiple db's * Clear password field when widget is hidden, resolves #2502

The database refactor in #2491 removed auto-open functionality.

- New Database Wizard [#1952] - Advanced Search [#1797] - Automatic update checker [#2648] - KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739] - Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439] - Remove KeePassHttp support [#1752] - CLI: output info to stderr for easier scripting [#2558] - CLI: Add --quiet option [#2507] - CLI: Add create command [#2540] - CLI: Add recursive listing of entries [#2345] - CLI: Fix stdin/stdout encoding on Windows [#2425] - SSH Agent: Support OpenSSH for Windows [#1994] - macOS: TouchID Quick Unlock [#1851] - macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583] - Linux: Prevent Klipper from storing secrets in clipboard [#1969] - Linux: Use polling based file watching for NFS [#2171] - Linux: Enable use of browser plugin in Snap build [#2802] - TOTP QR Code Generator [#1167] - High-DPI Scaling for 4k screens [#2404] - Make keyboard shortcuts more consistent [#2431] - Warn user if deleting referenced entries [#1744] - Allow toolbar to be hidden and repositioned [#1819, #2357] - Increase max allowed database timeout to 12 hours [#2173] - Password generator uses existing password length by default [#2318] - Improve alert message box button labels [#2376] - Show message when a database merge makes no changes [#2551] - Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790] - Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]

phoerious and others added 18 commits November 18, 2018 18:33

WIP (does not compile yet)

3489750

WIP (does not compile yet)

c8b29d4

WIP (does not compile yet)

000dd74

Finish overall refactoring and fix compilation errors (not usable yet)

81f727f

Fix crashes

46b9174

Fix various signal connections

8206124

Fix modified signals

7b56693

Fix modified signals

465df47

Fix locking and saving

1be41e0

Fix layout warning

e5712e1

Correct crash when unlocking database due to dangling pointers

15261dc

Fix signals and recycle bin crash

ab95ef0

Fix pre-selected open/save directory

05cdbd3

Fix loading of recent databases

18c5ef9

Fix signal connections for new databases

3487902

Ignore if database already locked

19a3cc2

Fix CLI and tests compilation

c7fa2b5

Fix tests

72e0ab3

phoerious added the pr: refactoring Pull request refactors code label Nov 18, 2018

phoerious added this to the v2.4.0 milestone Nov 18, 2018

phoerious requested a review from droidmonkey November 18, 2018 21:04

phoerious force-pushed the feature/database-refactor branch 7 times, most recently from 03b853f to c457363 Compare November 18, 2018 21:33

Fix CI nags

eee05f1

phoerious force-pushed the feature/database-refactor branch from c457363 to eee05f1 Compare November 18, 2018 21:36

Remove QDebug includes (it wasn't me!)

76d7353

phoerious force-pushed the feature/database-refactor branch from e686220 to 76d7353 Compare November 18, 2018 21:51

phoerious added 2 commits November 18, 2018 23:04

Delete DatabaseWidget after removing the tab

d56cf92

Fix remaining instances of modifiedImmediate() in tests and fix temp …

1e68822

…file not opened

phoerious force-pushed the feature/database-refactor branch from 1667143 to 1e68822 Compare November 18, 2018 22:35

Delete DatabaseWidget using deleteLater()

8744ff6

droidmonkey requested changes Nov 19, 2018

View reviewed changes

Fix CI assertion failure

228bea3

phoerious force-pushed the feature/database-refactor branch from d870d20 to 228bea3 Compare November 19, 2018 17:46

droidmonkey and others added 4 commits November 19, 2018 12:56

Fix Argon2 KDF being overwritten by implicit upgrade

f869905

Fix round 1 review remarks

aa2cdad

Remove unused metadataModified() signal

1ea2015

Change upgrade condition to !KDF_AES_KDBX3

7ba8722

droidmonkey requested changes Nov 20, 2018

View reviewed changes

src/core/Metadata.cpp Show resolved Hide resolved

src/gui/DatabaseOpenWidget.cpp Outdated Show resolved Hide resolved

src/gui/entry/EditEntryWidget.h Show resolved Hide resolved

phoerious mentioned this pull request Nov 20, 2018

Prevent accidentally closing database with new password in open password generator #2494

Closed

Use normalized signatures for signal/slot connections

2128d5f

droidmonkey requested changes Nov 21, 2018

View reviewed changes

Review fixes, final round

d858b37

droidmonkey approved these changes Nov 22, 2018

View reviewed changes

phoerious merged commit d612cad into develop Nov 22, 2018

phoerious deleted the feature/database-refactor branch November 22, 2018 11:51

This was referenced Nov 22, 2018

Tray icon will not change if base locked from browser extension #2480

Closed

Update tray icon when locking from KeePassXC-Browser #2482

Closed

phoerious mentioned this pull request Nov 23, 2018

Fix bugs introduced by database refactor #2491 #2503

Merged

phoerious pushed a commit that referenced this pull request Nov 23, 2018

Re-implement AutoOpen functionality after refactor (#2504)

ff7191e

The database refactor in #2491 removed auto-open functionality.

Uh oh!

Conversation

phoerious commented Nov 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Types of changes

Checklist:

Uh oh!

droidmonkey left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

droidmonkey left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

phoerious commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

droidmonkey commented Nov 20, 2018

Uh oh!

phoerious commented Nov 20, 2018

Uh oh!

droidmonkey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

droidmonkey left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

phoerious commented Nov 18, 2018 •

edited

Loading

droidmonkey left a comment •

edited

Loading

droidmonkey left a comment •

edited

Loading

phoerious commented Nov 20, 2018 •

edited

Loading