From 162b9a59bd366589020da75b54c1d3ba2a39534d Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sat, 18 Apr 2020 21:10:08 +0200 Subject: [PATCH 01/21] Do NOT load tracks while collecting indices for TrackModel --- src/widget/wtrackmenu.cpp | 16 +++++----------- src/widget/wtrackmenu.h | 7 +++---- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index 4312ca939b0..d7328b35672 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -602,18 +602,12 @@ void WTrackMenu::loadTracks(QModelIndexList indexList) { return; } - QModelIndexList indices; - indices.reserve(indexList.size()); + m_trackIndexList.reserve(indexList.size()); for (const auto& index : indexList) { - TrackPointer pTrack = m_pTrackModel->getTrack(index); - // Checking if passed indexList is valid - if (pTrack) { - indices.push_back(index); - } + m_trackIndexList.push_back(index); } - m_pTrackIndexList = indices; - if (!m_pTrackIndexList.empty()) { + if (!m_trackIndexList.empty()) { updateMenus(); } } @@ -668,7 +662,7 @@ QModelIndexList WTrackMenu::getTrackIndices() const { // Indices are associated with a TrackModel. Can only be obtained // if a TrackModel is available. DEBUG_ASSERT(m_pTrackModel); - return m_pTrackIndexList; + return m_trackIndexList; } void WTrackMenu::slotOpenInFileBrowser() { @@ -1187,7 +1181,7 @@ void WTrackMenu::slotPurge() { void WTrackMenu::clearTrackSelection() { m_pTrackPointerList.clear(); - m_pTrackIndexList.clear(); + m_trackIndexList.clear(); } bool WTrackMenu::featureIsEnabled(Feature flag) const { diff --git a/src/widget/wtrackmenu.h b/src/widget/wtrackmenu.h index 8d45d870878..8dacc482dd7 100644 --- a/src/widget/wtrackmenu.h +++ b/src/widget/wtrackmenu.h @@ -142,12 +142,11 @@ class WTrackMenu : public QMenu { void loadSelectionToGroup(QString group, bool play = false); void clearTrackSelection(); + TrackModel* m_pTrackModel{}; + QModelIndexList m_trackIndexList; + // Source of track list when TrackModel is not set. TrackPointerList m_pTrackPointerList; - // Source of track list when TrackModel is set. - QModelIndexList m_pTrackIndexList; - - TrackModel* m_pTrackModel{}; const ControlProxy* m_pNumSamplers{}; const ControlProxy* m_pNumDecks{}; From 111b56e889f981425609a2a0a585631f69c0346b Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sat, 18 Apr 2020 21:22:15 +0200 Subject: [PATCH 02/21] Separate different uses cases in WTrackMenu --- src/widget/wtrackmenu.cpp | 78 ++++++++++++++-------------------- src/widget/wtrackmenu.h | 42 ++++++++++-------- src/widget/wtrackproperty.cpp | 2 +- src/widget/wtracktableview.cpp | 2 +- src/widget/wtracktext.cpp | 2 +- 5 files changed, 60 insertions(+), 66 deletions(-) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index d7328b35672..f3ee2999c6a 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -567,34 +567,27 @@ void WTrackMenu::updateMenus() { } } -void WTrackMenu::loadTracks(TrackIdList trackIdList) { - // Clean all forms of track store - clearTrackSelection(); - +void WTrackMenu::loadTrack( + const TrackPointer& pTrack) { // This asserts that this function is only accessible when a track model is not set, // thus maintaining only the TrackPointerList in state and avoiding storing // duplicate state with TrackIdList and QModelIndexList. - DEBUG_ASSERT(!m_pTrackModel); - - TrackPointerList trackPointers; - trackPointers.reserve(trackIdList.size()); - for (const auto& trackId : trackIdList) { - TrackPointer pTrack = m_pTrackCollectionManager->internalCollection()->getTrackById(trackId); - if (pTrack) { - trackPointers.push_back(pTrack); - } - } - m_pTrackPointerList = trackPointers; - - if (!m_pTrackPointerList.empty()) { - updateMenus(); + VERIFY_OR_DEBUG_ASSERT(!m_pTrackModel) { + return; } -} -void WTrackMenu::loadTracks(QModelIndexList indexList) { // Clean all forms of track store clearTrackSelection(); + if (!pTrack) { + return; + } + m_trackPointerList = TrackPointerList{pTrack}; + updateMenus(); +} + +void WTrackMenu::loadTrackModelIndices( + const QModelIndexList& trackIndexList) { // This asserts that this function is only accessible when a track model is set, // thus maintaining only the QModelIndexList in state and avoiding storing // duplicate state with TrackIdList and TrackPointerList. @@ -602,38 +595,30 @@ void WTrackMenu::loadTracks(QModelIndexList indexList) { return; } - m_trackIndexList.reserve(indexList.size()); - for (const auto& index : indexList) { - m_trackIndexList.push_back(index); - } + // Clean all forms of track store + clearTrackSelection(); + m_trackIndexList = trackIndexList; if (!m_trackIndexList.empty()) { updateMenus(); } } -void WTrackMenu::loadTrack(TrackId trackId) { - loadTracks(TrackIdList{trackId}); -} - -void WTrackMenu::loadTrack(QModelIndex index) { - loadTracks(QModelIndexList{index}); -} - TrackIdList WTrackMenu::getTrackIds() const { TrackIdList trackIds; if (m_pTrackModel) { - const QModelIndexList indices = getTrackIndices(); - trackIds.reserve(indices.size()); - for (const auto& index : indices) { + trackIds.reserve(m_trackIndexList.size()); + for (const auto& index : m_trackIndexList) { const auto trackId = m_pTrackModel->getTrackId(index); - if (trackId.isValid()) { - trackIds.push_back(trackId); + if (!trackId.isValid()) { + // Skip unavailable tracks + continue; } + trackIds.push_back(trackId); } } else { - trackIds.reserve(m_pTrackPointerList.size()); - for (const auto& pTrack : m_pTrackPointerList) { + trackIds.reserve(m_trackPointerList.size()); + for (const auto& pTrack : m_trackPointerList) { const auto trackId = pTrack->getId(); DEBUG_ASSERT(trackId.isValid()); trackIds.push_back(pTrack->getId()); @@ -644,16 +629,17 @@ TrackIdList WTrackMenu::getTrackIds() const { TrackPointerList WTrackMenu::getTrackPointers() const { if (!m_pTrackModel) { - return m_pTrackPointerList; + return m_trackPointerList; } - const QModelIndexList indices = getTrackIndices(); TrackPointerList trackPointers; - trackPointers.reserve(indices.size()); - for (const auto& index : indices) { + trackPointers.reserve(m_trackIndexList.size()); + for (const auto& index : m_trackIndexList) { const auto pTrack = m_pTrackModel->getTrack(index); - if (pTrack) { - trackPointers.push_back(pTrack); + if (!pTrack) { + // Skip unavailable tracks + continue; } + trackPointers.push_back(pTrack); } return trackPointers; } @@ -1180,7 +1166,7 @@ void WTrackMenu::slotPurge() { } void WTrackMenu::clearTrackSelection() { - m_pTrackPointerList.clear(); + m_trackPointerList.clear(); m_trackIndexList.clear(); } diff --git a/src/widget/wtrackmenu.h b/src/widget/wtrackmenu.h index 8dacc482dd7..f0664eb75a3 100644 --- a/src/widget/wtrackmenu.h +++ b/src/widget/wtrackmenu.h @@ -52,10 +52,15 @@ class WTrackMenu : public QMenu { TrackModel* trackModel = nullptr); ~WTrackMenu() override; - void loadTrack(TrackId trackId); - void loadTrack(QModelIndex index); - void loadTracks(TrackIdList trackList); - void loadTracks(QModelIndexList indexList); + void loadTrackModelIndex( + const QModelIndex& trackIndex) { + loadTrackModelIndices(QModelIndexList{trackIndex}); + } + void loadTrackModelIndices( + const QModelIndexList& trackIndexList); + + void loadTrack( + const TrackPointer& pTrack); // WARNING: This function hides non-virtual QMenu::popup(). // This has been done on purpose to ensure menu doesn't popup without loaded track(s). @@ -117,13 +122,16 @@ class WTrackMenu : public QMenu { void slotPurge(); private: - // These getters make sure the required lists are - // derived from a single source in state, which is the - // m_pTrackIndexList if m_pTrackModel is set and - // m_pTrackPointerList if m_pTrackModel is not set. + // This getter verifies that m_pTrackModel is set when + // invoked. + QModelIndexList getTrackIndices() const; + TrackIdList getTrackIds() const; + + // TODO: This function desperately needs to be replaced + // by an iterator pattern that loads (and drops) tracks + // lazily one-by-one during the traversal!! TrackPointerList getTrackPointers() const; - QModelIndexList getTrackIndices() const; void createMenus(); void createActions(); @@ -146,7 +154,7 @@ class WTrackMenu : public QMenu { QModelIndexList m_trackIndexList; // Source of track list when TrackModel is not set. - TrackPointerList m_pTrackPointerList; + TrackPointerList m_trackPointerList; const ControlProxy* m_pNumSamplers{}; const ControlProxy* m_pNumDecks{}; @@ -229,14 +237,14 @@ class WTrackMenu : public QMenu { struct UpdateExternalTrackCollection { QPointer externalTrackCollection; QAction* action{}; - }; - QList m_updateInExternalTrackCollections; + }; + QList m_updateInExternalTrackCollections; - bool m_bPlaylistMenuLoaded; - bool m_bCrateMenuLoaded; + bool m_bPlaylistMenuLoaded; + bool m_bCrateMenuLoaded; - Features m_eActiveFeatures; - const Features m_eTrackModelFeatures; -}; + Features m_eActiveFeatures; + const Features m_eTrackModelFeatures; + }; Q_DECLARE_OPERATORS_FOR_FLAGS(WTrackMenu::Features) diff --git a/src/widget/wtrackproperty.cpp b/src/widget/wtrackproperty.cpp index d1e1cdfcf67..42742c9cf2b 100644 --- a/src/widget/wtrackproperty.cpp +++ b/src/widget/wtrackproperty.cpp @@ -93,7 +93,7 @@ void WTrackProperty::dropEvent(QDropEvent *event) { void WTrackProperty::contextMenuEvent(QContextMenuEvent *event) { if (m_pCurrentTrack) { - m_pTrackMenu->loadTrack(m_pCurrentTrack->getId()); + m_pTrackMenu->loadTrack(m_pCurrentTrack); // Create the right-click menu m_pTrackMenu->popup(event->globalPos()); } diff --git a/src/widget/wtracktableview.cpp b/src/widget/wtracktableview.cpp index c50b6e56cf5..334df0fc5f4 100644 --- a/src/widget/wtracktableview.cpp +++ b/src/widget/wtracktableview.cpp @@ -377,7 +377,7 @@ void WTrackTableView::contextMenuEvent(QContextMenuEvent* event) { } // Update track indices in context menu QModelIndexList indices = selectionModel()->selectedRows(); - m_pTrackMenu->loadTracks(indices); + m_pTrackMenu->loadTrackModelIndices(indices); //Create the right-click menu m_pTrackMenu->popup(event->globalPos()); diff --git a/src/widget/wtracktext.cpp b/src/widget/wtracktext.cpp index edf135a4ee6..dc336433139 100644 --- a/src/widget/wtracktext.cpp +++ b/src/widget/wtracktext.cpp @@ -85,7 +85,7 @@ void WTrackText::dropEvent(QDropEvent *event) { void WTrackText::contextMenuEvent(QContextMenuEvent* event) { if (m_pCurrentTrack) { - m_pTrackMenu->loadTrack(m_pCurrentTrack->getId()); + m_pTrackMenu->loadTrack(m_pCurrentTrack); // Create the right-click menu m_pTrackMenu->popup(event->globalPos()); } From 0f7f95c2178fbae67232ef532d838e4eced3c03c Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sat, 18 Apr 2020 22:23:42 +0200 Subject: [PATCH 03/21] Remove redundant checks, remove temporary variables, streamline loops --- src/widget/wtrackmenu.cpp | 137 +++++++++++++++----------------------- 1 file changed, 54 insertions(+), 83 deletions(-) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index f3ee2999c6a..68394dea94b 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -652,42 +652,35 @@ QModelIndexList WTrackMenu::getTrackIndices() const { } void WTrackMenu::slotOpenInFileBrowser() { - TrackPointerList trackPointerList = getTrackPointers(); + const auto trackPointers = getTrackPointers(); QStringList locations; - for (const TrackPointer& trackPointer : trackPointerList) { - locations << trackPointer->getLocation(); + locations.reserve(trackPointers.size()); + for (const auto& pTrack : trackPointers) { + locations << pTrack->getLocation(); } mixxx::DesktopHelper::openInFileBrowser(locations); } void WTrackMenu::slotImportTrackMetadataFromFileTags() { - auto trackPointers = getTrackPointers(); - - for (const auto& pTrack : trackPointers) { - if (pTrack) { - // The user has explicitly requested to reload metadata from the file - // to override the information within Mixxx! Custom cover art must be - // reloaded separately. - SoundSourceProxy(pTrack).updateTrackFromSource( - SoundSourceProxy::ImportTrackMetadataMode::Again); - } + for (const auto& pTrack : getTrackPointers()) { + // The user has explicitly requested to reload metadata from the file + // to override the information within Mixxx! Custom cover art must be + // reloaded separately. + SoundSourceProxy(pTrack).updateTrackFromSource( + SoundSourceProxy::ImportTrackMetadataMode::Again); } } void WTrackMenu::slotExportTrackMetadataIntoFileTags() { - auto trackPointers = getTrackPointers(); - mixxx::DlgTrackMetadataExport::showMessageBoxOncePerSession(); - for (const auto& pTrack : trackPointers) { - if (pTrack) { - // Export of metadata is deferred until all references to the - // corresponding track object have been dropped. Otherwise - // writing to files that are still used for playback might - // cause crashes or at least audible glitches! - mixxx::DlgTrackMetadataExport::showMessageBoxOncePerSession(); - pTrack->markForMetadataExport(); - } + for (const auto& pTrack : getTrackPointers()) { + // Export of metadata is deferred until all references to the + // corresponding track object have been dropped. Otherwise + // writing to files that are still used for playback might + // cause crashes or at least audible glitches! + mixxx::DlgTrackMetadataExport::showMessageBoxOncePerSession(); + pTrack->markForMetadataExport(); } } @@ -916,29 +909,28 @@ void WTrackMenu::slotUnlockBpm() { } void WTrackMenu::slotScaleBpm(int scale) { - const TrackPointerList trackPointers = getTrackPointers(); - for (const auto& pTrack : trackPointers) { - if (!pTrack->isBpmLocked()) { - BeatsPointer pBeats = pTrack->getBeats(); - if (pBeats) { - pBeats->scale(static_cast(scale)); - } + for (const auto& pTrack : getTrackPointers()) { + if (pTrack->isBpmLocked()) { + continue; + } + BeatsPointer pBeats = pTrack->getBeats(); + if (!pBeats) { + continue; } + pBeats->scale(static_cast(scale)); } } void WTrackMenu::lockBpm(bool lock) { - const TrackPointerList trackPointers = getTrackPointers(); // TODO: This should be done in a thread for large selections - for (const auto& pTrack : trackPointers) { + for (const auto& pTrack : getTrackPointers()) { pTrack->setBpmLocked(lock); } } void WTrackMenu::slotColorPicked(mixxx::RgbColor::optional_t color) { - const TrackPointerList trackPointers = getTrackPointers(); // TODO: This should be done in a thread for large selections - for (const auto& pTrack : trackPointers) { + for (const auto& pTrack : getTrackPointers()) { pTrack->setColor(color); } @@ -972,61 +964,54 @@ void WTrackMenu::loadSelectionToGroup(QString group, bool play) { //slot for reset played count, sets count to 0 of one or more tracks void WTrackMenu::slotClearPlayCount() { - const TrackPointerList trackPointers = getTrackPointers(); - for (const auto& pTrack : trackPointers) { + for (const auto& pTrack : getTrackPointers()) { pTrack->resetPlayCounter(); } } void WTrackMenu::slotClearBeats() { - const TrackPointerList trackPointers = getTrackPointers(); // TODO: This should be done in a thread for large selections - for (const auto& pTrack : trackPointers) { - if (!pTrack->isBpmLocked()) { - pTrack->setBeats(BeatsPointer()); + for (const auto& pTrack : getTrackPointers()) { + if (pTrack->isBpmLocked()) { + continue; } + pTrack->setBeats(BeatsPointer()); } } void WTrackMenu::slotClearMainCue() { - const TrackPointerList trackPointers = getTrackPointers(); - for (const auto& pTrack : trackPointers) { + for (const auto& pTrack : getTrackPointers()) { pTrack->removeCuesOfType(mixxx::CueType::MainCue); } } void WTrackMenu::slotClearOutroCue() { - const TrackPointerList trackPointers = getTrackPointers(); - for (const auto& pTrack : trackPointers) { + for (const auto& pTrack : getTrackPointers()) { pTrack->removeCuesOfType(mixxx::CueType::Outro); } } void WTrackMenu::slotClearIntroCue() { - const TrackPointerList trackPointers = getTrackPointers(); - for (const auto& pTrack : trackPointers) { + for (const auto& pTrack : getTrackPointers()) { pTrack->removeCuesOfType(mixxx::CueType::Intro); } } void WTrackMenu::slotClearKey() { - const TrackPointerList trackPointers = getTrackPointers(); - for (const auto& pTrack : trackPointers) { + for (const auto& pTrack : getTrackPointers()) { pTrack->resetKeys(); } } void WTrackMenu::slotClearReplayGain() { - const TrackPointerList trackPointers = getTrackPointers(); - for (const auto& pTrack : trackPointers) { + for (const auto& pTrack : getTrackPointers()) { pTrack->setReplayGain(mixxx::ReplayGain()); } } void WTrackMenu::slotClearWaveform() { - const TrackPointerList trackPointers = getTrackPointers(); AnalysisDao& analysisDao = m_pTrackCollectionManager->internalCollection()->getAnalysisDAO(); - for (const auto& pTrack : trackPointers) { + for (const auto& pTrack : getTrackPointers()) { analysisDao.deleteAnalysesForTrack(pTrack->getId()); pTrack->setWaveform(WaveformPointer()); pTrack->setWaveformSummary(WaveformPointer()); @@ -1034,15 +1019,13 @@ void WTrackMenu::slotClearWaveform() { } void WTrackMenu::slotClearLoop() { - const TrackPointerList trackPointers = getTrackPointers(); - for (const auto& pTrack : trackPointers) { + for (const auto& pTrack : getTrackPointers()) { pTrack->removeCuesOfType(mixxx::CueType::Loop); } } void WTrackMenu::slotClearHotCues() { - const TrackPointerList trackPointers = getTrackPointers(); - for (const auto& pTrack : trackPointers) { + for (const auto& pTrack : getTrackPointers()) { pTrack->removeCuesOfType(mixxx::CueType::HotCue); } } @@ -1116,53 +1099,41 @@ void WTrackMenu::addToAutoDJ(PlaylistDAO::AutoDJSendLoc loc) { } void WTrackMenu::slotCoverInfoSelected(const CoverInfoRelative& coverInfo) { - const TrackPointerList trackPointers = getTrackPointers(); - for (const auto& pTrack : trackPointers) { + for (const auto& pTrack : getTrackPointers()) { pTrack->setCoverInfo(coverInfo); } } void WTrackMenu::slotReloadCoverArt() { - const TrackPointerList trackPointers = getTrackPointers(); - if (!trackPointers.empty()) { - guessTrackCoverInfoConcurrently(trackPointers); - } + guessTrackCoverInfoConcurrently(getTrackPointers()); } void WTrackMenu::slotRemove() { - if (m_pTrackModel) { - const QModelIndexList indices = getTrackIndices(); - if (!indices.empty()) { - m_pTrackModel->removeTracks(indices); - } + if (!m_pTrackModel) { + return; } + m_pTrackModel->removeTracks(getTrackIndices()); } void WTrackMenu::slotHide() { - if (m_pTrackModel) { - const QModelIndexList indices = getTrackIndices(); - if (!indices.empty()) { - m_pTrackModel->hideTracks(indices); - } + if (!m_pTrackModel) { + return; } + m_pTrackModel->hideTracks(getTrackIndices()); } void WTrackMenu::slotUnhide() { - if (m_pTrackModel) { - const QModelIndexList indices = getTrackIndices(); - if (!indices.empty()) { - m_pTrackModel->unhideTracks(indices); - } + if (!m_pTrackModel) { + return; } + m_pTrackModel->unhideTracks(getTrackIndices()); } void WTrackMenu::slotPurge() { - if (m_pTrackModel) { - const QModelIndexList indices = getTrackIndices(); - if (!indices.empty()) { - m_pTrackModel->purgeTracks(indices); - } + if (!m_pTrackModel) { + return; } + m_pTrackModel->purgeTracks(getTrackIndices()); } void WTrackMenu::clearTrackSelection() { From 42897378fd0e967971f8259367540cd471470915 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sat, 18 Apr 2020 22:24:28 +0200 Subject: [PATCH 04/21] Add a fast check for empty selections, avoid loading tracks --- src/widget/wtrackmenu.cpp | 10 +++++++++- src/widget/wtrackmenu.h | 2 ++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index 68394dea94b..af28823a02b 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -67,8 +67,16 @@ WTrackMenu::~WTrackMenu() { // forward declared in the header file. } +bool WTrackMenu::isEmpty() const { + if (m_pTrackModel) { + return m_trackIndexList.isEmpty(); + } else { + return m_trackPointerList.isEmpty(); + } +} + void WTrackMenu::popup(const QPoint& pos, QAction* at) { - if (getTrackPointers().empty()) { + if (isEmpty()) { return; } QMenu::popup(pos, at); diff --git a/src/widget/wtrackmenu.h b/src/widget/wtrackmenu.h index f0664eb75a3..4d4f3f16201 100644 --- a/src/widget/wtrackmenu.h +++ b/src/widget/wtrackmenu.h @@ -133,6 +133,8 @@ class WTrackMenu : public QMenu { // lazily one-by-one during the traversal!! TrackPointerList getTrackPointers() const; + bool isEmpty() const; + void createMenus(); void createActions(); void setupActions(); From f07b73fe3402a03bcd2d085b25f2550b4f1d0678 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sat, 18 Apr 2020 22:25:04 +0200 Subject: [PATCH 05/21] Only load the minimum number of tracks --- src/widget/wtrackmenu.cpp | 10 ++++++++-- src/widget/wtrackmenu.h | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index af28823a02b..77948771915 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -635,13 +635,19 @@ TrackIdList WTrackMenu::getTrackIds() const { return trackIds; } -TrackPointerList WTrackMenu::getTrackPointers() const { +TrackPointerList WTrackMenu::getTrackPointers( + int maxSize) const { if (!m_pTrackModel) { return m_trackPointerList; } TrackPointerList trackPointers; trackPointers.reserve(m_trackIndexList.size()); for (const auto& index : m_trackIndexList) { + DEBUG_ASSERT(maxSize < 0 || + maxSize <= trackPointers.size()); + if (maxSize >= 0 && maxSize <= trackPointers.size()) { + return trackPointers; + } const auto pTrack = m_pTrackModel->getTrack(index); if (!pTrack) { // Skip unavailable tracks @@ -946,7 +952,7 @@ void WTrackMenu::slotColorPicked(mixxx::RgbColor::optional_t color) { } void WTrackMenu::loadSelectionToGroup(QString group, bool play) { - const TrackPointerList trackPointers = getTrackPointers(); + const auto trackPointers = getTrackPointers(1); if (trackPointers.empty()) { return; } diff --git a/src/widget/wtrackmenu.h b/src/widget/wtrackmenu.h index 4d4f3f16201..d03c7bc6c10 100644 --- a/src/widget/wtrackmenu.h +++ b/src/widget/wtrackmenu.h @@ -131,7 +131,7 @@ class WTrackMenu : public QMenu { // TODO: This function desperately needs to be replaced // by an iterator pattern that loads (and drops) tracks // lazily one-by-one during the traversal!! - TrackPointerList getTrackPointers() const; + TrackPointerList getTrackPointers(int maxSize = -1) const; bool isEmpty() const; From 4e0a23c5d233606ed2fce9978dbc65c3335ca19a Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sat, 18 Apr 2020 22:26:25 +0200 Subject: [PATCH 06/21] Avoid loading unused tracks --- src/widget/wtrackmenu.cpp | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index 77948771915..5b29a3ae574 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -1058,29 +1058,31 @@ void WTrackMenu::slotClearAllMetadata() { } void WTrackMenu::slotShowTrackInfo() { - const auto trackPointers = getTrackPointers(); - if (trackPointers.empty()) { - return; - } if (m_pTrackModel) { - const auto indices = getTrackIndices(); - m_pTrackInfo->loadTrack(indices.at(0)); + if (m_trackIndexList.isEmpty()) { + return; + } + m_pTrackInfo->loadTrack(m_trackIndexList.at(0)); } else { - m_pTrackInfo->loadTrack(trackPointers.at(0)); + if (m_trackPointerList.isEmpty()) { + return; + } + m_pTrackInfo->loadTrack(m_trackPointerList.at(0)); } m_pTrackInfo->show(); } void WTrackMenu::slotShowDlgTagFetcher() { - const auto trackPointers = getTrackPointers(); - if (trackPointers.empty()) { - return; - } if (m_pTrackModel) { - const auto indices = getTrackIndices(); - m_pTagFetcher->loadTrack(indices.at(0)); + if (m_trackIndexList.isEmpty()) { + return; + } + m_pTagFetcher->loadTrack(m_trackIndexList.at(0)); } else { - m_pTagFetcher->loadTrack(trackPointers.at(0)); + if (m_trackPointerList.isEmpty()) { + return; + } + m_pTagFetcher->loadTrack(m_trackPointerList.at(0)); } m_pTagFetcher->show(); } From c26e23dfd1d54cdaf0fda19bdaa804447cb8af56 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 19 Apr 2020 01:24:34 +0200 Subject: [PATCH 07/21] Clean up and fix WTrackMenu::updateMenus() --- src/widget/wtrackmenu.cpp | 51 +++++++++++++++++---------------------- src/widget/wtrackmenu.h | 5 +++- 2 files changed, 26 insertions(+), 30 deletions(-) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index 5b29a3ae574..0ddb9af481e 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -67,11 +67,11 @@ WTrackMenu::~WTrackMenu() { // forward declared in the header file. } -bool WTrackMenu::isEmpty() const { +int WTrackMenu::getTrackCount() const { if (m_pTrackModel) { - return m_trackIndexList.isEmpty(); + return m_trackIndexList.size(); } else { - return m_trackPointerList.isEmpty(); + return m_trackPointerList.size(); } } @@ -417,10 +417,12 @@ void WTrackMenu::setupActions() { void WTrackMenu::updateMenus() { const auto trackIds = getTrackIds(); + // TODO: Loading all tracks just by opening the context menu + // menu is a real UX performance killer!! const auto trackPointers = getTrackPointers(); // Gray out some stuff if multiple songs were selected. - const bool singleTrackSelected = trackPointers.size() == 1; + const bool singleTrackSelected = getTrackCount() == 1; if (featureIsEnabled(Feature::LoadTo)) { int iNumDecks = m_pNumDecks->get(); @@ -496,9 +498,10 @@ void WTrackMenu::updateMenus() { if (featureIsEnabled(Feature::Reset)) { bool allowClear = true; - for (int i = 0; i < trackPointers.size() && allowClear; ++i) { - if (trackPointers.at(0)->isBpmLocked()) { + for (const auto& pTrack : trackPointers) { + if (pTrack->isBpmLocked()) { allowClear = false; + break; } } m_pClearBeatsAction->setEnabled(allowClear); @@ -506,30 +509,20 @@ void WTrackMenu::updateMenus() { if (featureIsEnabled(Feature::BPM)) { bool anyLocked = false; //true if any of the selected items are locked - for (int i = 0; i < trackPointers.size() && !anyLocked; ++i) { - if (trackPointers.at(i)->isBpmLocked()) { + for (const auto& pTrack : trackPointers) { + if (pTrack->isBpmLocked()) { anyLocked = true; + break; } } - if (anyLocked) { - m_pBpmUnlockAction->setEnabled(true); - m_pBpmLockAction->setEnabled(false); - m_pBpmDoubleAction->setEnabled(false); - m_pBpmHalveAction->setEnabled(false); - m_pBpmTwoThirdsAction->setEnabled(false); - m_pBpmThreeFourthsAction->setEnabled(false); - m_pBpmFourThirdsAction->setEnabled(false); - m_pBpmThreeHalvesAction->setEnabled(false); - } else { - m_pBpmUnlockAction->setEnabled(false); - m_pBpmLockAction->setEnabled(true); - m_pBpmDoubleAction->setEnabled(true); - m_pBpmHalveAction->setEnabled(true); - m_pBpmTwoThirdsAction->setEnabled(true); - m_pBpmThreeFourthsAction->setEnabled(true); - m_pBpmFourThirdsAction->setEnabled(true); - m_pBpmThreeHalvesAction->setEnabled(true); - } + m_pBpmUnlockAction->setEnabled(anyLocked); + m_pBpmLockAction->setEnabled(!anyLocked); + m_pBpmDoubleAction->setEnabled(!anyLocked); + m_pBpmHalveAction->setEnabled(!anyLocked); + m_pBpmTwoThirdsAction->setEnabled(!anyLocked); + m_pBpmThreeFourthsAction->setEnabled(!anyLocked); + m_pBpmFourThirdsAction->setEnabled(!anyLocked); + m_pBpmThreeHalvesAction->setEnabled(!anyLocked); } if (featureIsEnabled(Feature::Color)) { @@ -542,8 +535,8 @@ void WTrackMenu::updateMenus() { // Check if all other selected tracks have the same color bool multipleTrackColors = false; - for (int i = 1; i < trackPointers.size(); ++i) { - if (trackColor != trackPointers.at(i)->getColor()) { + for (const auto& pTrack : trackPointers) { + if (trackColor != pTrack->getColor()) { trackColor = mixxx::RgbColor::nullopt(); multipleTrackColors = true; break; diff --git a/src/widget/wtrackmenu.h b/src/widget/wtrackmenu.h index d03c7bc6c10..dd0fb23338b 100644 --- a/src/widget/wtrackmenu.h +++ b/src/widget/wtrackmenu.h @@ -133,7 +133,10 @@ class WTrackMenu : public QMenu { // lazily one-by-one during the traversal!! TrackPointerList getTrackPointers(int maxSize = -1) const; - bool isEmpty() const; + bool isEmpty() const { + return getTrackCount() == 0; + } + int getTrackCount() const; void createMenus(); void createActions(); From 204d3f0c49af9acf7bd38d860958e5ae1bd49411 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 19 Apr 2020 10:48:27 +0200 Subject: [PATCH 08/21] Check for emptiness upfront --- src/widget/wtrackmenu.cpp | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index 0ddb9af481e..892fe6539ac 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -1051,30 +1051,24 @@ void WTrackMenu::slotClearAllMetadata() { } void WTrackMenu::slotShowTrackInfo() { + if (isEmpty()) { + return; + } if (m_pTrackModel) { - if (m_trackIndexList.isEmpty()) { - return; - } m_pTrackInfo->loadTrack(m_trackIndexList.at(0)); } else { - if (m_trackPointerList.isEmpty()) { - return; - } m_pTrackInfo->loadTrack(m_trackPointerList.at(0)); } m_pTrackInfo->show(); } void WTrackMenu::slotShowDlgTagFetcher() { + if (isEmpty()) { + return; + } if (m_pTrackModel) { - if (m_trackIndexList.isEmpty()) { - return; - } m_pTagFetcher->loadTrack(m_trackIndexList.at(0)); } else { - if (m_trackPointerList.isEmpty()) { - return; - } m_pTagFetcher->loadTrack(m_trackPointerList.at(0)); } m_pTagFetcher->show(); From 978a52abd35a543ba8c4371b350bf5ded48c4103 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 19 Apr 2020 10:49:55 +0200 Subject: [PATCH 09/21] Extract special case from conditional expression --- src/widget/wtrackmenu.cpp | 86 +++++++++++++++++++-------------------- 1 file changed, 43 insertions(+), 43 deletions(-) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index 892fe6539ac..88c129f658b 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -1150,48 +1150,48 @@ bool WTrackMenu::featureIsEnabled(Feature flag) const { return false; } - if (m_pTrackModel) { - if (flag == Feature::AutoDJ) { - return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_ADDTOAUTODJ); - } else if (flag == Feature::LoadTo) { - return m_pTrackModel->hasCapabilities( - TrackModel::TRACKMODELCAPS_LOADTODECK) || - m_pTrackModel->hasCapabilities( - TrackModel::TRACKMODELCAPS_LOADTOSAMPLER) || - m_pTrackModel->hasCapabilities( - TrackModel::TRACKMODELCAPS_LOADTOPREVIEWDECK); - } else if (flag == Feature::Playlist) { - return m_pTrackModel->hasCapabilities( - TrackModel::TRACKMODELCAPS_ADDTOPLAYLIST); - } else if (flag == Feature::Crate) { - return m_pTrackModel->hasCapabilities( - TrackModel::TRACKMODELCAPS_ADDTOCRATE); - } else if (flag == Feature::Remove) { - return m_pTrackModel->hasCapabilities( - TrackModel::TRACKMODELCAPS_REMOVE) || - m_pTrackModel->hasCapabilities( - TrackModel::TRACKMODELCAPS_REMOVE_PLAYLIST) || - m_pTrackModel->hasCapabilities( - TrackModel::TRACKMODELCAPS_REMOVE_CRATE); - } else if (flag == Feature::Metadata) { - return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_EDITMETADATA); - } else if (flag == Feature::Reset) { - return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_EDITMETADATA) && - m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_RESETPLAYED); - } else if (flag == Feature::BPM) { - return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_EDITMETADATA); - } else if (flag == Feature::Color) { - return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_EDITMETADATA); - } else if (flag == Feature::HideUnhidePurge) { - return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_HIDE) || - m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_UNHIDE) || - m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_PURGE); - } else if (flag == Feature::Properties) { - return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_EDITMETADATA); - } else { - return true; - } + if (!m_pTrackModel) { + return !m_eTrackModelFeatures.testFlag(flag); + } + + if (flag == Feature::AutoDJ) { + return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_ADDTOAUTODJ); + } else if (flag == Feature::LoadTo) { + return m_pTrackModel->hasCapabilities( + TrackModel::TRACKMODELCAPS_LOADTODECK) || + m_pTrackModel->hasCapabilities( + TrackModel::TRACKMODELCAPS_LOADTOSAMPLER) || + m_pTrackModel->hasCapabilities( + TrackModel::TRACKMODELCAPS_LOADTOPREVIEWDECK); + } else if (flag == Feature::Playlist) { + return m_pTrackModel->hasCapabilities( + TrackModel::TRACKMODELCAPS_ADDTOPLAYLIST); + } else if (flag == Feature::Crate) { + return m_pTrackModel->hasCapabilities( + TrackModel::TRACKMODELCAPS_ADDTOCRATE); + } else if (flag == Feature::Remove) { + return m_pTrackModel->hasCapabilities( + TrackModel::TRACKMODELCAPS_REMOVE) || + m_pTrackModel->hasCapabilities( + TrackModel::TRACKMODELCAPS_REMOVE_PLAYLIST) || + m_pTrackModel->hasCapabilities( + TrackModel::TRACKMODELCAPS_REMOVE_CRATE); + } else if (flag == Feature::Metadata) { + return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_EDITMETADATA); + } else if (flag == Feature::Reset) { + return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_EDITMETADATA) && + m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_RESETPLAYED); + } else if (flag == Feature::BPM) { + return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_EDITMETADATA); + } else if (flag == Feature::Color) { + return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_EDITMETADATA); + } else if (flag == Feature::HideUnhidePurge) { + return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_HIDE) || + m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_UNHIDE) || + m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_PURGE); + } else if (flag == Feature::Properties) { + return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_EDITMETADATA); + } else { + return true; } - - return !m_eTrackModelFeatures.testFlag(flag); } From 0a0553aee97edf3904f6ca7bbc9738f65ce37755 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 19 Apr 2020 10:55:05 +0200 Subject: [PATCH 10/21] Replace if/else-if cascade with switch statement --- src/widget/wtrackmenu.cpp | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index 88c129f658b..34a00a3b39b 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -1154,44 +1154,45 @@ bool WTrackMenu::featureIsEnabled(Feature flag) const { return !m_eTrackModelFeatures.testFlag(flag); } - if (flag == Feature::AutoDJ) { + switch (flag) { + case Feature::AutoDJ: return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_ADDTOAUTODJ); - } else if (flag == Feature::LoadTo) { + case Feature::LoadTo: return m_pTrackModel->hasCapabilities( - TrackModel::TRACKMODELCAPS_LOADTODECK) || + TrackModel::TRACKMODELCAPS_LOADTODECK) || m_pTrackModel->hasCapabilities( TrackModel::TRACKMODELCAPS_LOADTOSAMPLER) || m_pTrackModel->hasCapabilities( TrackModel::TRACKMODELCAPS_LOADTOPREVIEWDECK); - } else if (flag == Feature::Playlist) { + case Feature::Playlist: return m_pTrackModel->hasCapabilities( TrackModel::TRACKMODELCAPS_ADDTOPLAYLIST); - } else if (flag == Feature::Crate) { + case Feature::Crate: return m_pTrackModel->hasCapabilities( TrackModel::TRACKMODELCAPS_ADDTOCRATE); - } else if (flag == Feature::Remove) { + case Feature::Remove: return m_pTrackModel->hasCapabilities( - TrackModel::TRACKMODELCAPS_REMOVE) || + TrackModel::TRACKMODELCAPS_REMOVE) || m_pTrackModel->hasCapabilities( TrackModel::TRACKMODELCAPS_REMOVE_PLAYLIST) || m_pTrackModel->hasCapabilities( TrackModel::TRACKMODELCAPS_REMOVE_CRATE); - } else if (flag == Feature::Metadata) { + case Feature::Metadata: return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_EDITMETADATA); - } else if (flag == Feature::Reset) { + case Feature::Reset: return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_EDITMETADATA) && m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_RESETPLAYED); - } else if (flag == Feature::BPM) { + case Feature::BPM: return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_EDITMETADATA); - } else if (flag == Feature::Color) { + case Feature::Color: return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_EDITMETADATA); - } else if (flag == Feature::HideUnhidePurge) { + case Feature::HideUnhidePurge: return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_HIDE) || m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_UNHIDE) || m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_PURGE); - } else if (flag == Feature::Properties) { + case Feature::Properties: return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_EDITMETADATA); - } else { + default: return true; } } From eafc89575341e645b1226a4c96bb16d8952837b5 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 19 Apr 2020 10:55:25 +0200 Subject: [PATCH 11/21] Handle all enum literals in switch statement --- src/widget/wtrackmenu.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index 34a00a3b39b..31db64c9b38 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -1190,9 +1190,12 @@ bool WTrackMenu::featureIsEnabled(Feature flag) const { return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_HIDE) || m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_UNHIDE) || m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_PURGE); + case Feature::FileBrowser: + return true; case Feature::Properties: return m_pTrackModel->hasCapabilities(TrackModel::TRACKMODELCAPS_EDITMETADATA); default: + DEBUG_ASSERT(!"unreachable"); return true; } } From a7bab955789f4124d622921a1ea95dbfffdb4168 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 19 Apr 2020 11:45:32 +0200 Subject: [PATCH 12/21] Fix unused variable warning --- src/widget/wtrackmenu.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index 31db64c9b38..d6234625b85 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -622,7 +622,7 @@ TrackIdList WTrackMenu::getTrackIds() const { for (const auto& pTrack : m_trackPointerList) { const auto trackId = pTrack->getId(); DEBUG_ASSERT(trackId.isValid()); - trackIds.push_back(pTrack->getId()); + trackIds.push_back(trackId); } } return trackIds; From 57447bdafe4b15c512dc95c062eb69ac182fa677 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 19 Apr 2020 14:17:07 +0200 Subject: [PATCH 13/21] Handle empty selection in WTrackMenu::updateMenus() --- src/widget/wtrackmenu.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index d6234625b85..e8821ee92fb 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -416,6 +416,10 @@ void WTrackMenu::setupActions() { } void WTrackMenu::updateMenus() { + if (isEmpty()) { + return; + } + const auto trackIds = getTrackIds(); // TODO: Loading all tracks just by opening the context menu // menu is a real UX performance killer!! @@ -600,9 +604,7 @@ void WTrackMenu::loadTrackModelIndices( clearTrackSelection(); m_trackIndexList = trackIndexList; - if (!m_trackIndexList.empty()) { - updateMenus(); - } + updateMenus(); } TrackIdList WTrackMenu::getTrackIds() const { From 0499b008e34f0375892f45bae56e3971e33d0785 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 19 Apr 2020 14:27:26 +0200 Subject: [PATCH 14/21] Fix update of BPM context menu --- src/widget/wtrackmenu.cpp | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index e8821ee92fb..c122638448b 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -512,21 +512,34 @@ void WTrackMenu::updateMenus() { } if (featureIsEnabled(Feature::BPM)) { - bool anyLocked = false; //true if any of the selected items are locked - for (const auto& pTrack : trackPointers) { - if (pTrack->isBpmLocked()) { - anyLocked = true; - break; + bool anyBpmLocked = false; + if (m_pTrackModel) { + const int bpmLockedCol = + m_pTrackModel->fieldIndex(LIBRARYTABLE_BPM_LOCK); + for (const auto trackIndex : m_trackIndexList) { + QModelIndex bpmLockedIndex = + trackIndex.sibling(trackIndex.row(), bpmLockedCol); + if (bpmLockedIndex.data().toBool()) { + anyBpmLocked = true; + break; + } + } + } else { + for (const auto& pTrack : m_trackPointerList) { + if (pTrack->isBpmLocked()) { + anyBpmLocked = true; + break; + } } } - m_pBpmUnlockAction->setEnabled(anyLocked); - m_pBpmLockAction->setEnabled(!anyLocked); - m_pBpmDoubleAction->setEnabled(!anyLocked); - m_pBpmHalveAction->setEnabled(!anyLocked); - m_pBpmTwoThirdsAction->setEnabled(!anyLocked); - m_pBpmThreeFourthsAction->setEnabled(!anyLocked); - m_pBpmFourThirdsAction->setEnabled(!anyLocked); - m_pBpmThreeHalvesAction->setEnabled(!anyLocked); + m_pBpmUnlockAction->setEnabled(anyBpmLocked); + m_pBpmLockAction->setEnabled(!anyBpmLocked); + m_pBpmDoubleAction->setEnabled(!anyBpmLocked); + m_pBpmHalveAction->setEnabled(!anyBpmLocked); + m_pBpmTwoThirdsAction->setEnabled(!anyBpmLocked); + m_pBpmThreeFourthsAction->setEnabled(!anyBpmLocked); + m_pBpmFourThirdsAction->setEnabled(!anyBpmLocked); + m_pBpmThreeHalvesAction->setEnabled(!anyBpmLocked); } if (featureIsEnabled(Feature::Color)) { From c5436850d678ff7d332c663a536448e8f904b9e7 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 19 Apr 2020 14:47:24 +0200 Subject: [PATCH 15/21] Fix update of cover art context menu --- src/widget/wtrackmenu.cpp | 53 ++++++++++++++++++++++++++++++++++----- 1 file changed, 47 insertions(+), 6 deletions(-) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index c122638448b..0c77a4d0856 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -492,12 +492,53 @@ void WTrackMenu::updateMenus() { if (featureIsEnabled(Feature::Metadata)) { m_pImportMetadataFromMusicBrainzAct->setEnabled(singleTrackSelected); - // We load a single track to get the necessary context for the cover (we use - // last to be consistent with selectionChanged above). - TrackPointer pTrack = trackPointers.last(); - - m_pCoverMenu->setCoverArt( - pTrack->getCoverInfoWithLocation()); + // We use the last selected track for the cover art context to be + // consistent with selectionChanged above. + if (m_pTrackModel) { + const QModelIndex lastIndex = m_trackIndexList.last(); + CoverInfo coverInfo; + coverInfo.source = static_cast( + lastIndex + .sibling( + lastIndex.row(), + m_pTrackModel->fieldIndex(LIBRARYTABLE_COVERART_SOURCE)) + .data() + .toInt()); + coverInfo.type = static_cast( + lastIndex + .sibling( + lastIndex.row(), + m_pTrackModel->fieldIndex(LIBRARYTABLE_COVERART_TYPE)) + .data() + .toInt()); + coverInfo.hash = + lastIndex + .sibling( + lastIndex.row(), + m_pTrackModel->fieldIndex(LIBRARYTABLE_COVERART_HASH)) + .data() + .toUInt(); + coverInfo.coverLocation = + lastIndex + .sibling( + lastIndex.row(), + m_pTrackModel->fieldIndex(LIBRARYTABLE_COVERART_LOCATION)) + .data() + .toString(); + coverInfo.trackLocation = + lastIndex + .sibling( + lastIndex.row(), + m_pTrackModel->fieldIndex(LIBRARYTABLE_LOCATION)) + .data() + .toString(); + m_pCoverMenu->setCoverArt(coverInfo); + } else { + TrackPointer pTrack = m_trackPointerList.last(); + m_pCoverMenu->setCoverArt( + pTrack->getCoverInfoWithLocation()); + } + m_pMetadataMenu->addMenu(m_pCoverMenu); } if (featureIsEnabled(Feature::Reset)) { From 9a5f5a076c65dc679855f46cffe6d355ca01a64d Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 19 Apr 2020 14:55:19 +0200 Subject: [PATCH 16/21] Fix update of Reset context menu --- src/widget/wtrackmenu.cpp | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index 0c77a4d0856..a2dd232e51e 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -541,18 +541,8 @@ void WTrackMenu::updateMenus() { m_pMetadataMenu->addMenu(m_pCoverMenu); } - if (featureIsEnabled(Feature::Reset)) { - bool allowClear = true; - for (const auto& pTrack : trackPointers) { - if (pTrack->isBpmLocked()) { - allowClear = false; - break; - } - } - m_pClearBeatsAction->setEnabled(allowClear); - } - - if (featureIsEnabled(Feature::BPM)) { + if (featureIsEnabled(Feature::Reset) || + featureIsEnabled(Feature::BPM)) { bool anyBpmLocked = false; if (m_pTrackModel) { const int bpmLockedCol = @@ -573,14 +563,19 @@ void WTrackMenu::updateMenus() { } } } - m_pBpmUnlockAction->setEnabled(anyBpmLocked); - m_pBpmLockAction->setEnabled(!anyBpmLocked); - m_pBpmDoubleAction->setEnabled(!anyBpmLocked); - m_pBpmHalveAction->setEnabled(!anyBpmLocked); - m_pBpmTwoThirdsAction->setEnabled(!anyBpmLocked); - m_pBpmThreeFourthsAction->setEnabled(!anyBpmLocked); - m_pBpmFourThirdsAction->setEnabled(!anyBpmLocked); - m_pBpmThreeHalvesAction->setEnabled(!anyBpmLocked); + if (featureIsEnabled(Feature::Reset)) { + m_pClearBeatsAction->setEnabled(!anyBpmLocked); + } + if (featureIsEnabled(Feature::BPM)) { + m_pBpmUnlockAction->setEnabled(anyBpmLocked); + m_pBpmLockAction->setEnabled(!anyBpmLocked); + m_pBpmDoubleAction->setEnabled(!anyBpmLocked); + m_pBpmHalveAction->setEnabled(!anyBpmLocked); + m_pBpmTwoThirdsAction->setEnabled(!anyBpmLocked); + m_pBpmThreeFourthsAction->setEnabled(!anyBpmLocked); + m_pBpmFourThirdsAction->setEnabled(!anyBpmLocked); + m_pBpmThreeHalvesAction->setEnabled(!anyBpmLocked); + } } if (featureIsEnabled(Feature::Color)) { From a289454ae8d15e254879330a6074468bb12ffe93 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sun, 19 Apr 2020 14:56:57 +0200 Subject: [PATCH 17/21] Fix update of track color context menu --- src/widget/wtrackmenu.cpp | 53 +++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 21 deletions(-) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index a2dd232e51e..125d5c97c27 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -420,11 +420,6 @@ void WTrackMenu::updateMenus() { return; } - const auto trackIds = getTrackIds(); - // TODO: Loading all tracks just by opening the context menu - // menu is a real UX performance killer!! - const auto trackPointers = getTrackPointers(); - // Gray out some stuff if multiple songs were selected. const bool singleTrackSelected = getTrackCount() == 1; @@ -581,25 +576,41 @@ void WTrackMenu::updateMenus() { if (featureIsEnabled(Feature::Color)) { m_pColorPickerAction->setColorPalette( ColorPaletteSettings(m_pConfig).getTrackColorPalette()); - - // Get color of first selected track - TrackPointer pTrack = trackPointers.at(0); - auto trackColor = pTrack->getColor(); - - // Check if all other selected tracks have the same color - bool multipleTrackColors = false; - for (const auto& pTrack : trackPointers) { - if (trackColor != pTrack->getColor()) { - trackColor = mixxx::RgbColor::nullopt(); - multipleTrackColors = true; - break; + std::optional oneColor = std::nullopt; + if (m_pTrackModel) { + const int column = + m_pTrackModel->fieldIndex(LIBRARYTABLE_COLOR); + oneColor = mixxx::RgbColor::fromQVariant( + m_trackIndexList.first().sibling( + m_trackIndexList.first().row(), column) + .data()); + if (oneColor) { + for (const auto trackIndex : m_trackIndexList) { + const auto otherColor = mixxx::RgbColor::fromQVariant( + trackIndex.sibling( + trackIndex.row(), column) + .data()); + if (oneColor != otherColor) { + // Multiple, different colors + oneColor = std::nullopt; + break; + } + } + } + } else { + oneColor = m_trackPointerList.first()->getColor(); + for (const auto& pTrack : m_trackPointerList) { + if (oneColor != pTrack->getColor()) { + // Multiple, different colors + oneColor = std::nullopt; + break; + } } } - - if (multipleTrackColors) { - m_pColorPickerAction->resetSelectedColor(); + if (oneColor) { + m_pColorPickerAction->setSelectedColor(oneColor); } else { - m_pColorPickerAction->setSelectedColor(trackColor); + m_pColorPickerAction->resetSelectedColor(); } } From d6025f869a45cb3535da65bb1f769287c058ca5b Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 20 Apr 2020 13:03:35 +0200 Subject: [PATCH 18/21] Extract utility functions from WTrackMenu::updateMenus() --- src/widget/wtrackmenu.cpp | 211 ++++++++++++++++++++------------------ src/widget/wtrackmenu.h | 4 + 2 files changed, 118 insertions(+), 97 deletions(-) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index 125d5c97c27..4f3ea4229e2 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -415,6 +415,115 @@ void WTrackMenu::setupActions() { } } +bool WTrackMenu::isAnyTrackBpmLocked() const { + if (m_pTrackModel) { + const int column = + m_pTrackModel->fieldIndex(LIBRARYTABLE_BPM_LOCK); + for (const auto trackIndex : m_trackIndexList) { + QModelIndex bpmLockedIndex = + trackIndex.sibling(trackIndex.row(), column); + if (bpmLockedIndex.data().toBool()) { + return true; + } + } + } else { + for (const auto& pTrack : m_trackPointerList) { + if (pTrack->isBpmLocked()) { + return true; + } + } + } + return false; +} + +std::optional WTrackMenu::getCommonTrackColor() const { + VERIFY_OR_DEBUG_ASSERT(!isEmpty()) { + return std::nullopt; + } + std::optional commonColor; + if (m_pTrackModel) { + const int column = + m_pTrackModel->fieldIndex(LIBRARYTABLE_COLOR); + commonColor = mixxx::RgbColor::fromQVariant( + m_trackIndexList.first().sibling( + m_trackIndexList.first().row(), column) + .data()); + if (!commonColor) { + return std::nullopt; + } + for (const auto trackIndex : m_trackIndexList) { + const auto otherColor = mixxx::RgbColor::fromQVariant( + trackIndex.sibling( + trackIndex.row(), column) + .data()); + if (commonColor != otherColor) { + // Multiple, different colors + return std::nullopt; + } + } + } else { + auto commonColor = m_trackPointerList.first()->getColor(); + if (!commonColor) { + return std::nullopt; + } + for (const auto& pTrack : m_trackPointerList) { + if (commonColor != pTrack->getColor()) { + // Multiple, different colors + return std::nullopt; + } + } + } + return commonColor; +} + +CoverInfo WTrackMenu::getCoverInfoOfLastTrack() const { + VERIFY_OR_DEBUG_ASSERT(!isEmpty()) { + return CoverInfo(); + } + if (m_pTrackModel) { + const QModelIndex lastIndex = m_trackIndexList.last(); + CoverInfo coverInfo; + coverInfo.source = static_cast( + lastIndex + .sibling( + lastIndex.row(), + m_pTrackModel->fieldIndex(LIBRARYTABLE_COVERART_SOURCE)) + .data() + .toInt()); + coverInfo.type = static_cast( + lastIndex + .sibling( + lastIndex.row(), + m_pTrackModel->fieldIndex(LIBRARYTABLE_COVERART_TYPE)) + .data() + .toInt()); + coverInfo.hash = + lastIndex + .sibling( + lastIndex.row(), + m_pTrackModel->fieldIndex(LIBRARYTABLE_COVERART_HASH)) + .data() + .toUInt(); + coverInfo.coverLocation = + lastIndex + .sibling( + lastIndex.row(), + m_pTrackModel->fieldIndex(LIBRARYTABLE_COVERART_LOCATION)) + .data() + .toString(); + coverInfo.trackLocation = + lastIndex + .sibling( + lastIndex.row(), + m_pTrackModel->fieldIndex(LIBRARYTABLE_LOCATION)) + .data() + .toString(); + return coverInfo; + } else { + return m_trackPointerList.last()->getCoverInfoWithLocation(); + } +} + void WTrackMenu::updateMenus() { if (isEmpty()) { return; @@ -489,75 +598,13 @@ void WTrackMenu::updateMenus() { // We use the last selected track for the cover art context to be // consistent with selectionChanged above. - if (m_pTrackModel) { - const QModelIndex lastIndex = m_trackIndexList.last(); - CoverInfo coverInfo; - coverInfo.source = static_cast( - lastIndex - .sibling( - lastIndex.row(), - m_pTrackModel->fieldIndex(LIBRARYTABLE_COVERART_SOURCE)) - .data() - .toInt()); - coverInfo.type = static_cast( - lastIndex - .sibling( - lastIndex.row(), - m_pTrackModel->fieldIndex(LIBRARYTABLE_COVERART_TYPE)) - .data() - .toInt()); - coverInfo.hash = - lastIndex - .sibling( - lastIndex.row(), - m_pTrackModel->fieldIndex(LIBRARYTABLE_COVERART_HASH)) - .data() - .toUInt(); - coverInfo.coverLocation = - lastIndex - .sibling( - lastIndex.row(), - m_pTrackModel->fieldIndex(LIBRARYTABLE_COVERART_LOCATION)) - .data() - .toString(); - coverInfo.trackLocation = - lastIndex - .sibling( - lastIndex.row(), - m_pTrackModel->fieldIndex(LIBRARYTABLE_LOCATION)) - .data() - .toString(); - m_pCoverMenu->setCoverArt(coverInfo); - } else { - TrackPointer pTrack = m_trackPointerList.last(); - m_pCoverMenu->setCoverArt( - pTrack->getCoverInfoWithLocation()); - } + m_pCoverMenu->setCoverArt(getCoverInfoOfLastTrack()); m_pMetadataMenu->addMenu(m_pCoverMenu); } if (featureIsEnabled(Feature::Reset) || featureIsEnabled(Feature::BPM)) { - bool anyBpmLocked = false; - if (m_pTrackModel) { - const int bpmLockedCol = - m_pTrackModel->fieldIndex(LIBRARYTABLE_BPM_LOCK); - for (const auto trackIndex : m_trackIndexList) { - QModelIndex bpmLockedIndex = - trackIndex.sibling(trackIndex.row(), bpmLockedCol); - if (bpmLockedIndex.data().toBool()) { - anyBpmLocked = true; - break; - } - } - } else { - for (const auto& pTrack : m_trackPointerList) { - if (pTrack->isBpmLocked()) { - anyBpmLocked = true; - break; - } - } - } + const bool anyBpmLocked = isAnyTrackBpmLocked(); if (featureIsEnabled(Feature::Reset)) { m_pClearBeatsAction->setEnabled(!anyBpmLocked); } @@ -576,39 +623,9 @@ void WTrackMenu::updateMenus() { if (featureIsEnabled(Feature::Color)) { m_pColorPickerAction->setColorPalette( ColorPaletteSettings(m_pConfig).getTrackColorPalette()); - std::optional oneColor = std::nullopt; - if (m_pTrackModel) { - const int column = - m_pTrackModel->fieldIndex(LIBRARYTABLE_COLOR); - oneColor = mixxx::RgbColor::fromQVariant( - m_trackIndexList.first().sibling( - m_trackIndexList.first().row(), column) - .data()); - if (oneColor) { - for (const auto trackIndex : m_trackIndexList) { - const auto otherColor = mixxx::RgbColor::fromQVariant( - trackIndex.sibling( - trackIndex.row(), column) - .data()); - if (oneColor != otherColor) { - // Multiple, different colors - oneColor = std::nullopt; - break; - } - } - } - } else { - oneColor = m_trackPointerList.first()->getColor(); - for (const auto& pTrack : m_trackPointerList) { - if (oneColor != pTrack->getColor()) { - // Multiple, different colors - oneColor = std::nullopt; - break; - } - } - } - if (oneColor) { - m_pColorPickerAction->setSelectedColor(oneColor); + const auto commonColor = getCommonTrackColor(); + if (commonColor) { + m_pColorPickerAction->setSelectedColor(commonColor); } else { m_pColorPickerAction->resetSelectedColor(); } diff --git a/src/widget/wtrackmenu.h b/src/widget/wtrackmenu.h index dd0fb23338b..50e6290a9a8 100644 --- a/src/widget/wtrackmenu.h +++ b/src/widget/wtrackmenu.h @@ -155,6 +155,10 @@ class WTrackMenu : public QMenu { void loadSelectionToGroup(QString group, bool play = false); void clearTrackSelection(); + bool isAnyTrackBpmLocked() const; + std::optional getCommonTrackColor() const; + CoverInfo getCoverInfoOfLastTrack() const; + TrackModel* m_pTrackModel{}; QModelIndexList m_trackIndexList; From 529fd9f54597a0ad09a35b36715fc0662049094d Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 20 Apr 2020 23:15:42 +0200 Subject: [PATCH 19/21] Add a debug assertion to emphasize the use case --- src/widget/wtrackmenu.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index 4f3ea4229e2..f609c50d613 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -1028,6 +1028,8 @@ void WTrackMenu::loadSelectionToGroup(QString group, bool play) { if (trackPointers.empty()) { return; } + // Only the first track was requested + DEBUG_ASSERT(trackPointers.size() == 1); // If the track load override is disabled, check to see if a track is // playing before trying to load it if (!(m_pConfig->getValueString( From a1591de011781d211ddc7957666bf8f815461eb3 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 21 Apr 2020 10:36:17 +0200 Subject: [PATCH 20/21] Join and reformat lines # Conflicts: # src/widget/wtrackmenu.cpp --- src/widget/wtrackmenu.cpp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index f609c50d613..bb438ea2b1e 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -445,17 +445,13 @@ std::optional WTrackMenu::getCommonTrackColor() const { const int column = m_pTrackModel->fieldIndex(LIBRARYTABLE_COLOR); commonColor = mixxx::RgbColor::fromQVariant( - m_trackIndexList.first().sibling( - m_trackIndexList.first().row(), column) - .data()); + m_trackIndexList.first().sibling(m_trackIndexList.first().row(), column).data()); if (!commonColor) { return std::nullopt; } for (const auto trackIndex : m_trackIndexList) { const auto otherColor = mixxx::RgbColor::fromQVariant( - trackIndex.sibling( - trackIndex.row(), column) - .data()); + trackIndex.sibling(trackIndex.row(), column).data()); if (commonColor != otherColor) { // Multiple, different colors return std::nullopt; @@ -603,7 +599,7 @@ void WTrackMenu::updateMenus() { } if (featureIsEnabled(Feature::Reset) || - featureIsEnabled(Feature::BPM)) { + featureIsEnabled(Feature::BPM)) { const bool anyBpmLocked = isAnyTrackBpmLocked(); if (featureIsEnabled(Feature::Reset)) { m_pClearBeatsAction->setEnabled(!anyBpmLocked); From bcf4de60026a00a74edd66ba07d4c359a7563297 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 21 Apr 2020 10:46:07 +0200 Subject: [PATCH 21/21] Add reset action to "Adjust BPM" menu --- src/widget/wtrackmenu.cpp | 9 +++++++++ src/widget/wtrackmenu.h | 1 + 2 files changed, 10 insertions(+) diff --git a/src/widget/wtrackmenu.cpp b/src/widget/wtrackmenu.cpp index bb438ea2b1e..9f78ada57b3 100644 --- a/src/widget/wtrackmenu.cpp +++ b/src/widget/wtrackmenu.cpp @@ -268,6 +268,12 @@ void WTrackMenu::createActions() { connect(m_pBpmThreeFourthsAction, &QAction::triggered, this, [this] { slotScaleBpm(Beats::THREEFOURTHS); }); connect(m_pBpmFourThirdsAction, &QAction::triggered, this, [this] { slotScaleBpm(Beats::FOURTHIRDS); }); connect(m_pBpmThreeHalvesAction, &QAction::triggered, this, [this] { slotScaleBpm(Beats::THREEHALVES); }); + + m_pBpmResetAction = new QAction(tr("Reset BPM"), m_pBPMMenu); + connect(m_pBpmResetAction, + &QAction::triggered, + this, + &WTrackMenu::slotClearBeats); } if (featureIsEnabled(Feature::Color)) { @@ -343,6 +349,8 @@ void WTrackMenu::setupActions() { m_pBPMMenu->addAction(m_pBpmLockAction); m_pBPMMenu->addAction(m_pBpmUnlockAction); m_pBPMMenu->addSeparator(); + m_pBPMMenu->addAction(m_pBpmResetAction); + m_pBPMMenu->addSeparator(); addMenu(m_pBPMMenu); } @@ -613,6 +621,7 @@ void WTrackMenu::updateMenus() { m_pBpmThreeFourthsAction->setEnabled(!anyBpmLocked); m_pBpmFourThirdsAction->setEnabled(!anyBpmLocked); m_pBpmThreeHalvesAction->setEnabled(!anyBpmLocked); + m_pBpmResetAction->setEnabled(!anyBpmLocked); } } diff --git a/src/widget/wtrackmenu.h b/src/widget/wtrackmenu.h index 50e6290a9a8..6fba694aa0c 100644 --- a/src/widget/wtrackmenu.h +++ b/src/widget/wtrackmenu.h @@ -220,6 +220,7 @@ class WTrackMenu : public QMenu { QAction* m_pBpmThreeFourthsAction{}; QAction* m_pBpmFourThirdsAction{}; QAction* m_pBpmThreeHalvesAction{}; + QAction* m_pBpmResetAction{}; // Track color WColorPickerAction* m_pColorPickerAction{};