KeePassX PR Migration: #190 Search for Group Names#168
Conversation
| { | ||
| Entry* entry = m_entryView->currentEntry(); | ||
| Q_ASSERT(entry); | ||
|
|
There was a problem hiding this comment.
@debfx what's the point of placing Q_ASSERT here?
There was a problem hiding this comment.
Asked myself the same question, especially because there is an if (!entry) return right after it which handles a non-existing entry much better and without crashing the program.
There was a problem hiding this comment.
The idea is to catch bugs when working on the code (thus the assert) but not crash the application in normal use.
Apparently it has caught a bug in this case so it seems to be useful.
There was a problem hiding this comment.
Well, it wasn't exactly a bug. It was only an empty search result and the function did what it was supposed to do: nothing. The bug was rather the assertion which crashed the program when hitting enter in the search box when there were no results.
There was a problem hiding this comment.
Some code clearly calls switchToGroup/EntryEdit() when it does not make sense. I'd rather get to the bottom of it and fix the root cause.
The if (...) return is only meant as a safety measure to not crash the program on non-fatal bugs.
There was a problem hiding this comment.
It's a signal that was directly triggered by a QT event. So if you want to catch that earlier you have to plug in an additional function in between which does nothing else but also returning when there is no valid entry. The code which connects the signal is this in SearchWidget.cpp:104:
mx.connect(m_ui->searchEdit, SIGNAL(returnPressed()), SLOT(switchToEntryEdit()));
Connecting another member function to only catch empty search results seems very superfluous to me when switchToEntryEdit() already handles that case perfectly.
There was a problem hiding this comment.
Actually, thinking about it more, switchToEntryEdit() already IS the intermediate member which catches invalid entries, because then it delegates further to switchToEntryEdit(Entry*, bool). Having an assertion there makes sense, but having an assertion already in switchToEntryEdit() is a mistake.
It's perfectly possible that m_entryView->currentEntry() returns no entry when there is none. Checking for that and returning is correct, asserting there would be one is not.
phoerious
force-pushed
the
migrate/190-search-groups
branch
from
f2cd5bc to
cfc86ce
Compare
phoerious
force-pushed
the
migrate/190-search-groups
branch
from
cfc86ce to
a056f57
Compare
This offers parity with KeePass
phoerious
force-pushed
the
migrate/190-search-groups
branch
from
a056f57 to
6885ba9
Compare
- 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]
phoerious
added
pr: new feature
5 participants
Description
This is a rebased version of KeePassX pull request keepassx/keepassx#190 by @jsoref.
It allows to not only search for entries in the database, but also for their group names.
Additionally, I removed two assertions which lead to crashes when the search result was empty and KeePassXC was compiled in Debug mode.
How Has This Been Tested?
Searching for group names of entries works as expected. All existing unit tests pass.
Types of changes
Checklist: