Fix empty filter bug #19

gramanas · 2017-06-07T12:09:56Z

When user types "filter:" and nothing after it, the query should return the
whole library since in the search we have just an empty filter.

Basically I removes the IS NULL sqlNode that was created in SearchQueryParser::parseTokens

daschuer · 2017-06-07T14:12:28Z

@jmigual Please have a look in this.

This is NULL feature was explicit introduced here:
6aac36c#diff-74cc91f7c2236563aad44e59ee0654f7

Do you remember why?

Is there a use case for an empty search query?
Like finding all tracks without a genre?

jmigual · 2017-06-07T15:16:55Z

Yes it was introduced to search for tracks that do not have a certain field. I introduced it because in the sorting tree the elements that belong to the "Unknown" tag need to be able to be selected when the user clicks it.

For example if I have tracks that do not have artist they'll show all grouped under the "Unknown" tag and when I click it I will expect to see this tracks and, since the tree is a query helper, I introduced the NULL feature.

gramanas · 2017-06-07T15:41:01Z

But it can't be generated when a user has typed nothing. Maybe a special keyword could be used to generate it, eg NULL.

gramanas · 2017-06-07T15:55:26Z

I added a nullFilterMatcher in SearchQueryParser witch captures paterns like "artist:0"
so you can use this in the Unknown tag.

jmigual · 2017-06-07T16:01:06Z

But in the way it's now if the user types
"artist:" the NULL is added so I don't understand why the 0 must be added.

gramanas · 2017-06-07T16:04:02Z

because if the user types "artist:" the full library should be returned, cause this is the way the filters worked so far. I also find it more logical, like you've typed what filter you want to use (eg artist:) and you see the whole library, then you start typing the artist's name and the results narrow to your search. If you want to get the NULL values you use the special filter ":0". Ofc if you find ":0" strange we could make it something else

gramanas · 2017-06-07T16:54:03Z

@daschuer @jmigual Ok, the new null filter matcher matches stuff like "artist:=0" I modified the query generated by the unknown tag in the tree to be able to generate this one.

Also the old (artist IS NULL) didn't work for me, I got 282 songs in my library without artist. Only 4 of them were shown. So I changed it to ((artist IS NULL) OR (artist LIKE "")) and now it shows everything. From my tests everything works fine, please have a look and comment if you have any problems with what is happening.

daschuer · 2017-06-07T18:21:18Z

"artist:0" does not work well, because it also stands for as artist containing "0". The original solution was quite natural for me. On one hand you are right that you are narrowing the we search while typing, on the other hand it does not work anyway, because while you type you are actually searching for a track containing "artist" only after the colon the real search starts.
We may consider to use artist:"" but I like the original behaviour more.

@Be-ing what do you think about this?

gramanas · 2017-06-07T21:18:33Z

I am open to suggestions to change the zero, I don't like it that much either. But I really feel that artist: alone should return the whole library.

Also we should change crate: to also have an exact match with crate:= and think about the following: Should crate:=0 or whatever we make it to be return all the songs that are NOT in a crate?

Following the same logic I feel that this whole IS NULL thing should maybe be a modifier like - so I am thinking on something in the line of !artist: since ! already means NOT in computer word.

For the exact match of the crate we just need to add a function in CrateStorage witch instead of creating the query using LIKE it does so using the equality operator.

jmigual · 2017-06-07T21:28:23Z

If I remember properly (I have to check it though) the meaning of := wasn't the same of : so we have both versions and they do different things. I think it was something like := is the exact match and : is a similar match using SQLite LIKE function.

jmigual · 2017-06-07T21:30:28Z

And for the empty search artist: i prefer the way that is it now because you are specifying all the tracks where artist is a null string, if you want to see all the library you can just remove the query and you'll see all the library.

Bye the way @gramanas take in account that if you change the NULL definition you must update the query tree helper in the sidebar so it continues working.

gramanas · 2017-06-07T21:43:08Z

I did update the query tree helper, it works correctly. About the IS NULL query as I said , I got 282 songs in my library without artist. Only 4 of them were shown with your code. So I changed it to ((artist IS NULL) OR (artist LIKE "")) and now it shows everything. Check in the searchqueryparser.cpp where it generates the IS NULL query.

Ok, if you prefer artist: to show the NULL stuff it's ok. It's just not the way it works right now.
When we decide something I will implement it happily!

Be-ing · 2017-06-08T14:12:42Z

We may consider to use artist:"" but I like the original behaviour more.
@Be-ing what do you think about this?

What about using artist:="" to explicitly search for an empty field and make artist: by itself do nothing as @gramanas is proposing? I would prefer this to complicating the query syntax with another special operator.

Also we should change crate: to also have an exact match with crate:= and think about the following: Should crate:=0 or whatever we make it to be return all the songs that are NOT in a crate?

-crate:="" should return all tracks that are in any crate.

daschuer · 2017-06-08T16:12:33Z

That would work, but is IMHO not better then the current solution, because we do not handle "text" inputs very well. I also think hat his feature can only discovered by a programmer. I have ask my wife, to search for missing artists she supprising just types "artist:" What do you DJ friends expect? Am 08.06.2017 4:12 nachm. schrieb "Be" <[email protected]>:

…

We may consider to use artist:"" but I like the original behaviour more. @Be-ing <https://github.com/be-ing> what do you think about this? What about using artist:="" to explicitly search for an empty field? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABsfInyexxwTVpGGxlV_OEL9FLn0GGe9ks5sCAFcgaJpZM4NynDl> .

Be-ing · 2017-06-08T16:24:19Z

I think you are right that artist:="" is not easily discoverable. An observant user may notice that query generated when clicking "Unknown" in the tree, but the subtle details of the = and the empty string together would be difficult to notice. So I think using plain artist: to find tracks without an artist is best, even if it is a bit strange if you're expecting the entire library. If you want the entire library, just delete the filter.

uklotzde · 2017-06-08T20:12:02Z

From a theoretical point of view I prefer continuous functions with consistent behaviour. The more characters you add, the more you narrow your search: No characters -> Match all.

But I must agree with @Be-ing. You convinced me with your last argument 👍 We don't need a multitude of possible inputs for the same result when on the other hand we have to introduce ambiguous inputs for handling the missing use cases.

gramanas · 2017-06-08T20:46:14Z

Ok, I'll fix and push as you said. But then crate: should return all tracks that are not in a crate. We should look at that later.

uklotzde · 2017-06-08T21:13:15Z

Sounds reasonable: "crate:" => all tracks that are not contained in any crate

One use case I could think of is to identify all tracks that have not been sorted into crates, yet.

gramanas · 2017-06-09T00:23:03Z

I did as you said and almost everything works.

I am having some trouble with the SearchQueryParserTest.TextFilterEmpty again. No matter what I try, while the sql works fine the match function does not... @uklotzde any ideas?

daschuer · 2017-06-09T20:37:59Z

src/library/features/mixxxlibrary/mixxxlibrarytreemodel.cpp

+        }
+        if (value != "0") {
+            value = "\"" + value + "\"";
+        }


Is this a leftover form the artist:0 version?

gramanas · 2017-06-09T21:03:01Z

Yes, I'll remove it. for some reason I didn't see it :/

daschuer · 2017-06-11T20:31:13Z

SearchQueryParserTest.TextFilterEmpty fails.

There seams to be also an escaping issue. I have a crate "40's" and "crate:40's" does not return any result. Other crates are working fine.

gramanas · 2017-06-11T22:34:47Z

There is no escaping happening anywhere in the crate filter, @uklotzde @Be-ing it appears I missed that part. I'll try adding it here.

About the TextFIlterEmpty test: I believe TextFilterNode::match() has some problems.
This line:
if (value.toString().contains(m_argument, Qt::CaseInsensitive)) { return true; }

when the query has an empty string as an argument artist:"" for example,
it will always return true.

Apparently the SqlNode that was created could not match() as it should so I removed it and made TextFilter to be aware of the empty argument and act accordingly Add escape to crate filter argument

gramanas · 2017-06-12T13:00:13Z

I added escape in crate filter and I think I also fixed the bug.

Now for the empty text filter:
As it was the parser generated a SqlNode with the IS NULL argument. Then the SqlNode::match() function always returns true cause it's not supposed to be used this way.

I modified the TextFilter and the ExactFilter to be aware that the argument is empty. Everything seems to work fine.

@jmigual The GLOB clause in the exact filter can't match stuff like this [ MISC ]. I got some tracks with this as a genre and the exact filter can't find them. It seems that GLOB gets lists of characters to match enclosed in brackets. So this genre:="[ MISC ]" Matches one character from the enclosed list of characters. I haven't given this any thought since it's not a major issue. Maybe instead of GLOB we could use = and use GLOB only when wildstars * are necessary.

uklotzde · 2017-06-12T19:14:23Z

src/library/searchquery.cpp

-        if (value.toString().contains(m_argument, Qt::CaseInsensitive)) {
-            return true;
+        if (m_argument.isEmpty()) {
+            if  (value.toString().isEmpty()) {


Both if statements on the 2nd level can be simplified: return

Handling the special case m_argument.isEmpty() separately seems reasonable to me.

gramanas · 2017-06-13T22:14:35Z

@daschuer @jmigual what do you think, should we merge it?

jmigual · 2017-06-14T08:45:23Z

@jmigual The GLOB clause in the exact filter can't match stuff like this [ MISC ]. I got some tracks with this as a genre and the exact filter can't find them. It seems that GLOB gets lists of characters to match enclosed in brackets. So this genre:="[ MISC ]" Matches one character from the enclosed list of characters. I haven't given this any thought since it's not a major issue. Maybe instead of GLOB we could use = and use GLOB only when wildstars * are necessary.

In this case I added the GLOB to allow the user to search for things like artist:=A* which should return all the artists with a name starting with A (if I remember properly this is used by the tree query helper). If we replace GLOB by = we remove this possibility. Also, with this type of query, there's no way to know if the user is creating a glob like a[abc] (which should return aa, ab, ac) or it's really a genre name.

A solution may be to remove the possibility to search with a glob like a[abc] and always escape the [ and ] characters.

daschuer · 2017-06-14T13:17:58Z

I am in favour to keep the syntax simple in the first place. artist:bla schould return all artists containing bla, no special character replacement like * or [.
Later we can also consider to replace this by a phonetic search. artist:dune schould match the artist Dúné, artist:pink schould match P!nk, and artist:*NSYNC schould match 'N Sync and even spelling errors schould be considered.

With the syntax artist:="" we can do all more technical things we discussed above. We can also invent a new syntax for fully GLOB cause compliant.

Be-ing · 2017-06-14T19:03:47Z

Regarding the issue with GLOB and brackets, I agree with @daschuer that the syntax should remain very simple, so the filters should escape any [ and ] characters the user enters. The use case for using those as they behave unescaped should be fulfilled with a way to combine QueryNodes with an OR relationship instead of AND. IMO that is not something we need to deal with right now. I have opened a Launchpad ticket so we do not forget about it.

gramanas · 2017-06-14T19:19:00Z

Ok but don't think this is a place to fix such an issue so let's merge this branch and work on it in another.

Be-ing

Some minor style nitpicks, but nothing that needs to hold back merging this.

Be-ing · 2017-06-14T21:01:51Z

src/library/searchqueryparser.cpp

+                          &m_pTrackCollection->crates(), argument);
+                } else {
+                    //TODO(gramanas) It should generate a query to
+                    //match all the tracks that are not in a crate.


An explicit continue line here would make it a little easier to read.

Be-ing · 2017-06-14T21:02:06Z

src/library/searchqueryparser.cpp

+                    //match all the tracks that are not in a crate.
+                }
+            }
+            else {


Please put on one line:
} else {

Be-ing · 2017-06-14T21:05:09Z

src/test/searchqueryparsertest.cpp

    auto pQuery(
-        m_parser.parseQuery("comment:", searchColumns, ""));
+        m_parser.parseQuery("album: ", searchColumns, ""));


It would be good to add a test to check that "album: " and "album:" are treated the same.

daschuer · 2017-06-14T21:35:21Z

Works now good. Sorry for the delay.

gramanas · 2017-06-14T22:39:35Z

No problem, thanks!

CI: Update Docker image for GitHub pre-commit workflow

daschuer reviewed Jun 9, 2017

View reviewed changes

gramanas force-pushed the EmptyFilterBug branch from 6ed3db5 to 9f40252 Compare June 9, 2017 21:06

Fix empty filter bug

87d78ba

Apparently the SqlNode that was created could not match() as it should so I removed it and made TextFilter to be aware of the empty argument and act accordingly Add escape to crate filter argument

gramanas force-pushed the EmptyFilterBug branch from 9f40252 to 87d78ba Compare June 12, 2017 12:41

uklotzde reviewed Jun 12, 2017

View reviewed changes

Be-ing approved these changes Jun 14, 2017

View reviewed changes

daschuer merged commit 94ad1d5 into daschuer:jmigual-library-redesign Jun 14, 2017

gramanas deleted the EmptyFilterBug branch June 14, 2017 22:39

daschuer pushed a commit that referenced this pull request Sep 30, 2021

Merge pull request #19 from Holzhaus/github-pre-commit

bff6a40

CI: Update Docker image for GitHub pre-commit workflow

Fix empty filter bug #19

Fix empty filter bug #19

Conversation

gramanas commented Jun 7, 2017

daschuer commented Jun 7, 2017

jmigual commented Jun 7, 2017

gramanas commented Jun 7, 2017

gramanas commented Jun 7, 2017 • edited Loading

jmigual commented Jun 7, 2017

gramanas commented Jun 7, 2017

gramanas commented Jun 7, 2017

daschuer commented Jun 7, 2017

gramanas commented Jun 7, 2017

jmigual commented Jun 7, 2017

jmigual commented Jun 7, 2017

gramanas commented Jun 7, 2017

Be-ing commented Jun 8, 2017 • edited Loading

daschuer commented Jun 8, 2017 via email

Be-ing commented Jun 8, 2017

uklotzde commented Jun 8, 2017

gramanas commented Jun 8, 2017

uklotzde commented Jun 8, 2017

gramanas commented Jun 9, 2017

daschuer Jun 9, 2017

Choose a reason for hiding this comment

gramanas commented Jun 9, 2017 via email

daschuer commented Jun 11, 2017

gramanas commented Jun 11, 2017 • edited Loading

gramanas commented Jun 12, 2017

uklotzde Jun 12, 2017

Choose a reason for hiding this comment

gramanas commented Jun 13, 2017

jmigual commented Jun 14, 2017

daschuer commented Jun 14, 2017 • edited Loading

Be-ing commented Jun 14, 2017 • edited Loading

gramanas commented Jun 14, 2017

Be-ing left a comment

Choose a reason for hiding this comment

Be-ing Jun 14, 2017

Choose a reason for hiding this comment

Be-ing Jun 14, 2017

Choose a reason for hiding this comment

Be-ing Jun 14, 2017

Choose a reason for hiding this comment

daschuer commented Jun 14, 2017

gramanas commented Jun 14, 2017

gramanas commented Jun 7, 2017 •

edited

Loading

Be-ing commented Jun 8, 2017 •

edited

Loading

gramanas commented Jun 11, 2017 •

edited

Loading

daschuer commented Jun 14, 2017 •

edited

Loading

Be-ing commented Jun 14, 2017 •

edited

Loading