refactor(search): De-couple baloo URL parsing logic from UI
ClosedPublic

Authored by iasensio on Mon, Nov 11, 1:01 PM.

Details

Summary

Extracts the logic that parses baloosearch: urls into a new model class. The parser logic itself is kept as is.
The search box UI is later updated using the model fields.

This refactor has been proposed by @bruns in the review of D24422, as it largely simplifies the unit tests and further expansion/improvements.

Test Plan

No behavior changes.
Test case is added in the follow-up revision: D25258

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
bruns added a comment.Mon, Nov 11, 2:11 PM

This definitely goes in the right direction, thanks!

src/search/dolphinquerymodel.cpp
33 ↗(On Diff #69583)

not necessary, as already done by Baloo::Query::fromSearchUrl()

61 ↗(On Diff #69583)

Use a plain C array instead of a QList.

66 ↗(On Diff #69583)

{ goes on the line above

src/search/dolphinquerymodel.h
45 ↗(On Diff #69583)

Does not access DolphinQueryModel, so can be deleted here and declared in an anonymous namespace in dolphinquerymodel.cpp

50 ↗(On Diff #69583)

m_fileType

bruns added a comment.Mon, Nov 11, 2:36 PM

Please add a comment to the summary that the parser logic is exactly kept as is.

src/search/dolphinquerymodel.cpp
62 ↗(On Diff #69583)

add a comment this is a copy of the DolphinFacetsWidget::isRatingTerm() method.

src/search/dolphinquerymodel.h
43 ↗(On Diff #69583)

keep the API small, no need for this function.

iasensio updated this revision to Diff 69598.Mon, Nov 11, 6:19 PM
iasensio marked 6 inline comments as done.
  • API style and simplification
iasensio added inline comments.Mon, Nov 11, 6:25 PM
src/search/dolphinquerymodel.h
45 ↗(On Diff #69583)

I'm not really sure about this. Wouldn't it be useful to set the terms here and access them from the outside to avoid duplication, for instance, in DolphinFacetsWidgetitself?

iasensio edited the summary of this revision. (Show Details)Mon, Nov 11, 6:27 PM
iasensio edited the test plan for this revision. (Show Details)
elvisangelaccio requested changes to this revision.Mon, Nov 11, 9:44 PM
elvisangelaccio added inline comments.
src/search/dolphinquerymodel.cpp
55–57 ↗(On Diff #69598)

Please move the documentation in the header file and add doxygen tags (/** ... */).

I'd also "invert" the definition: is this class says this is a search them, then the UI (e.g. DolphinFacetsWidget) will be able to handle it.

60–63 ↗(On Diff #69598)

Why not a simpler QStringList ?

src/search/dolphinquerymodel.h
26–30 ↗(On Diff #69598)

I'm not sure about this name. DolphinQueryModel kind of implies that this class follows the Qt model/view architecture, i.e. that there is also a DolphinQueryView class.

It seems to me that this class is just a tiny wrapper above Baloo:Query, so maybe we could call it just DolphinQuery?

30 ↗(On Diff #69598)

Missing DOLPHIN_EXPORT attribute.

33–34 ↗(On Diff #69598)

Not needed, let the compiler worry about generating a default ctor/dtor.

38 ↗(On Diff #69598)

I'd call it searchUrl() to make clear this is the url passed to fromBalooSearchUrl().

I'd also move all these function definitions to the .cpp file.

39 ↗(On Diff #69598)

This is Baloo::Query::searchString(), why not call it the same?

40 ↗(On Diff #69598)

Please add apidox to explain what this method does (i.e. the first of Baloo::Query::types(), otherwise an empty string).

41 ↗(On Diff #69598)

Bonus point if you also document this function ;)

42 ↗(On Diff #69598)

This is `Baloo::Query::includeFolder(), why not call it the same?

src/search/dolphinsearchbox.cpp
520

Please use qAsConst or put the container in const variable, otherwise it will detach.

This revision now requires changes to proceed.Mon, Nov 11, 9:44 PM
bruns added inline comments.Mon, Nov 11, 9:54 PM
src/search/dolphinquerymodel.cpp
55–57 ↗(On Diff #69598)

The intention is to first factor the model out, i.e. no behavior changes.

Afterwards, the model can be changed to e.g. use Baloo::AdvancedQueryParser, which splits out *all* terms with a prefix (modified, type, rating, ...). The Widget can then pick out the ones it handles specifically and keep the remainder verbatim.

60–63 ↗(On Diff #69598)

QLatin1String x[] = {...} is definitely simpler then QStringList.

src/search/dolphinquerymodel.h
45 ↗(On Diff #69583)

But the DolphinFacetsWidget still has to handle each term prefix individually, it can gain no knowledge by using this.

src/search/dolphinquerymodel.cpp
55–57 ↗(On Diff #69598)

Ah right, I didn't realize this is copied from DolphinFacetsWidget's implementation.

60–63 ↗(On Diff #69598)

Well it's a bit... unusual, at least in dolphin we never use a plain C array of QLatin1Strings

bruns added inline comments.Mon, Nov 11, 10:33 PM
src/search/dolphinquerymodel.cpp
60–63 ↗(On Diff #69598)

Probably because iterating over a plain C array was awkward before C++11 range-based for loops. Array is preferred, as it needs no allocation and does one pointer dereference less.

iasensio updated this revision to Diff 69655.Tue, Nov 12, 10:19 PM
iasensio marked 12 inline comments as done.

Address inline comments

  • Address comments in header
  • Move isSearchTerm() into an anonymous namespace
  • Use const value in for-range loop
  • Rename class
iasensio added inline comments.Tue, Nov 12, 10:20 PM
src/search/dolphinquerymodel.h
39 ↗(On Diff #69598)

Now we set that value to keep the logic as is, but it is one of the things that will change in the fix.
It is the text to show in the searchbar input.

I changed the filenames to address a change in the class name, and it seems that phab didn't preserve the inline comments, sorry.
I've tried to address the majority of them, now with better understanding of @bruns intentions about the refactor itself.
Thanks for the insightful reviews.

src/search/dolphinquerymodel.h
39 ↗(On Diff #69598)

I see, then please put this information in the documentation of this method ;)

iasensio updated this revision to Diff 69657.Tue, Nov 12, 10:44 PM
iasensio marked an inline comment as done.
  • Clarification for text() method
bruns requested changes to this revision.Tue, Nov 12, 10:51 PM
bruns added inline comments.
src/search/dolphinsearchbox.cpp
503

const& instead of *.

This revision now requires changes to proceed.Tue, Nov 12, 10:51 PM
src/search/dolphinsearchbox.h
158

Since we renamed the class, this should be renamed too. updateFromQuery(const DolphinQuery *query) ?

bruns added inline comments.Tue, Nov 12, 10:56 PM
src/search/dolphinquery.cpp
48

nitpick: space should be on the right side of &.

iasensio updated this revision to Diff 69658.Tue, Nov 12, 11:07 PM
iasensio marked 3 inline comments as done.
  • Update method signatures
bruns added inline comments.Tue, Nov 12, 11:08 PM
src/search/dolphinfacetswidget.cpp
156 ↗(On Diff #69657)

This is included from D25255, use arc diff HEAD^1 to only submit the topmost commit

src/search/dolphinsearchbox.h
156

Also here (\a query).

Please remove the Depends D25255 from the summary, this one is independent.

bruns accepted this revision.Tue, Nov 12, 11:11 PM

Otherwise, LGTM now.

Please remove the Depends D25255 from the summary, this one is independent.

I'm afraid it won't work well without that, because it handles the empty case for type().
For the sake of simplicity, I could also abandon D25255 and keep the change here.

src/search/dolphinquerymodel.h
39 ↗(On Diff #69598)

Kind of broke Phabricator. ⚙ I updated to this:

/** @return the user text part of the query, to be shown in the searchbar */
QString text() const;
iasensio updated this revision to Diff 69659.Tue, Nov 12, 11:37 PM
iasensio marked an inline comment as done.
  • Rename also in dox comment
iasensio updated this revision to Diff 69660.Tue, Nov 12, 11:41 PM
iasensio marked an inline comment as done.

Proper diff from D25255

Please remove the Depends D25255 from the summary, this one is independent.

I'm afraid it won't work well without that, because it handles the empty case for type().
For the sake of simplicity, I could also abandon D25255 and keep the change here.

Without D25255, the type will not be reset to "Any", just as before.

After this has landed, the unit tests should go in, then D25255 (amended by the unit test change, XFAIL--).

Without D25255, the type will not be reset to "Any", just as before.

After this has landed, the unit tests should go in, then D25255 (amended by the unit test change, XFAIL--).

Ah ok, I see the path now. I removed the dependency.
But please note that D25255 is not covered by the unit test since it is a UI fix (the model does ok).

bruns added inline comments.Wed, Nov 13, 1:19 PM
src/search/dolphinsearchbox.cpp
148

extra space

503

extra space

iasensio updated this revision to Diff 69700.Wed, Nov 13, 7:26 PM
iasensio marked 2 inline comments as done.
  • Fix extra space
elvisangelaccio accepted this revision.Wed, Nov 13, 8:48 PM

Please push to the release/19.12 branch. Thanks!

This revision is now accepted and ready to land.Wed, Nov 13, 8:48 PM

Please push to the release/19.12 branch. Thanks!

@iasensio Instructions for how to do this can be found here: https://community.kde.org/Infrastructure/Phabricator#Landing_on_the_.22Stable_branch.22

This revision was automatically updated to reflect the committed changes.

Instructions for how to do this can be found here: https://community.kde.org/Infrastructure/Phabricator#Landing_on_the_.22Stable_branch.22

@ngraham Thanks! Really helpful stuff!