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

[David Faure]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20171101/62905f88/attachment.htm>