[dolphin] Improve parsing of Baloo query searchString
AbandonedPublic

Authored by iasensio on Oct 5 2019, 1:21 PM.

Details

Reviewers
elvisangelaccio
meven
bruns
ngraham
Group Reviewers
Dolphin
Summary

Fix the parsing of Baloo query searchString to represent its parameters properly in the search box.

The searchbox panel now updates its state on URL changes by parsing a search URL, currently exposing the following bugs:

  1. Baloo terms (rating, modified) are added to the user search text:
  2. Extra quotes are added to the search text: https://bugs.kde.org/show_bug.cgi?id=412952
  3. Doesn't properly selects Filename or Content (just uses previous selection)

Other related issues has been splitted to the following revisions:

  • Add missing parsing of type 'Folder' D24448
  • Do not toggle 'From Here'/'All Files' on reloads D24449
  • Reset search options to default when needed D24450
  • File type is not updated for places' default search URLs (e.g. baloosearch://videos) D24433
  • When selecting 'All Files' we lose track of which folder the user was previously on. D24577

BUG: 412952
FIXED-IN: 19.12

Test Plan

A test scenario is proposed in D25135, covering different cases for issues #1 and #2.

Apart from those, the 3rd issue cannot be tested due to the widget "privateness":

  • Search type (Filename / Content) is selected correctly
    • Filename: baloosearch://?json=%7B%22searchString%22:%22filename:file%22%7D
    • Content: baloosearch://?json=%7B%22searchString%22:%22file%22%7D

Diff Detail

Repository
R318 Dolphin
Branch
search_parse_frombaloo
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18466
Build 18484: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a project: Dolphin. · View Herald TranscriptOct 5 2019, 1:21 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
iasensio requested review of this revision.Oct 5 2019, 1:21 PM
iasensio edited the summary of this revision. (Show Details)Oct 5 2019, 11:28 PM
iasensio updated this revision to Diff 67390.Oct 6 2019, 5:20 PM
  • Add parsing for filename/content
bruns requested changes to this revision.Oct 6 2019, 6:48 PM
bruns added a subscriber: bruns.

Please do one change at a time.

This revision now requires changes to proceed.Oct 6 2019, 6:48 PM

Please do one change at a time.

Sure thing, it was meant as a whole "polish" of the search URL update, but it ended up with a lot of minor details.
As they are kind of related, if you don't mind I will keep this revision as the main one with only the biggest change and link from here to the minor splitted ones.

iasensio updated this revision to Diff 67395.Oct 6 2019, 10:24 PM
  • Clean patch into several revisions
iasensio edited the summary of this revision. (Show Details)Oct 6 2019, 10:52 PM
iasensio edited the test plan for this revision. (Show Details)
iasensio edited the summary of this revision. (Show Details)Oct 12 2019, 12:38 AM
iasensio edited the summary of this revision. (Show Details)Oct 17 2019, 10:35 AM

I just saw a new bug reporting on something this should fix, namely the double quotes around the search terms (https://bugs.kde.org/show_bug.cgi?id=412952), so updating the summary.
Since now the search box reloads its contents on URL changes, this kind of bugs are more exposed to the users.

ngraham accepted this revision.Oct 17 2019, 4:39 PM
ngraham added a subscriber: ngraham.

LGTM now. @elvisangelaccio and @bruns?

iasensio updated this revision to Diff 68342.Oct 20 2019, 10:24 AM
  • Rebase to master

I just saw a new bug reporting on something this should fix, namely the double quotes around the search terms (https://bugs.kde.org/show_bug.cgi?id=412952), so updating the summary.
Since now the search box reloads its contents on URL changes, this kind of bugs are more exposed to the users.

Are https://bugs.kde.org/show_bug.cgi?id=412980 and https://bugs.kde.org/show_bug.cgi?id=413184 also related?

Are https://bugs.kde.org/show_bug.cgi?id=412980 and https://bugs.kde.org/show_bug.cgi?id=413184 also related?

I think they're not. I have checked and this behavior was present before D24369.
It seems to be related with searches being cached, and when a file is renamed the current view is refreshed, but the cached version is not.

src/search/dolphinsearchbox.cpp
540–541

Sorry, missed this one.

bruns added a comment.Oct 20 2019, 1:13 PM

Required changes for the Summary:

  • describe the issue in textual form, image references are to volatile

Required for the test plan:

  • enumerate actual test cases

Also, an automatic test case would be helpful, might be necessary to splict the function into a parsing function and integration into dolphin.

iasensio updated this revision to Diff 68359.Oct 20 2019, 1:27 PM
iasensio marked an inline comment as done.
  • Split off foreach change
iasensio edited the summary of this revision. (Show Details)Oct 20 2019, 7:14 PM
iasensio edited the test plan for this revision. (Show Details)
iasensio updated this revision to Diff 68386.EditedOct 20 2019, 7:16 PM
iasensio edited the test plan for this revision. (Show Details)
  • Check for empty searchString
iasensio edited the test plan for this revision. (Show Details)Oct 20 2019, 7:24 PM
src/search/dolphinsearchbox.cpp
548

Prefer QLatin1String for comparisons.

550

Also here

chehrlic added inline comments.
src/search/dolphinsearchbox.cpp
539

Wouldn't it be better to use

const QString searchString = query.searchString();
const auto subTerms = searchString.splitRef(QLatin1Char(' '), QString::SkipEmptyParts);
for (const QStringRef &subTerm : subTerms) {
 ...

to avoid the creation of the substrings?

577

QLatin1Char(' ') instead " "

iasensio added inline comments.Oct 22 2019, 12:11 PM
src/search/dolphinsearchbox.cpp
539

Nice, will do in this patch if @elvisangelaccio agrees, or else in a different one. Thanks for this kind of comments; I'm quite lost in all of the C++/Qt optimization subtleties.

iasensio updated this revision to Diff 68568.Oct 22 2019, 8:02 PM
iasensio marked 3 inline comments as done.
iasensio edited the test plan for this revision. (Show Details)
  • Use QLatin1 methods

Also, an automatic test case would be helpful, might be necessary to splict the function into a parsing function and integration into dolphin.

I will look into adding a test case for this, but it may take me some time to know the test framework. For now I would try testing on fromBalooSearchUrl() and checking on the UI.

Splitting the parsing from the integration might require a harder refactor since the search terms are hardcoded into facetsWidget->isRatingTerm(), and also an intermediate struct/object would have to be generated. While looking on that, I realized that a more complete and robust parsing (although also more complex) could use Baloo::AdvancedQueryParser directly, to fill in the query.term() instead of directly parsing the searchString. Then, we would loop on the query.term().subTerms() objects and set the UI accordingly.

Anyway I think that would be a job for two different extra commits (test case and advanced parsing). Does this make sense?

iasensio updated this revision to Diff 69222.Nov 3 2019, 5:21 PM
  • Prevent crash on single quotation mark
iasensio updated this revision to Diff 69223.Nov 3 2019, 5:30 PM
  • Prevent crash on single quotation mark (for real)
iasensio edited the test plan for this revision. (Show Details)Nov 3 2019, 5:37 PM
iasensio edited the summary of this revision. (Show Details)Nov 4 2019, 3:39 PM
iasensio edited the test plan for this revision. (Show Details)

Hi there! Any new thoughts on this one?
I would really like to see it merged, now that we are exposing the search options some more, and so the current issues.
Please, tell me it something else is required on my part.

ngraham accepted this revision.Nov 8 2019, 2:13 PM

LGTM bug needs @elvisangelaccio's blessing

bruns requested changes to this revision.Nov 8 2019, 6:45 PM

The problem with the original code is it mixes the model and the view.

Please do the following:

  1. Split the current code to model/view, i.e. move the "rating + filename + remainder" into a separate trivial class
  2. Populate the model class from the existing parser, populate the view from the model
  3. Add the unit test you have written (of course, no longer testing the DolphinSearchBox, but the model) (some tests have to be marked as XFAIL).
  4. Update the parser, update the XFAILs

Splitting model/view makes it much easier to extend the searchbox later, and to e.g. reuse Baloos parser.

This revision now requires changes to proceed.Nov 8 2019, 6:45 PM

The problem with the original code is it mixes the model and the view.

Please do the following:

  1. Split the current code to model/view, i.e. move the "rating + filename + remainder" into a separate trivial class
  2. Populate the model class from the existing parser, populate the view from the model
  3. Add the unit test you have written (of course, no longer testing the DolphinSearchBox, but the model) (some tests have to be marked as XFAIL).
  4. Update the parser, update the XFAILs

    Splitting model/view makes it much easier to extend the searchbox later, and to e.g. reuse Baloos parser.

Not sure we have time to do all of that before the 19.12 freeze though (which is next Thursday). So I'd suggest to merge this patch and then open a refactoring task targeting dolphin 20.04. Would you be ok with that?

The problem with the original code is it mixes the model and the view.

Please do the following:

  1. Split the current code to model/view, i.e. move the "rating + filename + remainder" into a separate trivial class
  2. Populate the model class from the existing parser, populate the view from the model
  3. Add the unit test you have written (of course, no longer testing the DolphinSearchBox, but the model) (some tests have to be marked as XFAIL).
  4. Update the parser, update the XFAILs

    Splitting model/view makes it much easier to extend the searchbox later, and to e.g. reuse Baloos parser.

Not sure we have time to do all of that before the 19.12 freeze though (which is next Thursday). So I'd suggest to merge this patch and then open a refactoring task targeting dolphin 20.04. Would you be ok with that?

I have manage to follow @bruns refactor suggestions, but it is a stack of 4 different commits. I will upload them in parallel with this "quick fix", so you can choose whichever you find more suitable for the the 19.12/20.04 release.
Sorry for the bad timing in relation to 19.12.

iasensio abandoned this revision.Wed, Nov 13, 12:02 AM

Abandon in favor of D25255

Abandon in favor of D25255

Oops, I meant D25260