Skip to content
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

Refactor Database and Database widgets #2491

Merged
merged 30 commits into from Nov 22, 2018

Conversation

Projects
None yet
2 participants
@phoerious
Copy link
Member

phoerious commented Nov 18, 2018

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 #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 #1389 and its duplicates #2146 #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]

phoerious and others added some commits Nov 3, 2018

@phoerious phoerious added the refactor label Nov 18, 2018

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

@phoerious phoerious requested a review from droidmonkey Nov 18, 2018

@phoerious phoerious force-pushed the feature/database-refactor branch 7 times, most recently from 03b853f to c457363 Nov 18, 2018

@phoerious phoerious force-pushed the feature/database-refactor branch from c457363 to eee05f1 Nov 18, 2018

@phoerious phoerious force-pushed the feature/database-refactor branch from e686220 to 76d7353 Nov 18, 2018

phoerious added some commits Nov 18, 2018

@phoerious phoerious force-pushed the feature/database-refactor branch from 1667143 to 1e68822 Nov 18, 2018

@droidmonkey
Copy link
Member

droidmonkey left a comment

Round 1: Got up to Database.h

Show resolved Hide resolved src/browser/BrowserService.cpp Outdated
Show resolved Hide resolved src/cli/Add.cpp Outdated
Show resolved Hide resolved src/cli/Edit.cpp Outdated
Show resolved Hide resolved src/cli/Merge.cpp Outdated
Show resolved Hide resolved src/cli/Merge.cpp Outdated
Show resolved Hide resolved src/core/Database.h Outdated
Show resolved Hide resolved src/core/Database.h Outdated
Show resolved Hide resolved src/core/Database.h Outdated
Show resolved Hide resolved src/core/Database.h
Show resolved Hide resolved src/core/Database.h

@phoerious phoerious force-pushed the feature/database-refactor branch from d870d20 to 228bea3 Nov 19, 2018

droidmonkey and others added some commits Nov 19, 2018

@droidmonkey
Copy link
Member

droidmonkey left a comment

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

Still need to review DatabaseWidget and DatabaseTabWidget

Show resolved Hide resolved src/core/Metadata.cpp
Show resolved Hide resolved src/gui/DatabaseOpenWidget.cpp Outdated
Show resolved Hide resolved src/gui/entry/EditEntryWidget.h
@phoerious

This comment has been minimized.

Copy link
Member Author

phoerious commented Nov 20, 2018

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

This comment has been minimized.

Copy link
Member

droidmonkey commented Nov 20, 2018

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

@phoerious

This comment has been minimized.

Copy link
Member Author

phoerious commented Nov 20, 2018

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

@droidmonkey
Copy link
Member

droidmonkey left a comment

Final batch of comments

Show resolved Hide resolved src/gui/DatabaseTabWidget.cpp
Show resolved Hide resolved src/gui/DatabaseTabWidget.cpp
Show resolved Hide resolved src/gui/DatabaseTabWidget.cpp
Show resolved Hide resolved src/gui/DatabaseTabWidget.cpp
Show resolved Hide resolved src/gui/DatabaseTabWidget.cpp
Show resolved Hide resolved src/gui/DatabaseTabWidget.cpp
Show resolved Hide resolved src/gui/DatabaseTabWidget.cpp
Show resolved Hide resolved src/gui/DatabaseTabWidget.cpp
Show resolved Hide resolved src/gui/DatabaseWidget.h
@droidmonkey
Copy link
Member

droidmonkey left a comment

Ready for merging!

@phoerious phoerious merged commit d612cad into develop Nov 22, 2018

4 checks passed

CodeFactor 2 issues fixed. 2 issues found.
Details
MacOS (KeepassXC) TeamCity build finished
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details

@phoerious phoerious deleted the feature/database-refactor branch Nov 22, 2018

droidmonkey added a commit that referenced this pull request Nov 23, 2018

Fix bugs introduced by database refactor #2491 (#2503)
* 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

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

Re-implement AutoOpen functionality after refactor (#2504)
The database refactor in #2491 removed auto-open functionality.

droidmonkey added a commit that referenced this pull request Mar 19, 2019

Release 2.4.0
- 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]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.