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

Delay back button appearance when performing a quick restart #30863

Merged
merged 6 commits into from
Dec 2, 2024

Conversation

frenzibyte
Copy link
Member

Adding tests for the BackButtonState property seems to be too much work for not much benefit, but I can add one if insisted.

Note that I've created another bindable for this rather than turning AllowBackButton itself into a bindable, this is because we still want to allow the user to exit right after performing a quick restart (since exiting is controlled by AllowBackButton, meanwhile the new bindable only affects the visual state of the button itself).

Preview:

CleanShot.2024-11-24.at.05.38.50.mp4

osu.Game/OsuGame.cs Outdated Show resolved Hide resolved
@bdach
Copy link
Collaborator

bdach commented Nov 25, 2024

Maybe even the above is too complex and the back button should just not show on the player loader. Dunno. Will need @peppy's opinion to proceed.

@peppy
Copy link
Member

peppy commented Nov 25, 2024

The original rationale from me was that if loading takes too long, the player needs an out (even if they don't have a keyboard).

I haven't looked into the code here yet but the premise of delaying it for the same delay as the metadata is sound enough.

@peppy peppy self-requested a review November 27, 2024 06:52
Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems pretty good.

/// <summary>
/// Whether the back button is currently displayed.
/// </summary>
public readonly IBindable<bool> BackButtonVisibility = new Bindable<bool>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arguably could be made private until there's a reason to have it public:

diff --git a/osu.Game/OsuGame.cs b/osu.Game/OsuGame.cs
index 514209524e..c52755197b 100644
--- a/osu.Game/OsuGame.cs
+++ b/osu.Game/OsuGame.cs
@@ -178,7 +178,7 @@ public partial class OsuGame : OsuGameBase, IKeyBindingHandler<GlobalAction>, IL
         /// <summary>
         /// Whether the back button is currently displayed.
         /// </summary>
-        public readonly IBindable<bool> BackButtonVisibility = new Bindable<bool>();
+        private readonly IBindable<bool> backButtonVisibility = new Bindable<bool>();
 
         IBindable<LocalUserPlayingState> ILocalUserPlayInfo.PlayingState => playingState;
 
@@ -1194,7 +1194,7 @@ protected override void LoadComplete()
                 if (mode.NewValue != OverlayActivation.All) CloseAllOverlays();
             };
 
-            BackButtonVisibility.ValueChanged += visible =>
+            backButtonVisibility.ValueChanged += visible =>
             {
                 if (visible.NewValue)
                     BackButton.Show();
@@ -1594,14 +1594,14 @@ private void screenChanged(IScreen current, IScreen newScreen)
 
             if (current is IOsuScreen currentOsuScreen)
             {
-                BackButtonVisibility.UnbindFrom(currentOsuScreen.BackButtonVisibility);
+                backButtonVisibility.UnbindFrom(currentOsuScreen.BackButtonVisibility);
                 OverlayActivationMode.UnbindFrom(currentOsuScreen.OverlayActivationMode);
                 API.Activity.UnbindFrom(currentOsuScreen.Activity);
             }
 
             if (newScreen is IOsuScreen newOsuScreen)
             {
-                BackButtonVisibility.BindTo(newOsuScreen.BackButtonVisibility);
+                backButtonVisibility.BindTo(newOsuScreen.BackButtonVisibility);
                 OverlayActivationMode.BindTo(newOsuScreen.OverlayActivationMode);
                 API.Activity.BindTo(newOsuScreen.Activity);
 


/// <summary>
/// Whether a footer (and a back button) should be displayed underneath the screen.
/// </summary>
/// <remarks>
/// Temporarily, the back button is shown regardless of whether <see cref="AllowBackButton"/> is true.
/// Temporarily, the back button is shown regardless of whether <see cref="AllowUserExit"/> is true.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking this behaviour, it looks back-to-front to me. Confused:

BackButton.Hide();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That comment meant the footer's back button, not our classic back button. I'll update the comment.

peppy
peppy previously approved these changes Nov 28, 2024
@peppy peppy requested a review from bdach November 28, 2024 05:18
@@ -80,7 +80,7 @@ public partial class Editor : ScreenWithBeatmapBackground, IKeyBindingHandler<Gl

public override float BackgroundParallaxAmount => 0.1f;

public override bool AllowBackButton => false;
public override bool AllowUserExit => false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I the only one weirded out by this sort of thing? Why does editor apparently "not allow user exit"? The user can exit it just fine.

Copy link
Member Author

@frenzibyte frenzibyte Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I failed to notice this one. Looks to have been done this way historically so the back button doesn't display, while still allowing the user to exit via Back action by manual handling in Editor.OnPressed.

This can make better sense with this PR by setting the initial back button state to hidden instead of overriding this.

Copy link
Member Author

@frenzibyte frenzibyte Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should hopefully not cause any behavioural change to the editor due to the handling of the exit action moving from Editor.OnPressed all the way back to BackReceptor. I can't think of it breaking so I pushed the change.

Comment on lines +1197 to +1203
backButtonVisibility.ValueChanged += visible =>
{
if (visible.NewValue)
BackButton.Show();
else
BackButton.Hide();
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this sort of setup mean it's possible to put yourself in the situation where the back button is in view but the screen actually isn't "user-exitable" or however you'd put it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true, but for the sake of simplicity, it should be OsuScreen's responsibility to never show the back button if it explicitly declares to not be exitable. Otherwise, I will have to account for AllowUserExit in this bindable and make this bindable always be triggered when a screen is changed, and that just increases the complexity and takes us back to step one which is "why is this being done differently".

At the moment, I've made OsuScreen decide the initial state of the back button appearance based on whether it is declared to be user-exitable or not:

/// <summary>
/// The initial visibility state of the back button when this screen is entered for the first time.
/// </summary>
protected virtual bool InitialBackButtonVisibility => AllowUserExit;

@bdach bdach merged commit 6c0ccc5 into ppy:master Dec 2, 2024
9 of 10 checks passed
@frenzibyte frenzibyte deleted the improve-back-button-display branch December 2, 2024 20:07
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.

3 participants