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
