dfaure added inline comments.
INLINE COMMENTS
> kio_tags.cpp:64
> + QString tag;
> + tag = url.path().section(QLatin1Char('?'), 0, 0);
> + while (tag.startsWith(QDir::separator()))
- merge with previous line
- can path() really contain '?' ? I thought that wasn't possible (? delimits
the query).
> kio_tags.cpp:119
> + Q_UNUSED(permissions);
> + Q_UNUSED(flags);
>
missing KIO::Overwrite support? Can the dest already exist?
> kio_tags.cpp:156
>
> - case FileUrl:
> - ForwardingSlaveBase::get(QUrl::fromLocalFile(fileUrl));
> - return;
> + if ((result.urlType == TagFileUrl) || (result.urlType == LocalUrl)) {
> + ForwardingSlaveBase::get(QUrl::fromLocalFile(result.filePath));
And otherwise? what happens if it's neither TagFileUrl nor LocalUrl? should
error(not supported) be called? The job would never finish otherwise.
> kio_tags.cpp:163
> {
> - Q_UNUSED(permissions);
> Q_UNUSED(flags);
>
same question, can dest already exist?
(if so, then the right thing to do is error(), unless `flags & Overwrite`)
> kio_tags.cpp:203
> + for (const QString& tag : md.tags()) {
> + if (tag == srcResult.tag ||
> (tag.startsWith(QString(srcResult.tag + QLatin1Char('/'))))) {
> + QString newTag = tag;
the QString() isn't needed (repeats)
> kio_tags.cpp:262
> + } else {
> + ForwardingSlaveBase::mimetype(QUrl::fromLocalFile(result.filePath));
> }
missing return, to avoid emitting finished() twice
(like you did after the other calls to ForwardingSlaveBase)
> kio_tags.cpp:292
> + TagListJob* tagJob = new TagListJob();
> + tagJob->exec();
>
can this fail?
> kio_tags.cpp:331
> + // Extract any file path and determine tag from url
> + if (url.scheme() == QLatin1String("file")) {
> + result.urlType = LocalUrl;
That's better written as if (url.isLocalFile())
> kio_tags.cpp:337
> + bool validTag = relaxValidation;
> + if (url.path().contains(QLatin1Char('?')) || chopLastSection) {
> + QUrl choppedUrl = url;
same question as before
> kio_tags.cpp:356
> + QString query;
> + const QStringList tagSections =
> result.tag.split(QLatin1Char('/'));
> + for (int i = 0; i < tagSections.size(); ++i) {
could this whole splitting be optimized by saying something like this?
result.tag.prepend("tag:");
result.tag.replace('/', " AND tag:");
It's just that split is slow (allocates a temporary container etc.).
If this is called often, and the above isn't good enough, then something like
what I did in kcontacts, commit
https://phabricator.kde.org/R174:5de361ff80ce0a015b62a3a76fe676355d8d3a52, can
be used.
But maybe split is fine here, I guess it's likely not time critical.
> kio_tags.cpp:393
> + result.pathUDSResults << createUDSEntryForTag(QStringLiteral("."),
> result.tag);
> + int index = result.tag.count(QLatin1Char('/')) + !result.tag.isEmpty();
> + QStringList tagPaths;
This conversion from bool to int sounds ... fragile.
I would do
+ (result.tag.isEmpty() ? 0 : 1)
> kio_tags.cpp:397
> + for (const QString& tag : tags) {
> + QString tagSection = tag.section(QLatin1Char('/'), index, index,
> QString::SectionSkipEmpty);
> + if (result.tag.isEmpty() || (tag.startsWith(result.tag,
> Qt::CaseInsensitive) && !result.tag.isEmpty())) {
`tagSection` isn't used by the if condition itself, so it could move into the
if().
> kio_tags.cpp:398
> + QString tagSection = tag.section(QLatin1Char('/'), index, index,
> QString::SectionSkipEmpty);
> + if (result.tag.isEmpty() || (tag.startsWith(result.tag,
> Qt::CaseInsensitive) && !result.tag.isEmpty())) {
> + if (!tagPaths.contains(tagSection, Qt::CaseInsensitive) &&
> !tagSection.isEmpty()) {
The `&& !result.tag.isEmpty()` is useless.
If we're evaluating that part of the condition (the part after the "||"), then
result.tag is NOT empty, by definition.
> kio_tags.cpp:422
> + KIO::UDSEntry uds;
> + if (KIO::StatJob* job = KIO::stat(match, KIO::HideProgressInfo)) {
> + // we do not want to wait for the event loop to delete the job
KIO::stat never returns nullptr, so the if() isn't useful.
REPOSITORY
R293 Baloo
BRANCH
master-nestedTags (branched from master)
REVISION DETAIL
https://phabricator.kde.org/D8098
To: smithjd, #frameworks, vhanda, #dolphin, ngraham
Cc: dfaure, nicolasfella, ngraham
