Support filtering for multiple statuses when searching beatmaps in the map picker

[vladfrangu]

This PR adds in the ability to filter by multiple statuses when searching for beatmaps

(I'll be honest, I've only done this because I wanted to be able to do status=r status=l when playing osu, as there are some awesome loved maps I'd want to try to play :D)

osu.-.2024-03-16.at.21.35.13.mp4

Discussion: should this parse the status input instead, meaning support status=ranked,loved? I was going to try to implement it that way but it felt too obscure and complex (also unintuitive to me)

[bdach]

We don't really use the "conventional commits" commit message... convention here. Not sure I'd bother.

[vladfrangu]

Will apply the feedback possibly later today, thanks! (and also add in tests to the status>= feature so its tested

As for conventional commits... I'm just too used to using them daily 😅, but if need be I can reword the commits!

[bdach]

As for conventional commits... I'm just too used to using them daily 😅, but if need be I can reword the commits!

No need for that, they can stay.

[vladfrangu]

I've applied the requested changes, but I'd love some feedback on how to handle the addition of the values to the set. I don't really like the way I did it but I also dunno if there's a better way (as I'm quite rusty in C#). Also, not fully sure it SortedSet is the ideal struct for this.. 😅

[bdach]

So I pushed in some further commits to resolve minor stylistic nits but in general I've got a bit of a doubt as to whether we want this in this form.

The reason that makes me question this a bit is that when you specify a compound status such as status>r status<r, it would previously match nothing, because the way compound range filters are parsed is that the final range is an intersection of all provided ranges, which would indeed yield empty set in this particular case. This PR makes it so that the same filter is now evaluated as a union, and as such evaluates to mostly ranked!=r.

I dunno if we want this. It's different to stable and kinda different to every single other filter. @ppy/team-client this is very not important but this change is basically good to go otherwise so if you can help with a quick yes/no then we can just check this off the review list for good in either direction...

addressed

[smoogipoo]

Going off the video in the OP alone, my expectation would be for it to be an intersection.

I was wondering whether it would be possible to create a keyword in-between two filter expressions to make it into a union. For example:
status=r or status=l
status=r ? status=l
and specifically, it would only turn into a union if the two expressions were the same.

Not sure how complex this would be though - this PR adds support for one expression in a very isolated way.

[bdach]

Not sure how complex this would be though

Given current structure it's likely a completely different PR that supersedes this one entirely and allows arbitrary compound boolean expression. I dunno about complexity, it's probably going to be not-trivial at least.

I was considering different operators but I couldn't figure a good one to use. Maybe we can do something like status=[ranked,loved] or something instead?

[smoogipoo]

Sure, or removing the [] as suggested in the OP (otherwise parsing becomes more complex when spaces are involved). I'm not sure it's as non-trivial as it sounds actually, because it looks like the query parser can already handle this case:

[bdach]

Commas within a single key-value pair should be fine yeah. It's the merging of multiple key=value terms with different boolean expressions that I was worried about.

[vladfrangu]

Should I then refactor this to support status=ranked,loved? What should happen when someone does status>=ranked,loved? Check that the status is greater than both?

[bdach]

Should I then refactor this to support status=ranked,loved?

I guess that's the consensus yes.

What should happen when someone does status>=ranked,loved? Check that the status is greater than both?

Filter should fail to parse. Comma-delimited values should only be supported on equality queries.

[vladfrangu]

Gotcha gotcha, will look into it today!

One last question then...what should happen when someone does status=ranked status=loved? Should the latter override the former, or stay as an intersection?

[bdach]

Intersection please.

[vladfrangu]

And I assume status>=r status>=l should result in only the latter check matching? Just covering my bases so I know what's expected! Or, do these cases make sense (I'll be adding tests for each of them too):

• status=r status=l -> match ranked or loved (? this isn't an intersection tho.. so then it'd maybe match only loved, like in stable?)
• status=r,l -> match ranked or loved
• status>=ranked -> current stable behavior, match ranked and anything over it
• status>=r,l -> do nothing
• status>r status<r -> matches...nothing? This one will be rougher to implement me thinks but it should be finee
• status>r status<l-> matches anything between ranked and loved

[bdach]

• status=r status=l -> match ranked or loved (? this isn't an intersection tho.. so then it'd maybe match only loved, like in stable?)

This should ideally match nothing, but that might be annoying, so fine to let the last filter win if needed.

• status>r status<r -> matches...nothing? This one will be rougher to implement me thinks but it should be finee

Correct, nothing would be the goal.

• status>r status<l-> matches anything between ranked and loved

Between ranked and loved, both ends exclusive. But yes.

Rest seems fine.

[vladfrangu]

I've pushed the requested changes, and added test cases for most queries. Hopefully I didn't miss anything obvious (outside of maybe code style)

[bdach]

I'm gonna be honest, I really didn't like shoehorning the Values thing into OptionalRange<T>; it felt really artificial and made the type have a weird duality of purpose of sorts. I also want to just finally move this PR along. So I kept the tests and adapted them to the OptionalSet<T> thing we had earlier and it feels miles better imo.

[vladfrangu]

Yeah, checking the code I agree, its definitely better than before, thanks! It also helps me learn more about c# and such 🙏💙