PR review updates

src/core/library.c

@@ -222,7 +222,7 @@ struct mLibrary* mLibraryLoad(const char* path) {
 		goto error;
 	}
 
-	static const char insertRom[] = "INSERT INTO roms (crc32, md5, sha1, size, internalCode, platform, :models) VALUES (:crc32, :md5, :sha1, :size, :internalCode, :platform, :models);";
+	static const char insertRom[] = "INSERT INTO roms (crc32, md5, sha1, size, internalCode, platform, models) VALUES (:crc32, :md5, :sha1, :size, :internalCode, :platform, :models);";
 	if (sqlite3_prepare_v2(library->db, insertRom, -1, &library->insertRom, NULL)) {
 		goto error;
 	}

src/platform/qt/library/LibraryController.cpp

@@ -61,13 +61,7 @@ LibraryController::LibraryController(QWidget* parent, const QString& path, Confi
 	QObject::connect(m_libraryModel, &QAbstractItemModel::rowsInserted, &m_expandThrottle, qOverload<>(&QTimer::start));
 
 	LibraryStyle libraryStyle = LibraryStyle(m_config->getOption("libraryStyle", int(LibraryStyle::STYLE_LIST)).toInt());
-	// Make sure setViewStyle does something
+	updateViewStyle(libraryStyle);
-	if (libraryStyle == LibraryStyle::STYLE_TREE) {
-		m_currentStyle = LibraryStyle::STYLE_LIST;
-	} else {
-		m_currentStyle = LibraryStyle::STYLE_TREE;
-	}
-	setViewStyle(libraryStyle);
 
 	QVariant librarySort = m_config->getQtOption("librarySort");
 	QVariant librarySortOrder = m_config->getQtOption("librarySortOrder");
@@ -89,6 +83,10 @@ void LibraryController::setViewStyle(LibraryStyle newStyle) {
 	if (m_currentStyle == newStyle) {
 		return;
 	}
+	updateViewStyle(newStyle);
+}
+
+void LibraryController::updateViewStyle(LibraryStyle newStyle) {
 	QString selected;
 	if (m_currentView) {
 		QModelIndex selectedIndex = m_currentView->selectionModel()->currentIndex();
@@ -266,16 +264,24 @@ void LibraryController::resizeEvent(QResizeEvent*) {
 	resizeTreeView(false);
 }
 
+// This function automatically reallocates the horizontal space between the
+// columns in the view in a useful way when the window is resized.
 void LibraryController::resizeTreeView(bool expand) {
+	// When new items are added to the model, make sure they are revealed.
 	if (expand) {
 		m_treeView->expandAll();
 	}
 
+	// Start off by asking the view how wide it thinks each column should be.
 	int viewportWidth = m_treeView->viewport()->width();
 	int totalWidth = m_treeView->header()->sectionSizeHint(LibraryModel::MAX_COLUMN);
 	for (int column = 0; column < LibraryModel::MAX_COLUMN; column++) {
 		totalWidth += m_treeView->columnWidth(column);
 	}
+
+	// If there would be empty space, ask the view to redistribute it.
+	// The final column is set to fill any remaining width, so this
+	// should (at least) fill the window.
 	if (totalWidth < viewportWidth) {
 		totalWidth = 0;
 		for (int column = 0; column <= LibraryModel::MAX_COLUMN; column++) {
@@ -283,6 +289,10 @@ void LibraryController::resizeTreeView(bool expand) {
 			totalWidth += m_treeView->columnWidth(column);
 		}
 	}
+
+	// If the columns would be too wide for the view now, try shrinking the
+	// "Location" column down to reduce horizontal scrolling, with a fixed
+	// minimum width of 100px.
 	if (totalWidth > viewportWidth) {
 		int locationWidth = m_treeView->columnWidth(LibraryModel::COL_LOCATION);
 		if (locationWidth > 100) {
@@ -291,7 +301,6 @@ void LibraryController::resizeTreeView(bool expand) {
 				newLocationWidth = 100;
 			}
 			m_treeView->setColumnWidth(LibraryModel::COL_LOCATION, newLocationWidth);
-			totalWidth = totalWidth - locationWidth + newLocationWidth;
 		}
 	}
 }

src/platform/qt/library/LibraryController.h

@@ -75,6 +75,7 @@ protected:
 
 private:
 	void loadDirectory(const QString&, bool recursive = true); // Called on separate thread
+	void updateViewStyle(LibraryStyle newStyle);
 
 	ConfigController* m_config = nullptr;
 	std::shared_ptr<mLibrary> m_library;

src/platform/qt/library/LibraryEntry.cpp

@@ -12,7 +12,21 @@
 
 using namespace QGBA;
 
-static inline uint64_t checkHash(size_t filesize, uint32_t crc32) {
+static inline uint64_t getSha1Prefix(const uint8_t* sha1) {
+	return *reinterpret_cast<const quint64*>(sha1);
+}
+
+static inline uint64_t getSha1Prefix(const QByteArray& sha1) {
+	if (sha1.size() < 8) {
+		return 0;
+	}
+	return getSha1Prefix((const uint8_t*)sha1.constData());
+}
+
+static inline uint64_t checkHash(size_t filesize, uint32_t crc32, uint64_t sha1Prefix) {
+	if (sha1Prefix) {
+		return sha1Prefix;
+	}
 	return (uint64_t(filesize) << 32) ^ ((crc32 + 1ULL) * (uint32_t(filesize) + 1ULL));
 }
 
@@ -27,6 +41,7 @@ LibraryEntry::LibraryEntry(const mLibraryEntry* entry)
 	, platformModels(entry->platformModels)
 	, filesize(entry->filesize)
 	, crc32(entry->crc32)
+	, sha1(reinterpret_cast<const char*>(entry->sha1), sizeof(entry->sha1))
 {
 }
 
@@ -50,9 +65,9 @@ bool LibraryEntry::operator==(const LibraryEntry& other) const {
 }
 
 uint64_t LibraryEntry::checkHash() const {
-	return ::checkHash(filesize, crc32);
+	return ::checkHash(filesize, crc32, getSha1Prefix(sha1));
 }
 
 uint64_t LibraryEntry::checkHash(const mLibraryEntry* entry) {
-	return ::checkHash(entry->filesize, entry->crc32);
+	return ::checkHash(entry->filesize, entry->crc32, getSha1Prefix(entry->sha1));
 }

src/platform/qt/library/LibraryEntry.h

@@ -37,6 +37,7 @@ struct LibraryEntry {
 	int platformModels;
 	size_t filesize;
 	uint32_t crc32;
+	QByteArray sha1;
 
 	LibraryEntry& operator=(const LibraryEntry&) = default;
 	LibraryEntry& operator=(LibraryEntry&&) = default;

src/platform/qt/library/LibraryModel.cpp

@@ -22,25 +22,28 @@ static const QStringList iconSets{
 	"GBC",
 	"GB",
 	"SGB",
-	// "DS",
 };
 
+static QHash<QString, QIcon> platformIcons;
+
 LibraryModel::LibraryModel(QObject* parent)
 	: QAbstractItemModel(parent)
 	, m_treeMode(false)
 	, m_showFilename(false)
 {
-	for (const QString& platform : iconSets) {
+	if (platformIcons.isEmpty()) {
-		QString pathTemplate = QStringLiteral(":/res/%1-icon%2").arg(platform.toLower());
+		for (const QString& platform : iconSets) {
-		QIcon icon;
+			QString pathTemplate = QStringLiteral(":/res/%1-icon%2").arg(platform.toLower());
-		icon.addFile(pathTemplate.arg("-256.png"), QSize(256, 256));
+			QIcon icon;
-		icon.addFile(pathTemplate.arg("-128.png"), QSize(128, 128));
+			icon.addFile(pathTemplate.arg("-256.png"), QSize(256, 256));
-		icon.addFile(pathTemplate.arg("-32.png"), QSize(32, 32));
+			icon.addFile(pathTemplate.arg("-128.png"), QSize(128, 128));
-		icon.addFile(pathTemplate.arg("-24.png"), QSize(24, 24));
+			icon.addFile(pathTemplate.arg("-32.png"), QSize(32, 32));
-		icon.addFile(pathTemplate.arg("-16.png"), QSize(16, 16));
+			icon.addFile(pathTemplate.arg("-24.png"), QSize(24, 24));
-		// This will silently and harmlessly fail if QSvgIconEngine isn't compiled in.
+			icon.addFile(pathTemplate.arg("-16.png"), QSize(16, 16));
-		icon.addFile(pathTemplate.arg(".svg"));
+			// This will silently and harmlessly fail if QSvgIconEngine isn't compiled in.
-		m_icons[platform] = icon;
+			icon.addFile(pathTemplate.arg(".svg"));
+			platformIcons[platform] = icon;
+		}
 	}
 }
 
@@ -174,9 +177,13 @@ void LibraryModel::updateEntries(const QList<LibraryEntry>& items) {
 }
 
 void LibraryModel::removeEntries(const QList<QString>& items) {
-	SpanSet removedRootSpans;
+	SpanSet removedRootSpans, removedGameSpans;
 	QHash<QString, SpanSet> removedTreeSpans;
 	int firstModifiedIndex = m_games.size();
+
+	// Remove the items from the game index and assemble a span
+	// set so that we can later inform the view of which rows
+	// were removed in an optimized way.
 	for (const QString& item : items) {
 		int pos = m_gameIndex.value(item, -1);
 		Q_ASSERT(pos >= 0);
@@ -189,12 +196,18 @@ void LibraryModel::removeEntries(const QList<QString>& items) {
 		QList<const LibraryEntry*>& pathItems = m_pathIndex[entry->base];
 		int pathPos = pathItems.indexOf(entry);
 		Q_ASSERT(pathPos >= 0);
+		removedGameSpans.add(pos);
 		removedTreeSpans[entry->base].add(pathPos);
-		if (!m_treeMode) {
-			removedRootSpans.add(pos);
-		}
 		m_gameIndex.remove(item);
 	}
+
+	if (!m_treeMode) {
+		// If not using a tree view, all entries are root entries.
+		removedRootSpans = removedGameSpans;
+	}
+
+	// Remove the paths from the path indexes.
+	// If it's a tree view, inform the view.
 	for (const QString& base : removedTreeSpans.keys()) {
 		SpanSet& spanSet = removedTreeSpans[base];
 		spanSet.merge();
@@ -223,6 +236,9 @@ void LibraryModel::removeEntries(const QList<QString>& items) {
 			}
 		}
 	}
+
+	// Remove the games from the backing store and path indexes,
+	// and tell the view to remove the root items.
 	removedRootSpans.merge();
 	removedRootSpans.sort(true);
 	for (const SpanSet::Span& span : removedRootSpans.spans) {
@@ -233,10 +249,21 @@ void LibraryModel::removeEntries(const QList<QString>& items) {
 				m_pathIndex.remove(base);
 			}
 		} else {
+			// In list view, remove games from the backing store immediately
 			m_games.erase(m_games.begin() + span.left, m_games.begin() + span.right + 1);
 		}
 		endRemoveRows();
 	}
+	if (m_treeMode) {
+		// In tree view, remove them after cleaning up the path indexes.
+		removedGameSpans.merge();
+		removedGameSpans.sort(true);
+		for (const SpanSet::Span& span : removedGameSpans.spans) {
+			m_games.erase(m_games.begin() + span.left, m_games.begin() + span.right + 1);
+		}
+	}
+
+	// Finally, update the game index for the remaining items.
 	for (int i = m_games.size() - 1; i >= firstModifiedIndex; i--) {
 		m_gameIndex[m_games[i]->fullpath] = i;
 	}
@@ -294,8 +321,7 @@ int LibraryModel::rowCount(const QModelIndex& parent) const {
 	return m_games.size();
 }
 
-QVariant LibraryModel::folderData(const QModelIndex& index, int role) const
+QVariant LibraryModel::folderData(const QModelIndex& index, int role) const {
-{
 	// Precondition: index and role must have already been validated
 	if (role == Qt::DecorationRole) {
 		return qApp->style()->standardIcon(QStyle::SP_DirOpenIcon);
@@ -311,26 +337,34 @@ QVariant LibraryModel::folderData(const QModelIndex& index, int role) const
 }
 
 QVariant LibraryModel::data(const QModelIndex& index, int role) const {
-	if (role != Qt::DisplayRole &&
+	switch (role) {
-			role != Qt::EditRole &&
+		case Qt::DisplayRole:
-			role != Qt::ToolTipRole &&
+		case Qt::EditRole:
-			role != Qt::DecorationRole &&
+		case Qt::TextAlignmentRole:
-			role != Qt::TextAlignmentRole &&
+		case FullPathRole:
-			role != FullPathRole) {
+			break;
-		return QVariant();
+		case Qt::ToolTipRole:
+			if (index.column() > COL_LOCATION) {
+				return QVariant();
+			}
+			break;
+		case Qt::DecorationRole:
+			if (index.column() != COL_NAME) {
+				return QVariant();
+			}
+			break;
+		default:
+			return QVariant();
 	}
+
 	if (!checkIndex(index)) {
 		return QVariant();
 	}
-	if (role == Qt::ToolTipRole && index.column() > COL_LOCATION) {
+
-		return QVariant();
-	}
-	if (role == Qt::DecorationRole && index.column() != COL_NAME) {
-		return QVariant();
-	}
 	if (role == Qt::TextAlignmentRole) {
 		return index.column() == COL_SIZE ? (int)(Qt::AlignTrailing | Qt::AlignVCenter) : (int)(Qt::AlignLeading | Qt::AlignVCenter);
 	}
+
 	const LibraryEntry* entry = nullptr;
 	if (m_treeMode) {
 		if (!index.parent().isValid()) {
@@ -341,26 +375,28 @@ QVariant LibraryModel::data(const QModelIndex& index, int role) const {
 	} else if (!index.parent().isValid() && index.row() < (int)m_games.size()) {
 		entry = m_games[index.row()].get();
 	}
+
 	if (entry) {
 		if (role == FullPathRole) {
 			return entry->fullpath;
 		}
 		switch (index.column()) {
-			case COL_NAME:
+		case COL_NAME:
-				if (role == Qt::DecorationRole) {
+			if (role == Qt::DecorationRole) {
-					return m_icons.value(entry->displayPlatform(), qApp->style()->standardIcon(QStyle::SP_FileIcon));
+				return platformIcons.value(entry->displayPlatform(), qApp->style()->standardIcon(QStyle::SP_FileIcon));
-				}
+			}
-				return entry->displayTitle(m_showFilename);
+			return entry->displayTitle(m_showFilename);
-			case COL_LOCATION:
+		case COL_LOCATION:
-				return QDir::toNativeSeparators(entry->base);
+			return QDir::toNativeSeparators(entry->base);
-			case COL_PLATFORM:
+		case COL_PLATFORM:
-				return nicePlatformFormat(entry->platform);
+			return nicePlatformFormat(entry->platform);
-			case COL_SIZE:
+		case COL_SIZE:
-				return (role == Qt::DisplayRole) ? QVariant(niceSizeFormat(entry->filesize)) : QVariant(int(entry->filesize));
+			return (role == Qt::DisplayRole) ? QVariant(niceSizeFormat(entry->filesize)) : QVariant(int(entry->filesize));
-			case COL_CRC32:
+		case COL_CRC32:
-				return (role == Qt::DisplayRole) ? QVariant(QStringLiteral("%0").arg(entry->crc32, 8, 16, QChar('0'))) : QVariant(entry->crc32);
+			return (role == Qt::DisplayRole) ? QVariant(QStringLiteral("%0").arg(entry->crc32, 8, 16, QChar('0'))) : QVariant(entry->crc32);
 		}
 	}
+
 	return QVariant();
 }
 

src/platform/qt/library/LibraryModel.h

@@ -17,6 +17,7 @@
 #include "LibraryEntry.h"
 
 class QTreeView;
+class LibraryModelTest;
 
 namespace QGBA {
 
@@ -62,6 +63,8 @@ public:
 	LibraryEntry entry(const QString& game) const;
 
 private:
+	friend class ::LibraryModelTest;
+
 	QModelIndex indexForPath(const QString& path);
 	QModelIndex indexForPath(const QString& path) const;
 
@@ -78,7 +81,6 @@ private:
 	QStringList m_pathOrder;
 	QHash<QString, QList<const LibraryEntry*>> m_pathIndex;
 	QHash<QString, int> m_gameIndex;
-	QHash<QString, QIcon> m_icons;
 };
 
 }

src/platform/qt/test/library.cpp

@@ -128,13 +128,18 @@ private slots:
 	void testList() {
 		addTestGames1();
 		QCOMPARE(model->rowCount(), 3);
+		QCOMPARE(model->m_games.size(), 3);
 		addTestGames2();
 		QCOMPARE(model->rowCount(), 8);
+		QCOMPARE(model->m_games.size(), 8);
 		updateGame();
+		QCOMPARE(model->m_games.size(), 8);
 		model->removeEntries({ "/gba/Another.gba", "/gb/Game 6.gb" });
 		QCOMPARE(model->rowCount(), 6);
+		QCOMPARE(model->m_games.size(), 6);
 		model->removeEntries({ "/gb/Old Game.gb", "/gb/Game 7.gb" });
 		QCOMPARE(model->rowCount(), 4);
+		QCOMPARE(model->m_games.size(), 4);
 	}
 
 	void testTree() {
@@ -144,19 +149,24 @@ private slots:
 		QCOMPARE(model->rowCount(), 2);
 		QCOMPARE(model->rowCount(model->index(gbRow, 0)), 1);
 		QCOMPARE(model->rowCount(model->index(gbaRow, 0)), 2);
+		QCOMPARE(model->m_games.size(), 3);
 		addTestGames2();
 		QCOMPARE(model->rowCount(), 2);
 		QCOMPARE(model->rowCount(model->index(gbRow, 0)), 3);
 		QCOMPARE(model->rowCount(model->index(gbaRow, 0)), 5);
+		QCOMPARE(model->m_games.size(), 8);
 		updateGame();
+		QCOMPARE(model->m_games.size(), 8);
 		removeGames1();
 		QCOMPARE(model->rowCount(), 2);
 		QCOMPARE(model->rowCount(model->index(gbRow, 0)), 2);
 		QCOMPARE(model->rowCount(model->index(gbaRow, 0)), 4);
+		QCOMPARE(model->m_games.size(), 6);
 		removeGames2();
 		QVERIFY2(!find("gb").isValid(), "did not remove gb folder");
 		QCOMPARE(model->rowCount(), 1);
 		QCOMPARE(model->rowCount(model->index(0, 0)), 4);
+		QCOMPARE(model->m_games.size(), 4);
 	}
 
 	void modeSwitchTest1() {

src/platform/qt/utils.cpp

@@ -6,7 +6,9 @@
 #include "utils.h"
 
 #include <mgba/core/library.h>
+#ifdef M_CORE_GB
 #include <mgba/gb/interface.h>
+#endif
 
 #include <QCoreApplication>
 #include <QHostAddress>