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

feat: support regex search #676

Merged
merged 1 commit into from
Nov 18, 2024
Merged

feat: support regex search #676

merged 1 commit into from
Nov 18, 2024

Conversation

lievenhey
Copy link
Contributor

Since we use QSortFilterProxyModel we get this for free. This patch adds support for regex search by not escaping the search term if it is prefix with %.
For the flamegraph there is a custom implementation, which changes the current QString::contains to a QRegularExpression::match call.

Closes: #666

@lievenhey lievenhey requested a review from milianw September 17, 2024 09:54
@lievenhey lievenhey force-pushed the complex-search branch 2 times, most recently from 61eeef7 to a1fe6f0 Compare September 17, 2024 10:51
@GitMensch
Copy link
Contributor

That's a nice implementation! The tooltip also helps for documentation purposes (I'm still not sure if there should be a note on that in the README as well).

src/flamegraph.cpp Outdated Show resolved Hide resolved
src/flamegraph.cpp Outdated Show resolved Hide resolved
src/flamegraph.cpp Outdated Show resolved Hide resolved
src/resultsbottomuppage.ui Outdated Show resolved Hide resolved
src/resultscallercalleepage.ui Outdated Show resolved Hide resolved
src/resultstopdownpage.ui Outdated Show resolved Hide resolved
src/flamegraph.cpp Outdated Show resolved Hide resolved
src/util.cpp Outdated Show resolved Hide resolved
Since we use QSortFilterProxyModel we get this for free.
This patch adds support for regex search by not escaping the search term
if it is prefix with %.
For the flamegraph there is a custom implementation, which changes the
current QString::contains to a QRegularExpression::match call.

Closes: #666
{
SearchResults result;
if (searchValue.isEmpty()) {
if (expression.pattern().isEmpty()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this should be !expression.isValid() || expression.pattern().isEmpty()

Copy link
Contributor

Choose a reason for hiding this comment

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

if it isn't valid we should not get here, at least not when executed from the UI (the function could possibly assert that or do an early-exit for the invalid expression)

Copy link
Member

Choose a reason for hiding this comment

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

ack, that lgtm as-is

@lievenhey lievenhey requested a review from milianw October 10, 2024 16:51
{
SearchResults result;
if (searchValue.isEmpty()) {
if (expression.pattern().isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

ack, that lgtm as-is

src/flamegraph.cpp Show resolved Hide resolved
@milianw
Copy link
Member

milianw commented Nov 18, 2024

otherwise code lgtm, tests would obviously be nice to have, but I don't think we have any integration tests of that sort yet :(

@milianw milianw merged commit 9da82e4 into master Nov 18, 2024
25 checks passed
@milianw milianw deleted the complex-search branch November 18, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

caller/callee: optional "extended" search
3 participants