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
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
102576c
adddede LastPlayed as filter option in beatmap carousel
Elvendir Mar 18, 2023
216a88e
limited max Date comparison
Elvendir Mar 18, 2023
6dead81
Generalized tryUpdateLastPlayedRange to tryUpdateDateRange and Added…
Elvendir Mar 19, 2023
4b053b4
changed regex match to be inline with standard
Elvendir Apr 1, 2023
52adb99
Update osu.Game.Tests/NonVisual/Filtering/FilterQueryParserTest.cs
Elvendir Apr 4, 2023
d6c6507
Update osu.Game/Screens/Select/FilterQueryParser.cs
Elvendir Apr 4, 2023
0c1d6eb
- rewrote upper and lower bound tests
Elvendir Apr 5, 2023
df17051
-renamed function inverse() to reverseInequalityOperator() for clarity
Elvendir Apr 5, 2023
c2f225f
Made Operator.Equal not parse for date filter and added corresponding…
Elvendir Apr 5, 2023
928145c
Enforce integer value before y and M
Elvendir Apr 5, 2023
8e156fd
Enforce integer through regex match instead
Elvendir Apr 6, 2023
8ba9677
Merge branch 'master' into add-last-played-filter
peppy Jun 6, 2023
1113355
Merge branch 'master' into add-last-played-filter
bdach Feb 14, 2024
f0f37df
Revert unnecessary change
bdach Feb 14, 2024
d7dfc8b
Add failing test coverage for empty date filter not parsing
bdach Feb 14, 2024
c24328d
Abandon date filter if no meaningful time bound found during parsing
bdach Feb 14, 2024
a8ae0a0
Simplify parsing
bdach Feb 14, 2024
f1d69ab
Rename test
bdach Feb 14, 2024
414066f
Inline problematic function (and rename things to make more sense)
bdach Feb 14, 2024
f7bea00
Improve test coverage
bdach Feb 14, 2024
ae9a266
Sprinkle some raw string prefixes
bdach Feb 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
81 changes: 81 additions & 0 deletions osu.Game.Tests/NonVisual/Filtering/FilterQueryParserTest.cs
Expand Up @@ -316,5 +316,86 @@ public bool TryParseCustomKeywordCriteria(string key, Operator op, string value)
return false;
}
}

//Date criteria testing
Elvendir marked this conversation as resolved.
Show resolved Hide resolved

private static readonly object[] correct_date_query_examples =
{
new object[] { "600" },
new object[] { "0.5s" },
new object[] { "120m" },
new object[] { "48h120s" },
new object[] { "10y24M" },
new object[] { "10y60d120s" },
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.

};

[Test]
[TestCaseSource(nameof(correct_date_query_examples))]
public void TestValidDateQueries(string dateQuery)
{
string query = $"played={dateQuery} time";
bdach marked this conversation as resolved.
Show resolved Hide resolved
var filterCriteria = new FilterCriteria();
FilterQueryParser.ApplyQueries(filterCriteria, query);
Assert.AreEqual(true, filterCriteria.LastPlayed.HasFilter);
}

private static readonly object[] incorrect_date_query_examples =
{
new object[] { "7m27" },
new object[] { "7m7m7m" },
new object[] { "5s6m" },
new object[] { "7d7y" },
new object[] { ":0" },
new object[] { "0:3:6" },
new object[] { "0:3:" },
new object[] { "\"three days\"" }
};

[Test]
[TestCaseSource(nameof(incorrect_date_query_examples))]
public void TestInvalidDateQueries(string dateQuery)
{
string query = $"played={dateQuery} time";
var filterCriteria = new FilterCriteria();
FilterQueryParser.ApplyQueries(filterCriteria, query);
Assert.AreEqual(false, filterCriteria.LastPlayed.HasFilter);
}

private static readonly object[] list_operators =
{
new object[] { "=", false, false, true, true },
new object[] { ":", false, false, true, true },
new object[] { "<", false, true, false, false },
new object[] { "<=", false, true, true, false },
new object[] { "<:", false, true, true, false },
new object[] { ">", true, false, false, false },
new object[] { ">=", true, false, false, true },
new object[] { ">:", true, false, false, true }
};

[Test]
[TestCaseSource(nameof(list_operators))]
public void TestComparisonDateQueries(string ope, bool minIsNull, bool maxIsNull, bool isLowerInclusive, bool isUpperInclusive)
{
string query = $"played{ope}50";
var filterCriteria = new FilterCriteria();
FilterQueryParser.ApplyQueries(filterCriteria, query);
Assert.AreEqual(isLowerInclusive, filterCriteria.LastPlayed.IsLowerInclusive);
Assert.AreEqual(isUpperInclusive, filterCriteria.LastPlayed.IsUpperInclusive);
Assert.AreEqual(maxIsNull, filterCriteria.LastPlayed.Max == null);
Assert.AreEqual(minIsNull, filterCriteria.LastPlayed.Min == null);
}
bdach marked this conversation as resolved.
Show resolved Hide resolved

[Test]
public void TestOutofrangeDateQuery()
{
const string query = "played=10000y";
var filterCriteria = new FilterCriteria();
FilterQueryParser.ApplyQueries(filterCriteria, query);
Assert.AreEqual(true, filterCriteria.LastPlayed.HasFilter);
Assert.AreEqual(DateTimeOffset.MinValue.AddMilliseconds(1), filterCriteria.LastPlayed.Min);
}
}
}
1 change: 1 addition & 0 deletions osu.Game/Screens/Select/Carousel/CarouselBeatmap.cs
Expand Up @@ -48,6 +48,7 @@ private bool checkMatch(FilterCriteria criteria)
match &= !criteria.CircleSize.HasFilter || criteria.CircleSize.IsInRange(BeatmapInfo.Difficulty.CircleSize);
match &= !criteria.OverallDifficulty.HasFilter || criteria.OverallDifficulty.IsInRange(BeatmapInfo.Difficulty.OverallDifficulty);
match &= !criteria.Length.HasFilter || criteria.Length.IsInRange(BeatmapInfo.Length);
match &= !criteria.LastPlayed.HasFilter || criteria.LastPlayed.IsInRange(BeatmapInfo.LastPlayed ?? DateTimeOffset.MinValue);
match &= !criteria.BPM.HasFilter || criteria.BPM.IsInRange(BeatmapInfo.BPM);

match &= !criteria.BeatDivisor.HasFilter || criteria.BeatDivisor.IsInRange(BeatmapInfo.BeatDivisor);
Expand Down
1 change: 1 addition & 0 deletions osu.Game/Screens/Select/FilterCriteria.cs
Expand Up @@ -31,6 +31,7 @@ public class FilterCriteria
public OptionalRange<double> BPM;
public OptionalRange<int> BeatDivisor;
public OptionalRange<BeatmapOnlineStatus> OnlineStatus;
public OptionalRange<DateTimeOffset> LastPlayed;
public OptionalTextFilter Creator;
public OptionalTextFilter Artist;

Expand Down
96 changes: 95 additions & 1 deletion osu.Game/Screens/Select/FilterQueryParser.cs
Expand Up @@ -61,6 +61,10 @@ private static bool tryParseKeywordCriteria(FilterCriteria criteria, string key,
case "length":
return tryUpdateLengthRange(criteria, op, value);

case "played":
case "lastplayed":
return tryUpdateDateRange(ref criteria.LastPlayed, op, value);

case "divisor":
return TryUpdateCriteriaRange(ref criteria.BeatDivisor, op, value, tryParseInt);

Expand Down Expand Up @@ -109,7 +113,8 @@ private static Operator parseOperator(string value)
value.EndsWith("ms", StringComparison.Ordinal) ? 1 :
value.EndsWith('s') ? 1000 :
value.EndsWith('m') ? 60000 :
value.EndsWith('h') ? 3600000 : 1000;
value.EndsWith('h') ? 3600000 :
value.EndsWith('d') ? 86400000 : 1000;
bdach marked this conversation as resolved.
Show resolved Hide resolved

private static bool tryParseFloatWithPoint(string value, out float result) =>
float.TryParse(value, NumberStyles.AllowDecimalPoint, CultureInfo.InvariantCulture, out result);
Expand Down Expand Up @@ -368,5 +373,94 @@ private static bool tryUpdateLengthRange(FilterCriteria criteria, Operator op, s

return tryUpdateCriteriaRange(ref criteria.Length, op, totalLength, minScale / 2.0);
}

private static bool tryUpdateDateRange(ref FilterCriteria.OptionalRange<DateTimeOffset> dateRange, Operator op, string val)
{
GroupCollection? match = null;

match ??= tryMatchRegex(val, @"^((?<years>\d+(\.\d+)?)y)?((?<months>\d+(\.\d+)?)M)?((?<days>\d+(\.\d+)?)d)?((?<hours>\d+(\.\d+)?)h)?((?<minutes>\d+(\.\d+)?)m)?((?<seconds>\d+(\.\d+)?)s)?$");
match ??= tryMatchRegex(val, @"^(?<days>\d+(\.\d+)?)$");

if (match == null)
return false;

DateTimeOffset dateTimeOffset = DateTimeOffset.Now;

try
{
List<string> keys = new List<string> { "seconds", "minutes", "hours", "days", "months", "years" };

foreach (string key in keys)
{
if (match[key].Success)
{
if (!tryParseDoubleWithPoint(match[key].Value, out double length))
return false;

switch (key)
{
case "seconds":
dateTimeOffset = dateTimeOffset.AddSeconds(-length);
break;

case "minutes":
dateTimeOffset = dateTimeOffset.AddMinutes(-length);
break;

case "hours":
dateTimeOffset = dateTimeOffset.AddHours(-length);
break;

case "days":
dateTimeOffset = dateTimeOffset.AddDays(-length);
break;

case "months":
dateTimeOffset = dateTimeOffset.AddMonths(-(int)Math.Floor(length));
dateTimeOffset = dateTimeOffset.AddDays(-30 * (length - Math.Floor(length)));
break;

case "years":
dateTimeOffset = dateTimeOffset.AddYears(-(int)Math.Floor(length));
dateTimeOffset = dateTimeOffset.AddDays(-365 * (length - Math.Floor(length)));
break;
}
}
}
}
// If DateTime to compare is out-scope put it to Min
catch (Exception)
Elvendir marked this conversation as resolved.
Show resolved Hide resolved
{
dateTimeOffset = DateTimeOffset.MinValue;
dateTimeOffset = dateTimeOffset.AddMilliseconds(1);
}

return tryUpdateCriteriaRange(ref dateRange, invert(op), dateTimeOffset);
}

// Function to reverse an Operator
bdach marked this conversation as resolved.
Show resolved Hide resolved
private static Operator invert(Operator ope)
{
switch (ope)
{
default:
return Operator.Equal;

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.

bdach marked this conversation as resolved.
Show resolved Hide resolved

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.


case Operator.GreaterOrEqual:
return Operator.LessOrEqual;

case Operator.Less:
return Operator.Greater;

case Operator.LessOrEqual:
return Operator.GreaterOrEqual;
}
}
}
}