BasePlaylistFeature::updateChildModel: use QSet to reduce tree iterat…

…ions

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

@@ -309,20 +309,25 @@ bool PlaylistDAO::setPlaylistLocked(const int playlistId, const bool locked) {
         LOG_FAILED_QUERY(query);
         return false;
     }
-    emit lockChanged(playlistId);
+    const QSet<int> ids{playlistId};
+    emit lockChanged(ids);
     return true;
 }
 
-int PlaylistDAO::setPlaylistsLocked(const QStringList& idStringList, const bool lock) {
-    if (idStringList.isEmpty()) {
+int PlaylistDAO::setPlaylistsLocked(const QSet<int> 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 {

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<int> 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<int> playlistIds);
     void trackAdded(int playlistId, TrackId trackId, int position);
     void trackRemoved(int playlistId, TrackId trackId, int position);
     void tracksChanged(const QSet<int>& playlistIds); // added/removed/reordered

src/library/trackset/baseplaylistfeature.cpp

@@ -152,15 +152,15 @@ void BasePlaylistFeature::connectPlaylistDAO() {
     connect(&m_playlistDao,
             &PlaylistDAO::lockChanged,
             this,
-            &BasePlaylistFeature::slotPlaylistTableLockChanged);
+            &BasePlaylistFeature::slotPlaylistContentOrLockChanged);
     connect(&m_playlistDao,
             &PlaylistDAO::deleted,
             this,
             &BasePlaylistFeature::slotPlaylistTableChanged);
     connect(&m_playlistDao,
             &PlaylistDAO::tracksChanged,
             this,
-            &BasePlaylistFeature::slotPlaylistContentChanged);
+            &BasePlaylistFeature::slotPlaylistContentOrLockChanged);
     connect(&m_playlistDao,
             &PlaylistDAO::renamed,
             this,
@@ -715,34 +715,36 @@ void BasePlaylistFeature::htmlLinkClicked(const QUrl& link) {
     }
 }
 
-void BasePlaylistFeature::updateChildModel(int playlistId) {
-    // qDebug() << "BasePlaylistFeature::updateChildModel:" << playlistId;
-    // TODO(ronso0) refactor for efficiency by using a QSet<int> 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<int>& 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);
         TreeItem* pTreeItem = m_pSidebarModel->getItem(index);
         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);
+            }
         }
     }
 }

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<int>& playlistIds) = 0;
     virtual void slotPlaylistTableRenamed(int playlistId, const QString& newName) = 0;
-    virtual void slotPlaylistContentChanged(QSet<int> 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<int>& playlistIds);
     virtual void clearChildModel();
     virtual QString fetchPlaylistLabel(int playlistId) = 0;
     virtual void decorateChild(TreeItem* pChild, int playlistId) = 0;

src/library/trackset/playlistfeature.cpp

@@ -304,27 +304,24 @@ void PlaylistFeature::slotPlaylistTableChanged(int playlistId) {
     }
 }
 
-void PlaylistFeature::slotPlaylistContentChanged(QSet<int> playlistIds) {
+void PlaylistFeature::slotPlaylistContentOrLockChanged(const QSet<int>& playlistIds) {
+    // qDebug() << "slotPlaylistContentOrLockChanged() playlistId:" << playlistId;
+    QSet<int> 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(
         int playlistId, const QString& newName) {
     Q_UNUSED(newName);
     //qDebug() << "slotPlaylistTableChanged() playlistId:" << playlistId;
     if (m_playlistDao.getHiddenType(playlistId) == PlaylistDAO::PLHT_NOT_HIDDEN) {
-        updateChildModel(playlistId);
+        QSet<int> ids = {playlistId};
+        updateChildModel(ids);
     }
 }
 

src/library/trackset/playlistfeature.h

@@ -37,8 +37,7 @@ class PlaylistFeature : public BasePlaylistFeature {
 
   private slots:
     void slotPlaylistTableChanged(int playlistId) override;
-    void slotPlaylistContentChanged(QSet<int> playlistIds) override;
-    void slotPlaylistTableLockChanged(int playlistId) override;
+    void slotPlaylistContentOrLockChanged(const QSet<int>& playlistIds) override;
     void slotPlaylistTableRenamed(int playlistId, const QString& newName) override;
 
   protected:

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<int> 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<int> playlistIds) {
+void SetlogFeature::slotPlaylistContentOrLockChanged(const QSet<int>& playlistIds) {
+    // qDebug() << "slotPlaylistContentOrLockChanged() playlistId:" << playlistId;
+    QSet<int> 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<int> ids = {playlistId};
+        updateChildModel(ids);
     }
 }
 

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<int> playlistIds) override;
-    void slotPlaylistTableLockChanged(int playlistId) override;
+    void slotPlaylistContentOrLockChanged(const QSet<int>& playlistIds) override;
     void slotPlaylistTableRenamed(int playlistId, const QString& newName) override;
     void slotDeleteAllChildPlaylists();