From a21dc9bfa2e88b3f87270269a6e1c28c6d668be1 Mon Sep 17 00:00:00 2001 From: ronso0 Date: Fri, 10 Feb 2023 00:36:14 +0100 Subject: [PATCH] BasePlaylistFeature::updateChildModel: use QSet to reduce tree iterations for example, updateChildModel would iterate over the tree model for each playlist id received from PlaylistDAO::tracksChanged. With a QSet the sidebar can now simply by updated (renamed, decorated) after bulk-un/lock in History which previously required to rebuild the model and thereby reset the expanded/collapsed states of nodes. --- src/library/dao/playlistdao.cpp | 18 ++++++--- src/library/dao/playlistdao.h | 4 +- src/library/trackset/baseplaylistfeature.cpp | 40 ++++++++++---------- src/library/trackset/baseplaylistfeature.h | 5 +-- src/library/trackset/playlistfeature.cpp | 17 ++++----- src/library/trackset/playlistfeature.h | 3 +- src/library/trackset/setlogfeature.cpp | 26 +++++-------- src/library/trackset/setlogfeature.h | 3 +- 8 files changed, 56 insertions(+), 60 deletions(-) diff --git a/src/library/dao/playlistdao.cpp b/src/library/dao/playlistdao.cpp index 74faadfc864..c8e8f7cfd71 100644 --- a/src/library/dao/playlistdao.cpp +++ b/src/library/dao/playlistdao.cpp @@ -309,20 +309,25 @@ bool PlaylistDAO::setPlaylistLocked(const int playlistId, const bool locked) { LOG_FAILED_QUERY(query); return false; } - emit lockChanged(playlistId); + const QSet ids{playlistId}; + emit lockChanged(ids); return true; } -int PlaylistDAO::setPlaylistsLocked(const QStringList& idStringList, const bool lock) { - if (idStringList.isEmpty()) { +int PlaylistDAO::setPlaylistsLocked(const QSet playlistIds, const bool lock) { + if (playlistIds.isEmpty()) { return 0; } if (lock) { - qInfo() << "Locking" << idStringList.size() << "playlists"; + qInfo() << "Locking" << playlistIds.size() << "playlists"; } else { - qInfo() << "Unlocking" << idStringList.size() << "playlists"; + qInfo() << "Unlocking" << playlistIds.size() << "playlists"; } + QStringList idStringList; + for (int id : qAsConst(playlistIds)) { + idStringList.append(QString::number(id)); + } QString idString = idStringList.join(","); QSqlQuery query(m_database); @@ -338,7 +343,8 @@ int PlaylistDAO::setPlaylistsLocked(const QStringList& idStringList, const bool return -1; } - return idStringList.length(); + emit lockChanged(playlistIds); + return playlistIds.size(); } bool PlaylistDAO::isPlaylistLocked(const int playlistId) const { diff --git a/src/library/dao/playlistdao.h b/src/library/dao/playlistdao.h index 687cfabfe28..a49bf872fa3 100644 --- a/src/library/dao/playlistdao.h +++ b/src/library/dao/playlistdao.h @@ -69,7 +69,7 @@ class PlaylistDAO : public QObject, public virtual DAO { void renamePlaylist(const int playlistId, const QString& newName); // Lock or unlock a playlist bool setPlaylistLocked(const int playlistId, const bool locked); - int setPlaylistsLocked(const QStringList& idStringList, const bool lock); + int setPlaylistsLocked(const QSet playlistIds, const bool lock); // Find out the state of a playlist lock bool isPlaylistLocked(const int playlistId) const; // Append a list of tracks to a playlist @@ -136,7 +136,7 @@ class PlaylistDAO : public QObject, public virtual DAO { void added(int playlistId); void deleted(int playlistId); void renamed(int playlistId, const QString& newName); - void lockChanged(int playlistId); + void lockChanged(const QSet playlistIds); void trackAdded(int playlistId, TrackId trackId, int position); void trackRemoved(int playlistId, TrackId trackId, int position); void tracksChanged(const QSet& playlistIds); // added/removed/reordered diff --git a/src/library/trackset/baseplaylistfeature.cpp b/src/library/trackset/baseplaylistfeature.cpp index 08f4ed4de68..5d912cbaf8d 100644 --- a/src/library/trackset/baseplaylistfeature.cpp +++ b/src/library/trackset/baseplaylistfeature.cpp @@ -152,7 +152,7 @@ void BasePlaylistFeature::connectPlaylistDAO() { connect(&m_playlistDao, &PlaylistDAO::lockChanged, this, - &BasePlaylistFeature::slotPlaylistTableLockChanged); + &BasePlaylistFeature::slotPlaylistContentOrLockChanged); connect(&m_playlistDao, &PlaylistDAO::deleted, this, @@ -160,7 +160,7 @@ void BasePlaylistFeature::connectPlaylistDAO() { connect(&m_playlistDao, &PlaylistDAO::tracksChanged, this, - &BasePlaylistFeature::slotPlaylistContentChanged); + &BasePlaylistFeature::slotPlaylistContentOrLockChanged); connect(&m_playlistDao, &PlaylistDAO::renamed, this, @@ -715,18 +715,15 @@ void BasePlaylistFeature::htmlLinkClicked(const QUrl& link) { } } -void BasePlaylistFeature::updateChildModel(int playlistId) { - // qDebug() << "BasePlaylistFeature::updateChildModel:" << playlistId; - // TODO(ronso0) refactor for efficiency by using a QSet to avoid - // countless TreeItem iterations when bulk-locking - // See slotPlaylistContentChanged() - // This would also allow updating the tree on lockChanged signal, instead of - // rebuilding it (collapsed/expanded states are reset). - if (playlistId == kInvalidPlaylistId) { +void BasePlaylistFeature::updateChildModel(const QSet& playlistIds) { + // qDebug() << "BasePlaylistFeature::updateChildModel"; + if (playlistIds.isEmpty()) { return; } - QString playlistLabel = fetchPlaylistLabel(playlistId); - QVariant variantId = QVariant(playlistId); + + int id = kInvalidPlaylistId; + QString label; + bool ok = false; for (int row = 0; row < m_pSidebarModel->rowCount(); ++row) { QModelIndex index = m_pSidebarModel->index(row, 0); @@ -734,15 +731,20 @@ void BasePlaylistFeature::updateChildModel(int playlistId) { DEBUG_ASSERT(pTreeItem != nullptr); if (pTreeItem->hasChildren()) { for (TreeItem* pChild : qAsConst(pTreeItem->children())) { - if (pChild->getData() == variantId) { - pTreeItem = pChild; - break; + id = pChild->getData().toInt(&ok); + if (ok && id != kInvalidPlaylistId && playlistIds.contains(id)) { + label = m_playlistDao.getPlaylistName(id); + pChild->setLabel(label); + decorateChild(pChild, id); } } - } - if (pTreeItem->getData() == variantId) { - pTreeItem->setLabel(playlistLabel); - decorateChild(pTreeItem, playlistId); + } else { + id = pTreeItem->getData().toInt(&ok); + if (ok && id != kInvalidPlaylistId && playlistIds.contains(id)) { + label = m_playlistDao.getPlaylistName(id); + pTreeItem->setLabel(label); + decorateChild(pTreeItem, id); + } } } } diff --git a/src/library/trackset/baseplaylistfeature.h b/src/library/trackset/baseplaylistfeature.h index 1c8fb02c6f1..fbeac41b5f2 100644 --- a/src/library/trackset/baseplaylistfeature.h +++ b/src/library/trackset/baseplaylistfeature.h @@ -54,9 +54,8 @@ class BasePlaylistFeature : public BaseTrackSetFeature { slotPlaylistTableChanged(playlistId); selectPlaylistInSidebar(playlistId, false); }; - virtual void slotPlaylistTableLockChanged(int playlistId) = 0; + virtual void slotPlaylistContentOrLockChanged(const QSet& playlistIds) = 0; virtual void slotPlaylistTableRenamed(int playlistId, const QString& newName) = 0; - virtual void slotPlaylistContentChanged(QSet playlistIds) = 0; void slotCreatePlaylist(); void renameItem(const QModelIndex& index) override; void deleteItem(const QModelIndex& index) override; @@ -83,7 +82,7 @@ class BasePlaylistFeature : public BaseTrackSetFeature { QString label; }; - virtual void updateChildModel(int selected_id); + virtual void updateChildModel(const QSet& playlistIds); virtual void clearChildModel(); virtual QString fetchPlaylistLabel(int playlistId) = 0; virtual void decorateChild(TreeItem* pChild, int playlistId) = 0; diff --git a/src/library/trackset/playlistfeature.cpp b/src/library/trackset/playlistfeature.cpp index e1298d47271..d3414bbbf21 100644 --- a/src/library/trackset/playlistfeature.cpp +++ b/src/library/trackset/playlistfeature.cpp @@ -304,19 +304,15 @@ void PlaylistFeature::slotPlaylistTableChanged(int playlistId) { } } -void PlaylistFeature::slotPlaylistContentChanged(QSet playlistIds) { +void PlaylistFeature::slotPlaylistContentOrLockChanged(const QSet& playlistIds) { + // qDebug() << "slotPlaylistContentOrLockChanged() playlistId:" << playlistId; + QSet idsToBeUpdated; for (const auto playlistId : qAsConst(playlistIds)) { if (m_playlistDao.getHiddenType(playlistId) == PlaylistDAO::PLHT_NOT_HIDDEN) { - updateChildModel(playlistId); + idsToBeUpdated.insert(playlistId); } } -} - -void PlaylistFeature::slotPlaylistTableLockChanged(int playlistId) { - // qDebug() << "slotPlaylistTableLockChanged() playlistId:" << playlistId; - if (m_playlistDao.getHiddenType(playlistId) == PlaylistDAO::PLHT_NOT_HIDDEN) { - updateChildModel(playlistId); - } + updateChildModel(idsToBeUpdated); } void PlaylistFeature::slotPlaylistTableRenamed( @@ -324,7 +320,8 @@ void PlaylistFeature::slotPlaylistTableRenamed( Q_UNUSED(newName); //qDebug() << "slotPlaylistTableChanged() playlistId:" << playlistId; if (m_playlistDao.getHiddenType(playlistId) == PlaylistDAO::PLHT_NOT_HIDDEN) { - updateChildModel(playlistId); + QSet ids = {playlistId}; + updateChildModel(ids); } } diff --git a/src/library/trackset/playlistfeature.h b/src/library/trackset/playlistfeature.h index 162f6a6cfbe..9f4d04db7dc 100644 --- a/src/library/trackset/playlistfeature.h +++ b/src/library/trackset/playlistfeature.h @@ -37,8 +37,7 @@ class PlaylistFeature : public BasePlaylistFeature { private slots: void slotPlaylistTableChanged(int playlistId) override; - void slotPlaylistContentChanged(QSet playlistIds) override; - void slotPlaylistTableLockChanged(int playlistId) override; + void slotPlaylistContentOrLockChanged(const QSet& playlistIds) override; void slotPlaylistTableRenamed(int playlistId, const QString& newName) override; protected: diff --git a/src/library/trackset/setlogfeature.cpp b/src/library/trackset/setlogfeature.cpp index 3aecfb04841..41171468533 100644 --- a/src/library/trackset/setlogfeature.cpp +++ b/src/library/trackset/setlogfeature.cpp @@ -299,6 +299,7 @@ QModelIndex SetlogFeature::constructChildModel(int selectedId) { QString SetlogFeature::fetchPlaylistLabel(int playlistId) { // Setup the sidebar playlist model + // TODO(ronso0) Why not m_playlistDao.getPlaylistName(id) ?? QSqlTableModel playlistTableModel(this, m_pLibrary->trackCollectionManager()->internalCollection()->database()); playlistTableModel.setTable("Playlists"); @@ -445,19 +446,15 @@ void SetlogFeature::lockOrUnlockAllChildPlaylists(bool lock) { return; } - QStringList ids; + QSet ids; for (auto* child : qAsConst(yearChildren)) { bool ok = false; int childId = child->getData().toInt(&ok); if (ok && childId != kInvalidPlaylistId) { - ids.append(child->getData().toString()); + ids.insert(childId); } } m_playlistDao.setPlaylistsLocked(ids, lock); - - // refresh sidebar model manually since PlaylistDAO::setPlaylistsLocked doesn't - // emit a signal we can connect to slotPlaylistTableLockChanged() - slotPlaylistTableChanged(kInvalidPlaylistId); } void SetlogFeature::slotDeleteAllChildPlaylists() { @@ -646,26 +643,23 @@ void SetlogFeature::slotPlaylistTableChanged(int playlistId) { } } -void SetlogFeature::slotPlaylistContentChanged(QSet playlistIds) { +void SetlogFeature::slotPlaylistContentOrLockChanged(const QSet& playlistIds) { + // qDebug() << "slotPlaylistContentOrLockChanged() playlistId:" << playlistId; + QSet idsToBeUpdated; for (const auto playlistId : qAsConst(playlistIds)) { if (m_playlistDao.getHiddenType(playlistId) == PlaylistDAO::PLHT_SET_LOG) { - updateChildModel(playlistId); + idsToBeUpdated.insert(playlistId); } } -} - -void SetlogFeature::slotPlaylistTableLockChanged(int playlistId) { - // qDebug() << "slotPlaylistTableLockChanged() playlistId:" << playlistId; - if (m_playlistDao.getHiddenType(playlistId) == PlaylistDAO::PLHT_SET_LOG) { - updateChildModel(playlistId); - } + updateChildModel(idsToBeUpdated); } void SetlogFeature::slotPlaylistTableRenamed(int playlistId, const QString& newName) { Q_UNUSED(newName); //qDebug() << "slotPlaylistTableRenamed() playlistId:" << playlistId; if (m_playlistDao.getHiddenType(playlistId) == PlaylistDAO::PLHT_SET_LOG) { - updateChildModel(playlistId); + QSet ids = {playlistId}; + updateChildModel(ids); } } diff --git a/src/library/trackset/setlogfeature.h b/src/library/trackset/setlogfeature.h index ff601eae61c..1ed07e4b41a 100644 --- a/src/library/trackset/setlogfeature.h +++ b/src/library/trackset/setlogfeature.h @@ -41,8 +41,7 @@ class SetlogFeature : public BasePlaylistFeature { private slots: void slotPlayingTrackChanged(TrackPointer currentPlayingTrack); void slotPlaylistTableChanged(int playlistId) override; - void slotPlaylistContentChanged(QSet playlistIds) override; - void slotPlaylistTableLockChanged(int playlistId) override; + void slotPlaylistContentOrLockChanged(const QSet& playlistIds) override; void slotPlaylistTableRenamed(int playlistId, const QString& newName) override; void slotDeleteAllChildPlaylists();