Dive pictures: index local file name by canonical filname

The connection canonical filename to local filename was done via
two maps:
  1) canonical filename -> hash
  2) hash -> local filename
But the local filename was always queried from the canonical filename.
Therefore, directly index the former with the latter.

On startup, convert the old map to the new one.

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

5 files changed, 100 insertions, 48 deletions

diff --git a/core/imagedownloader.cpp b/core/imagedownloader.cpp
index 18ecd5cd6..c201e28fe 100644
--- a/core/imagedownloader.cpp
+++ b/core/imagedownloader.cpp
@@ -44,22 +44,27 @@ void ImageDownloader::saveImage(QNetworkReply *reply)
 		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());
+		if (imageData.isEmpty()) {
+			emit failed(filename);
+		} else {
+			QString path = QStandardPaths::standardLocations(QStandardPaths::CacheLocation).first();
+			QDir dir(path);
+			if (!dir.exists())
+				dir.mkpath(path);
+			QCryptographicHash hash(QCryptographicHash::Sha1);
+			hash.addData(filename.toUtf8());
+			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();
+				learnPictureFilename(filename, imageFile.fileName());
+				hashPicture(filename);	// hashPicture transforms canonical into local filename
+			}
+			emit loaded(filename);
 		}
-		emit loaded(filename);
 	}
 
 	reply->deleteLater();
diff --git a/core/qthelper.cpp b/core/qthelper.cpp
index 6527cbe21..7ca80a96e 100644
--- a/core/qthelper.cpp
+++ b/core/qthelper.cpp
@@ -1044,7 +1044,7 @@ extern "C" void reverseGeoLookup(degrees_t latitude, degrees_t longitude, uint32
 
 QHash<QString, QByteArray> hashOf;
 QMutex hashOfMutex;
-QHash<QByteArray, QString> localFilenameOf;
+QHash<QString, QString> localFilenameOf;
 
 static QByteArray getHash(const QString &filename)
 {
@@ -1126,19 +1126,62 @@ static void convertThumbnails(const QHash <QString, QImage> &thumbnails)
 	}
 }
 
+// TODO: This is a temporary helper struct. Remove in due course with convertLocalFilename().
+struct HashToFile {
+	QByteArray hash;
+	QString filename;
+	bool operator< (const HashToFile &h) const {
+		return hash < h.hash;
+	}
+};
+
+// During a transition period, convert the hash->localFilename into a canonicalFilename->localFilename.
+// TODO: remove this code in due course
+static void convertLocalFilename(const QHash<QByteArray, QString> &hashToLocal)
+{
+	// Bail out early if there is nothing to do
+	if (hashToLocal.isEmpty())
+		return;
+
+	// Create a vector of hash/filename pairs and sort by hash.
+	// Elements can than be accessed with binary search.
+	QHash<QByteArray, QString> canonicalFilenameByHash;
+	QVector<HashToFile> h2f;
+	h2f.reserve(hashOf.size());
+	for (auto it = hashOf.cbegin(); it != hashOf.cend(); ++it)
+		h2f.append({ it.value(), it.key() });
+	std::sort(h2f.begin(), h2f.end());
+
+	// Make the canonical-to-local connection
+	for (auto it = hashToLocal.cbegin(); it != hashToLocal.cend(); ++it) {
+		QByteArray hash = it.key();
+		HashToFile dummy { hash, QString() };
+		for(auto it2 = std::lower_bound(h2f.begin(), h2f.end(), dummy);
+		    it2 != h2f.end() && it2->hash == hash; ++it2) {
+			// Note that learnPictureFilename cares about all the special cases,
+			// i.e. either filename being empty or both filenames being equal.
+			learnPictureFilename(it2->filename, it.value());
+		}
+		QString canonicalFilename = canonicalFilenameByHash.value(it.key());
+	}
+}
+
 void read_hashes()
 {
 	QFile hashfile(hashfile_name());
 	if (hashfile.open(QIODevice::ReadOnly)) {
 		QDataStream stream(&hashfile);
-		stream >> localFilenameOf;
+		QHash<QByteArray, QString> localFilenameByHash;
+		stream >> localFilenameByHash;		// For backwards compatibility
 		QMutexLocker locker(&hashOfMutex);
 		stream >> hashOf;
 		locker.unlock();
 		QHash <QString, QImage> thumbnailCache;
 		stream >> thumbnailCache;		// For backwards compatibility
+		stream >> localFilenameOf;
 		hashfile.close();
 		convertThumbnails(thumbnailCache);
+		convertLocalFilename(localFilenameByHash);
 	}
 	QMutexLocker locker(&hashOfMutex);
 	localFilenameOf.remove("");
@@ -1160,24 +1203,16 @@ void write_hashes()
 
 	if (hashfile.open(QIODevice::WriteOnly)) {
 		QDataStream stream(&hashfile);
-		stream << localFilenameOf;
+		stream << QHash<QByteArray, QString>();	// Empty hash to filename - for backwards compatibility
 		stream << hashOf;
 		stream << QHash<QString,QImage>();	// Empty thumbnailCache - for backwards compatibility
+		stream << localFilenameOf;
 		hashfile.commit();
 	} else {
 		qWarning() << "Cannot open hashfile for writing: " << hashfile.fileName();
 	}
 }
 
-void add_hash(const QString &filename, const QByteArray &hash)
-{
-	if (hash.isEmpty())
-		return;
-	QMutexLocker locker(&hashOfMutex);
-	hashOf[filename] =  hash;
-	localFilenameOf[hash] = filename;
-}
-
 // Add hash if not already known
 extern "C" void register_hash(const char *filename, const char *hash)
 {
@@ -1189,7 +1224,6 @@ extern "C" void register_hash(const char *filename, const char *hash)
 	if (!hashOf.contains(filenameString)) {
 		QByteArray hashBuf = QByteArray::fromHex(hash);
 		hashOf[filename] =  hashBuf;
-		localFilenameOf[hashBuf] = filenameString;
 	}
 }
 
@@ -1199,30 +1233,28 @@ QByteArray hashFile(const QString &filename)
 	QFile imagefile(filename);
 	if (imagefile.exists() && imagefile.open(QIODevice::ReadOnly)) {
 		hash.addData(&imagefile);
-		add_hash(filename, hash.result());
 		return hash.result();
 	} else {
 		return QByteArray();
 	}
 }
 
-void learnHash(const QString &originalName, const QString &localName, const QByteArray &hash)
+void learnPictureFilename(const QString &originalName, const QString &localName)
 {
-	if (hash.isNull())
+	if (originalName.isEmpty() || localName.isEmpty())
 		return;
-	add_hash(localName, hash);
 	QMutexLocker locker(&hashOfMutex);
-	hashOf[originalName] = hash;
+	// Only keep track of images where original and local names differ
+	if (originalName == localName)
+		localFilenameOf.remove(originalName);
+	else
+		localFilenameOf[originalName] = localName;
 }
 
 QString localFilePath(const QString &originalFilename)
 {
 	QMutexLocker locker(&hashOfMutex);
-
-	if (hashOf.contains(originalFilename) && localFilenameOf.contains(hashOf[originalFilename]))
-		return localFilenameOf[hashOf[originalFilename]];
-	else
-		return originalFilename;
+	return localFilenameOf.value(originalFilename, originalFilename);
 }
 
 // This works on a copy of the string, because it runs in asynchronous context
@@ -1230,8 +1262,11 @@ void hashPicture(QString filename)
 {
 	QByteArray oldHash = getHash(filename);
 	QByteArray hash = hashFile(localFilePath(filename));
-	if (!hash.isNull() && hash != oldHash)
+	if (!hash.isEmpty() && hash != oldHash) {
+		QMutexLocker locker(&hashOfMutex);
+		hashOf[filename] =  hash;
 		mark_divelist_changed(true);
+	}
 }
 
 QStringList imageExtensionFilters() {
@@ -1242,6 +1277,20 @@ QStringList imageExtensionFilters() {
 	return filters;
 }
 
+// This works on a copy of the string, because it runs in asynchronous context
+static void learnImage(QString filename)
+{
+	QByteArray hash = hashFile(filename);
+	// TODO: This is inefficient: we search the hash map by value. But firstly,
+	// this is running in asynchronously, so it doesn't block the UI. Secondly,
+	// we might not want to learn pictures by hash anyway (the user might have
+	// edited the picture, which changes the hash.
+	for (auto it = hashOf.cbegin(); it != hashOf.cend(); ++it) {
+		if (it.value() == hash)
+			learnPictureFilename(it.key(), filename);
+	}
+}
+
 void learnImages(const QDir dir, int max_recursions)
 {
 	QStringList files;
@@ -1258,7 +1307,7 @@ void learnImages(const QDir dir, int max_recursions)
 		files.append(dir.absoluteFilePath(file));
 	}
 
-	QtConcurrent::blockingMap(files, hashFile);
+	QtConcurrent::blockingMap(files, learnImage);
 }
 
 extern "C" const char *local_file_path(struct picture *picture)
diff --git a/core/qthelper.h b/core/qthelper.h
index f48032d13..65a818262 100644
--- a/core/qthelper.h
+++ b/core/qthelper.h
@@ -32,7 +32,7 @@ QByteArray hashFile(const QString &filename);
 QString hashString(const char *filename);
 QString thumbnailFileName(const QString &filename);
 void learnImages(const QDir dir, int max_recursions);
-void add_hash(const QString &filename, const QByteArray &hash);
+void learnPictureFilename(const QString &originalName, const QString &localName);
 void hashPicture(QString filename);
 extern "C" char *hashstring(const char *filename);
 QString localFilePath(const QString &originalFilename);
diff --git a/desktop-widgets/divelistview.cpp b/desktop-widgets/divelistview.cpp
index 20d708957..ded3acbc7 100644
--- a/desktop-widgets/divelistview.cpp
+++ b/desktop-widgets/divelistview.cpp
@@ -999,7 +999,7 @@ void DiveListView::loadImageFromURL(QUrl url)
 			stream.writeRawData(imageData.data(), imageData.length());
 			imageFile.waitForBytesWritten(-1);
 			imageFile.close();
-			learnHash(url.toString(), imageFile.fileName(), hash.result());
+			learnPictureFilename(url.toString(), imageFile.fileName());
 			matchImagesToDives(QStringList(url.toString()));
 		}
 	}
diff --git a/tests/testpicture.cpp b/tests/testpicture.cpp
index 5dd261316..1833a2bcf 100644
--- a/tests/testpicture.cpp
+++ b/tests/testpicture.cpp
@@ -44,14 +44,12 @@ void TestPicture::addPicture()
 	QVERIFY(pic1->longitude.udeg == 11334500);
 	QVERIFY(pic2->offset.seconds == 1321);
 
-	QByteArray hash1 = hashFile(localFilePath(pic1->filename));
-	QByteArray hash2 = hashFile(localFilePath(pic2->filename));
-	learnHash(pic1->filename, PIC1_NAME, hash1);
-	learnHash(pic2->filename, PIC2_NAME, hash2);
+	hashPicture(pic1->filename);
+	hashPicture(pic2->filename);
+	learnPictureFilename(pic1->filename, PIC1_NAME);
+	learnPictureFilename(pic2->filename, PIC2_NAME);
 	QCOMPARE(hashstring(pic1->filename), PIC1_HASH);
 	QCOMPARE(hashstring(pic2->filename), PIC2_HASH);
-	QCOMPARE(hashstring(PIC1_NAME), PIC1_HASH);
-	QCOMPARE(hashstring(PIC2_NAME), PIC2_HASH);
 	QCOMPARE(localFilePath(pic1->filename), QString(PIC1_NAME));
 	QCOMPARE(localFilePath(pic2->filename), QString(PIC2_NAME));
 }