Fix double warning display for database open #1037
Conversation
louib
force-pushed
the
fix_double_warning_invalid_key_file
branch
from
564de84
to
bec6679
Compare
src/gui/DatabaseOpenWidget.cpp
Outdated
| { | ||
| CompositeKey masterKey; | ||
| CompositeKey* masterKey = new CompositeKey(); |
There was a problem hiding this comment.
Shouldn't this be a smart pointer of some sort? Right now I don't see you deleting this instance anywhere.
There was a problem hiding this comment.
It could be, yeah. It's deleted line 215 if the key is invalid, otherwise it's deleted at the end of openDatabase()
There was a problem hiding this comment.
Im in favor of using smart/scoped pointers for these key components from here on out.
There was a problem hiding this comment.
@droidmonkey @phoerious now using a QScopedPointer in openDatabase()
There was a problem hiding this comment.
You should use and return a QSharedPointer here, not a QScopedPointer. With a QScopedPointer you have no idea when the object will be deleted and it may as well be deleted never or twice.
src/gui/DatabaseOpenWidget.cpp
Outdated
| { | ||
| CompositeKey masterKey; | ||
| CompositeKey* masterKey = new CompositeKey(); |
There was a problem hiding this comment.
You should use and return a QSharedPointer here, not a QScopedPointer. With a QScopedPointer you have no idea when the object will be deleted and it may as well be deleted never or twice.
|
@louib @phoerious is this ready for merging? |
|
As I said, I would prefer a QSharedPointer instead of a QScopedPointer as the ownership of pointers as return values is unclear otherwise. |
phoerious
force-pushed
the
fix_double_warning_invalid_key_file
branch
from
d394c10
to
b219107
Compare
phoerious
force-pushed
the
fix_double_warning_invalid_key_file
branch
from
b219107
to
f11ea93
Compare
phoerious
force-pushed
the
fix_double_warning_invalid_key_file
branch
from
f11ea93
to
0c18f6a
Compare
|
Thanks for that @phoerious! |
- Fixed entries with empty URLs being reported to KeePassHTTP clients [#1031] - Fixed YubiKey detection and enabled CLI tool for AppImage binary [#1100] - Added AppStream description [#1082] - Improved TOTP compatibility and added new Base32 implementation [#1069] - Fixed error handling when processing invalid cipher stream [#1099] - Fixed double warning display when opening a database [#1037] - Fixed unlocking databases with --pw-stdin [#1087] - Added ability to override QT_PLUGIN_PATH environment variable for AppImages [#1079] - Fixed transform seed not being regenerated when saving the database [#1068] - Fixed only one YubiKey slot being polled [#1048] - Corrected an issue with entry icons while merging [#1008] - Corrected desktop and tray icons in Snap package [#1030] - Fixed screen lock and Google fallback settings [#1029]
Second attempt to fix #741, after #744
Rebased on
2.2.2.How has this been tested?
Locally, by opening a database with a keyfile, and testing the error message when providing an invalid path.
Types of changes
Checklist:
-DWITH_ASAN=ON. [REQUIRED]