Skip to content
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

Add exact match support for song select searches #24045

Merged
merged 8 commits into from
Jun 27, 2023

Conversation

peppy
Copy link
Member

@peppy peppy commented Jun 26, 2023

As requested at #24005 (reply in thread). Seemed like a quick one to add and quite beneficial QoL wise so knocked it out.

Of note, this could possibly be implemented in the following code block instead:

internal static void ApplyQueries(FilterCriteria criteria, string query)
{
foreach (Match match in query_syntax_regex.Matches(query))
{
string key = match.Groups["key"].Value.ToLowerInvariant();
var op = parseOperator(match.Groups["op"].Value);
string value = match.Groups["value"].Value;
if (tryParseKeywordCriteria(criteria, key, value, op))
query = query.Replace(match.ToString(), "");
}
criteria.SearchText = query;
}

Only noticed this after writing the version I did. Not sure if it makes more or less sense.

@@ -246,6 +272,7 @@ public void TestApplyArtistQueriesWithSpaces()
Assert.AreEqual("really like yes", filterCriteria.SearchText.Trim());
Assert.AreEqual(3, filterCriteria.SearchTerms.Length);
Assert.AreEqual("name with space", filterCriteria.Artist.SearchTerm);
Assert.That(filterCriteria.Artist.Exact, Is.True);
Copy link
Member Author

Choose a reason for hiding this comment

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

Please double check this during review. Currently this change forces all searches like artist="this artist" to exact mode. The quotes here were used to group the search to be scoped to artist, but not necessarily expecting exact match.

I think this seems fine, but worth double-checking with expectations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably fine until proven otherwise.

@bdach
Copy link
Collaborator

bdach commented Jun 26, 2023

I'm not sure... this works?

osu_2023-06-26_21-45-24

Shouldn't the second result not be there? Reading the relevant discussion that's what I'd say the user wants out of this feature.

@peppy
Copy link
Member Author

peppy commented Jun 26, 2023

Shouldn't the second result not be there? Reading the relevant discussion that's what I'd say the user wants out of this feature.

Hmm, I don't know about that. I implemented this based on the way quotations work in other search engines / contexts. But maybe for osu!'s context it's fine to change it to match the full context?

@@ -133,10 +153,23 @@ public bool Matches(string value)
if (string.IsNullOrEmpty(value))
return false;

if (Exact)
return Regex.IsMatch(value, $@"(^|\s){searchTerm}($|\s)", RegexOptions.IgnoreCase | RegexOptions.CultureInvariant);
Copy link
Collaborator

@bdach bdach Jun 26, 2023

Choose a reason for hiding this comment

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

Why is this not just

Suggested change
return Regex.IsMatch(value, $@"(^|\s){searchTerm}($|\s)", RegexOptions.IgnoreCase | RegexOptions.CultureInvariant);
return value.Equals(searchTerm, StringComparison.OrdinalIgnoreCase);

What's the whitespace-or-string-{start,end} thing doing here?

Copy link
Member Author

Choose a reason for hiding this comment

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

because you don't want "you" to match "your"

Copy link
Collaborator

@bdach bdach Jun 26, 2023

Choose a reason for hiding this comment

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

Okay, so it won't match "your", but it might match "will you", or "you will", or whatever, because what it's trying to do is trying to test full word phrases rather than substrings.

Again, doesn't fix the problem with song with a title of just "you", as typing "you" will still match a bunch of things with the word "you" in it. It'll be some results less but I imagine still a lot. Probably fine though.

@bdach
Copy link
Collaborator

bdach commented Jun 26, 2023

I implemented this based on the way quotations work in other search engines / contexts. But maybe for osu!'s context it's fine to change it to match the full context?

I'm not opposed to having it behave like search engines do, but I am confused by the OP claiming that this solves a need in the linked discussion that this change does not in fact appear to solve.

@peppy
Copy link
Member Author

peppy commented Jun 26, 2023

A good reason for matching standard implementation is because it's also supported in the same way in elasticsearch (aka beatmap listing and other online components).

We could probably add something similar to what the user suggests as a follow up for the specific case they want (ie. exact=), although obviously that won't work with title= prefixes so well. Another option would be double quotes or something equally custom.

@bdach
Copy link
Collaborator

bdach commented Jun 26, 2023

After the initial confusion has been cleared up this looks to be working pretty well now that I've recalibrated my expectations. Pushed a few minor fixes (4cb122d is particularly important, as not escaping bare user input before putting it into a regex could lead to a runtime crash), but looks good otherwise. Please check though.

@peppy peppy merged commit 9b08aaf into ppy:master Jun 27, 2023
@peppy peppy deleted the exact-match-song-select branch June 27, 2023 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants