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

[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...