Implement song select v2 length and bpm statistic pill

[Joehuu]

• Part of Implement new components for song select wedge redesign #28063

This component was undertested on the last PR, so added more tests mostly taken from TestSceneBeatmapInfoWedge.

I didn't take action on @frenzibyte's design proposal in #28063 (comment), because the height increases and causes the left side and the component itself (with no varying bpm) to have dead space and would need a relayout. The latest figma design iteration did move it to the right of the beatmap title, but I'd guess it'll hide the background more with the proposed change.

Removing the "mostly" somewhat alleviates the jumping problem. If it's not enough, I would rather make it a left-aligned component again (like current lazer song select design and stable's) to make people not relearn the location.

[bdach]

Why is this split in two? Why is there a LengthAndBPMStatisticPill and a local variant apparently? When are we ever going to use this component in a non-local context on song select?

[Joehuu]

This follows the structure set in #29415. LengthAndBPMStatisticPill is abstract and purely layout code. It will also be used in the beatmap overlay (i.e. online), where the data / bindable flow is a simpler implementation because mods won't be involved (as it is currently).

It will avoid this mess as local and online data / bindable flow will be isolated:

osu/osu.Game/Overlays/BeatmapSetOverlay.cs

Lines 40 to 46 in 24a0a3c

	/// <remarks>
	/// Isolates the beatmap set overlay from the game-wide selected mods bindable
	/// to avoid affecting the beatmap details section (i.e. <see cref="AdvancedStats.StatisticRow"/>).
	/// </remarks>
	[Cached]
	[Cached(typeof(IBindable<IReadOnlyList<Mod>>))]
	protected readonly Bindable<IReadOnlyList<Mod>> SelectedMods = new Bindable<IReadOnlyList<Mod>>(Array.Empty<Mod>());

[bdach]

I don't see much discussion of the structure in question in that PR. And I'm skeptical of it. It feels like what this is going to lead to is fifty components instantiating local mod setting trackers (which we already have way too many knocking about) and thus the end result will be dog slow.

There's no real reason why the "local" logic couldn't live in a higher-level component.

@peppy when reviewing that previous PR did you agree with this "local"-"non-local" class split thing?

[peppy]

when reviewing that previous PR did you agree with this "local"-"non-local" class split thing?

No, I may have overlooked that. It seems like a hacky way to work around things (which is weird because we've already spent so much work making common base interfaces for beatmap metadata classes)..

[Joehuu]

this is going to lead to is fifty components instantiating local mod setting tracker

There is already #29151 trying to do the same thing, and I believe users will want length, bpm, and od in 3 separate BeatmapAttributeText for example. Not really an argument, but just pointing out that each skin component will be +1 on the mod setting trackers if that PR is accepted.

There's no real reason why the "local" logic couldn't live in a higher-level component.

By higher-level component, do you mean BeatmapInfoWedgeV2? frenzi also mentioned this in the old review link below.

This structure has skinning in mind, so the barebones components themselves should have the "local" logic unless BeatmapInfoWedgeV2 will be the only skinnable component. But that will make it hard to modify the layout and we would have to use many SettingsSource if we want it somewhat skinnable.

we've already spent so much work making common base interfaces for beatmap metadata classes

Although we have IBeatmapInfo, there is still WorkingBeatmap, which is completely local. There is an old review from frenzi on my old structure that having them coexist seems wrong.

[bdach]

There is already #29151 trying to do the same thing, and I believe users will want length, bpm, and od in 3 separate BeatmapAttributeText for example. Not really an argument

It's not because that was written by an external contributor whose coding style and practices do not match ours and that PR to me is severely lacking.

This structure has skinning in mind, so the barebones components themselves should have the "local" logic unless BeatmapInfoWedgeV2 will be the only skinnable component. But that will make it hard to modify the layout and we would have to use many SettingsSource if we want it somewhat skinnable.

I don't know if we want that? Do we want people to arbitrarily remove items from the wedge? How are they going to lay the thing out properly in a pixel perfect way?

Also think of it this way: if we want that sort of thing, then maybe we want a master headless component that provides that data in a central manner to any song select component via DI.

Although we have IBeatmapInfo, there is still WorkingBeatmap, which is completely local. There is an old review from frenzi on my old structure that having them coexist seems wrong.

I'm not sure what that review has to do with anything here. All it's saying is not to mix BeatmapInfo and WorkingBeatmap which I agree with but fail to see the significance of in terms of this split?

[bdach]

@peppy ping as requested (thread I'd like feedback on in particular is #29437 (comment))