Use IDisposable flow for common logo tracking/proxy operations for better robustness
#33660
Conversation
| @@ -451,7 +454,7 @@ private void updateLogoState(ButtonSystemState lastState = ButtonSystemState.Ini | |||
| break; | |||
|
|
|||
| case ButtonSystemState.EnteringMode: | |||
| logoTrackingContainer.StartTracking(logo, lastState == ButtonSystemState.Initial ? MainMenu.FADE_OUT_DURATION : 0, Easing.InSine); | |||
| logoTracking = logoTrackingContainer.StartTracking(logo, lastState == ButtonSystemState.Initial ? MainMenu.FADE_OUT_DURATION : 0, Easing.InSine); | |||
There was a problem hiding this comment.
This is a little dodgy because it's overwriting a non-null IDisposable, which in normal circumstances would be a resource leak, but in this particular case is not, so I guess I'll overlook it...
There was a problem hiding this comment.
It should throw if it's about to overwrite, if that's any consolation
There was a problem hiding this comment.
Uh... no, this does actually overwrite, every time, on the main menu. At least as far as I could tell with a debugger. I'm not talking theory here.
First, this runs on Initial button system state:
osu/osu.Game/Screens/Menu/ButtonSystem.cs
Lines 428 to 435 in 8c0e535
Then, when progressing to song select, the overwrite happens when button system progresses to EnteringMode:
osu/osu.Game/Screens/Menu/ButtonSystem.cs
Lines 456 to 457 in 8c0e535
It only doesn't throw in this case because it's an "idempotent" tracking start, as in it's the same logo instance on the same screen.
There was a problem hiding this comment.
Okay this is kind of weird. Let me do a second pass.
The root cause of the linked issue is probably related to a screen being skipped completely in the chain, as far as I recall it can bypass calling the arriving method completely and this is expected from
ScreenStack. I'm not focusing on that here, just refactoring two methods to use theIDisposableflow we have come to adopt for operations like this in recent times. This has bugged me while working through song select v2 work as well.Closes #33637.