D8332: Added baloo urls into places model

[Kevin Ottens]

ervin requested changes to this revision.
ervin added a comment.
This revision now requires changes to proceed.


  All the fiddling with URLs makes me wonder if that wouldn't be better done on the KIO implementations side... but that's out of scope for that patch I think.

INLINE COMMENTS

> kfileplacesmodeltest.cpp:98
>  
> +    // disable baloo
> +    KConfig config(QStringLiteral("baloofilerc"));

Shouldn't we have tests verifying the extra URLs are properly inserted when baloo is enabled? Seems like it's trying to avoid testing the behavior change coming from the rest of the commit.

> kfileplacesmodel.cpp:68
> +          bookmarkManager(nullptr),
> +          fileIndexingEnabled(isFileIndexingEnabled())
> +    {

Do we really want to keep that state? It's never reevaluated so could be a const if we keep it.

Asks the question of what happens if the setting changes at runtime though.

> kfileplacesmodel.cpp:499
> +        bool allowedHere = appName.isEmpty() || (appName == QCoreApplication::instance()->applicationName());        
> +        bool allowBalooUrl = isBalooUrl(url) ? fileIndexingEnabled : true;
> +

I find that naming confusing I mean, semantically fileIndexingEnabled would be your "allowBalooUrl". Could we come up with something better? Like "isSupportedUrl" perhaps? This is what it is about somewhat, we support all schemes except for the baloo ones depending on fileIndexingEnabled.

> kfileplacesview.cpp:1301
> +    const QString path = url.toDisplayString(QUrl::PreferLocalFile);
> +    if (path.endsWith(QLatin1String("yesterday"))) {
> +        const QDate date = QDate::currentDate().addDays(-1);

Don't you want to match /yesterday and so on here? Like you're matching /documents and so on for the method below.

> kfileplacesview.cpp:1329
> +    if (day > 0) {
> +        date = date.arg(QString("-%1").arg(day, 2, 10, QChar('0')));
> +    } else {

This is unusual, why not concatenate + arg() in that case instead of the nested arg calls?

Would allow you to drop the else part too.

REPOSITORY
  R241 KIO

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

To: renatoo, #frameworks, #dolphin, #kde_applications, dvratil, #vdg, ngraham, ervin
Cc: ervin, usta, mlaurent, dvratil, ngraham, #frameworks
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20171031/b97343dc/attachment-0001.html>