Resolves#36099
This PR fixes keyboard navigation in the beatmap select carousel for
lazer by implementing page-wise traversal with the Page Up and Page Down
keys and changing it from only scrolling to actually selecting items.
**Changes:**
- Added handling for `TraversalType.Page` in the keyboard traversal
switch.
- Implemented `traverseKeyboardPage(int direction)` method to move the
selection by approximately one "page" of visible items, accounting for
partially obscured items like the search bar. Also it does not wrap
around (like the current PageUp/Down functionality).
- Added new key bindings:
- `PageUp` → SelectPreviousPage
- `PageDown` → SelectNextPage
The code may be very explicit for the scroll logic with the page keys,
so I would appreciate some feedback when the PR is reviewed.
The naming of the keybinds may need to be adjusted. `Next page` and
`previous page` may be somewhat misleading.
**Behavior after the change:**
- Pressing Page Up/Down now moves the selection by a page of items.
- After navigating, pressing Left/Right selects the navigated song
instead of moving relative to the previous position.
**See:**
https://www.youtube.com/watch?v=JXmKAhhKiCc
---------
Signed-off-by: Linus Genz <linuslinuxgenz@gmail.com>
Co-authored-by: Dean Herbert <pe@ppy.sh>
Part of the `ScreenFooter` refactor, which intends to move the footer
content handling to `OsuScreen`. This commit updates the `ScreenFooter`
test to operate on entire `OsuScreen`s, in order to better test the
entire flow of pushing a screen, and having it create and add its own
content to the footer.
This should be 80-90% identical to the original test case structure
wise, just that instead of manually manipulating the footer with
`SetButtons()`, various screens with the appropriate buttons are being
moved around the screen stack.
Additionally this adds some more tests handling common use cases, as
well as removes `TestLoadOverlayAfterFooterIsDisplayed()`, since as far
as I understand the behaviour described in it doesn't actually happen in
production code. From what I can see, Screens instantiate their overlays
in `load()`, and then register them in `LoadComplete()`. There seems to
be no case where a `ShearedOverlayContainer` is created in the middle of
a screen's lifecycle.
Part of the screen footer refactor.
Once footer content is being managed by `OsuScreen`, the current tests
which simply create the tested overlay and `ScreenFooter` in a container
will no longer work.
This PR refactors them to use `ScreenTestScene` with the setup being
creating a dedicated testing `OsuScreen` which does the bare minimum to
create the tested overlay and necessary components (eg.
`FooterButtonFreeModsV2` for `TestSceneFreeModsOverlay`).
Most of the changes here can be described as
`%s/<...>Overlay/screen.Overlay/g`, with some minor touchups as
necessary, given that we're now testing a more complete flow which
checks more things that were previously not handled by the tests.
## [Move footer to front in
ScreenTestScene](https://github.com/ppy/osu/commit/f8740e0403b3c0badd60d394c737f2aa912a9ed6)
Self-explanatory. Without it the footer would show below the actual
overlay, breaking tests depending on manual input. For the sake of tests
not breaking in CI, both #36718 and this have this included - would
prefer the former to be merged first since it was already reviewed
there.
## `TestSceneModSelectOverlay`
There were a few tests (`TestColumnHidingOnIsValidChange`,
`TestColumnHidingOnTextFilterChange`, and
`TestHidingOverlayClearsTextSearch`) that would create a custom overlay
instance instead of the globally provided one. I've tested both and the
tests run fine with the default overlay, so they're now using that
instead.
## `TestSceneFreeModSelectOverlay`
Updated to use footer v2.
---------
Co-authored-by: Dean Herbert <pe@ppy.sh>
Rebase of https://github.com/smoogipoo/osu/pull/193
Going forward, the client will have to know the type of pool being
invited to so that it can enter the appropriate screen when clicking the
notification.
Unfortunately, SignalR does not support overloading methods, or even
adding parameters to them, so this PR deprecates the
`MatchmakingRoomInvited` event and adds its replacement
`MatchmakingRoomInvitedWithParams` with a complex `invitation` parameter
that we _can_ extend in the future if required (such as potentially
adding the name of the pool).
This also prepares the notification by extracting some code to a
`Complete` method receiving said `invitation` parameter. This part of
code will be further modified to enter the correct screen:
https://github.com/smoogipoo/osu/blob/0a4018045b9d908f66c63dee65d0059d05b26e43/osu.Game/Screens/OnlinePlay/Matchmaking/Queue/QueueController.cs#L200
In particular, I have tested that new clients continue to work with the
old server (dev.ppy.sh) in quick play
| | Old Server | New Server |
| ------------- |:-------------:| :-----:|
| Old Client | 🟢 | 🟢 |
| New Client | 🟢 | 🟢 |
- [x] Depends on https://github.com/ppy/osu/pull/36741 for merge
conflict avoidance
RFC, cc @OliBomby
## [Adjust behaviour of automatic bank assignment during
placement](https://github.com/ppy/osu/commit/547f55e9b3ded668fe6e1c8865a2d625e64a2f45)
Diatribe time!
This is fallout of the discussion about auto bank in
https://github.com/ppy/osu/issues/36705.
Auto bank in lazer as written before this commit is confused. On stable,
auto bank is closer to "no bank", as in "go look up the current sample
timing point, get the bank of that, and use that". lazer has no timing
points anymore, but people still want auto bank. So what do?
Auto bank for normal samples is somewhat sane still. It only works
during placement, and will just copy the normal bank of the previous
object - if one exists. That said, one *might not* exist, but the
resulting object will still have its normal sample created with
`editorAutoBank: true`. That is largely cosmetic and without
consequences, but this commit fixes that.
Auto bank for *addition* samples, however... Hoo boy.
- For placed objects, auto bank means "take the normal sample, read its
bank, and use that". Simple enough, right?
- Hoooooowever. During placement, auto bank before this commit used to
mean "look at the *previous object*, check if it has an addition sound
and then use its bank, if not use *the previous object's* normal sample
and then use its bank" which is a completely different thing with its
own implications. Like, say, what happens if the previous object uses
the auto addition bank too? What should be copied over? Should it be the
notion of "auto bank" in that the addition bank should match the normal
bank, or should it be the literal bank that the previous object is
using?
This change attempts to define this unambiguously. "Auto additions bank"
means "the same bank as the normal bank of this object", full stop.
## [Do not touch sample toggle state if there are no selected
objects](https://github.com/ppy/osu/commit/052cde5987e48800ec68ab2528c7e0ce3140e6e0)
Fixes issue described in
https://github.com/ppy/osu/issues/36705#issuecomment-3953917163 wherein
opening a sample popover will disable addition bank toggles and toggle
off all addition samples.
---------
Co-authored-by: Dean Herbert <pe@ppy.sh>
- Closes https://github.com/ppy/osu/issues/30293
- Fixes https://osu.ppy.sh/community/forums/topics/2179339?n=1
Aside from fixing the off-by-one error that I mentioned in
https://github.com/ppy/osu/issues/30293#issuecomment-2413801663, this
also:
- Brings back the behaviour wherein if timing points are arranged very
weird and nightcore would play e.g. two first beats in a timing point
back-to-back, the second timing point is silent.
- Brings back the behaviour wherein the finish sample only plays if
`OmitFirstBarLine` on the timing point is disabled.
However:
- This does not bring back the behaviour wherein hat samples only play
if the slider tick rate is even because that only kind of makes sense in
common time, and if common time is mixed with waltz time or other time
signatures, it just gets weird.
- Also stable has zero attempt for compensating for waltz time anyway,
lazer's behaviour is bespoke, so that is not going to match any way you
cut it.
My testing procedure essentially consisted of getting stable to log when
it was playing nightcore samples and cross-checking the first 30sec or
so of https://osu.ppy.sh/beatmapsets/534385#osu/1131956 (check out the
timing of that beatmap, for something ranked it is DEEPLY messed up).
I guess I can add test cases if deemed required but I already wasted
much more time than I would have liked here...
- The mods button should not be visible because it is not hooked up
properly to actually work with selecting user mods. As written it
would show (and feign the ability to modify) required mods on the
playlist item.
You could probably adjust the logic to make user mods controllable via
this button but this is simpler for now.
- The random and options buttons could perhaps remain, but they are of
very limited use on this screen anyway.
The previous iteration disabled all footer buttons altogether, so I'm
just following precedent here mostly.
Of note, this test scene still doesn't use the new footer properly.
Should these even be here? Maybe it's better to leave these real world
flows to a `Navigation` style test so we're not dealing with weird test
setups that kinda do something but don't... or maybe do it in three
different ways (two footers present, three back buttons etc.)
Dunno. But this kind of covers what I want (except it doesn't cover the
new footer usage).