Repository navigation
Conversation
This PR effectively deals with two issues: - Fixes ppy#35973, by making the hidden button containers fill flows instead of ordinary `Container`s. Once moved to a `Container`, the button would no longer get relayouted on resize (for instance when using Ctrl+Enter to enable autoplay and immediately start the map), causing it to extend under the button next to it. - Because of the above change, buttons hidden by a blocking overlay would shift to the left, instead of staying in their original positions. This is resolved by not moving the said buttons into the hidden button containers, and instead setting `BypassAutoSizeAxes` for the entire period in which they're hidden, as opposed to only during the reappear animation.
LiquidPL
added
area:song-select
type/cosmetic
| // On a screen with two overlays, if the user were to hide the first, and then | ||
| // appear the second one before the transforms for reappearing the buttons were | ||
| // to finish, the register overlay handler for the second overlay would run before | ||
| // the clear handler for the second one. This could cause some buttons having | ||
| // `BypassAutoSizeAxes` enabled while they shouldn't. | ||
| // | ||
| // To avoid that, we immediately fire the autosize restore delegate if it happens | ||
| // to exist at this point in time. |
There was a problem hiding this comment.
Probably include a test case for this if possible. TestSceneScreenFooter.TestShowOverlayHidesOtherOverlays is like 90% there, probably just needs a clever enough assert in it.
Also comment probably needs to elaborate on why this needs to be force-completed rather than e.g. canceled directly (it's because the set of buttons whose autosize will be restored here is not necessarily a subset of the set of buttons whose autosize will be bypassed lower down).
There was a problem hiding this comment.
I see the comment is updated (2a29f36) but I don't see new tests, did you try writing one at all?
There was a problem hiding this comment.
Still trying to figure out a way to test this properly.
| private FillFlowContainer<ScreenFooterButton> hiddenLeftButtonsContainer = null!; | ||
| private FillFlowContainer<ShearedButton> hiddenRightButtonsContainer = null!; |
There was a problem hiding this comment.
Of note these changes rely on the children being transferred from their original placements in exactly the same order. Any SetLayoutPosition() call on any children here probably breaks things and makes buttons move about during the hide in an undesirable manner.
This was not previously the case because of "clever" parlor trickery (fill flow does layout by setting positions on their children directly, which means said positions transfer even when the drawables are moved to a plain Container. That's also what causes the bug this is fixing, too.)
Probably no action required here, but something worth keeping notes on (and maybe adding an inline comment on).
There was a problem hiding this comment.
FWIW, I was considering proposing a refactor of the footer code in such a fashion that effectively each update of the footer content bindable would have its own set of containers, instead of all of them sharing a single one. Something along the lines of:
ScreenFooter.SetButtons()is called.ScreenFooterstarts transforms for disappearing the currently present buttons in the footer, and schedules the disposal of the containers they were stored in for after all the transforms have completed. At this point nothing inScreenFooteror elsewhere should have a reason to touch said buttons or containers.ScreenFootercreates a new set of containers, inserts them into the drawable, and proceeds with inserting the new buttons into said containers and animating their appearance as usual.- The previous set of buttons disappears away, the containers get disposed alongside their contents, and we're left with only the new footer content in the footer.
This approach would effectively sidestep having to manually move the buttons into the hidden containers when changing footer content, and prevent any issues stemming from any mistakes made during such a move.
Just to be clear, I'm not trying to suggest this be implemented right now, I just wanted to ask if it makes sense, and if there's possibly some hidden complexity somewhere that would make this infeasible. Because if there is not, I think this would make the entire footer somewhat more pleasant to understand and work with.
There was a problem hiding this comment.
As far as I can tell your proposal only talks about SetButtons() but not about RegisterActiveOverlayContainer(). The latter case is more annoying interesting because not all buttons get offscreened in that case so you can't really just ditch the old buttons - and also they need to be restored once the overlay is dismissed.
Am I missing something?
There was a problem hiding this comment.
The way I see it, RegisterActiveOverlayContainer would stay the same as it'd be after this PR is merged. Once an overlay is shown the relevant buttons are offscreened but they still remain in the container for that screen specific footer content. Buttons would get fully offscreened and expired only when the footer content bindable changes.
The way I see things, we don't expect the base footer content to be changed while an overlay is present (aside from screen changes I suppose) so I think this should be fine.
Either way, this is a thing to be dealt with in the future, so feel free to ignore this and just focus on this PR instead.
# Conflicts: # osu.Game/Screens/Footer/ScreenFooter.cs
This PR effectively deals with two issues:
Fixes Using Ctrl+Enter in song select causes a broken animation in the footer #35973, by making the hidden button containers fill flows instead of ordinary
Containers. Once moved to aContainer, the button would no longer get relayouted on resize (for instance when using Ctrl+Enter to enable autoplay and immediately start the map), causing it to extend under the button next to it.Because of the above change, buttons hidden by a blocking overlay would shift to the left, instead of staying in their original positions. This is resolved by not moving the said buttons into the hidden button containers, and instead setting
BypassAutoSizeAxesfor the entire period in which they're hidden, as opposed to only during the reappear animation.Before:
Screencast_20261103_004610.webm
After:
Screencast_20261103_004354.webm