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

Add sound feedback when changing editor screen via key bindings #27448

Closed
wants to merge 2 commits into from

Conversation

frenzibyte
Copy link
Member

I've took an explicit approach towards this. Playing sounds for every change to tab selection in every OsuTabControl gets too crowded when it's a secondary operation, e.g. the video attached in #27400 (comment). Instead, I believe it is rather better to add sound feedback on a case-by-case manner.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Mar 1, 2024
@Joehuu
Copy link
Member

Joehuu commented Mar 3, 2024

If this change gets accepted, we can also resolve #18551 with the diff below (or similar) if we want to increase scope a little bit:

diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneTabControl.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneTabControl.cs
index b6c23f8227..5cd5da7d84 100644
--- a/osu.Game.Tests/Visual/UserInterface/TestSceneTabControl.cs
+++ b/osu.Game.Tests/Visual/UserInterface/TestSceneTabControl.cs
@@ -27,7 +27,8 @@ public partial class TestSceneTabControl : OsuManualInputManagerTestScene
                 {
                     Margin = new MarginPadding(4),
                     Size = new Vector2(229, 24),
-                    AutoSort = true
+                    AutoSort = true,
+                    IsSwitchable = true,
                 },
                 text = new OsuSpriteText
                 {
@@ -56,6 +57,15 @@ public void TestSelection()
                 InputManager.Click(MouseButton.Left);
             });
 
+            AddStep("select by switching tab", () =>
+            {
+                filter.Current.SetDefault();
+                InputManager.PressKey(Key.ControlLeft);
+                InputManager.PressKey(Key.Tab);
+                InputManager.ReleaseKey(Key.Tab);
+                InputManager.ReleaseKey(Key.ControlLeft);
+            });
+
             AddStep("select programmatically with feedback", () =>
             {
                 filter.Current.SetDefault();
diff --git a/osu.Game/Graphics/UserInterface/OsuTabControl.cs b/osu.Game/Graphics/UserInterface/OsuTabControl.cs
index 327b82c5c7..504f40f2c6 100644
--- a/osu.Game/Graphics/UserInterface/OsuTabControl.cs
+++ b/osu.Game/Graphics/UserInterface/OsuTabControl.cs
@@ -98,6 +98,14 @@ protected override void UpdateAfterChildren()
                 strip.Width = Interpolation.ValueAt(Math.Clamp(Clock.ElapsedFrameTime, 0, 1000), strip.Width, StripWidth, 0, 500, Easing.OutQuint);
         }
 
+        public override void SwitchTab(int direction, bool wrap = true)
+        {
+            base.SwitchTab(direction, wrap);
+
+            if (SelectedTab is OsuTabItem selectedTab)
+                selectedTab.TabSelectSounds.PlayClickSample();
+        }
+
         public void SelectTabWithSound(T item)
         {
             Current.Value = item;

The SwitchTab() is not guaranteed to switch (e.g. one item, should return a bool with true if it succeeds), but BeatmapDetailAreaTabControl is the only usage with IsSwitchable though.

@frenzibyte
Copy link
Member Author

Playing on SwitchTab is sort of an overkill, since the point is to play a sound when the user is specifically switching between tabs (i.e. when the "switch tab" action is invoked). SwitchTab could be called when a user closes a tab they currently have selected (see SwitchTabOnRemoval).

Now that I think about it, perhaps we want to do something much similar to TextBox, i.e. user-triggered event methods. I'll explore that path.

Current.Value = item;

if (SelectedTab is OsuTabItem selectedTab)
selectedTab.TabSelectSounds.PlayClickSample();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I dunno. The entire point of HoverClickSounds is to handle sample playback automatically. At the point this has to be done for correct UX in selected usages, is there even any point left to it?

This sort of thing is breaching multiple levels of abstraction and patching a system that - given the requirements as I understand them - is fundamentally broken.

At that point I'd say that the possible alternative of deleting HoverClickSounds and replacing with standard inline sample playbacks should be looked at. Or playing a sample on keyboard interaction in Editor even. Because this is just too weird to live IMO.

And yes I know the usage count means that getting rid of this is not going to be pretty. But I'm not looking forward to having to pepper PlayClickSample() calls everywhere or wondering whether a particular method of selecting a tab is going to play a sound or not.

Copy link
Sponsor Member

@peppy peppy Mar 7, 2024

Choose a reason for hiding this comment

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

Can we just do this in line with how we did it elsewhere and expose framework support?

ie.

protected override void OnUserChange(T value)
{
base.OnUserChange(value);
playSample(value);
TooltipText = getTooltipText(value);
}

and either an induced click, or a simple SelectTab method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's what I meant in #27448 (comment).

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

So this is waiting on changes from you then..?

@frenzibyte
Copy link
Member Author

Will be rewritten to use ppy/osu-framework#6218

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.

No audatory feedback when switching editor modes using keybinds
4 participants