How do I get QTemporaryFile and QtConcurrent::run to play nicely?

[J.Hilk]

@Vadi2 what @jsulm said, or move the file creation

QTemporaryFile 
file.open()

into the QtConCurrent::Run part as well.

[Vadi2]

Ohh, clever. I could create a lambda for my QtConcurrent::run that'll create the temporary file, run the unzip function, and still delete the file when it goes out of scope!

[VRonin]

You don't really need the QTemporaryFile at all, you can use the QByteArray directly (you can use QBuffer to connect QByteArray to the QIODevice Interface). Anyway you need to create on the heap (new and pass a parent just to be safe) and delete it in the slot connected to &QFutureWatcher<bool>::finished

Side note: is Host::unzip static and thread safe?

[Vadi2]

In theory I don't, but to keep it simple my unzip function accepts just the filepath to unzip.

Yeah, my function is const, so it should be thread safe, right?

bool Host::unzip(const QString &archivePath, const QString &destination, const QDir &tmpDir) const
{
    int err = 0;
    //from: https://gist.github.com/mobius/1759816
    struct zip_stat zs;
    struct zip_file* zf;
    zip_uint64_t bytesRead = 0;
    char buf[4096]; // Was 100 but that seems unduly stingy...!
    zip* archive = zip_open(archivePath.toStdString().c_str(), 0, &err);
    if (err != 0) {
//        zip_error_to_str(buf, sizeof(buf), err, errno);

        zip_error_t error;
        zip_error_init_with_code(&error, err);
        qDebug() << "unzip error:" << zip_error_strerror(&error);
        zip_error_fini(&error);

        return false;
    }

    // We now scan for directories first, and gather needed ones first, not
    // just relying on (zero length) archive entries ending in '/' as some
    // (possibly broken) archive building libraries seem to forget to
    // include them.
    QMap<QString, QString> directoriesNeededMap;
    //   Key is: relative path stored in archive
    // Value is: absolute path needed when extracting files
    for (zip_int64_t i = 0, total = zip_get_num_entries(archive, 0); i < total; ++i) {
        if (!zip_stat_index(archive, static_cast<zip_uint64_t>(i), 0, &zs)) {
            QString entryInArchive(QString::fromUtf8(zs.name));
            QString pathInArchive(entryInArchive.section(QLatin1Literal("/"), 0, -2));
            // TODO: We are supposed to validate the fields (except the
            // "valid" one itself) in zs before using them:
            // i.e. check that zs.name is valid ( zs.valid & ZIP_STAT_NAME )
            if (entryInArchive.endsWith(QLatin1Char('/'))) {
                if (!directoriesNeededMap.contains(pathInArchive)) {
                    directoriesNeededMap.insert(pathInArchive, pathInArchive);
                }
            } else {
                if (!pathInArchive.isEmpty() && !directoriesNeededMap.contains(pathInArchive)) {
                    directoriesNeededMap.insert(pathInArchive, pathInArchive);
                }
            }
        }
    }

    // Now create the needed directories:
    QMapIterator<QString, QString> itPath(directoriesNeededMap);
    while (itPath.hasNext()) {
        itPath.next();
        QString folderToCreate = QStringLiteral("%1/%2").arg(destination, itPath.value());
        if (!tmpDir.exists(folderToCreate)) {
            if (!tmpDir.mkpath(folderToCreate)) {
                zip_close(archive);
                return false; // Abort reading rest of archive
            }
            tmpDir.refresh();
        }
    }

    // Now extract the files
    for (zip_int64_t i = 0, total = zip_get_num_entries(archive, 0); i < total; ++i) {
        // No need to check return value as we've already done it first time
        zip_stat_index(archive, static_cast<zip_uint64_t>(i), 0, &zs);
        QString entryInArchive(QString::fromUtf8(zs.name));
        if (!entryInArchive.endsWith(QLatin1Char('/'))) {
            // TODO: check that zs.size is valid ( zs.valid & ZIP_STAT_SIZE )
            zf = zip_fopen_index(archive, static_cast<zip_uint64_t>(i), 0);
            if (!zf) {
                zip_close(archive);
                return false;
            }

            QFile fd(QStringLiteral("%1%2").arg(destination, entryInArchive));

            if (!fd.open(QIODevice::ReadWrite | QIODevice::Truncate)) {
                zip_fclose(zf);
                zip_close(archive);
                return false;
            }

            bytesRead = 0;
            zip_uint64_t bytesExpected = zs.size;
            while (bytesRead < bytesExpected && fd.error() == QFileDevice::NoError) {
                zip_int64_t len = zip_fread(zf, buf, sizeof(buf));
                if (len < 0) {
                    fd.close();
                    zip_fclose(zf);
                    zip_close(archive);
                    return false;
                }

                if (fd.write(buf, len) == -1) {
                    fd.close();
                    zip_fclose(zf);
                    zip_close(archive);
                    return false;
                }
                bytesRead += static_cast<zip_uint64_t>(len);
            }
            fd.close();
            zip_fclose(zf);
        }
    }

    err = zip_close(archive);
    if (err) {
        zip_error_to_str(buf, sizeof(buf), err, errno);
        return false;
    }

    return true;
}

[Vadi2]

Here is my working code! Seems to work, hope I don't have gaping flaws in it:

    connect(getReply,
            &QNetworkReply::finished,
            this,
            std::bind(
                    [=](QNetworkReply* reply) {
                        // don't do anything if there was an error
                        if (reply->error() != QNetworkReply::NoError) {
                            return;
                        }
                        QByteArray downloadedArchive = reply->readAll();

                        tempThemesArchive = new QTemporaryFile();
                        if (!tempThemesArchive->open()) {
                            return;
                        }
                        tempThemesArchive->write(downloadedArchive);
                        tempThemesArchive->close();

                        QTemporaryDir temporaryDir;
                        if (!temporaryDir.isValid()) {
                            return;
                        }

                        // perform unzipping in a worker thread so as not to freeze the UI
                        auto future =
                                QtConcurrent::run(mpHost.data(), &Host::unzip, tempThemesArchive->fileName(), QStringLiteral("%1/.config/mudlet/edbee/").arg(QDir::homePath()), temporaryDir.path());
                        auto watcher = new QFutureWatcher<bool>;
                        QObject::connect(watcher, &QFutureWatcher<bool>::finished, [=]() {
                            if (future.result() == false) {
                                qWarning() << "failed to unzip";
                                return;
                            }

                            loadEdbeeThemes(true);
                            theme_download_label->hide();
                            tempThemesArchive->deleteLater();
                        });
                        watcher->setFuture(future);
                        reply->deleteLater();
                    },
                    getReply));

[VRonin]

@Vadi2 said in How do I get QTemporaryFile and QtConcurrent::run to play nicely?:

Yeah, my function is const, so it should be thread safe, right?

Wrong (as a simple example, imagine if the Host object gets deleted). Arguably I did not bother reading all your code and it might as well be thread safe if converted to static.

There are "less manual" solutions to handle compression anyway:

• QuaZip for zip only
• KArchive for zip, 7zip, tar, gz and ar

[kshegunov]

@VRonin said in How do I get QTemporaryFile and QtConcurrent::run to play nicely?:

Wrong

Indeed, very wrong. I'd add here a very short example to illustrate, sufficed to say it has given me many sleepless nights debugging third-party code:

class SomeClass
{
public:
    void someMethod() const // Means neither thread-safe nor reentrant
    {
        // Do something that writes to the member and suddenly oooOOOPS!
    }

private:
     mutable SomeType member;
};

[VRonin]

@Vadi2 said in How do I get QTemporaryFile and QtConcurrent::run to play nicely?:

In theory I don't, but to keep it simple my unzip function accepts just the filepath to unzip.

You can also use the built in qUncompress to unzip the data

[Vadi2]

I wish I could, but I've read that it doesn't deal with zip archives.

We also have had bad experience with QuaZip compiling on Windows, so we had to take it out - and KDE dependencies are not an option.

I have followed the good advice here and unzip is now like this:

static bool unzip(const QString &archivePath, const QString &destination, const QDir &tmpDir);

I didn't realise const function was not enforced by the compiler! I take it that static is?

Thanks for the help everyone on this :)

[kshegunov]

@Vadi2 said in How do I get QTemporaryFile and QtConcurrent::run to play nicely?:

I didn't realise const function was not enforced by the compiler!

It is enforced, but in the example the member's declared as mutable, which I find pretty abominable to begin with. mutable basically means that the specific member can be modified from anywhere as it is "not part of the class interface". It's equivalent to stripping down the const manually, e.g.:

class SomeClass
{
public:
    void someMethod() const
    {
        SomeType & playingWithFireHere = const_cast<SomeType &>(member)
    }

private:
     SomeType member;
};

[J.Hilk]

@Vadi2
just a small note.

You never delete tempThemesArchive, nether after finishing the unzip nor after exiting because of an error. That'll leak memory.

[kshegunov]

Actually there's tempThemesArchive->deleteLater(); I see, I haven't checked the whole source though ...

[Vadi2]

Ohh oops - in case of an error, it would not have gotten deleted. I've fixed it now:

    connect(getReply,
            &QNetworkReply::finished,
            this,
            std::bind(
                    [=](QNetworkReply* reply) {
                        // don't do anything if there was an error
                        if (reply->error() != QNetworkReply::NoError) {
                            return;
                        }
                        QByteArray downloadedArchive = reply->readAll();

                        tempThemesArchive = new QTemporaryFile();
                        if (!tempThemesArchive->open()) {
                            return;
                        }
                        tempThemesArchive->write(downloadedArchive);
                        tempThemesArchive->close();

                        QTemporaryDir temporaryDir;
                        if (!temporaryDir.isValid()) {
                            return;
                        }

                        // perform unzipping in a worker thread so as not to freeze the UI
                        auto future =
                                QtConcurrent::run(mudlet::unzip, tempThemesArchive->fileName(), QStringLiteral("%1/.config/mudlet/edbee/").arg(QDir::homePath()), temporaryDir.path());
                        auto watcher = new QFutureWatcher<bool>;
                        QObject::connect(watcher, &QFutureWatcher<bool>::finished, [=]() {
                            if (future.result() == true) {
                                loadEdbeeThemes(true);
                            }

                            theme_download_label->hide();
                            tempThemesArchive->deleteLater();
                        });
                        watcher->setFuture(future);
                        reply->deleteLater();
                    },
                    getReply));

[J.Hilk]

@kshegunov you're right, but that applies only if the whole operation is is successful.

[Vadi2]

I'm happy with being able to keep all the code together using lambdas instead of having people jump around slot function definitions. Do others find this style readable? I'm sure there is room for improvement.

[kshegunov]

@Vadi2 said in How do I get QTemporaryFile and QtConcurrent::run to play nicely?:

Do others find this style readable?

Not me. I don't write functions that are longer than 1 screen (about 100-120 lines) and I don't write into a lambda anything that's longer than few lines, but I'm old-fashioned.

[J.Hilk]

@Vadi2 For me personally its a bit to much especially as your lambda function has a nested lambda in it.

I probably would have made this a seperated class .

[VRonin]

@Vadi2 said in How do I get QTemporaryFile and QtConcurrent::run to play nicely?:

and KDE dependencies are not an option

Just to be clear, the KDE API was redesigned in version 5 and now only uses Qt Calls so using KArchive does not link your application in any way to the KDE environment or the Linux OS. I have cross platform applications using those APIs. KArchive clearly states that works on: Android, FreeBSD, Linux, MacOSX and Windows.

@Vadi2 said in How do I get QTemporaryFile and QtConcurrent::run to play nicely?:

I wish I could, but I've read that it doesn't deal with zip archives.

I use KArchive so I did not test it but by looking at the documentation of the method, it makes me think it does:

from http://doc.qt.io/qt-5/qbytearray.html#qUncompress

If you want to use this function to uncompress external data that was compressed using zlib...