I had a bit of a struggle getting header traversal logic to work well.
The constraints I had in place were a bit weird:
- Group panels should toggle or potentially fall into the prev/next
group
- Set panels should just traverse around them
The current method of using `CheckValidForGroupSelection` return type
for traversal did not mesh with the above two cases. Just trust me on
this one since it's quite hard to explain in words.
After some re-thinking, I've gone with a simpler approach with one
important change to UX: Now when group traversing with a beatmap set
header currently keyboard focused, the first operation will be to reset
keyboard selection to the selected beatmap, rather than traverse.
I find this non-offensive – at most it means a user will need to press
their group traversal key one extra time.
I've also changed group headers to always toggle expansion when doing
group traversal with them selected.
To make all this work, the meaning of `Activation` has changed somewhat.
It is now the primary path for carousel implementations to change
selection of an item. It is what the `Drawable` panels call when they
are clicked.
Selection changes are not performed implicitly by `Carousel` – an
implementation should decide when it actually wants to change the
selection, usually in `HandleItemActivated`.
Having less things mutating `CurrentSelection` is better in my eyes, as
we see this variable as only being mutated internally when utmost
required (ie the user has requested the change). With this change,
`CurrentSelection` can no longer become of a non-`T` type (in the
beatmap carousel implementation at least).
This might pave a path forward for making `CurrentSelection` typed, but
that comes with a few other concerns so I'll look at that as a
follow-up.
I was trying to be smart about things and make use of our
`SynchronisationContext` setup, but it turns out to not work in all
cases due to the context being missing depending on where you are
calling the method from.
For now let's prefer the "works everywhere" method of scheduling the
final work back to update.
Less abstraction is better. As far as I can tell, we don't need a custom
model for this. If there's any tracking to be done, it should be done
within `BeatmapCarousel`'s implementation (or a filter).
With the animation logic being external, it was going to make it very
hard to apply the scroll offset when a new panel is added or removed
before the current selection.
There's no real reason for the animations to be local to beatmap
carousel. If there's a usage in the future where the animation is to
change, we can add more customisation to `Carousel` itself.
Also:
- Adds hinting at grouping and header status of items
- Passes through criteria and prepare for grouping tests.
- Makes `Filters` list `protected` because naming clash with `Filter()`
on `BeatmapCarousel`.
The intention here is to make things more testable going forward.
Specifically, to remove the "back-door" entrance into `BeatmapCarousel`
where `BeatmapSets` can be set by tests and bypas/block realm retrieval.
Closes https://github.com/ppy/osu/issues/30163.
If I'm to be blunt, the decoupled stuff in song select makes my head
spin. I spent a solid 20 minutes thinking how I was going to fix this
one but then finally realised that generally most of the cause there
was the fact that `AdvancedStats` was seeing the new rulesets *before*
the "ensure global selected mods are valid for current ruleset" logic,
and so decided to just _delay_ that until the decoupled transfer
thingamajig happens.
I was honestly considering combining `BeatmapInfo`, `Ruleset`, and
`Mods` into one property on `AdvancedStats`. I figured I'd rather not
push my luck and try the baseline version first, but I honestly think
that direction is going to be required at some point to properly corral
all of the decoupled madness taking place in song select.
The only exception to the rule here was "when screen isn't active apply
without debounce" but I'm not sure we want this. It would cause a
stutter on returning to song select and I'm not even sure this is a
common scenario.
I'd rather remove it and see if someone finds an actual case where this
is an issue.
