(search) Fix searching tags with spaces
ClosedPublic

Authored by iasensio on Jan 2 2020, 3:20 PM.

Details

Summary

Tags containing blank spaces were not handled properly in the search widget.
Now we enclose them in quotes and strip the quotes before setting them to the widget.

Test Plan

No artifacts when searching tags containing spaces
Added test cases to bin/dolphinquerytest

Diff Detail

Repository
R318 Dolphin
Branch
search_tags_space (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20832
Build 20850: arc lint + arc unit
iasensio created this revision.Jan 2 2020, 3:20 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptJan 2 2020, 3:20 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
iasensio requested review of this revision.Jan 2 2020, 3:20 PM
iasensio updated this revision to Diff 72624.Jan 2 2020, 3:42 PM
This comment was removed by iasensio.
iasensio updated this revision to Diff 72626.Jan 2 2020, 3:44 PM

Sorry, I messed up with the diffs. Back to normal

ngraham accepted this revision.Jan 2 2020, 3:55 PM

Thanks!

This revision is now accepted and ready to land.Jan 2 2020, 3:55 PM
elvisangelaccio requested changes to this revision.Jan 6 2020, 12:31 PM

Is it possible to add a test case in dolphinquerytest.cpp?

src/search/dolphinfacetswidget.cpp
168–169

IMHO the new helper function should only strip the quotes. I'd keep the QString::mid(10) logic here, because it's clear that we are removing the modified>= prefix.

This revision now requires changes to proceed.Jan 6 2020, 12:31 PM
iasensio updated this revision to Diff 72889.Jan 6 2020, 2:10 PM
iasensio marked an inline comment as done.

Only strip quotes and keep QString::mid() for clarity

iasensio added inline comments.Jan 6 2020, 7:17 PM
src/search/dolphinfacetswidget.cpp
168–169

Done. It's more clear indeed.
I just kept stripQuotes() for the tagscase, since it's not necessary in the other ones.

meven added a subscriber: meven.Jan 8 2020, 12:14 PM
meven added inline comments.
src/search/dolphinfacetswidget.cpp
38

Maybe check that the first and last characters are quotes first before removing "
a tag could be 'my_documents"' (unlikely but...)

iasensio planned changes to this revision.Jan 8 2020, 2:45 PM

Is it possible to add a test case in dolphinquerytest.cpp?

Sorry, I totally oversaw your comment. That test unit only checks on DolphinQuery, not on the UI.
But in fact this function is copied verbatim from there, so it makes sense to use one and only function, and to be able to test it.

With your suggestion and @meven's one, I think I'll strip the quotes out directly in DoplhinQuery, so after parsing and splitting the terms, the tag:"some weird quoted tag" just becomes tag:some weird quoted tag and no further quote stripping is needed here. I'll add a test case too.

iasensio updated this revision to Diff 73108.Jan 9 2020, 12:06 AM
iasensio marked an inline comment as done.

Better approach, by stripping out DolphinQuery

  • Widget: Add quotes to selected tags with spaces
  • Query: Strip quotes around value for every search term
  • Do not strip out unpaired quotes
  • Add test cases
iasensio edited the summary of this revision. (Show Details)Jan 9 2020, 12:11 AM
iasensio edited the test plan for this revision. (Show Details)
iasensio added a reviewer: meven.
iasensio updated this revision to Diff 73109.Jan 9 2020, 12:55 AM

Remove unnecessary variable

elvisangelaccio accepted this revision.Jan 19 2020, 9:37 PM
This revision is now accepted and ready to land.Jan 19 2020, 9:37 PM
This revision was automatically updated to reflect the committed changes.

Seems like a candidate for the 19.12 branch, no?

Seems like a candidate for the 19.12 branch, no?

Since I'm also using master I had to check to be sure. Tag filter was added later and it's not on 19.12.