From 022e9ee7be3801c20ddcb41ba0996bb38eb9385c Mon Sep 17 00:00:00 2001 From: ronso0 Date: Tue, 11 Nov 2025 15:23:04 +0100 Subject: [PATCH] BasetrackCache: fix false positive tracks results when using extra filter --- .../analysis/analysislibrarytablemodel.cpp | 21 +++++++-- src/library/basesqltablemodel.cpp | 3 ++ src/library/basetrackcache.cpp | 43 +++++++++++-------- 3 files changed, 45 insertions(+), 22 deletions(-) diff --git a/src/library/analysis/analysislibrarytablemodel.cpp b/src/library/analysis/analysislibrarytablemodel.cpp index 7d9f51ce950c..061281314708 100644 --- a/src/library/analysis/analysislibrarytablemodel.cpp +++ b/src/library/analysis/analysislibrarytablemodel.cpp @@ -1,10 +1,23 @@ #include "library/analysis/analysislibrarytablemodel.h" +#include + #include "moc_analysislibrarytablemodel.cpp" +#include "util/datetime.h" namespace { -const QString RECENT_FILTER = "datetime_added > datetime('now', '-7 days')"; +inline QString recentFilter() { + // Create a user-formatted query equal to SQL query + // datetime_added > datetime('now', '-7 days') + QDateTime dt(QDateTime::currentDateTimeUtc()); + dt = dt.addDays(-7); + const QString dateStr = QLocale::system().toString(dt.date(), QLocale::ShortFormat); + + // FIXME alternatively, use "added:-days" and add the respective + // literal parser to DateAddedFilterNode + return QStringLiteral("added:>%1").arg(dateStr); +} } // anonymous namespace @@ -13,12 +26,12 @@ AnalysisLibraryTableModel::AnalysisLibraryTableModel(QObject* parent, : LibraryTableModel(parent, pTrackCollectionManager, "mixxx.db.model.prepare") { // Default to showing recent tracks. - setSearch("", RECENT_FILTER); + setSearch("", recentFilter()); } void AnalysisLibraryTableModel::showRecentSongs() { // Search with the recent filter. - search(currentSearch(), RECENT_FILTER); + search(currentSearch(), recentFilter()); } void AnalysisLibraryTableModel::showAllSongs() { @@ -27,5 +40,5 @@ void AnalysisLibraryTableModel::showAllSongs() { } void AnalysisLibraryTableModel::searchCurrentTrackSet(const QString& text, bool useRecentFilter) { - search(text, useRecentFilter ? RECENT_FILTER : ""); + search(text, useRecentFilter ? recentFilter() : ""); } diff --git a/src/library/basesqltablemodel.cpp b/src/library/basesqltablemodel.cpp index ee1dfd895286..8a815a1b8c62 100644 --- a/src/library/basesqltablemodel.cpp +++ b/src/library/basesqltablemodel.cpp @@ -422,6 +422,9 @@ void BaseSqlTableModel::setSearch(const QString& searchText, const QString& extr return; } + // Note: don't use SQL strings as extraFilter, this will cause issues in + // BaseTrackCache::filterAndSort() which is responsible for adding/removing + // dirty tracks from the view. m_currentSearch = searchText; m_currentSearchFilter = extraFilter; } diff --git a/src/library/basetrackcache.cpp b/src/library/basetrackcache.cpp index a7e7d9454b4b..a348124e2da6 100644 --- a/src/library/basetrackcache.cpp +++ b/src/library/basetrackcache.cpp @@ -458,30 +458,35 @@ void BaseTrackCache::filterAndSort(const QSet& trackIds, buildIndex(); } + // The id string we need for our QSqlQuery QStringList idStrings; + // The id set we need for removing/adding dirty tracks + QSet ids; // TODO(rryan) consider making this the data passed in and a separate // QVector for output QSet dirtyTracks; - for (const auto& trackId: trackIds) { + for (const auto& trackId : trackIds) { idStrings << trackId.toString(); + ids << trackId; if (m_dirtyTracks.contains(trackId)) { dirtyTracks.insert(trackId); } } - QStringList queryFragments; - if (!extraFilter.isNull() && extraFilter != "") { - queryFragments << QString("(%1)").arg(extraFilter); - } - if (idStrings.size() > 0) { - queryFragments << QString("%1 in (%2)") - .arg(m_idColumn, idStrings.join(",")); + // Note: don't use the extraFilter for m_pQueryParser->parseQuery(), just + // append it to searchQuery if not empty and let the parser construct + // a SQL query from it. + // The issue with the extraFilter is that the parser is assuming the input + // is a SQL string and creates a SqlNode from it, and the issue with that is + // that SqlNode::match(TrackPointer) always returns true -- which leads to + // false positive matches when we iterate over the dirty tracks below. + QString searchPlusExtraFilter = searchQuery; + if (!extraFilter.isEmpty()) { + searchPlusExtraFilter += ' '; + searchPlusExtraFilter += extraFilter; } - const std::unique_ptr pQuery = - m_pQueryParser->parseQuery( - searchQuery, - queryFragments.join(" AND ")); + m_pQueryParser->parseQuery(searchPlusExtraFilter, QString()); QString filter = pQuery->toSql(); if (!filter.isEmpty()) { @@ -539,18 +544,20 @@ void BaseTrackCache::filterAndSort(const QSet& trackIds, } for (TrackId trackId : std::as_const(dirtyTracks)) { - // Only get the track if it is in the cache. Tracks that - // are not cached in memory cannot be dirty. + // Only get the track if it is in the cache. + // Tracks that are not cached in memory cannot be dirty. // Bypass getCachedTrack() to not invalidate m_recentTrackId TrackPointer pTrack = GlobalTrackCacheLocker().lookupTrackById(trackId); if (!pTrack) { continue; } - // The track should be in the result set if the search is empty or the - // track matches the search. - bool shouldBeInResultSet = searchQuery.isEmpty() || - pQuery->match(pTrack); + // The track should be in the result set if + // the search and extra filter are empty + // or + // the track matches the search and ids (if not empty) contains its id + bool shouldBeInResultSet = searchPlusExtraFilter.isEmpty() || + ((ids.isEmpty() || ids.contains(trackId)) && pQuery->match(pTrack)); // If the track is in this result set. bool isInResultSet = trackToIndex->contains(trackId);