Crate filter #1263

gramanas · 2017-05-24T01:21:12Z

Hello, my first PR for GSoC.
Also my first PR ever!

I implemented a new CrateFilterNode witch is responsible for the crate: filter at search.

It queries the database to get a list of track_Ids witch is then added to the final query
that gets executed at BaseTrackCache::filterAndSort().

TODO's

optimization
tests

Please have a look at the code and leave your comments.

Be-ing

I'm glad we're off to an early start! It looks pretty good so far. I have tested it and it works. Combining multiple crate filters and negation work too, which opens up interesting new possibilities for library organization.

Be-ing · 2017-05-24T03:13:23Z

src/library/searchquery.cpp

+bool CrateFilterNode::match(const TrackPointer& pTrack) const {
+    QString id = pTrack->getId().toString();
+    for (const QString& it : m_trackIds) {
+        if (it == id) {


String operations are expensive and should be minimized. Instead, store a list of TrackIds from the query, then you can simply check if pTrack->getId() is in that list. Do the conversion to QStrings only where needed in the toSql method.

I can use contains(const QString & str, Qt::CaseSensitivity cs = Qt::CaseSensitive)

Be-ing · 2017-05-24T03:13:36Z

src/library/basetrackcache.cpp

@@ -14,7 +14,7 @@

 namespace {

-const bool sDebug = false;
+const bool sDebug = true;


This will need to be reverted before merging.

Be-ing · 2017-05-24T03:13:54Z

src/library/searchquery.cpp

@@ -7,6 +7,12 @@
 #include "library/dao/trackschema.h"
 #include "util/db/sqllikewildcards.h"

+namespace {
+
+const bool sDebug = true;


Likewise, this will need to be set to false before merging.

Be-ing · 2017-05-24T03:29:30Z

src/library/searchquery.cpp

+    QString escapedArgument = escaper.escapeString(kSqlLikeMatchAll + m_argument + kSqlLikeMatchAll);
+
+    QSqlQuery query(m_database);
+    QString queryString = CrateStorage::formatSubselectQueryForCrateTrackIdsByEscapedName(escapedArgument);


This function name is really long. Can you think of a shorter one that still explains what the function does?

Be-ing · 2017-05-24T03:31:14Z

src/library/searchquery.h

+    void getTrackIds();
+
+    QSqlDatabase m_database;
+    QString m_sqlColumn;


This is unused and can be removed.

Be-ing · 2017-05-24T03:32:13Z

src/library/searchqueryparser.cpp

@@ -114,10 +117,11 @@ void SearchQueryParser::parseTokens(QStringList tokens,
            continue;
        }

+        bool negate = token.startsWith(kNegatePrefix);


Good idea to factor this out.

Be-ing · 2017-05-24T03:33:30Z

src/library/searchqueryparser.cpp

@@ -114,10 +117,11 @@ void SearchQueryParser::parseTokens(QStringList tokens,
            continue;
        }

+        bool negate = token.startsWith(kNegatePrefix);
+


There are unnecessary space characters on this line. Can you configure your editor to show trailing spaces at the end of a line and/or automatically remove them?

Be-ing · 2017-05-24T03:36:13Z

src/library/searchqueryparser.cpp

+                if (negate) {
+                    pNode = std::make_unique<NotNode>(std::move(pNode));
+                }
+                pQuery->addNode(std::move(pNode));


This negation logic is repeated for each type of QueryNode. Can it be factored out?

Be-ing · 2017-05-24T03:37:20Z

src/library/searchquery.cpp

+      m_sqlColumn(sqlColumn),
+      m_argument(argument) {
+    getTrackIds();
+    }


} should be the only character on the line.

Be-ing · 2017-05-24T03:37:58Z

src/library/searchquery.cpp

@@ -52,7 +58,8 @@ QVariant getTrackValueForColumn(const TrackPointer& pTrack, const QString& colum
        return static_cast<int>(pTrack->getKey());
    } else if (column == LIBRARYTABLE_BPM_LOCK) {
        return pTrack->isBpmLocked();
-    }
+    } // TODO(gramanas) Add getCrates that returns a QStringList with the crates a song is on
+    // used in CrateFilterNode::match()


I don't think this will be necessary.

yes, it was an old plan that I forgot to get rid of

Be-ing · 2017-05-24T06:23:39Z

src/library/searchqueryparser.cpp

    m_allFilters.append(m_numericFilters);
    m_allFilters.append(m_specialFilters);

    m_fuzzyMatcher = QRegExp(QString("^~(%1)$").arg(m_allFilters.join("|")));
    m_textFilterMatcher = QRegExp(QString("^-?(%1):(.*)$").arg(m_textFilters.join("|")));
+    m_crateFilterMatcher = QRegExp(QString("^-?(crate):(.*)$"));


Considering this again, I think it'd be better to reuse m_textFilterMatcher for crates. The way it is currently, both the m_crateFilterMatcher and m_textFilterMatcher regexes are evaluated for each QueryNode that isn't a crate query, which is wasteful. Instead, you could add "crate" to m_textFilters and check if the first capture group in the regex is equal to "crate".

you are right, it's more logical to it this way. Done as well

Be-ing

Looking better!

Be-ing · 2017-05-24T14:34:42Z

src/library/crate/cratestorage.h

@@ -280,8 +280,8 @@ class CrateStorage: public SqlStorage {
            CrateId crateId); // no db access

    // The name must be escaped with FieldEscaper
-    static QString formatSubselectQueryForCrateTrackIdsByEscapedName(
-                                                              const QString& crateName); 
+    static QString formatQueryForTrackIdsByEscapedName(


This is a better name, but it's not clear what name should be passed to the function. How about formatQueryForTrackIdsByCrateName?

Be-ing · 2017-05-24T14:37:12Z

src/library/searchquery.cpp

-        if (it == id) {
-            return true;
-        }
+    if (m_trackIds.contains(id)) {


This is still doing string comparison, it's just not explicit anymore. Make m_trackIds a QList<TrackId>

Also, you don't need to wrap this in an if statement. Just return m_trackIds.contains(pTrack->getId()) would work.

Be-ing · 2017-05-24T16:37:08Z

I think this is ready for merge. Tests and optimizations can come in the next PR. @uklotzde could you take a look?

uklotzde · 2017-05-25T09:12:47Z

Welcome on board, Anast :) I will be your co-mentor for GSoC 2017. What qualifies me? I recently finished a complete rewrite of the DB code for crates and also did some cleanup in the query and search framework of Mixxx.

Great to see that you already started working on the code! This is also what I would propose, try to split your work into manageable sub-tasks that we can review and integrate early. Whenever you notice that you have finished some piece of work that is worth integrating: Create a dedicated branch, cherry-pick or copy those changes from your main development branch, split and tailor these changes into comprehensible commits ("git rebase -i" helps a lot) and open a pull request.

I recommend constantly rebasing private development branches instead of merging, to keep the change set narrow and clear. After you published a PR for a branch you should should not (at least not without good reason) rebase it anymore on the current master to keep the comments and diffs in sync.

Uwe

uklotzde · 2017-05-25T09:16:47Z

src/library/searchquery.cpp

@@ -7,6 +7,12 @@
 #include "library/dao/trackschema.h"
 #include "util/db/sqllikewildcards.h"

+namespace {
+
+const bool sDebug = false;


I don't like this kind of hard-coded switches in the code. Unfortunately I noticed that our Logging class does not offer a function to query the current log level! I will shortly publish a small PR that adds the missing functionality.

uklotzde · 2017-05-25T09:18:38Z

src/library/searchquery.cpp

+    query.prepare(queryString);
+
+    if (query.exec()) {
+        if (sDebug) {


This conditional would only be necessary if formatting of the debug log message is expensive -> Remove if()

uklotzde · 2017-05-25T09:23:20Z

src/library/crate/cratestorage.cpp

@@ -410,6 +410,19 @@ QString CrateStorage::formatSubselectQueryForCrateTrackIds(
            crateId.toString());
 }

+//static
+QString CrateStorage::formatQueryForTrackIdsByCrateName(


"formatQueryForTrackIdsByCrateNameLike"

uklotzde · 2017-05-25T09:26:25Z

src/library/crate/cratestorage.h

@@ -279,6 +279,9 @@ class CrateStorage: public SqlStorage {
    static QString formatSubselectQueryForCrateTrackIds(
            CrateId crateId); // no db access

+    // The name must be escaped with FieldEscaper


Escaping of the crate name argument and surrounding it with LIKE wildcards should be done within this function. The caller should not be aware of this. The CrateStorage class is responsible for shielding the caller from the database-dependent details.

Please remove this comment after fixing the query

This function is not used any more, I think I should delete it as a whole.

uklotzde · 2017-05-25T09:30:38Z

src/library/searchquery.cpp

+
+void CrateFilterNode::getTrackIds() {
+    FieldEscaper escaper(m_database);
+    QString escapedArgument = escaper.escapeString(kSqlLikeMatchAll + m_argument + kSqlLikeMatchAll);


Only escape the argument, not the LIKE wildcards:
kSqlLikeMatchAll + FieldEscaper(m_database).escape(m_argument) + kSqlLikeMatchAll
The result might be equal in this special case, but generally we don't want to escape special SQL characters that are part of the query statement and not part of any parameter.

What you say does not work. I tried it and the query fails execution. Also I copied my code from TextFilterNode who uses the esacaper in the exact same way.

Ups, sorry, you are right.

uklotzde · 2017-05-25T09:33:06Z

src/library/crate/cratestorage.cpp

+//static
+QString CrateStorage::formatQueryForTrackIdsByCrateName(
+        const QString& crateName) {
+    return QString("SELECT %1 FROM %2 JOIN %3 ON %4 = %5 WHERE %6 LIKE %7").arg(


You need to use SELECT DISTINCT to avoid duplicate TrackIds if the LIKE statement matches multiple crates.

uklotzde · 2017-05-25T09:50:18Z

src/library/searchquery.cpp

+}
+
+QString CrateFilterNode::toSql() const {
+    QStringList trackIds;


This approach is inefficient and doesn't scale well. Please use a subselect similar to formatSubselectQueryForCrateTrackIds():
formatSubselectQueryForCrateTrackIdsByCrateNameLike(const QString& crateNameLike)

uklotzde · 2017-05-25T09:51:30Z

src/library/searchquery.cpp

+                                 const QString& argument)
+    : m_database(database),
+      m_argument(argument) {
+    getTrackIds();


The database query is always executed, even if the result is only needed for match(). See my comments below on how to implement toSql() more efficiently.

uklotzde · 2017-05-25T09:52:24Z

src/library/searchquery.cpp

+    getTrackIds();
+}
+
+void CrateFilterNode::getTrackIds() {


A function named "get..." should return a value.

I can rename it to init() like the NumberFIlterNode

uklotzde · 2017-05-25T10:22:47Z

src/library/searchquery.cpp

+    FieldEscaper escaper(m_database);
+    QString escapedArgument = escaper.escapeString(kSqlLikeMatchAll + m_argument + kSqlLikeMatchAll);
+
+    QSqlQuery query(m_database);


This can be accomplished much more efficiently:

New member function: CrateTrackSelectResult CrateStorage::selectCrateTracksSortedByCrateNameLike(const QString& crateNameLike) const
-- Very similar to CrateStorage selectCrateTracksSorted()
-- Instead of providing a single crateId provide a crateNameLike argument and do a sub-select (that we also need for CrateFilterNode::toSql())

CrateFilterNode: Initialization
-- Perform DB query
-- Ignore crateId()s
-- Collect all distinct trackIds()s in a std::vector
-- Skip duplicate trackId()s resulting from multiple crates matching the LIKE query

CrateFilterNode::match()
-- Use std::binary_search() on the sorted vector for fast O(log(n)) lookup

Please refer to CrateFeature for a similar use case.

Optimization: Use lazy evaluation, i.e. the first call to match() should trigger the database query and populate the result set once.

The initialization already has all distinct trackIds in a vector. Why whould match trigger a db query again?

Ignore my comment about the possible optimization for now. But get to know what "lazy initialization/evaluation" means ;)

CrateTrackSelectResult CrateStorage::selectCrateTracksSortedByCrateNameLike(const QString& crateNameLike) const

Ah, I see. By returning a CrateTrackSelectResult, CrateStorage can take care of everything touching the DB so the CrateFilterNode only needs the const reference to CrateStorage from TrackCollection::crates() instead of the reference to the QSqlDatabase.

Optimization: Use lazy evaluation, i.e. the first call to match() should trigger the database query and populate the result set once.

But toSql() needs the results of the DB query before match() is called. Also both of those methods are const so they can't be used to store the result set.

Nope, toSql() and match() serve a different purpose. toSql() is used to query the database while match() is only applied for post-filtering all tracks that are already cached in memory, e.g. all dirty tracks that have been modified recently. Without any dirty tracks match() will not be invoked!

You can circumvent the const-correctness by using mutable members behind the scenes. But we can add this optimization later and avoid those dirty tricks ;)

CrateStorage should encapsulate access to crate tables as much as possible. It's an anti-corruption layer between the database and clients, basic OO ;)

Do you think it'd be helpful to add to the comment in src/util/db/fwdsqlqueryselectresult.h that DAO classes should implement methods returning FwdSqlQuerySelectResult objects when a caller wants info from the DB so callers don't need access to the DB directly?

uklotzde · 2017-05-25T10:25:29Z

src/library/searchquery.cpp

+}
+
+bool CrateFilterNode::match(const TrackPointer& pTrack) const {
+    return m_trackIds.contains(pTrack->getId());


Lookup in a QList doesn't scale well: O(n)
But this becomes obsolete anyway with my proposal for an optimized DB query.

Be-ing · 2017-05-25T15:34:24Z

@gramanas For information on Git rebasing, refer to the Git Book.

gramanas · 2017-05-25T22:13:37Z

src/library/searchqueryparser.cpp

@@ -5,8 +5,9 @@
 const char* kNegatePrefix = "-";
 const char* kFuzzyPrefix = "~";

-SearchQueryParser::SearchQueryParser(QSqlDatabase& database)


The test for the SearchQueryParser uses this constructor
and I couldn't figure out a way to get rid of it.
If you see the code it connects it to a new instance of the DB.
In the test there is no mention of a TrackCollection so SearchQueryParser
has two constructors now. Thankfully the constructor is only
called once in the whole codebase (at TrackCollection's constructor)
so it's not a huge problem. But I don't like it. From what I understand
it's normal to have functions that are used only for testing
so my concern might be a bit off.

Be-ing

Looking better. Still some room for improvement.

Be-ing · 2017-05-25T22:09:55Z

src/library/crate/cratestorage.cpp

@@ -453,6 +453,23 @@ CrateTrackSelectResult CrateStorage::selectTrackCratesSorted(TrackId trackId) co
    }
 }

+CrateTrackSelectResult CrateStorage::selectTracksSortedByCrateNameLike(const QString& crateNameLike) const {


This function should be responsible for the escaping. The caller shouldn't need to worry about that detail.

Yes, I got confused cause the other function was static and had no access to the m_database of CrateStorage. Should be ok now.

Be-ing · 2017-05-25T22:11:25Z

src/library/searchqueryparser.cpp

@@ -128,7 +129,7 @@ void SearchQueryParser::parseTokens(QStringList tokens,
            if (!argument.isEmpty()) {
                if (field == "crate") {
                    pNode = std::make_unique<CrateFilterNode>(
-                          m_database, argument);
+                          argument, m_pTrackCollection);


The CrateFilterNode shouldn't need the TrackCollection pointer, just the CrateStorage reference.

I though I could make it more general, but I guess there is no need.

uklotzde · 2017-05-25T23:16:12Z

src/library/crate/cratestorage.cpp

@@ -440,6 +453,23 @@ CrateTrackSelectResult CrateStorage::selectTrackCratesSorted(TrackId trackId) co
    }
 }

+CrateTrackSelectResult CrateStorage::selectTracksSortedByCrateNameLike(const QString& crateNameLike) const {
+    FwdSqlQuery query(m_database, QString(
+            "SELECT DISTINCT %1,%2 FROM %3 JOIN %4 ON %5 = %6 WHERE %7 LIKE %8 ORDER BY %1").arg(


DISTINCT is redundant here and should be omitted

uklotzde · 2017-05-25T23:20:07Z

src/library/crate/cratestorage.h

@@ -287,6 +290,8 @@ class CrateStorage: public SqlStorage {
            CrateId crateId) const;
    CrateTrackSelectResult selectTrackCratesSorted(
            TrackId trackId) const;
+    CrateTrackSelectResult selectTracksSortedByCrateNameLike(


"selectCrateTracks..." to follow the naming style of its siblings. The result contains (crateId, trackId) tuples and not only trackIds.

Now that we added a new function we could think about how to improve the naming of all related functions. But finally it it should be consistent.

In the interest of minimizing potential merge conflicts with #1117, I think we should hold off on renaming things for now.

uklotzde · 2017-05-25T23:28:39Z

src/library/crate/cratestorage.cpp

+                    CRATETABLE_ID,
+                    CRATETRACKSTABLE_CRATEID,
+                    CRATETABLE_NAME,
+                    crateNameLike));


Please bind crateNameLike as a parameter by-value to the query instead of formatting it into the string. Then you don't need to escape it manually
"...%7 LIKE :crateNameLike..."
query.bindValue(":crateNameLike", crateNameLike);

This does not work as you say. The query fails if I don't use escaper

uklotzde · 2017-05-25T23:33:21Z

src/library/searchquery.cpp

+
+void CrateFilterNode::init() {
+    FieldEscaper escaper(m_pTrackCollection->database());
+    QString escapedArgument = escaper.escapeString(kSqlLikeMatchAll + m_argument + kSqlLikeMatchAll);


Don't escape. When binding parameters to queries you almost never need to escape manually. Everything in queryutil.h is legacy code that shouldn't be used any more. I didn't remember that I eliminated it from the refactored CrateStorage ;)

Be-ing · 2017-05-25T23:34:42Z

src/library/searchquery.h


    bool match(const TrackPointer& pTrack) const override;
    QString toSql() const override;

  private:
    void init();

+    QSqlDatabase m_database;


This is unused now.

uklotzde · 2017-05-25T23:46:45Z

src/library/searchquery.cpp

+    return std::binary_search(m_trackIds.begin(), m_trackIds.end(), pTrack->getId());
+}
+
+QString CrateFilterNode::toSql() const {


This code simply doesn't scale. For example I have crates with thousands of tracks that I use for storing pre-computed searches.
Subselect: "id IN (" + CrateStorage::formatSubselectQueryForCrateTrackIdsWhereCrateNameLike(crateNameLike) + ")"

Please note, that you need to escape crateNameLike manually when formatting the subselect string! We don't have access to the SQL query of the caller and can't bind the parameter value by name here.

I also came to the conclusion that we might use "Where" instead of "By" for the filtering condition in those function names. Terms like "...SortedBy..." are simply misleading.

Ok, I think I got it.

Wouldn't this run the subselect query twice, once as a subquery in the main search filter query, and again to get the list of TrackIds for match()? That would be faster than building a QStringList from the vector of TrackIds?

Indeed, right now I run a query in CrateFilterNode::Init() function to fill the vector, and then I return the same SQL to be run by the query in BaseTrackCache::filterAndSort(). Maybe by using lazy evaluation this would work, but right now I run the query 2 times.

If that's the case you could move the query from the init method to the match method. If you don't need to store any information for later use, you can use a local variable for the TrackId vector. That way the second query will only be run if it is needed.

A local variable does not work here and a static local variable would be wrong. For lazy initialization you need to populate a (mutable) vector m_matchingTrackIds upon the first invocation of match() and use another (mutable) member variable m_matchingTrackIdsInitialized for tracking this.

Ok, so I remove the query execution from init(), I lazy initialize it in match and I leave toSql as it is, to return a SQL query.

I thought match() was only called once in the life of the QueryNode? If so, why do the TrackIds need to be stored?

Oh, nevermind. match() is called for each dirty TrackId.

uklotzde · 2017-05-28T21:31:14Z

src/library/crate/cratestorage.cpp

@@ -441,6 +457,26 @@ CrateTrackSelectResult CrateStorage::selectTrackCratesSorted(TrackId trackId) co
 }


+CrateTrackSelectResult CrateStorage::selectTracksSortedByCrateNameLike(const QString& crateName) const {
+    FieldEscaper escaper(m_database);


As already mentioned, please bind crateName as a named parameter to the query using bindValue(). FieldEscaper then becomes obsolete here.

I tried it the last time you mentioned it, and it didn't work. The name needs to be wrapped in kSqlLikeMatchAll (%). Also it needs to be inside quotes. bindValue() does not seem to do that. I can use it like this:
bindValue(kSqlLikeMatchAll + crateNameLike + kSqlLikeMatchAll)

uklotzde · 2017-05-28T21:37:59Z

src/library/searchquery.cpp

+}
+
+QString CrateFilterNode::toSql() const {
+    return QString("id IN (%1)").arg(CrateStorage::formatQueryForTrackIdsByCrateNameLike(


If you make formatQueryForTrackIdsByCrateNameLike() a non-static member function of CrateStorage you can get rid of the awkward database parameter and also the m_database member of CrateFilterNode. You are storing a pointer to CrateStorage anyway which already carries a database connection.

I got rid of field escaper instead witch used the database connection.

uklotzde · 2017-05-28T21:39:02Z

src/library/searchquery.h

@@ -14,6 +14,8 @@
 #include "proto/keys.pb.h"
 #include "util/assert.h"
 #include "util/memory.h"
+#include "library/trackcollection.h"


This #include doesn't seem to be needed.

uklotzde · 2017-05-28T21:40:29Z

src/library/searchquery.h

+class CrateFilterNode : public QueryNode {
+  public:
+    CrateFilterNode(const QSqlDatabase& database,
+                    const QString& argument,


Give argument a meaning by using a more expressive name: crateNameLike

uklotzde · 2017-05-28T21:40:48Z

src/library/searchquery.h

+
+  private:
+    QSqlDatabase m_database;
+    QString m_argument;


...also here: m_crateNameLike

uklotzde · 2017-05-28T21:41:32Z

src/library/searchquery.h

+    QString toSql() const override;
+
+  private:
+    QSqlDatabase m_database;


This member should vanish according to my comment regarding the database connection

uklotzde · 2017-05-28T21:45:58Z

src/library/searchqueryparser.h

@@ -11,14 +11,19 @@

 class SearchQueryParser {
  public:
-    SearchQueryParser(QSqlDatabase& database);
+    SearchQueryParser(TrackCollection* pTrackCollection);
+    SearchQueryParser(QSqlDatabase& database)


This constructor shouldn't be available anymore! Otherwise m_pTrackCollection is uninitialized.

Check my comment on:
#1263 (comment)

Maybe the test for searchqueryparser should take a TrackCollection instead of just a database connection.

uklotzde · 2017-05-28T21:46:56Z

src/library/searchqueryparser.h

@@ -27,6 +32,8 @@ class SearchQueryParser {
    QString getTextArgument(QString argument,
                            QStringList* tokens) const;

+    TrackCollection* m_pTrackCollection;
+
    QSqlDatabase m_database;


m_database is redundant and should instead be obtained from m_pTrackCollection if needed

Also related to the previous comment. If I get rid of this, then the test class can't work
because it uses the constructor that attaches a connection to m_database

uklotzde · 2017-05-28T21:49:51Z

src/library/searchqueryparser.h

@@ -11,14 +11,19 @@

 class SearchQueryParser {
  public:
-    SearchQueryParser(QSqlDatabase& database);
+    SearchQueryParser(TrackCollection* pTrackCollection);


Single argument constructors should be declared 'explicit by default! Otherwise the compiler will apply an implicit conversion from TrackCollection* to SearchQuery whenever it needs to without any warning.

If you could explain that a little more! I understand what explicit and implicit conversion is, but I don't really get your point here. Also by your logic the
SearchQueryParser(QSqlDatabase& database); constructor that was there was also wrong and should be explicit right?

Be-ing · 2017-05-29T02:22:15Z

src/library/searchqueryparser.h

@@ -36,6 +43,7 @@ class SearchQueryParser {

    QRegExp m_fuzzyMatcher;
    QRegExp m_textFilterMatcher;
+    QRegExp m_crateFilterMatcher;


You forgot to remove this now that m_textFilterMatcher is used for the crate filter.

gramanas · 2017-06-01T23:22:12Z

I did a rebase to clean the commit history from all these minor fixes etc.
I think, if I am not missing something, that this is ready to merge.

Be-ing

Just a minor change to make, otherwise looks good. What do you think @uklotzde?

Be-ing · 2017-06-02T03:40:46Z

src/library/searchqueryparser.cpp

-                    pNode = std::make_unique<NotNode>(std::move(pNode));
+                if (field == "crate") {
+                    pNode = std::make_unique<CrateFilterNode>(
+                          argument, &m_pTrackCollection->crates());


argument is the last... argument... for the constructors of the other QueryNodes. I think the CrateFilterNode should be consistent with that pattern.

I also noticed this, but didn't want to confuse you with minor issues ;) Method parameters should be ordered starting from broad, context-related arguments to more specialized arguments. Ok, since we both agree let's fix it now.

uklotzde · 2017-06-02T07:46:18Z

src/test/searchqueryparsertest.cpp

  protected:
    SearchQueryParserTest()
            : m_database(QSqlDatabase::addDatabase("QSQLITE")),
-              m_parser(m_database) {
+              m_parser(collection()) {
        QTemporaryFile databaseFile("mixxxdb.sqlite");


Please remove the manually created connection to a temporary database. It should be obsolete now since LibraryTest provides a TrackCollection with a database.

uklotzde · 2017-06-02T13:47:18Z

Just a recommendation for a readable Git commit history: "Small fixes" is just random noise. Please use more meaningful phrases like "Reorder constructor arguments" and "Remove unused database connection from test" instead.

You may also notice that I prefer imperative verbs instead of past tense. That's a common recommendation, more readable and uses fewer characters ;)

uklotzde · 2017-06-02T13:47:37Z

I will do a final test.

uklotzde · 2017-06-02T14:16:57Z

LGTM

daschuer · 2017-06-02T18:43:57Z

Does this mean, I can merge this to master? Or do we now have valid reason the cut out 2.1?

daschuer · 2017-06-02T18:47:48Z

@gramanas: Sorry, I was not able to follow all the changes in this PR. Could you update the initial commit description, including short hint what are the noteable/testable changes?
Thank you.

Be-ing · 2017-06-02T18:48:45Z

LGTM, let's merge this. I think @gramanas' next step should be adding tests for this new search filter that can also be merged to master. From there he can start working on hierarchical crates on top of #1117.

Add CrateFilterNode in library/searchquery.h witch is responsible for handling the crate filtering. Modify searchqueryParser.cpp to handle the crate keyword and create a CrateFIlterNode. Finally add a few functions in cratestorage.cpp to handle the SQL queries needed for the filter to work. Also change searchqueryparsertest.cpp due to the new SearchQueryParser constructor.

uklotzde · 2017-06-02T20:50:25Z

The changes are very limited and the final code quality is really good. I don't see any reason why we should hold this back.

gramanas · 2017-06-02T20:52:06Z

@daschuer Ok, I added a bit more in the commit message.

daschuer · 2017-06-02T21:09:34Z

Thank you all, working on this!

naught101 · 2017-06-03T03:41:13Z

Yes, thank you @gramanas and reviewers! This is something that will be super useful immediately. I'm keen to try it out :)

mixxxdj/mixxx#1263

Be-ing suggested changes May 24, 2017

View reviewed changes

Be-ing reviewed May 24, 2017

View reviewed changes

Be-ing suggested changes May 24, 2017

View reviewed changes

uklotzde reviewed May 25, 2017

View reviewed changes

gramanas commented May 25, 2017

View reviewed changes

Be-ing suggested changes May 25, 2017

View reviewed changes

uklotzde reviewed May 25, 2017

View reviewed changes

Be-ing reviewed May 25, 2017

View reviewed changes

uklotzde reviewed May 25, 2017

View reviewed changes

uklotzde reviewed May 28, 2017

View reviewed changes

src/library/searchquery.h Outdated

private:

QSqlDatabase m_database;

QString m_argument;

Copy link

Contributor

uklotzde May 28, 2017

Choose a reason for hiding this comment

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

...also here: m_crateNameLike

uklotzde reviewed May 28, 2017

View reviewed changes

Be-ing suggested changes May 29, 2017

View reviewed changes

gramanas force-pushed the CrateFilter branch from b686269 to cda0f85 Compare June 1, 2017 23:19

Be-ing suggested changes Jun 2, 2017

View reviewed changes

uklotzde reviewed Jun 2, 2017

View reviewed changes

gramanas force-pushed the CrateFilter branch from e06035d to aa9be23 Compare June 2, 2017 20:50

daschuer merged commit 3179611 into mixxxdj:master Jun 2, 2017

gramanas deleted the CrateFilter branch June 3, 2017 09:46

This was referenced Jun 4, 2017

Crate filter tests #1277

Merged

Library Layout Redesign Single pane #1117

Closed

esbrandt added a commit to esbrandt/manual that referenced this pull request Jun 24, 2017

Update list of search operators, as available in searchqueryparser, see

0f54d68

mixxxdj/mixxx#1263

esbrandt mentioned this pull request Jun 24, 2017

WIP Manual 2.1.x updates mixxxdj/manual#59

Merged

37 tasks

This was referenced Aug 22, 2022

crate: search filter #7726

Closed

Allow use of multiple libraries from GUI #8874

Open

Crate filter #1263

Crate filter #1263

Conversation

gramanas commented May 24, 2017 • edited Loading

TODO's

Be-ing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Be-ing commented May 24, 2017

uklotzde commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented May 25, 2017

Choose a reason for hiding this comment

Be-ing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gramanas commented May 24, 2017 •

edited

Loading

Be-ing May 24, 2017 •

edited

Loading

Be-ing May 24, 2017 •

edited

Loading

uklotzde May 25, 2017 •

edited

Loading

uklotzde May 25, 2017 •

edited

Loading

Be-ing May 25, 2017 •

edited

Loading

uklotzde May 25, 2017 •

edited

Loading

Be-ing May 25, 2017 •

edited

Loading

gramanas May 29, 2017 •

edited

Loading

uklotzde May 28, 2017 •

edited

Loading