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
