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

Implement advanced search #1797

Merged
merged 6 commits into from
Nov 17, 2018
Merged

Implement advanced search #1797

merged 6 commits into from
Nov 17, 2018

Conversation

droidmonkey
Copy link
Member

@droidmonkey droidmonkey commented Mar 31, 2018

Description

Fully implements advanced searching and other search related features/issues. Eliminates the searching of group names and notes. End search after selecting a folder if global search is active (Fixes #1501). Also took the opportunity to clean up the ugly/messy entry view and model code surrounding searching.

Fixes #239 using the following syntax:

[modifiers][field:]["]term["]

Where modifiers can be any combo of:

  • - exclude this term from results
  • + match this term exactly
  • * term is handled as a regex

Where field can be one of:

  • title
  • user[name]
  • pass[word]
  • url
  • notes
  • attr[ibute]
  • attach[ment]

If field is omitted or non-existent the default is to search title, username, url, and notes.

Where term can contain wildcard terms (if NOT regex) of:

  • * match anything (or nothing)
  • ? match any one character
  • | logical OR

Sample search queries:

  • user:jon url:google Search username for jon and url for google
  • user:jon|hifi Search username for jon OR hifi
  • +user:jon -url:google *notes:"secret note \d" Search username exactly jon, url cannot contain google, and notes contains secret note [digit]

Motivation and context

Long requested feature

How has this been tested?

Manually and with new search term parser tests.

Types of changes

  • ✅ 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]
  • ✅ My change requires a change to the documentation and I have updated it accordingly.
  • ✅ I have added tests to cover my changes.

@droidmonkey droidmonkey added this to the v2.4.0 milestone Mar 31, 2018
@droidmonkey droidmonkey requested a review from a team March 31, 2018 03:07
}

// Short circuit if we failed to match or we matched and are excluding this term
if (!found || term->exclude) {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be a continue instead? It is into a for and returning here will skip all next term in the for if the current is excluded

Copy link
Member Author

@droidmonkey droidmonkey Mar 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a short circuit because this function only returns true if all the terms match this particular entry (implicit AND of terms).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok make sense

@yan12125
Copy link
Contributor

* term is handled as a regex

I like this idea ;-) Looking forward to reusing this for #1769

@droidmonkey
Copy link
Member Author

droidmonkey commented Mar 31, 2018

Right, forgot about that, lemme extract the wildcard scrub and regex builder into the tools class

@droidmonkey droidmonkey force-pushed the feature/better-search branch 2 times, most recently from f6d24da to b0a8551 Compare April 3, 2018 01:35
@insideClaw
Copy link

insideClaw commented May 7, 2018

Sounds like exactly what was a deal-maker for me to start using KeePassXC, that it doesn't currently match a lot of sites despite similarities in the name and URL. +1

Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. One or two nitpicks. And some of the methods could use doc blocks.

src/core/EntrySearcher.cpp Outdated Show resolved Hide resolved
src/core/EntrySearcher.h Outdated Show resolved Hide resolved
src/core/Tools.cpp Outdated Show resolved Hide resolved
Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the state of this?

@droidmonkey
Copy link
Member Author

Going to make a couple additions and then it will be ready

@intika
Copy link

intika commented Oct 31, 2018

@droidmonkey Great work this is very useful tested it and it seem to work as expected...
Just one thing, the search syntax is not intuitive you should add a hint in the search box... instead of "Search..." something like "Search... (+pass:keyword)" or "Search... (+user:keyword)"... easy modification...
or a drop-down list but this would be for a future PR i guess

@droidmonkey
Copy link
Member Author

droidmonkey commented Nov 2, 2018

I made a pretty awesome search help pop-up. Any feedback?

search_help

@intika
Copy link

intika commented Nov 2, 2018

Amazing indeed... that will be useful to remember the syntax

@phoerious
Copy link
Member

Despicable. 😉
No, great pop-up, but I would prefer if it had the QLineEdit background. And I would replace the part in brackets after the description with only "(logical AND)". Making a verb of "and" sounds rather clunky to me.

@phoerious
Copy link
Member

Oh, and could you perhaps make a generic help pop-up widget of it? We could use that in other parts as well.

@droidmonkey droidmonkey force-pushed the feature/better-search branch 2 times, most recently from 08fecf3 to cfdac14 Compare November 16, 2018 15:39
Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor nitpicks, rest looks great.

src/gui/widgets/PopupHelpWidget.cpp Outdated Show resolved Hide resolved
src/gui/widgets/PopupHelpWidget.h Outdated Show resolved Hide resolved
src/gui/widgets/PopupHelpWidget.h Outdated Show resolved Hide resolved
* Remove searching of group title and notes
* End search when selecting a new group
* Correct entry searcher tests to align with new code
* Support quoted strings & per-field searching
* Support regex and exact matching
* Simplify search sequence
* Make search widget larger
* Add regex converter to Tools namespace
* Search clears if the search box does not have focus for 5 minutes (fixes #2178)
* Goto group from search results after double clicking the group name (fixes #2043)
* Support ! modifier (same as '-')
* Create reusable PopupHelpWidget as self-contained popup that can
be positioned around a parent widget and will follow the movement
and sizing of the window
* Eliminated KEEPASSXC_MAIN_WINDOW macro and replaced with
getMainWindow() function
* Add tests to cover search help show/hide
@droidmonkey
Copy link
Member Author

Done, and fixed the wording on the help popup, made background white, and fixed the border. Also squashed commits.

@droidmonkey droidmonkey merged commit 917c4cc into develop Nov 17, 2018
@droidmonkey droidmonkey deleted the feature/better-search branch November 17, 2018 16:51
droidmonkey added a commit that referenced this pull request Mar 19, 2019
- 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Focus selected folder after search Add ability to search by field with logic keywords
6 participants