Show daily challenge intro screen once per session #29542
Conversation
| // force showing intro on the first time when a new daily challenge is up. | ||
| statics.SetValue(Static.DailyChallengeIntroPlayed, false); |
There was a problem hiding this comment.
I'm not sure this reset works as intended.
- Log in
- Go to daily challenge, intro plays
- Exit daily challenge, log out and back in again
- Go to daily challenge, intro plays again
This is happening because daily challenge room ID resets to null when logging out and is received from the server afresh on login. This'll also happen when the user's connection drops out.
To make this work you probably want to store the room ID here or something.
There was a problem hiding this comment.
I think it might be fine for it to play again after a logout / login.
Imagine the case of a shared PC where osu! is left running. I'd expect each user to see it once.
There was a problem hiding this comment.
Sure, but as mentioned this will also happen on connection drop-outs which is not desired.
| @@ -461,6 +464,8 @@ private void beginAnimation() | |||
| { | |||
| Schedule(() => | |||
| { | |||
| statics.SetValue(Static.DailyChallengeIntroPlayed, true); | |||
There was a problem hiding this comment.
Not sure about this flag being set at the very end of the animation rather than at the start already.
There was a problem hiding this comment.
I imagined some users might find it more convenient to press the daily challenge screen to intro the screen then immediately hit back then go again, and I don't know if I want to allow that.
There was a problem hiding this comment.
Should be set after playing out.
I imagined some users might find it more convenient to press the daily challenge screen to intro the screen then immediately hit back then go again, and I don't know if I want to allow that.
Sounds like really bad UX. If users hate the intro that much then we've done something wrong. We don't create hacky UX workaround like this intentionally.
There was a problem hiding this comment.
Yeah that's basically what I'm trying to say, that we don't want that to happen.
Seems like some bad copy-paste in the past. Most of this is already being done in `TestSceneDailyChallenge`.
peppy
force-pushed
the
show-daily-challenge-intro-once-per-session
branch
from
105daeb
to
9b9986b
Compare
| @@ -71,6 +72,7 @@ public void TestDailyChallengeButton() | |||
| NotificationOverlay notificationOverlay = null!; | |||
| DependencyProvidingContainer buttonContainer = null!; | |||
|
|
|||
| AddStep("set intro played flag", () => Dependencies.Get<SessionStatics>().SetValue(Static.DailyChallengeIntroPlayed, true)); | |||
There was a problem hiding this comment.
Split this out into an isolated test rather than shoving more and more in such a weird place.
I've started you off in 9b9986b (my test intentionally fails, you can get it working). Once done you can remove the tests in here as they are not related to menu buttons.
There was a problem hiding this comment.
I didn't bother splitting the test since all of those reflect on the DailyChallengeButton itself, but sure.
| if (source.CurrentPlaylistItem.Value != null) | ||
| { | ||
| result.CurrentPlaylistItem.Value = result.CurrentPlaylistItem.Value.With(new Optional<IBeatmapInfo>(source.CurrentPlaylistItem.Value.Beatmap)); |
There was a problem hiding this comment.
Commentary for other readers: normally you'd expect such code to skip the explicit Optional<T> construction because Optional<T> defines an implicit conversion from T, but in this instance that implicit conversion cannot be applied because here T is IBeatmapInfo and the language spec forbids such cases:
For a given source type
$S$ and target type$T$ , if$S$ or$T$ are nullable value types, let$S_0$ and$T_0$ refer to their underlying types; otherwise,$S_0$ and$T_0$ are equal to$S$ and$T$ respectively. A class or struct is permitted to declare a conversion from a source type$S$ to a target type$T$ only if all of the following are true:
$S_0$ and$T_0$ are different types.- Either
$S_0$ or$T_0$ is the instance type of the class or struct that contains the operator declaration.- Neither
$S_0$ nor$T_0$ is an interface_type.- Excluding user-defined conversions, a conversion does not exist from
$S$ to$T$ or from$T$ to$S$ .
bdach
added
the
next release
CleanShot.2024-08-21.at.03.39.58-converted.mp4
Adding tests for this one will be a bit bothersome, but I can do it on request.