Standardise grouping and ordering of IRulesetInfo/RulesetInfos
#16852
Conversation
At first I was planning on making `CompareTo` implemented at `IRulesetInfo` itself and shared across classes, but turns out it only implements it explicitly and not allow direct `IRulesetInfo.Equals` calls. It messed with my head enough that I decided to just let each class have its own implementation and only allow same type.
Required for `APIBeatmap`s, which provide `Ruleset` instances with `OnlineID` available only. Also consistent with the comparer implementation.
|
One thing to keep in mind is that this won't be compatible with realm But I guess we can use this if it looks to be passing. |
|
Interesting point. |
osu.Game/Rulesets/RulesetInfo.cs
Outdated
| if (OnlineID >= 0 && other.OnlineID >= 0) | ||
| return OnlineID == other.OnlineID; |
There was a problem hiding this comment.
We don't do this in other models – why are we doing it here?
There was a problem hiding this comment.
Have mentioned it in commit description, but it's here to handle performing equality comparsions of rulesets provided by APIBeatmaps, in which they would only have an OnlineID present rather than ShortNames.
For cases like:
foreach (var rulesetBeatmaps in apiBeatmaps.GroupBy(b => b.Ruleset).OrderBy(g => g.Key))
...to work correctly.
There was a problem hiding this comment.
I don't think we should be doing this..
There was a problem hiding this comment.
Now that I look at how things are structured for APIBeatmap, I think APIRuleset is what we want going forward, with a custom Equals implementation that uses MatchesOnlineID. Curious if that's something you're against?
Would be a private class since its only benefit is the different and more correct Equals override.
There was a problem hiding this comment.
That's fine, I just don't want one Equals implementation of models to not match the others (right now, every implementation is the same across ruleset/beatmap/skin/etc).
| private class TestUnimplementedModOsuRuleset : OsuRuleset, ILegacyRuleset | ||
| { | ||
| public override string ShortName => "unimplemented"; | ||
|
|
||
| int ILegacyRuleset.LegacyID => -1; | ||
|
|
There was a problem hiding this comment.
Tests seem to pass without this being present – what's the reason for adding this here?
There was a problem hiding this comment.
Right, that's no longer required with 92e22c5.
This reverts commit c29cc78.
Since
RulesetInfoprovides anIComparable<T>implementation, all usages should always compare by it rather than arbitrarily ordering byOnlineID/ShortName.This fixes ruleset groups in
Editor's difficulty switch menu not being ordered appropriately:I have updated
EFRulesetInfoas well and isolated it from potentially comparing with realmRulesetInfos, in case that ever happens. Have tested performing an EF migration with this change and everything seem to still work.