Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix beatmap listing overlay not hiding via keyboard control when scrolled #14698

Merged
merged 3 commits into from Sep 10, 2021

Conversation

peppy
Copy link
Sponsor Member

@peppy peppy commented Sep 9, 2021

Closes #14684.

Note that this PR has a test regression. I have no idea why and I can't easily parse the code around the whole "supporter required" flow (it feels very convoluted). Hoping someone else can figure that out.

@bdach
Copy link
Collaborator

bdach commented Sep 9, 2021

The test regression is indirectly caused by recreating the overlay each time, as the tests silently depended on shared state. And that shared state was the state of these three drawables:

Children = new Drawable[]
{
foundContent = new FillFlowContainer<BeatmapPanel>(),
notFoundContent = new NotFoundDrawable(),
supporterRequiredContent = new SupporterRequiredDrawable(),
}

As seen above, they all start inside panelTarget, but both placeholders start with zero alpha. But then, rather than fading the placeholders back out on content change, the overlay does this dance:

if (lastContent == notFoundContent || lastContent == supporterRequiredContent)
{
// the placeholders may be used multiple times, so don't expire/dispose them.
transform.Schedule(() => panelTarget.Remove(lastContent));
}

So depending on whether it's the first or a subsequent time where the placeholder should be hidden, the correct assertion changes from "is the placeholder not present" to "is the placeholder not present or not in the hierarchy at all". In fact the entire test scene mixes and matches the two.

So the fix for the tests is to always check presence, but that addContentToPlaceholder() logic seems quite busted. A less WTF and more correct version of it should look something like this, I believe:

diff --git a/osu.Game/Overlays/BeatmapListingOverlay.cs b/osu.Game/Overlays/BeatmapListingOverlay.cs
index 6861d17f26..e19b3c505e 100644
--- a/osu.Game/Overlays/BeatmapListingOverlay.cs
+++ b/osu.Game/Overlays/BeatmapListingOverlay.cs
@@ -186,20 +186,17 @@ private void addContentToPlaceholder(Drawable content)
 
             if (lastContent != null)
             {
-                var transform = lastContent.FadeOut(100, Easing.OutQuint);
+                lastContent.FadeOut(100, Easing.OutQuint);
 
-                if (lastContent == notFoundContent || lastContent == supporterRequiredContent)
-                {
-                    // the placeholders may be used multiple times, so don't expire/dispose them.
-                    transform.Schedule(() => panelTarget.Remove(lastContent));
-                }
-                else
+                // Consider the case when the new content is smaller than the last content.
+                // If the auto-size computation is delayed until fade out completes, the background remain high for too long making the resulting transition to the smaller height look weird.
+                // At the same time, if the last content's height is bypassed immediately, there is a period where the new content is at Alpha = 0 when the auto-sized height will be 0.
+                // To resolve both of these issues, the bypass is delayed until a point when the content transitions (fade-in and fade-out) overlap and it looks good to do so.
+                var sequence = lastContent.Delay(25).Schedule(() => lastContent.BypassAutoSizeAxes = Axes.Y);
+
+                if (lastContent != notFoundContent && lastContent != supporterRequiredContent)
                 {
-                    // Consider the case when the new content is smaller than the last content.
-                    // If the auto-size computation is delayed until fade out completes, the background remain high for too long making the resulting transition to the smaller height look weird.
-                    // At the same time, if the last content's height is bypassed immediately, there is a period where the new content is at Alpha = 0 when the auto-sized height will be 0.
-                    // To resolve both of these issues, the bypass is delayed until a point when the content transitions (fade-in and fade-out) overlap and it looks good to do so.
-                    lastContent.Delay(25).Schedule(() => lastContent.BypassAutoSizeAxes = Axes.Y).Then().Schedule(() => lastContent.Expire());
+                    sequence.Then().Schedule(() => lastContent.Expire());
                 }
             }
 
@@ -208,6 +205,7 @@ private void addContentToPlaceholder(Drawable content)
 
             content.FadeInFromZero(200, Easing.OutQuint);
             currentContent = content;
+            currentContent.BypassAutoSizeAxes = Axes.None;
         }
 
         protected override void Dispose(bool isDisposing)

@bdach
Copy link
Collaborator

bdach commented Sep 9, 2021

At any rate I've just pushed a commit that should fix the test failures (does locally, at least). The patch above can be considered in separation.

bdach
bdach previously approved these changes Sep 9, 2021
@smoogipoo smoogipoo merged commit 78a83e0 into ppy:master Sep 10, 2021
@peppy peppy deleted the fix-beatmap-overlay-hide branch September 13, 2021 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search box focus in beatmap listing prevents it from closing via escape key when listing is scrolled
4 participants