Track search: use := and quotes to find exact matches #12063

ronso0 · 2023-10-05T08:15:51Z

A quick check that has proven to work:
[text property]:="string" finds only tracks where the property is exactly string.
title:="Blob" finds all track titles that exactly "Blob", not "What a Blob" etc.

May be extended to include spaces and special chars before and after to find Blob and ..Blob!

Bug fix or feature? Doesn't change the ui, hence I targeted 2.4
But it reuires a manual update.

Closes #10699

uklotzde · 2023-10-05T23:42:35Z

src/library/searchquery.cpp

        : m_database(database),
          m_sqlColumns(sqlColumns),
-          m_argument(argument) {
+          m_argument(argument),
+          m_exactMatch(exactMatch) {


bool is a poor type choice. An enum class StringMatch with Equals and Contains variants would be much more concise, maintainable, and extensible.

Thanks for the hint.
Though this is on hold for now, someone else may adopt it.

Done.
And I implemented it in the existing text filter matcher, so the changes are much smaller, no code duplication anymore.

Swiftb0y

couldn't resist "meinen senf abzugeben" ;)

Thank you.

Swiftb0y · 2023-10-09T12:19:01Z

src/test/searchqueryparsertest.cpp

+    EXPECT_STREQ(
+            qPrintable(QString("comment LIKE 'asdf'")),
+            qPrintable(pQuery->toSql()));


this tests implementation details IMO and does not belong in a test IMO. wdyt?

These are crucial implementation details, sure they need to be tested.
Please elaborate.

well it conflates two different levels of abstraction. The first part of the test checks for the behavior. That should be very stable and even if we changed some details about our database these won't change. On the other hand, all the tests that involve toSql don't only rely on the fact that we use SQL (something that would not be true if we were to transition to aoide) but even how exactly we do the fuzzy matching (using LIKE) even though there are other implementations as well. Do you see how this introduces coupling? The test should be ensuring the behavior, not the way it works internally.
A little (admittedly childish and simplified) analogy: If I need to put a nail into a wall, I can just use a hammer, I don't care if the hammerhead is made of iron or steel, both do the job (I think, I'm not a carpenter 😅).
Sure if you need to test specific properties of a hammer, that is legitimate to test, but it doesn't belong in the same test as "it puts a nail into a wall".

I'm aware that this is quite theoretical and likely a little controversial the code in question just follows the same pattern as the rest of the file. I'm just saying that the current design is not great and we should question it.

got it. of course, ideally, we'd just test the parser to return certain track ids of a sample library.

src/library/searchquery.cpp

Swiftb0y · 2023-10-09T12:32:42Z

src/library/searchqueryparser.cpp

 QString SearchQueryParser::getTextArgument(QString argument,
-                                           QStringList* tokens) const {
+        QStringList* tokens,
+        StringMatch* matchMode) const {


I dislike the implicitness of the out parameter. Eg its not fully transparent if the pointer is valid but shouldMatchExactly is false. then it'll just not set anything. A caller might expect it to always set it. The result is that if used incorrectly (which is not hard in this case), the caller could be left with an initialized variable.

Imo the better design would be not use an out-parameter in this case and instead just return a struct with both the matchmode and the argument.

Suggested change

QString SearchQueryParser::getTextArgument(QString argument,

QStringList* tokens) const {

QStringList* tokens,

StringMatch* matchMode) const {

struct TextArgumentResult {

QString argumentValue;

StringMatch matchmode;

};

TextArgumentResult SearchQueryParser::getTextArgument(QString argument,

QStringList* tokens) const {

Eg its not fully transparent if the pointer is valid but shouldMatchExactly is false. then it'll just not set anything.

For now, I fixed that by setting matchMode at the beginning.

A struct would for me, too, but it requires more refactoring.

not so much imo. PR underway

src/library/searchquery.cpp

Co-authored-by: Swiftb0y <[email protected]>

ronso0 · 2023-10-10T02:00:23Z

Looking good now.
@Swiftb0y Please let me know if you're happy with the fixup.

ronso0 · 2023-10-10T07:13:05Z

src/library/searchqueryparser.cpp

    if (argument.startsWith("=")) {
        // strip the '=' from the argument
        argument = argument.mid(1);
+        // TODO(ronso0) should 'tag:"string"' really be equal to 'tag:="string"?


edit
tag:=string and tag:="string"

src/library/searchqueryparser.cpp

Swiftb0y · 2023-10-10T11:08:25Z

lgtm otherwise. thank you.

src/library/searchqueryparser.cpp

Swiftb0y · 2023-10-10T14:34:50Z

fyi the parser could like be simplified even more if the tokenizer recognized the quotes and just consumed the entire thing as one token. That would make consumeQuotedArgument largely unnecessary...

ronso0 · 2023-10-11T09:14:54Z

fyi the parser could like be simplified even more if the tokenizer recognized the quotes and just consumed the entire thing as one token. That would make consumeQuotedArgument largely unnecessary...

Tbh I'm not motivated to do more optimization, this has already grown way beyond my initial commit (though I very much appreciate your input!).

Swiftb0y · 2023-10-11T10:09:37Z

Tbh I'm not motivated to do more optimization, this has already grown way beyond my initial commit (though I very much appreciate your input!).

Yes, I'm sorry for blowing up the scope. The previous comment was just a remark and not a request for more changes.

Swiftb0y

LGTM, Thank you very much.

Swiftb0y · 2023-10-11T10:12:18Z

remove draft and merge?

ronso0 · 2023-10-11T10:24:12Z

Ready!

TODO
update the manual

Swiftb0y · 2023-10-11T10:38:14Z

Thank you

ronso0 · 2023-10-11T22:52:09Z

I'd say this deserves a changelog entry to make the change a bit more discoverable for upgraders (if they already read the search documentation they probably won't notice the change..).
Wdyt?

ronso0 marked this pull request as draft October 5, 2023 08:15

github-actions bot added the library label Oct 5, 2023

ronso0 mentioned this pull request Oct 5, 2023

a single table cell is drawn as selected when selecting a previous search query #12064

Closed

uklotzde reviewed Oct 5, 2023

View reviewed changes

ronso0 force-pushed the search-exact branch from 42ba552 to 2d8d1fc Compare October 8, 2023 22:02

github-actions bot added the code quality label Oct 8, 2023

ronso0 force-pushed the search-exact branch 2 times, most recently from 9260d0b to fa98937 Compare October 9, 2023 11:30

Swiftb0y reviewed Oct 9, 2023

View reviewed changes

ronso0 changed the title ~~Track search: use = and quotes to find exact matches~~ Track search: use := and quotes to find exact matches Oct 9, 2023

ronso0 force-pushed the search-exact branch from fa98937 to 73112fc Compare October 9, 2023 13:24

Search: [text property]:="string" finds only equal strings

524bcc7

Co-authored-by: Swiftb0y <[email protected]>

ronso0 force-pushed the search-exact branch 2 times, most recently from 31f95ef to 62da287 Compare October 10, 2023 01:26

ronso0 commented Oct 10, 2023

View reviewed changes

ronso0 force-pushed the search-exact branch from 62da287 to d97ccf3 Compare October 10, 2023 09:57

refactor: remove unnecessary ~SearchQueryParser

d938e9a

ronso0 force-pushed the search-exact branch from d97ccf3 to 237e381 Compare October 10, 2023 09:59

Swiftb0y reviewed Oct 10, 2023

View reviewed changes

src/library/searchqueryparser.cpp Outdated Show resolved Hide resolved

Swiftb0y reviewed Oct 10, 2023

View reviewed changes

src/library/searchqueryparser.cpp Outdated Show resolved Hide resolved

ronso0 force-pushed the search-exact branch 2 times, most recently from ffcd6c0 to 51c2a38 Compare October 10, 2023 13:35

Swiftb0y reviewed Oct 10, 2023

View reviewed changes

src/library/searchqueryparser.cpp Outdated Show resolved Hide resolved

refactor: SearchQueryParser::getTextArgument

ce9128f

ronso0 force-pushed the search-exact branch from 51c2a38 to ce9128f Compare October 11, 2023 08:13

Swiftb0y approved these changes Oct 11, 2023

View reviewed changes

ronso0 marked this pull request as ready for review October 11, 2023 10:24

Swiftb0y merged commit 1087d9b into mixxxdj:2.4 Oct 11, 2023

ronso0 deleted the search-exact branch October 11, 2023 10:48

This was referenced Oct 11, 2023

Library: document new text filter operator '=' (find exact matches) mixxxdj/manual#590

Merged

Search: allow to find exact matches, like title:="Nice" #10699

Closed

ronso0 mentioned this pull request Oct 12, 2023

Search: add special BPM filters #12072

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track search: use := and quotes to find exact matches #12063

Track search: use := and quotes to find exact matches #12063

ronso0 commented Oct 5, 2023 •

edited

Loading

uklotzde Oct 5, 2023 •

edited

Loading

ronso0 Oct 6, 2023

ronso0 Oct 8, 2023

Swiftb0y left a comment

Swiftb0y Oct 9, 2023

ronso0 Oct 9, 2023

Swiftb0y Oct 9, 2023

ronso0 Oct 9, 2023

Swiftb0y Oct 9, 2023

ronso0 Oct 9, 2023

Swiftb0y Oct 9, 2023

Swiftb0y Oct 9, 2023

ronso0 commented Oct 10, 2023

ronso0 Oct 10, 2023 •

edited

Loading

Swiftb0y commented Oct 10, 2023

Swiftb0y commented Oct 10, 2023

ronso0 commented Oct 11, 2023

Swiftb0y commented Oct 11, 2023

Swiftb0y left a comment

Swiftb0y commented Oct 11, 2023

ronso0 commented Oct 11, 2023

Swiftb0y commented Oct 11, 2023

ronso0 commented Oct 11, 2023

Track search: use := and quotes to find exact matches #12063

Track search: use := and quotes to find exact matches #12063

Conversation

ronso0 commented Oct 5, 2023 • edited Loading

uklotzde Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y 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

ronso0 commented Oct 10, 2023

ronso0 Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Swiftb0y commented Oct 10, 2023

Swiftb0y commented Oct 10, 2023

ronso0 commented Oct 11, 2023

Swiftb0y commented Oct 11, 2023

Swiftb0y left a comment

Choose a reason for hiding this comment

Swiftb0y commented Oct 11, 2023

ronso0 commented Oct 11, 2023

Swiftb0y commented Oct 11, 2023

ronso0 commented Oct 11, 2023

ronso0 commented Oct 5, 2023 •

edited

Loading

uklotzde Oct 5, 2023 •

edited

Loading

ronso0 Oct 10, 2023 •

edited

Loading