Dive pictures: refactor image downloader

Instead of generating one ImageDownloader object per image to be
downloaded and running every image download in a separate worker
thread, use one global ImageDownloader object owned by the UI thread.

The images are downloaded using event based IO (as probably was the
intention of the QNetworkManager class).

User-visible change: after download from the internet, the thumbnail
is shown without having to change dives.

Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

2 files changed, 95 insertions, 75 deletions

diff --git a/core/imagedownloader.cpp b/core/imagedownloader.cpp
index b56dd8b04..995bf2e02 100644
--- a/core/imagedownloader.cpp
+++ b/core/imagedownloader.cpp
@@ -21,81 +21,71 @@ static QUrl cloudImageURL(const char *filename)
 	return QUrl::fromUserInput(QString("https://cloud.subsurface-divelog.org/images/").append(hash));
 }
 
-ImageDownloader::ImageDownloader(const QString &filename_in) : filename(filename_in)
+// Note: this is a global instead of a function-local variable on purpose.
+// We don't want this to be generated in a different thread context if
+// ImageDownloader::instance() is called from a worker thread.
+static ImageDownloader imageDownloader;
+ImageDownloader *ImageDownloader::instance()
 {
+	return &imageDownloader;
 }
 
-void ImageDownloader::load(bool fromHash)
+ImageDownloader::ImageDownloader()
 {
-	if (fromHash && loadFromUrl(cloudImageURL(qPrintable(filename))))
-		return;
-
-	// If loading from hash failed, try to load from filename
-	loadFromUrl(QUrl::fromUserInput(filename));
+	connect(&manager, &QNetworkAccessManager::finished, this, &ImageDownloader::saveImage);
 }
 
-bool ImageDownloader::loadFromUrl(const QUrl &url)
+void ImageDownloader::load(QString filename, bool fromHash)
 {
-	bool success = false;
-	if (url.isValid()) {
-		QEventLoop loop;
-		QNetworkAccessManager manager;
-		QNetworkRequest request(url);
-		connect(&manager, &QNetworkAccessManager::finished, this,
-			[this,&success] (QNetworkReply *reply) { saveImage(reply, success); });
-		connect(&manager, &QNetworkAccessManager::finished, &loop, &QEventLoop::quit);
-		qDebug() << "Downloading image from" << url;
-		QNetworkReply *reply = manager.get(request);
-		loop.exec();
-		delete reply;
-	}
-	return success;
+	QUrl url = fromHash ? cloudImageURL(qPrintable(filename)) : QUrl::fromUserInput(filename);
+
+	if (!url.isValid())
+		emit failed(filename);
+
+	QNetworkRequest request(url);
+	request.setAttribute(QNetworkRequest::User, filename);
+	request.setAttribute(static_cast<QNetworkRequest::Attribute>(QNetworkRequest::User + 1), fromHash);
+	manager.get(request);
 }
 
-void ImageDownloader::saveImage(QNetworkReply *reply, bool &success)
+void ImageDownloader::saveImage(QNetworkReply *reply)
 {
-	success = false;
-	QByteArray imageData = reply->readAll();
-	QImage image;
-	image.loadFromData(imageData);
-	if (image.isNull())
-		return;
-	success = true;
-	QCryptographicHash hash(QCryptographicHash::Sha1);
-	hash.addData(imageData);
-	QString path = QStandardPaths::standardLocations(QStandardPaths::CacheLocation).first();
-	QDir dir(path);
-	if (!dir.exists())
-		dir.mkpath(path);
-	QFile imageFile(path.append("/").append(hash.result().toHex()));
-	if (imageFile.open(QIODevice::WriteOnly)) {
-		qDebug() << "Write image to" << imageFile.fileName();
-		QDataStream stream(&imageFile);
-		stream.writeRawData(imageData.data(), imageData.length());
-		imageFile.waitForBytesWritten(-1);
-		imageFile.close();
-		learnHash(filename, imageFile.fileName(), hash.result());
+	QString filename = reply->request().attribute(QNetworkRequest::User).toString();
+
+	if (reply->error() != QNetworkReply::NoError) {
+		bool fromHash = reply->request().attribute(static_cast<QNetworkRequest::Attribute>(QNetworkRequest::User + 1)).toBool();
+		if (fromHash)
+			load(filename, false);
+		else
+			emit failed(filename);
+	} else {
+		QByteArray imageData = reply->readAll();
+		QCryptographicHash hash(QCryptographicHash::Sha1);
+		hash.addData(imageData);
+		QString path = QStandardPaths::standardLocations(QStandardPaths::CacheLocation).first();
+		QDir dir(path);
+		if (!dir.exists())
+			dir.mkpath(path);
+		QFile imageFile(path.append("/").append(hash.result().toHex()));
+		if (imageFile.open(QIODevice::WriteOnly)) {
+			qDebug() << "Write image to" << imageFile.fileName();
+			QDataStream stream(&imageFile);
+			stream.writeRawData(imageData.data(), imageData.length());
+			imageFile.waitForBytesWritten(-1);
+			imageFile.close();
+			learnHash(filename, imageFile.fileName(), hash.result());
+		}
+		emit loaded(filename);
 	}
-	// This should be called to make the picture actually show.
-	// Problem is DivePictureModel is not in core.
-	// Nevertheless, the image shows when the dive is selected the next time.
-	// DivePictureModel::instance()->updateDivePictures();
 
+	reply->deleteLater();
 }
 
 static void loadPicture(QString filename, bool fromHash)
 {
-	static QSet<QString> queuedPictures;
-	static QMutex pictureQueueMutex;
-
-	QMutexLocker locker(&pictureQueueMutex);
-	if (queuedPictures.contains(filename))
-		return;
-	queuedPictures.insert(filename);
-	locker.unlock();
-
-	ImageDownloader download(filename);
-	download.load(fromHash);
+	// This has to be done in UI main thread, because QNetworkManager refuses
+	// to treat requests from other threads.
+	QMetaObject::invokeMethod(ImageDownloader::instance(), "load", Qt::AutoConnection, Q_ARG(QString, filename), Q_ARG(bool, fromHash));
 }
 
 // Overwrite QImage::load() so that we can perform better error reporting.
@@ -108,13 +98,15 @@ static QImage loadImage(const QString &fileName, const char *format = nullptr)
 	return res;
 }
 
-QImage getHashedImage(const QString &file)
+// Returns: thumbnail, still loading
+static std::pair<QImage,bool> getHashedImage(const QString &file)
 {
-	QImage res;
+	QImage thumb;
+	bool stillLoading = false;
 	QUrl url = QUrl::fromUserInput(localFilePath(file));
 	if (url.isLocalFile())
-		res = loadImage(url.toLocalFile());
-	if (res.isNull()) {
+		thumb = loadImage(url.toLocalFile());
+	if (thumb.isNull()) {
 		// This did not load anything. Let's try to get the image from other sources
 		// Let's try to load it locally via its hash
 		QString filenameLocal = localFilePath(qPrintable(file));
@@ -124,22 +116,24 @@ QImage getHashedImage(const QString &file)
 			// Try the cloud server
 			// TODO: This is dead code at the moment.
 			loadPicture(file, true);
+			stillLoading = true;
 		} else {
 			// Load locally from translated file name
-			res = loadImage(filenameLocal);
-			if (!res.isNull()) {
+			thumb = loadImage(filenameLocal);
+			if (!thumb.isNull()) {
 				// Make sure the hash still matches the image file
 				hashPicture(filenameLocal);
 			} else {
 				// Interpret filename as URL
 				loadPicture(filenameLocal, false);
+				stillLoading = true;
 			}
 		}
 	} else {
 		// We loaded successfully. Now, make sure hash is up to date.
 		hashPicture(file);
 	}
-	return res;
+	return { thumb, stillLoading };
 }
 
 static QImage renderIcon(const char *id, int size)
@@ -157,6 +151,8 @@ Thumbnailer::Thumbnailer() : failImage(renderIcon(":filter-close", maxThumbnailS
 	// Currently, we only process one image at a time. Stefan Fuchs reported problems when
 	// calculating multiple thumbnails at once and this hopefully helps.
 	pool.setMaxThreadCount(1);
+	connect(ImageDownloader::instance(), &ImageDownloader::loaded, this, &Thumbnailer::imageDownloaded);
+	connect(ImageDownloader::instance(), &ImageDownloader::failed, this, &Thumbnailer::imageDownloadFailed);
 }
 
 Thumbnailer *Thumbnailer::instance()
@@ -217,7 +213,11 @@ void Thumbnailer::processItem(QString filename)
 	QImage thumbnail = getThumbnailFromCache(filename);
 
 	if (thumbnail.isNull()) {
-		thumbnail = getHashedImage(filename);
+		auto res = getHashedImage(filename);
+		if (res.second)
+			return;
+		thumbnail = res.first;
+
 		if (thumbnail.isNull()) {
 			thumbnail = failImage;
 		} else {
@@ -232,6 +232,21 @@ void Thumbnailer::processItem(QString filename)
 	workingOn.remove(filename);
 }
 
+void Thumbnailer::imageDownloaded(QString filename)
+{
+	// Image was downloaded and the filename connected with a hash.
+	// Try thumbnailing again.
+	QMutexLocker l(&lock);
+	workingOn[filename] = QtConcurrent::run(&pool, [this, filename]() { processItem(filename); });
+}
+
+void Thumbnailer::imageDownloadFailed(QString filename)
+{
+	emit thumbnailChanged(filename, failImage);
+	QMutexLocker l(&lock);
+	workingOn.remove(filename);
+}
+
 QImage Thumbnailer::fetchThumbnail(PictureEntry &entry)
 {
 	QMutexLocker l(&lock);
diff --git a/core/imagedownloader.h b/core/imagedownloader.h
index 92edea0b6..bee262854 100644
--- a/core/imagedownloader.h
+++ b/core/imagedownloader.h
@@ -10,13 +10,17 @@
 class ImageDownloader : public QObject {
 	Q_OBJECT
 public:
-	ImageDownloader(const QString &filename);
-	void load(bool fromHash);
-
+	static ImageDownloader *instance();
+	ImageDownloader();
+public slots:
+	void load(QString filename, bool fromHash);
+signals:
+	void loaded(QString filename);
+	void failed(QString filename);
 private:
-	bool loadFromUrl(const QUrl &);	// return true on success
-	void saveImage(QNetworkReply *reply, bool &success);
-	QString filename;
+	QNetworkAccessManager manager;
+	void loadFromUrl(const QString &filename, const QUrl &);
+	void saveImage(QNetworkReply *reply);
 };
 
 class PictureEntry;
@@ -35,6 +39,9 @@ public:
 	static int maxThumbnailSize();
 	static int defaultThumbnailSize();
 	static int thumbnailSize(double zoomLevel);
+public slots:
+	void imageDownloaded(QString filename);
+	void imageDownloadFailed(QString filename);
 signals:
 	void thumbnailChanged(QString filename, QImage thumbnail);
 private:
@@ -49,6 +56,4 @@ private:
 	QMap<QString,QFuture<void>> workingOn;
 };
 
-QImage getHashedImage(const QString &filename);
-
 #endif // IMAGEDOWNLOADER_H