From 1cb9feb9a20df8954e7e281d053e13d81f5c46ee Mon Sep 17 00:00:00 2001 From: Arttu Tarkiainen Date: Wed, 16 Oct 2019 16:17:43 +0300 Subject: Fix extracted files list formation in ExtractArchive operation In ExtractArchive, the ConnectionType used to connect signal 'currentFileChanged' to slot 'fileFinished' was changed to Qt::AutoConnection in c8de51cadbc5855ca1e77d038d7f09bf60d059ee, rather than using Qt::DirectConnection explicitly. Now the connection type defaults to queued as the sender and receiver live on a different thread. This seems to cause an issue where the slot function is not invoked for every emitted 'currentFileChanged' signal before returning from ExtractArchiveOperation::performOperation(). This can happen randomly on some installer runs, leaving entries out from ExtractArchive's "files" list for some components. The list formation works fine with Qt::DirectConnection, however reverting to this behavior might be a bad practice as it leaves 'fileFinished' vulnerable to accidental calls from another thread if we change the behavior in the future. Rather move the extracted files list formation to ExtractArchiveOperation::Callback like we already do for backup files. Task-number: QTIFW-1239 Change-Id: I7dbe73abefe00814e8652432d8abdbcaa83e0bac Reviewed-by: Iikka Eklund Reviewed-by: Katja Marttila --- src/libs/installer/extractarchiveoperation.cpp | 20 +++++--------------- src/libs/installer/extractarchiveoperation.h | 4 ---- src/libs/installer/extractarchiveoperation_p.h | 8 ++++++-- 3 files changed, 11 insertions(+), 21 deletions(-) diff --git a/src/libs/installer/extractarchiveoperation.cpp b/src/libs/installer/extractarchiveoperation.cpp index 12067a7c6..1c3e1b78a 100644 --- a/src/libs/installer/extractarchiveoperation.cpp +++ b/src/libs/installer/extractarchiveoperation.cpp @@ -59,7 +59,6 @@ bool ExtractArchiveOperation::performOperation() Receiver receiver; Callback callback; - connect(&callback, &Callback::currentFileChanged, this, &ExtractArchiveOperation::fileFinished); connect(&callback, &Callback::progressChanged, this, &ExtractArchiveOperation::progressChanged); if (PackageManagerCore *core = packageManager()) { @@ -70,8 +69,6 @@ bool ExtractArchiveOperation::performOperation() connect(runnable, &Runnable::finished, &receiver, &Receiver::runnableFinished, Qt::QueuedConnection); - m_files.clear(); - QFileInfo fileInfo(archivePath); emit outputTextChanged(tr("Extracting \"%1\"").arg(fileInfo.fileName())); @@ -96,6 +93,8 @@ bool ExtractArchiveOperation::performOperation() // - (dir) // -.txt (file) + QStringList files = callback.extractedFiles(); + QString fileDirectory = targetDir + QLatin1String("/installerResources/") + archivePath.section(QLatin1Char('/'), 1, 1, QString::SectionSkipEmpty) + QLatin1Char('/'); QString archiveFileName = archivePath.section(QLatin1Char('/'), 2, 2, QString::SectionSkipEmpty); @@ -114,10 +113,10 @@ bool ExtractArchiveOperation::performOperation() if (file.open(QIODevice::WriteOnly)) { setDefaultFilePermissions(file.fileName(), DefaultFilePermissions::NonExecutable); QDataStream out (&file); - for (int i = 0; i < m_files.count(); ++i) { - m_files[i] = replacePath(m_files.at(i), targetDir, QLatin1String(scRelocatable)); + for (int i = 0; i < files.count(); ++i) { + files[i] = replacePath(files.at(i), targetDir, QLatin1String(scRelocatable)); } - out << m_files; + out << files; setValue(QLatin1String("files"), file.fileName()); file.close(); } else { @@ -199,13 +198,4 @@ bool ExtractArchiveOperation::testOperation() return true; } -/*! - This slot is direct connected to the caller so please don't call it from another thread in the - same time. -*/ -void ExtractArchiveOperation::fileFinished(const QString &filename) -{ - m_files.prepend(filename); -} - } // namespace QInstaller diff --git a/src/libs/installer/extractarchiveoperation.h b/src/libs/installer/extractarchiveoperation.h index 0f584dc0a..e706159ed 100644 --- a/src/libs/installer/extractarchiveoperation.h +++ b/src/libs/installer/extractarchiveoperation.h @@ -55,11 +55,7 @@ Q_SIGNALS: private: void startUndoProcess(const QStringList &files); -private Q_SLOTS: - void fileFinished(const QString &progress); - private: - QStringList m_files; class Callback; class Runnable; class Receiver; diff --git a/src/libs/installer/extractarchiveoperation_p.h b/src/libs/installer/extractarchiveoperation_p.h index f333da366..9cc07246f 100644 --- a/src/libs/installer/extractarchiveoperation_p.h +++ b/src/libs/installer/extractarchiveoperation_p.h @@ -97,6 +97,10 @@ public: return m_backupFiles; } + QStringList extractedFiles() const { + return m_extractedFiles; + } + public slots: void statusChanged(QInstaller::PackageManagerCore::Status status) { @@ -113,13 +117,12 @@ public slots: } signals: - void currentFileChanged(const QString &filename); void progressChanged(double progress); private: void setCurrentFile(const QString &filename) Q_DECL_OVERRIDE { - emit currentFileChanged(QDir::toNativeSeparators(filename)); + m_extractedFiles.prepend(QDir::toNativeSeparators(filename)); } static QString generateBackupName(const QString &fn) @@ -157,6 +160,7 @@ private: private: HRESULT m_state = S_OK; BackupFiles m_backupFiles; + QStringList m_extractedFiles; }; class ExtractArchiveOperation::Runnable : public QObject, public QRunnable -- cgit v1.2.3