On untagged queries use query for crate names. #1388

poelzi · 2017-11-21T20:31:21Z

I found the current behavior not obvious and only understood the real power of the search engine while digging through the code.

Currently it is only possible to use the advanced search "crates:" to search
for tracks in specific crates. Untagged search queries will only be matched to
the track, but not to the crates the track is in.

This change allows you to use crates like tags and query them unspecific.
If you use genre as top level folder, you often end up with tracks from different genres in albums and compilations. This change allows you to create a crate like your genre folder and simply query for both at the same time.

Currently it is only possible to use the advanced search "crates:" to search for tracks in specific crates. Untagged search queries will only be matched to the track, but not to the crates the track is in. This change allows you to use crates like tags and query them unspecific.

Be-ing · 2017-11-21T20:43:27Z

Good idea! @gramanas, any thoughts?

gramanas · 2017-11-22T11:50:07Z

Yeah, it seems nice! I like it!

uklotzde · 2017-11-22T17:12:21Z

src/library/searchqueryparser.cpp

+                // 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>();


What does the prefix "g" stand for? Group? Why do we need to introduce another temporary here instead of just storing the OrNode in pNode?

pNode is not of GroupNode subtype and will therefor not have addNode. At least I was not able to get it compiling, maybe with 2 casts, but then I would argue it looks much better with a second variable.

uklotzde · 2017-11-22T17:14:34Z

Nice idea. Please simplify the code as suggested.

We should not forget to amend the manual after this feature has been merged.

uklotzde

The tests are failing and need to be updated.

Unfortuantely Travis doens't notices the test failures!?

Be-ing · 2017-11-25T04:51:06Z

Unfortuantely Travis doens't notices the test failures!?

The status Travis reports to GitHub is currently meaningless. See https://bugs.launchpad.net/mixxx/+bug/1699689

Be-ing · 2017-12-02T01:37:41Z

ping @poelzi, could you fix the tests?

…e/query_crates

Be-ing · 2017-12-19T13:54:01Z

Hi @poelzi, do you think you could finish this for the beta release on Friday?

Add testcase for crates searches. Add a blacklist so special handled searchColumns that are not reflected by a column can be filtered out when building the query.

poelzi · 2017-12-21T00:13:32Z

@Be-ing fixing took longer then I thought. Not because it was complicated, but I did not want to break the API much. I don't like that I ended up with a extra copy of the searchQuery columns, but I tried different API's and I did not like any of those. As this is not a hot path, I think it's OK. I tried caching the filtered list, but then the API got quite ugly downstream.

Be-ing · 2017-12-22T03:49:20Z

src/library/searchqueryparser.cpp

@@ -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;


remove debug message

Be-ing

Some minor issues that it would be nice to take care of. @poelzi could you make these quick changes now? If not, I think we should merge this as-is for the beta release.

Be-ing · 2017-12-22T03:51:43Z

src/library/searchqueryparser.cpp

@@ -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;
+                if (searchColumns.contains("crate")) {


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

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.

…e/query_crates

Be-ing · 2017-12-22T14:53:40Z

LGTM thank you! 👍

daschuer · 2017-12-22T17:35:46Z

This fails on our ci. Any idea?

Be-ing · 2017-12-22T17:49:55Z

Travis timed out and AppVeyor failed with the infamous vamp-sdk error. This unreliable CI is a big issue! We need to either ask for donations to pay for reliable plans or switch to GitLab and get more reliable CI for free.

Be-ing · 2017-12-24T16:56:59Z

Ping. Ready for merge?

poelzi · 2017-12-24T18:22:54Z

I don't see any problems, so yes :)

…

On 24 December 2017 17:57:00 CET, Be ***@***.***> wrote: Ping. Ready for merge? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #1388 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

Be-ing · 2017-12-24T18:45:50Z

Can you retarget this PR to the 2.1 branch? Click the Edit button at the top of the page.

uklotzde · 2017-12-25T11:06:09Z

src/library/searchqueryparser.cpp

@@ -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);


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;

uklotzde · 2017-12-25T11:09:02Z

src/library/searchqueryparser.cpp

@@ -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) {


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.

ferranpujolcamins · 2017-12-29T23:01:52Z

src/test/searchqueryparsertest.cpp

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


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

small optimization

Be-ing · 2018-01-06T21:44:41Z

I'm now having doubts about whether we should merge this. It could be really confusing if the user is not aware of what is being searched. Maybe it would be better to introduce shorthands for queries so you could type "c:crateName" instead of "crate:crateName:.

sblaisot · 2018-01-07T00:57:45Z

Appveyor 32bits failing build seems to be unrelated to the change. I re-launched the build.

poelzi · 2018-01-07T08:22:15Z

@Be-ing for me this change cause the expected behavior. I use crates like tags and want to search for them. I have a plan for a 2.2 change that will calm your concerns ;)
For my usecase, I have primary folder genre and then band or label. I often filter for zenon or psytrance for example, but want to have results form crates in this as well. It can also have genre it's only a primary selection.

Be-ing · 2018-01-07T12:15:56Z

I have a plan for a 2.2 change that will calm your concerns ;)

Could you explain what that is?

poelzi · 2018-01-07T23:25:53Z

@Be-ing the internal search api already has this functionality, the fields a unnamed query parameter is searched in. Currently it's just a fixed list of all useful columns, but I want to have a it selectable in the advanced functionality button next to the search field. So, if you don't like that unnamed queries search in crates, you can disable it.

Be-ing · 2018-01-07T23:34:32Z

The difference is that hardcoded listed of columns are metadata from files and shown as columns in the library table. On the other hand, crates only exist in Mixxx's database and are not currently listed as a column in the library table.

Your idea to make the fields searched by unqualified query strings customizable is interesting. I'm wondering if that much flexibility is really useful.

daschuer · 2018-03-22T07:15:51Z

Since this seems to be a controversial feature. Lets postpone it to 2.2.

daschuer · 2018-07-19T22:26:57Z

I think we should merge this. What do you think?

uklotzde · 2018-07-24T21:27:51Z

The code seems to be ok now.

Unfortunately the code gets more and more fragmented and his really hard to reason about. All the logic is split among different files and classes with literal strings triggering different code paths. In the end all this query parsing and composition needs to be rewritten, sometimes.

uklotzde · 2018-07-24T21:29:37Z

Target branch is still 2.1??

daschuer · 2018-07-24T21:30:12Z

Yes. This is usually the down-side when going in small steps ...
What do you think is a suitable the target branch?

uklotzde

Wrong target branch!

daschuer · 2018-07-24T21:31:30Z

This is not 2.1, because it is not a bugfix. 2.2 is ok for me.

uklotzde · 2018-07-24T21:33:32Z

2.2 is ok, even if it is a new feature. I don't expect any serious issues.

uklotzde

Target branch 2.2 or master are both ok for me.

daschuer · 2018-07-24T21:39:57Z

Thank you all for working on this branch!

Be-ing · 2018-08-02T13:03:43Z

As this is a new feature, it should not be in 2.2. I don't know how it is in that branch now because this was actually merged to master.

poelzi changed the title ~~On untagged queries us token as crates name.~~ On untagged queries use query for crate names. Nov 21, 2017

uklotzde reviewed Nov 22, 2017

View reviewed changes

uklotzde suggested changes Nov 24, 2017

View reviewed changes

uklotzde added this to the 2.1.0 milestone Nov 25, 2017

Merge branch 'master' of https://github.com/mixxxdj/mixxx into featur…

aadc3a0

…e/query_crates

Fix unnamed query testcases.

14b2607

Add testcase for crates searches. Add a blacklist so special handled searchColumns that are not reflected by a column can be filtered out when building the query.

Be-ing reviewed Dec 22, 2017

View reviewed changes

poelzi added 2 commits December 22, 2017 13:33

Merge branch 'master' of https://github.com/mixxxdj/mixxx into featur…

64a601e

…e/query_crates

remove debug print

d443740

poelzi changed the base branch from master to 2.1 December 25, 2017 04:23

uklotzde reviewed Dec 25, 2017

View reviewed changes

Be-ing added the library label Dec 27, 2017

ferranpujolcamins reviewed Dec 29, 2017

View reviewed changes

Merge remote-tracking branch 'origin/2.1' into feature/query_crates

085a85d

fix comments from @ferranpujolcamins and @uklotzde

4f0e367

small optimization

daschuer modified the milestones: 2.1.0, 2.2.0 Mar 22, 2018

Be-ing modified the milestones: 2.2.0, 2.3.0 Jun 23, 2018

uklotzde approved these changes Jul 24, 2018

View reviewed changes

uklotzde suggested changes Jul 24, 2018

View reviewed changes

daschuer changed the base branch from 2.1 to master July 24, 2018 21:31

uklotzde approved these changes Jul 24, 2018

View reviewed changes

daschuer merged commit dbd47d5 into mixxxdj:master Jul 24, 2018

Holzhaus added this to Done in 2.3 release Mar 14, 2020

mixxxbot mentioned this pull request Aug 23, 2022

search doesn't work with single quotes (apostrophes) #9395

Closed

On untagged queries use query for crate names. #1388

On untagged queries use query for crate names. #1388

Conversation

poelzi commented Nov 21, 2017

Be-ing commented Nov 21, 2017

gramanas commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Nov 22, 2017

uklotzde left a comment

Choose a reason for hiding this comment

Be-ing commented Nov 25, 2017

Be-ing commented Dec 2, 2017

Be-ing commented Dec 19, 2017

poelzi commented Dec 21, 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

Be-ing commented Dec 22, 2017

daschuer commented Dec 22, 2017

Be-ing commented Dec 22, 2017

Be-ing commented Dec 24, 2017

poelzi commented Dec 24, 2017 via email

Be-ing commented Dec 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Jan 6, 2018

sblaisot commented Jan 7, 2018

poelzi commented Jan 7, 2018

Be-ing commented Jan 7, 2018

poelzi commented Jan 7, 2018

Be-ing commented Jan 7, 2018

daschuer commented Mar 22, 2018

daschuer commented Jul 19, 2018

uklotzde commented Jul 24, 2018 • edited

uklotzde commented Jul 24, 2018

daschuer commented Jul 24, 2018

uklotzde left a comment

Choose a reason for hiding this comment

daschuer commented Jul 24, 2018

uklotzde commented Jul 24, 2018

uklotzde left a comment

Choose a reason for hiding this comment

daschuer commented Jul 24, 2018

Be-ing commented Aug 2, 2018

uklotzde commented Jul 24, 2018 •

edited