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 ability to search for difficulty names using square brackets #24921

Merged
merged 15 commits into from
Oct 24, 2023

Conversation

Pasi4K5
Copy link
Contributor

@Pasi4K5 Pasi4K5 commented Sep 25, 2023

Closes #24855

This adds the ability to search for difficulty names using the following Regex:
(\s|^)((\[(?>\[(?<level>)|[^[\]]+|\](?<-level>))*(?(level)(?!))\](\s|$))|(\[.*))

The syntax criteria for the difficulty search query are as follows:

  • Must be wrapped in square brackets ([]). If there is only an opening square bracket, it is treated as if the closing bracket was at the end of the query.
  • Must be separated from any surrounding text using one or multiple whitespaces ("disco prince[normal]" <- This does not search for difficulty names. "disco prince [normal]" <- This searches for "normal".). This is to make sure that most song titles, artists, tags etc. with square brackets in them are still searchable.

Other things to note:

  • If there are nested square brackets, the outer most ones are used. This means that it is possible to search for difficulty names with square brackets in them.
  • It is possible to have multiple difficulty name queries (e.g. "miiro [insane] [iljaaz]". not sure why anyone would do that, but it works).
  • Unlike in stable, the text between the square brackets does not need to exactly match the difficulty name. However, it is possible to use quotation marks inside the square brackets for exact matches. This can be useful to search for difficulties with a lot of spaces between letters, like "A r M i N's Extra" etc.
  • Currently, there is no way to escape a [. Not sure if that's necessary.

I added a bunch of TestCases to make sure the RegEx actually does what it's supposed to do.

internal static void ApplyQueries(FilterCriteria criteria, string query)
{
while (true)
Copy link
Member

Choose a reason for hiding this comment

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

This has to be rewritten to avoid using while(true). It's just too dangerous.

I also wonder if regex should even be used for this as it obfuscates things and is likely not adding much function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay. I did it without the Regex now, let me know if you think that this is a better solution.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure. Neither solution is really appealing. Will wait for another opinion.


string performExtraction(ref string query)
{
var searchTexts = new List<string>();
Copy link
Member

Choose a reason for hiding this comment

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

at the point of using lists and string splitting, it's probably less efficient than regex. not sure.

@peppy peppy self-requested a review October 24, 2023 05:55
@pull-request-size pull-request-size bot added size/M and removed size/L labels Oct 24, 2023
@peppy
Copy link
Member

peppy commented Oct 24, 2023

I've simplified this down to something I'm willing to work with. It doesn't support one test you had, but I think that's okay because it's such an edge case already. If you need to be searching for brackets inside difficulty names you should use diff: or diff= instead (added now).

Will leave this open for review and self-merge at end of day if no bites.

peppy
peppy previously approved these changes Oct 24, 2023
@bdach bdach self-requested a review October 24, 2023 07:14
@bdach
Copy link
Collaborator

bdach commented Oct 24, 2023

Just to be sure: there are actual beatmaps wherein the title of the track has square brackets (https://osu.ppy.sh/beatmapsets/355573#osu/787717). Doing what this PR does will break searching them with the square brackets:

before after
osu_2023-10-24_09-18-09 osu_2023-10-24_09-22-10

I can't quickly check what stable does, but is this to be considered okay?

@peppy
Copy link
Member

peppy commented Oct 24, 2023

Dunno. We could match them in addition I guess? at which point there's likely no meaning to adding the [] search at all (since the global search will cover this case already) and it should just be left to "diff:" as with other cases.

I'm fine with that direction, simplifies things further.

@bdach
Copy link
Collaborator

bdach commented Oct 24, 2023

We could match them in addition I guess? at which point there's likely no meaning to adding the [] search at all (since the global search will cover this case already)

I mean yes and no, you could still do things like Track Title [Difficulty Name] and effectively ad-hoc an AND filter. That's probably really niche, though.

I dunno. Current behaviour is probably fine until someone complains?

@peppy
Copy link
Member

peppy commented Oct 24, 2023

How about

diff --git a/osu.Game/Screens/Select/FilterCriteria.cs b/osu.Game/Screens/Select/FilterCriteria.cs
index 5666f9d1d4..46cc654925 100644
--- a/osu.Game/Screens/Select/FilterCriteria.cs
+++ b/osu.Game/Screens/Select/FilterCriteria.cs
@@ -69,28 +69,18 @@ public string SearchText
 
                 string remainingText = value;
 
-                // Match either an open difficulty tag to the end of string,
-                // or match a closed one with a whitespace after it.
-                //
-                // To keep things simple, the closing ']' may be included in the match group,
-                // and is trimmer post-match.
-                foreach (Match quotedSegment in Regex.Matches(value, "(^|\\s)\\[(.*)(\\]\\s|$)"))
-                {
-                    DifficultyName = new OptionalTextFilter
-                    {
-                        SearchTerm = quotedSegment.Groups[2].Value.Trim(']')
-                    };
-
-                    remainingText = remainingText.Replace(quotedSegment.Value, string.Empty);
-                }
-
                 // First handle quoted segments to ensure we keep inline spaces in exact matches.
-                foreach (Match quotedSegment in Regex.Matches(value, "(\"[^\"]+\"[!]?)"))
+                foreach (Match quotedSegment in Regex.Matches(remainingText, "(\"[^\"]+\"[!]?)"))
                 {
                     terms.Add(new OptionalTextFilter { SearchTerm = quotedSegment.Value });
                     remainingText = remainingText.Replace(quotedSegment.Value, string.Empty);
                 }
 
+                remainingText =
+                    remainingText
+                        .Replace("]", " ")
+                        .Replace("[", " ");
+
                 // Then handle the rest splitting on any spaces.
                 terms.AddRange(remainingText.Split(' ', StringSplitOptions.RemoveEmptyEntries).Select(s => new OptionalTextFilter
                 {

I think this would cover most scenarios people would be searching for. Still allows searching for bracket inclusion when using quotes or a prefix.

@bdach
Copy link
Collaborator

bdach commented Oct 24, 2023

That would just make square brackets a dead character, wouldn't it? For instance [orange] would match beatmaps with orange in the track title, even if they don't have a difficulty with orange in them.

I'd rather just keep what this branch does right now at that point.

@bdach bdach merged commit 19be005 into ppy:master Oct 24, 2023
@Pasi4K5 Pasi4K5 deleted the diff-name-search branch October 24, 2023 12:33
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.

Add ability to search for difficulty names using surrounding brackets
3 participants