D8098: Strip down and re-write the baloo tags KIO slave

[David Faure]

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

INLINE COMMENTS

> dfaure wrote in kio_tags.cpp:119
> missing KIO::Overwrite support? Can the dest already exist?

I see an added check for "already exists" and an early return (good), but no support for KIO::Overwrite. If the user uses dolphin to copy a tag they might click on "overwrite" and it won't work, if copy() just ignores KIO::Overwrite.

> dfaure wrote in kio_tags.cpp:163
> same question, can dest already exist?
> (if so, then the right thing to do is error(), unless `flags & Overwrite`)

same issue here, still no support for Overwrite.

> smithjd wrote in kio_tags.cpp:292
> No.

This was a rethorical question :-)
It can fail, see `TagListJob::start()`, therefore please check the return value of exec().

> kio_tags.cpp:148
>  
> -    case FileUrl:
> -        ForwardingSlaveBase::get(QUrl::fromLocalFile(fileUrl));
> +    if (result.urlType == LocalUrl) {
> +        ForwardingSlaveBase::get(result.localUrl);

Could be another "else if" for symmetry, or better, this could all be a switch statement.

It still seems odd to me that if it's not invalid, tag, or local, then get() finishes without an error, is that expected?

> kio_tags.cpp:287
>  // The ForwardingSlaveBase functions are always called with a file:// url
>  // In this case we just set the newUrl = url
>  bool TagsProtocol::rewriteUrl(const QUrl& url, QUrl& newURL)

No you don't ;) This comment confused me.
When calling ForwardingSlaveBase methods with a file URL, rewriteUrl isn't called at all (see `if (url.scheme() == q->mProtocol) {` in `ForwardingSlaveBasePrivate::internalRewriteUrl`.

So in fact rewriteUrl() is never called, AFAICS. You could verify that with a Q_ASSERT(false) in here, and extensive testing ;-) Maybe don't commit that though. But at least, if I'm right, change the comment to "So rewriteUrl is never called.".

> kio_tags.cpp:366
> +        if (result.tag.endsWith(QStringLiteral(".."))) {
> +            result.tag = result.tag.section(QDir::separator(), 0, -3);
> +        } else if (result.tag.endsWith(QChar('.')) || result.decodedUrl.contains(QLatin1Char('?')) || chopLastSection) {

Would it be simpler to use QDir::cleanPath() for all this path cleaning? Just an idea, ignore if bogus.

REPOSITORY
  R293 Baloo

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

To: smithjd, #frameworks, vhanda, #dolphin, ngraham, dfaure
Cc: dfaure, nicolasfella, ngraham
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20171122/ceadcb01/attachment-0001.html>