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 daily challenge not showing a replay button in results screen #29057

Merged
merged 5 commits into from
Jul 26, 2024

Conversation

frenzibyte
Copy link
Member

@frenzibyte frenzibyte commented Jul 25, 2024

The results screen pushed by DailyChallenge always load with a null selected score, so the replay download button never gets added. There's no reason not to have the replay download button visible when there's no score selected, so just...add it?

Failing test coverage was reverted due to out-of-scope issues (see discord).

Before:

CleanShot 2024-07-25 at 08 55 16

After:

CleanShot 2024-07-25 at 08 54 29

@bdach
Copy link
Collaborator

bdach commented Jul 25, 2024

Generally after looking this over from fifty sides (that's what happens with eldritch abstract entity hierarchies with everything overloaded to crap) I think this may be fine, and the nullcheck was just overeager application of NRT (blames back to da953b3).

However I am slightly unnerved by some stuff ReplayDownloadButton is doing in the Score value change callback and would probably want to see something like the following happen:

diff --git a/osu.Game/Screens/Ranking/ReplayDownloadButton.cs b/osu.Game/Screens/Ranking/ReplayDownloadButton.cs
index aac29ad269..5e2161c251 100644
--- a/osu.Game/Screens/Ranking/ReplayDownloadButton.cs
+++ b/osu.Game/Screens/Ranking/ReplayDownloadButton.cs
@@ -88,6 +88,8 @@ private void load(OsuGame? game, ScoreModelDownloader scoreDownloader)
                 State.ValueChanged -= exportWhenReady;
 
                 downloadTracker?.RemoveAndDisposeImmediately();
+                downloadTracker = null;
+                State.SetDefault();
 
                 if (score.NewValue != null)
                 {

just to make sure that if score changes from non-null to null, then the State of the button reverts to the default rather than staying at whatever the last score had, because if that is allowed to happen, then stuff like the export flow / exportWhenReady() suddenly start looking rather unsafe.

That could be worth a test case. Something like this is enough to show what I mean:

diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneReplayDownloadButton.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneReplayDownloadButton.cs
index 5b32f380b9..061e8ea7e1 100644
--- a/osu.Game.Tests/Visual/Gameplay/TestSceneReplayDownloadButton.cs
+++ b/osu.Game.Tests/Visual/Gameplay/TestSceneReplayDownloadButton.cs
@@ -117,6 +117,9 @@ public void TestButtonWithReplayStartsDownload()
 
             AddAssert("state entered downloading", () => downloadStarted);
             AddUntilStep("state left downloading", () => downloadFinished);
+
+            AddStep("change score to null", () => downloadButton.Score.Value = null);
+            AddUntilStep("state changed to unknown", () => downloadButton.State.Value, () => Is.EqualTo(DownloadState.Unknown));
         }
 
         [Test]

In theory if you have a download button in LocallyAvailable state, then reset the score to null and then request an export, it'll die.

@bdach bdach added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Jul 25, 2024
@bdach bdach merged commit 09a1fd2 into ppy:master Jul 26, 2024
11 of 13 checks passed
bdach added a commit to bdach/osu that referenced this pull request Jul 26, 2024
…ne screens

Struck me as weird when reviewing ppy#29057.
Like sure, that PR adds the replay button, but it's a bit terrible that
clicking the button quits the daily challenge screen and you're back at
main menu when done watching...?

Also extended to cover playlists and multiplayer, which have the same
issue.
@frenzibyte frenzibyte deleted the daily-challenge-replay-download-button branch July 26, 2024 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:daily-challenge area:results next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! size/S
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants