Library sidebar/features: some fixes, minor cleanup, improve comments

src/library/dao/playlistdao.cpp

@@ -754,8 +754,6 @@ void PlaylistDAO::addPlaylistToAutoDJQueue(const int playlistId, AutoDJSendLoc l
 }
 
 int PlaylistDAO::getPreviousPlaylist(const int currentPlaylistId, HiddenType hidden) const {
-    // Find out the highest position existing in the playlist so we know what
-    // position this track should have.
     QSqlQuery query(m_database);
     query.prepare(QStringLiteral(
             "SELECT max(id) as id FROM Playlists "

src/library/sidebarmodel.cpp

@@ -13,10 +13,10 @@
 
 namespace {
 
-// The time between selecting and activating (= clicking) a feature item
-// in the sidebar tree. This is essential to allow smooth scrolling through
-// a list of items with an encoder or the keyboard! A value of 300 ms has
-// been chosen as a compromise between usability and responsiveness.
+/// The time between selecting and activating (= clicking) a feature item
+/// in the sidebar tree. This is essential to allow smooth scrolling through
+/// a list of items with an encoder or the keyboard! A value of 300 ms has
+/// been chosen as a compromise between usability and responsiveness.
 constexpr int kPressedUntilClickedTimeoutMillis = 300;
 
 } // anonymous namespace
@@ -290,6 +290,8 @@ void SidebarModel::slotPressedUntilClickedTimeout() {
     }
 }
 
+/// Connected to WLibrarySidebar::pressed signal, called after left click,
+/// right click and selection change via keyboard or controller
 void SidebarModel::pressed(const QModelIndex& index) {
     stopPressedUntilClickedTimer();
     if (index.isValid()) {
@@ -317,6 +319,7 @@ void SidebarModel::clicked(const QModelIndex& index) {
     }
 }
 
+/// Invoked by double click and click on tree node expand icons
 void SidebarModel::doubleClicked(const QModelIndex& index) {
     stopPressedUntilClickedTimer();
     if (index.isValid()) {
@@ -337,9 +340,7 @@ void SidebarModel::rightClicked(const QPoint& globalPos, const QModelIndex& inde
     if (index.isValid()) {
         if (index.internalPointer() == this) {
             m_sFeatures[index.row()]->onRightClick(globalPos);
-        }
-        else
-        {
+        } else {
             TreeItem* pTreeItem = static_cast<TreeItem*>(index.internalPointer());
             if (pTreeItem) {
                 LibraryFeature* pFeature = pTreeItem->feature();
@@ -406,7 +407,7 @@ bool SidebarModel::dropAccept(const QModelIndex& index, const QList<QUrl>& urls,
 
 bool SidebarModel::hasTrackTable(const QModelIndex& index) const {
     if (index.internalPointer() == this) {
-     return m_sFeatures[index.row()]->hasTrackTable();
+        return m_sFeatures[index.row()]->hasTrackTable();
     }
     return false;
 }
@@ -429,7 +430,7 @@ bool SidebarModel::dragMoveAccept(const QModelIndex& index, const QUrl& url) {
     return result;
 }
 
-// Translates an index from the child models to an index of the sidebar models
+/// Translates an index from the child models to an index of the sidebar models
 QModelIndex SidebarModel::translateSourceIndex(const QModelIndex& index) {
     /* These method is called from the slot functions below.
      * QObject::sender() return the object which emitted the signal
@@ -454,8 +455,6 @@ QModelIndex SidebarModel::translateIndex(
         TreeItem* pItem = static_cast<TreeItem*>(index.internalPointer());
         translatedIndex = createIndex(index.row(), index.column(), pItem);
     } else {
-        //Comment from Tobias Rafreider --> Dead Code????
-
         for (int i = 0; i < m_sFeatures.size(); ++i) {
             if (m_sFeatures[i]->sidebarModel() == pModel) {
                 translatedIndex = createIndex(i, 0, this);

src/library/trackset/baseplaylistfeature.cpp

@@ -190,7 +190,7 @@ void BasePlaylistFeature::selectPlaylistInSidebar(int playlistId, bool select) {
         return;
     }
     QModelIndex index = indexFromPlaylistId(playlistId);
-    if (index.isValid() && m_pSidebarWidget) {
+    if (index.isValid()) {
         m_pSidebarWidget->selectChildIndex(index, select);
     }
 }
@@ -246,7 +246,7 @@ void BasePlaylistFeature::slotRenamePlaylist() {
 
     if (locked) {
         qDebug() << "Skipping playlist rename because playlist" << playlistId
-                 << "is locked.";
+                 << oldName << "is locked.";
         return;
     }
     QString newName;
@@ -585,7 +585,7 @@ void BasePlaylistFeature::slotExportPlaylist() {
                     "mixxx.db.model.playlist_export"));
 
     emit saveModelState();
-    pPlaylistTableModel->setTableModel(m_pPlaylistTableModel->getPlaylist());
+    pPlaylistTableModel->setTableModel(playlistId);
     pPlaylistTableModel->setSort(
             pPlaylistTableModel->fieldIndex(
                     ColumnCache::COLUMN_PLAYLISTTRACKSTABLE_POSITION),
@@ -621,13 +621,17 @@ void BasePlaylistFeature::slotExportPlaylist() {
 }
 
 void BasePlaylistFeature::slotExportTrackFiles() {
+    int playlistId = playlistIdFromIndex(m_lastRightClickedIndex);
+    if (playlistId == kInvalidPlaylistId) {
+        return;
+    }
     QScopedPointer<PlaylistTableModel> pPlaylistTableModel(
             new PlaylistTableModel(this,
                     m_pLibrary->trackCollectionManager(),
                     "mixxx.db.model.playlist_export"));
 
     emit saveModelState();
-    pPlaylistTableModel->setTableModel(m_pPlaylistTableModel->getPlaylist());
+    pPlaylistTableModel->setTableModel(playlistId);
     pPlaylistTableModel->setSort(pPlaylistTableModel->fieldIndex(
                                          ColumnCache::COLUMN_PLAYLISTTRACKSTABLE_POSITION),
             Qt::AscendingOrder);
@@ -732,9 +736,7 @@ void BasePlaylistFeature::updateChildModel(int playlistId) {
     }
 }
 
-/**
-  * Clears the child model dynamically, but the invisible root item remains
-  */
+/// Clears the child model dynamically, but the invisible root item remains
 void BasePlaylistFeature::clearChildModel() {
     m_lastRightClickedIndex = QModelIndex();
     m_pSidebarModel->removeRows(0, m_pSidebarModel->rowCount());

src/library/trackset/crate/cratefeature.cpp

@@ -164,19 +164,19 @@ void CrateFeature::connectLibrary(Library* pLibrary) {
 }
 
 void CrateFeature::connectTrackCollection() {
-    connect(m_pTrackCollection,
+    connect(m_pTrackCollection, // created new, duplicated or imported playlist to new crate
             &TrackCollection::crateInserted,
             this,
             &CrateFeature::slotCrateTableChanged);
-    connect(m_pTrackCollection,
+    connect(m_pTrackCollection, // renamed, un/locked, toggled AutoDJ source
             &TrackCollection::crateUpdated,
             this,
             &CrateFeature::slotCrateTableChanged);
     connect(m_pTrackCollection,
             &TrackCollection::crateDeleted,
             this,
             &CrateFeature::slotCrateTableChanged);
-    connect(m_pTrackCollection,
+    connect(m_pTrackCollection, // crate tracks hidden, unhidden or purged
             &TrackCollection::crateTracksChanged,
             this,
             &CrateFeature::slotCrateContentChanged);
@@ -723,10 +723,10 @@ void CrateFeature::slotAnalyzeCrate() {
 }
 
 void CrateFeature::slotExportPlaylist() {
-    CrateId crateId = m_crateTableModel.selectedCrate();
+    CrateId crateId = crateIdFromIndex(m_lastRightClickedIndex);
     Crate crate;
     if (m_pTrackCollection->crates().readCrateById(crateId, &crate)) {
-        qDebug() << "Exporting crate" << crate;
+        qDebug() << "Exporting crate" << crateId << crate;
     } else {
         qDebug() << "Failed to export crate" << crateId;
     }
@@ -768,7 +768,7 @@ void CrateFeature::slotExportPlaylist() {
     // Create a new table model since the main one might have an active search.
     QScopedPointer<CrateTableModel> pCrateTableModel(
             new CrateTableModel(this, m_pLibrary->trackCollectionManager()));
-    pCrateTableModel->selectCrate(m_crateTableModel.selectedCrate());
+    pCrateTableModel->selectCrate(crateId);
     pCrateTableModel->select();
 
     if (fileLocation.endsWith(".csv", Qt::CaseInsensitive)) {
@@ -780,8 +780,8 @@ void CrateFeature::slotExportPlaylist() {
         QList<QString> playlistItems;
         int rows = pCrateTableModel->rowCount();
         for (int i = 0; i < rows; ++i) {
-            QModelIndex index = m_crateTableModel.index(i, 0);
-            playlistItems << m_crateTableModel.getTrackLocation(index);
+            QModelIndex index = pCrateTableModel->index(i, 0);
+            playlistItems << pCrateTableModel->getTrackLocation(index);
         }
         exportPlaylistItemsIntoFile(
                 fileLocation,
@@ -810,6 +810,7 @@ void CrateFeature::slotExportTrackFiles() {
 
 void CrateFeature::storePrevSiblingCrateId(CrateId crateId) {
     QModelIndex actIndex = indexFromCrateId(crateId);
+    m_prevSiblingCrate = CrateId();
     for (int i = (actIndex.row() + 1); i >= (actIndex.row() - 1); i -= 2) {
         QModelIndex newIndex = actIndex.sibling(i, actIndex.column());
         if (newIndex.isValid()) {

src/library/trackset/setlogfeature.cpp

@@ -66,14 +66,8 @@ SetlogFeature::SetlogFeature(
 }
 
 SetlogFeature::~SetlogFeature() {
-    // If the history playlist we created doesn't have any tracks in it then
-    // delete it so we don't end up with tons of empty playlists. This is mostly
-    // for developers since they regularly open Mixxx without loading a track.
-    if (m_playlistId != kInvalidPlaylistId &&
-            m_playlistDao.tracksInPlaylist(m_playlistId) == 0) {
-        m_playlistDao.deletePlaylist(m_playlistId);
-    }
-    // Also clean history up when shutting down in case the track threshold changed
+    // Clean up history when shutting down in case the track threshold changed,
+    // incl. potentially empty current playlist
     deleteAllUnlockedPlaylistsWithFewerTracks();
 }
 
@@ -212,6 +206,8 @@ QModelIndex SetlogFeature::constructChildModel(int selectedId) {
                         .toDateTime();
 
         // Create the TreeItem whose parent is the invisible root item
+        // Show only [kNumToplevelHistoryEntries -1] recent playlists at the top
+        // level before grouping them by year.
         if (row >= kNumToplevelHistoryEntries) {
             int yearCreated = dateCreated.date().year();
 
@@ -280,6 +276,7 @@ void SetlogFeature::decorateChild(TreeItem* item, int playlistId) {
     }
 }
 
+/// Invoked on startup to create new current playlist and by "Finish current and start new"
 void SetlogFeature::slotGetNewPlaylist() {
     //qDebug() << "slotGetNewPlaylist() successfully triggered !";
 

src/widget/wlibrarysidebar.cpp

@@ -173,7 +173,7 @@ void WLibrarySidebar::dropEvent(QDropEvent * event) {
 }
 
 void WLibrarySidebar::toggleSelectedItem() {
-    QModelIndexList selectedIndices = this->selectionModel()->selectedRows();
+    QModelIndexList selectedIndices = selectionModel()->selectedRows();
     if (selectedIndices.size() > 0) {
         QModelIndex index = selectedIndices.at(0);
         // Activate the item so its content shows in the main library.
@@ -184,7 +184,7 @@ void WLibrarySidebar::toggleSelectedItem() {
 }
 
 bool WLibrarySidebar::isLeafNodeSelected() {
-    QModelIndexList selectedIndices = this->selectionModel()->selectedRows();
+    QModelIndexList selectedIndices = selectionModel()->selectedRows();
     if (selectedIndices.size() > 0) {
         QModelIndex index = selectedIndices.at(0);
         if(!index.model()->hasChildren(index)) {
@@ -198,6 +198,8 @@ bool WLibrarySidebar::isLeafNodeSelected() {
     return false;
 }
 
+/// Invoked by actual keypresses (requires widget focus) and emulated keypresses
+/// sent by LibraryControl
 void WLibrarySidebar::keyPressEvent(QKeyEvent* event) {
     switch (event->key()) {
     case Qt::Key_Return:
@@ -254,39 +256,39 @@ void WLibrarySidebar::keyPressEvent(QKeyEvent* event) {
         emit setLibraryFocus(FocusWidget::TracksTable);
         return;
     case kRenameSidebarItemShortcutKey: { // F2
-        // Rename crate or playlist (internal, external, history
+        // Rename crate or playlist (internal, external, history)
         QModelIndexList selectedIndices = selectionModel()->selectedRows();
         if (selectedIndices.isEmpty()) {
             return;
         }
-        // If an expanded item is selected let QTreeView collapse it
         QModelIndex selIndex = selectedIndices.first();
         VERIFY_OR_DEBUG_ASSERT(selIndex.isValid()) {
             qDebug() << "invalid sidebar index";
             return;
         }
         if (isExpanded(selIndex)) {
+            // Root views / knots can not be renamed
             return;
         }
         emit renameItem(selIndex);
         return;
     }
-    case kHideRemoveShortcutKey: { // Del / Cmd+Backspace deletes items
-        // Rename crate or playlist (internal, external, history
+    case kHideRemoveShortcutKey: { // Del (macOS: Cmd+Backspace)
+        // Delete crate or playlist (internal, external, history)
         if (event->modifiers() != kHideRemoveShortcutModifier) {
             return;
         }
         QModelIndexList selectedIndices = selectionModel()->selectedRows();
         if (selectedIndices.isEmpty()) {
             return;
         }
-        // If an expanded item is selected let QTreeView collapse it
         QModelIndex selIndex = selectedIndices.first();
         VERIFY_OR_DEBUG_ASSERT(selIndex.isValid()) {
             qDebug() << "invalid sidebar index";
             return;
         }
         if (isExpanded(selIndex)) {
+            // Root views / knots can not be deleted
             return;
         }
         emit deleteItem(selIndex);