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
Changes from 3 commits
2a35c50
aadc3a0
14b2607
64a601e
d443740
085a85d
4f0e367
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"; | ||
|
@@ -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); | ||
for (const auto& column: searchColumns) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
@@ -177,8 +188,23 @@ void SearchQueryParser::parseTokens(QStringList tokens, | |
} | ||
// Don't trigger on a lone minus sign. | ||
if (!token.isEmpty()) { | ||
pNode = std::make_unique<TextFilterNode>( | ||
m_pTrackCollection->database(), searchColumns, token); | ||
// 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 | ||
qDebug() << searchColumns; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove debug message |
||
if (searchColumns.contains("crate")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this ever be false? If not, please remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(), queryColumns, token)); | ||
|
||
pNode = std::move(gNode); | ||
} else { | ||
pNode = std::make_unique<TextFilterNode>( | ||
m_pTrackCollection->database(), queryColumns, token); | ||
} | ||
} | ||
} | ||
if (pNode) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(), "")); | ||
|
There was a problem hiding this comment.
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.