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 last played search filter in song select #23129

Merged
merged 21 commits into from Feb 14, 2024

Conversation

Elvendir
Copy link
Contributor

@Elvendir Elvendir commented Apr 4, 2023

Answers #22908.

Adds date criteria for search filter in song select.
Adds corresponding Tests.
InspectCode.ps1 run.

Date criteria is an OptionalRange<DateTimeOffset> to be inline with the dates in BeatmapInfo.

tryUpdateDateRange updates the criteria:

  • accepted format are: any combination of 0y 0M 0d 0h 0m 0s where 0 can be any double as long as the order yMdhms is respected, and any double that will be converted to days (as in stable),
  • dateTimeOffset.Add#### is used to move the criteria in the past starting from DateTimeOffset.Now,
  • in the case where dateTimeOffset.Add#### would decrease the criteria under 01/01/0001 it is instead set to DateTimeOffset.MinValue +1 ms,
  • the operator is inverted (> becomes < and vice versa) to match stable behavior (meaning that played<30 shows the beatmaps played during the last 30 days so with a BeatmapInfo.LastPlayed > DateTimeOffset.Now - 30 days) .

If a beatmap does not have a LastPlayed date it is set to DateTimeOffset.MinValue in order to always be shown for played>## and always hidden for played<## (as in stable).

Notes:

  • could remove decimal for years, months and other or maybe have some constraint on them like in tryUpdateLengthRange,
  • could add some kind of leniency like in tryUpdateLengthRange if deemed useful/needed,
  • could add any other date in BeatmapInfo to search filter like LastUpdated, DateAdded and so on,
  • could add another type of date search filter so that played=2015y shows the beatmaps lastplayed in 2015.

Comment on lines 330 to 331
new object[] { "0.1y0.1M2d" },
new object[] { "0.99y0.99M2d" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does anyone really want or need decimal months/years? Those are possibly the least intuitive units under the sun.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with it being not intuitive (except maybe 0.5y). However, as the other units (d, h, m, s) accepts decimals it is to not make the search fail if decimal is used in y or M. I think it does not hurt the player if enabled.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It may not hurt the player but it complicates implementation for little reason. I still say remove it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was gonna remove this myself but then saw other places already support decimal so decided not to bother in the end.

osu.Game/Screens/Select/FilterQueryParser.cs Outdated Show resolved Hide resolved
Elvendir and others added 2 commits April 4, 2023 19:29
Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
Copy link
Member

@Susko3 Susko3 left a comment

Choose a reason for hiding this comment

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

Please try to pull the latest master when submitting a PR, and not have it based on a month old commit.

Comment on lines 452 to 453
case Operator.Greater:
return Operator.Less;
Copy link
Member

Choose a reason for hiding this comment

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

Strictly speaking, the reverse of Greater is LessOrEqual. Same goes for the rest.

Comment on lines 449 to 450
case Operator.Equal:
return Operator.Equal;
Copy link
Member

Choose a reason for hiding this comment

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

This is very misleading. There is no inverse of the equals operator defined, so this should probably throw. (Also the default branch doesn't make much sense).

Function could be renamed to reverseInequalityOperator() to match this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed function as proposed because the previous name was indeed misleading. I also changed the default case to be an out of range exception for unsupported operators.

- removed equality in tests
-changed default case of reverseInequalityOperator() to out of range exception
@Elvendir
Copy link
Contributor Author

Elvendir commented Apr 5, 2023

@Susko3 Sorry, I don' t know when it is best to pull master. How often should master be pulled ? Should I do it now ?

Copy link
Contributor

@brandondong brandondong left a comment

Choose a reason for hiding this comment

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

As suggested in #25885 (comment), the changes I made in #25885 are now review suggestion diff's in this PR.

osu.Game/Screens/Select/FilterQueryParser.cs Outdated Show resolved Hide resolved
osu.Game/Screens/Select/FilterQueryParser.cs Outdated Show resolved Hide resolved
osu.Game/Screens/Select/FilterQueryParser.cs Outdated Show resolved Hide resolved
@bdach
Copy link
Collaborator

bdach commented Feb 14, 2024

Since this is a relatively recurrent feature request and the code was mostly there I've done a pass on it so that we can get it into users' hands.

@bdach bdach merged commit 692ff8e into ppy:master Feb 14, 2024
15 of 17 checks passed
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.

None yet

5 participants