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

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> dolphinquerymodel.cpp:55-57
> +/* Checks if a given search term can be handled by DolphinFacetsWidget.
> + * This is a copy of `DolphinFacetsWidget::isRatingTerm()` method.
> + */

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.

> dolphinquerymodel.cpp:60-63
> +    static const QLatin1String searchTokens[] {
> +        QLatin1String("modified>="),
> +        QLatin1String("rating>=")
> +    };

Why not a simpler `QStringList` ?

> dolphinquerymodel.h:26-30
> +/**
> + * @brief Simple query model that parses a search Url and fills the relevant
> + * data to display on dolphin search box.
> + */
> +class DolphinQueryModel

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`?

> dolphinquerymodel.h:30
> + */
> +class DolphinQueryModel
> +{

Missing `DOLPHIN_EXPORT` attribute.

> dolphinquerymodel.h:33-34
> +public:
> +    DolphinQueryModel() {};
> +    ~DolphinQueryModel() {};
> +

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

> dolphinquerymodel.h:38
> +
> +    QUrl url() const {return m_searchUrl;};
> +    QString text() const {return m_searchText;};

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.

> dolphinquerymodel.h:39
> +    QUrl url() const {return m_searchUrl;};
> +    QString text() const {return m_searchText;};
> +    QString type() const {return m_fileType;};

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

> dolphinquerymodel.h:40
> +    QString text() const {return m_searchText;};
> +    QString type() const {return m_fileType;};
> +    QStringList searchTerms() const {return m_searchTerms;};

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

> dolphinquerymodel.h:41
> +    QString type() const {return m_fileType;};
> +    QStringList searchTerms() const {return m_searchTerms;};
> +    QString customDir() const {return m_customDir;};

Bonus point if you also document this function ;)

> dolphinquerymodel.h:42
> +    QStringList searchTerms() const {return m_searchTerms;};
> +    QString customDir() const {return m_customDir;};
> +

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

> dolphinsearchbox.cpp:520
> +    m_facetsWidget->setFacetType(queryModel->type());
> +    for (const QString& searchTerm : queryModel->searchTerms()) {
> +        m_facetsWidget->setRatingTerm(searchTerm);

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

REPOSITORY
  R318 Dolphin

REVISION DETAIL
  https://phabricator.kde.org/D25257

To: iasensio, #dolphin, elvisangelaccio, bruns
Cc: bruns, kfm-devel, pberestov, iasensio, fprice, MrPepe, fbampaloukas, alexde, Codezela, feverfew, meven, spoorun, navarromorales, firef, ngraham, andrebarros, emmanuelp, mikesomov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20191111/1cb6d090/attachment.htm>