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

Fix issues with group functions #2410

Merged
merged 2 commits into from Oct 30, 2018

Conversation

Projects
None yet
2 participants
@droidmonkey
Copy link
Member

droidmonkey commented Oct 22, 2018

Description

Resolves #2408

Just some cleanup and streamlining in the Group class. Eliminated function that had undetermined results based on heuristics of the input (Group::findEntry). Aligned Group::findEntryByPath with Group::findGroupByPath.

Eliminated unnecessary Q_ASSERTS and integrated the checks into the code function instead.

Recommendation: rename Group::children() to Group::groups(). Children overshadows QObject function AND it is the only place where the name children is used, otherwise we just call them groups...

Recommendation 2: Look for other places where QString::isNull() is used and evaluate whether it makes sense. This function is for legacy purposes only and QString::isEmpty() is preferred by the Qt Docs.

Types of changes

  • Refactor of code

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]
  • My change requires a change to the documentation and I have updated it accordingly.
  • I have added tests to cover my changes.
for (Entry* entry : entriesRecursive(false)) {
if (entry->title() == entryId) {
return entry;
if (!uuid.isNull()) {

This comment has been minimized.

Copy link
@phoerious

phoerious Oct 23, 2018

Member
if (uuid.isNull()) {
    return nullptr;
}

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Oct 28, 2018

Author Member

Why? then this function would have three return statements (short circuit, in-loop, and fall-through). I don't agree with that approach. I actually changed it specifically to this fashion because the old method was confusing.

This comment has been minimized.

Copy link
@phoerious

phoerious Oct 28, 2018

Member

You have three indentation levels here. I prefer keeping it shallow.

for (Group* group : asConst(m_children)) {
Entry* entry = group->findEntryByPath(entryPath, basePath + group->name() + QString("/"));
for (Group* group : children()) {
Entry* entry = group->findEntryByPathRecursion(entryPath, basePath + group->name() + "/");

This comment has been minimized.

Copy link
@phoerious

phoerious Oct 23, 2018

Member

Recursive

for (Group* group : groupsRecursive(true)) {
if (group->uuid() == uuid) {
return group;
if (!uuid.isNull()) {

This comment has been minimized.

Copy link
@phoerious

phoerious Oct 23, 2018

Member

See above.

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Oct 28, 2018

Author Member

same comment, changing this will create three return statements unnecessarily

QVERIFY(entry == nullptr);

// An invalid UUID.
entry = db->rootGroup()->findEntry(QString("febfb01ebcdf9dbd90a3f1579dc"));
entry = db->rootGroup()->findEntryByUuid(QUuid("febfb01ebcdf9dbd90a3f1579dc"));
QVERIFY(entry == nullptr);

This comment has been minimized.

Copy link
@phoerious

phoerious Oct 23, 2018

Member

QVERIFY(!entry);

entry = db->rootGroup()->findEntryByUuid({});
QVERIFY(entry == nullptr);

entry = db->rootGroup()->findEntryByPath({});
QVERIFY(entry == nullptr);

This comment has been minimized.

Copy link
@phoerious

@phoerious phoerious added the bug label Oct 23, 2018

@droidmonkey droidmonkey force-pushed the fix/group-issues branch 2 times, most recently from b6519bf to a1d9f66 Oct 28, 2018

@droidmonkey

This comment has been minimized.

Copy link
Member Author

droidmonkey commented Oct 30, 2018

changes made to short circuit return nullptr.

droidmonkey added some commits Oct 22, 2018

@droidmonkey droidmonkey force-pushed the fix/group-issues branch from a1d9f66 to 0162f2a Oct 30, 2018

@droidmonkey droidmonkey merged commit fa687f2 into develop Oct 30, 2018

3 checks passed

CodeFactor No issues found.
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details

@droidmonkey droidmonkey deleted the fix/group-issues branch Oct 30, 2018

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.