Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

On untagged queries use query for crate names. #1388

Merged
merged 7 commits into from Jul 24, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/library/basetrackcache.cpp
Expand Up @@ -42,7 +42,8 @@ BaseTrackCache::BaseTrackCache(TrackCollection* pTrackCollection,
<< "grouping"
<< "comment"
<< "title"
<< "genre";
<< "genre"
<< "crate";

m_pKeyNotationCP = new ControlProxy("[Library]", "key_notation", this);
// Convert all the search column names to their field indexes because we use
Expand Down
29 changes: 23 additions & 6 deletions src/library/searchqueryparser.cpp
Expand Up @@ -29,6 +29,7 @@ SearchQueryParser::SearchQueryParser(TrackCollection* pTrackCollection)
<< "dateadded"
<< "datetime_added"
<< "date_added";
m_ignoredColumns << "crate";

m_fieldToSqlColumns["artist"] << "artist" << "album_artist";
m_fieldToSqlColumns["album_artist"] << "album_artist";
Expand Down Expand Up @@ -109,6 +110,16 @@ QString SearchQueryParser::getTextArgument(QString argument,
void SearchQueryParser::parseTokens(QStringList tokens,
QStringList searchColumns,
AndNode* pQuery) const {
// we need to create a filtered columns list that are handled differently
QStringList(queryColumns);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very uncommon style to declare a local variable! I wasn't even aware that the compiler accepts this construct that looks like invoking a single argument constructor of QStringList and instantly dropping the result.

QStringList queryColumns;

for (const auto& column: searchColumns) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the QStringList function parameters to const-ref or use qAsConst(searchColumn) from Qt 5 that Daniel has backported recently. Unfortunately range-based for loops don't play well with Qt's implicit sharing as we noticed.

if (m_ignoredColumns.contains(column)) {
continue;
}
queryColumns << column;
}


while (tokens.size() > 0) {
QString token = tokens.takeFirst().trimmed();
if (token.length() == 0) {
Expand Down Expand Up @@ -180,14 +191,20 @@ void SearchQueryParser::parseTokens(QStringList tokens,
// For untagged strings we search the track fields as well
// as the crate names the track is in. This allows the user
// to use crates like tags
std::unique_ptr<OrNode> gNode = std::make_unique<OrNode>();
qDebug() << searchColumns;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove debug message

if (searchColumns.contains("crate")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this ever be false? If not, please remove the if statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. the test cases use this a lot. Also, I'm thinking about the library rewrite branch and some additions I'm planning for the search query there. As I wrote, I did not want to break the API which means that you specify the columns your unnamed query should be matched against. Now we just have special matches as well and those need to be handled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay

std::unique_ptr<OrNode> gNode = std::make_unique<OrNode>();

gNode->addNode(std::make_unique<CrateFilterNode>(
&m_pTrackCollection->crates(), token));
gNode->addNode(std::make_unique<TextFilterNode>(
m_pTrackCollection->database(), searchColumns, token));
gNode->addNode(std::make_unique<CrateFilterNode>(
&m_pTrackCollection->crates(), token));
gNode->addNode(std::make_unique<TextFilterNode>(
m_pTrackCollection->database(), queryColumns, token));

pNode = std::move(gNode);
pNode = std::move(gNode);
} else {
pNode = std::make_unique<TextFilterNode>(
m_pTrackCollection->database(), queryColumns, token);
}
}
}
if (pNode) {
Expand Down
1 change: 1 addition & 0 deletions src/library/searchqueryparser.h
Expand Up @@ -34,6 +34,7 @@ class SearchQueryParser {
QStringList m_textFilters;
QStringList m_numericFilters;
QStringList m_specialFilters;
QStringList m_ignoredColumns;
QStringList m_allFilters;
QHash<QString, QStringList> m_fieldToSqlColumns;

Expand Down
47 changes: 47 additions & 0 deletions src/test/searchqueryparsertest.cpp
Expand Up @@ -709,6 +709,53 @@ TEST_F(SearchQueryParserTest, CrateFilter) {
qPrintable(pQuery->toSql()));
}

TEST_F(SearchQueryParserTest, ShortCrateFilter) {
// User's search term
QString crateName = "somecrate";
QString searchTerm = "ecrat";
QStringList searchColumns;
searchColumns << "crate"
<< "artist"
<< "comment";

// Parse the user query
auto pQuery(m_parser.parseQuery(QString("%1").arg(searchTerm),
searchColumns, ""));

// locations for test tracks
const QString kTrackALocationTest(QDir::currentPath() %
"/src/test/id3-test-data/cover-test-jpg.mp3");
const QString kTrackBLocationTest(QDir::currentPath() %
"/src/test/id3-test-data/cover-test-png.mp3");
const QString kTrackCLocationTest(QDir::currentPath() %
"/src/test/id3-test-data/artist.mp3");

// Create new crate and add it to the collection
Crate testCrate;
testCrate.setName(searchTerm);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@poelzi Shouldn't the crate name be set to crateName?

CrateId testCrateId;
collection()->insertCrate(testCrate, &testCrateId);

// Add the track in the collection
TrackId trackAId = addTrackToCollection(kTrackALocationTest);
TrackPointer pTrackA(Track::newDummy(kTrackALocationTest, trackAId));
TrackId trackBId = addTrackToCollection(kTrackBLocationTest);
TrackPointer pTrackB(Track::newDummy(kTrackBLocationTest, trackBId));
TrackId trackCId = addTrackToCollection(kTrackCLocationTest);
TrackPointer pTrackC(Track::newDummy(kTrackCLocationTest, trackCId));
pTrackC->setComment("garbage somecrate garbage");

// Add track A to the newly created crate
QList<TrackId> trackIds;
trackIds << trackAId;
collection()->addCrateTracks(testCrateId, trackIds);

EXPECT_TRUE(pQuery->match(pTrackA));
EXPECT_FALSE(pQuery->match(pTrackB));
EXPECT_TRUE(pQuery->match(pTrackC));
}


TEST_F(SearchQueryParserTest, CrateFilterEmpty) {
// Empty should match everything
auto pQuery(m_parser.parseQuery(QString("crate: "), QStringList(), ""));
Expand Down