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

Implement song select v2 difficulty name content component #29415

Merged
merged 10 commits into from
Aug 15, 2024

Conversation

Joehuu
Copy link
Member

@Joehuu Joehuu commented Aug 14, 2024

Essentially focuses on one component, removes the unnecessary api/online testing and support (for beatmap overlay), and is rewritten to address @frenzibyte's concerns about bindable flow.

The structure is that components will now have two subclasses (local and online) with bindable code that inherit the base class with all the layout code. So "local" can resolve IBindable<WorkingBeatmap> directly now for mostly skinning purposes, while "online" can expose a Bindable<APIBeatmap?> and do the BindTarget flow. Thoughts on this structure?

I believe the only downside is that these components are prepended with Local and it'll show in the skinning toolbox. We can probably do some name juggling so that the "local" becomes DifficultyNameContent and the base being BaseDifficultyNameContent or DifficultyNameContentBase, but it may cause confusion so named them local/online for now.

This will define the structure of all other component PRs, so holding off on those once the structure is agreed upon.

Can be tested in the real-world (i.e. old song select) just by adding ISerialisableDrawable to LocalDifficultyNameContent.

Note that the changed files haven't been moved to Visual/SongSelectV2. There are other v2 components that should be moved from Visual/SongSelect, but just want minimal changes here and will be done in another PR.

Also fixed an overlooked visual bug with a workaround. Not 100% sure the issue I linked is relevant.

Comment on lines 76 to 79
/// <summary>
/// This class is a workaround for the single-frame layout issues with `{Link|Text|Fill}FlowContainer`s.
/// See https://github.com/ppy/osu-framework/issues/3369.
/// </summary>
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Is this still relevant? It seems like this container has to exist to set IdleColour but I'm not sure if this xmldoc is valid.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. It mostly talks about the child, which is just an OsuSpriteText and not a {Link|Text|Fill}FlowContainer. Will remove this and just inline comment the child.


namespace osu.Game.Tests.Visual.SongSelectV2
{
public abstract partial class SongSelectComponentsTestScene : OsuTestScene
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Are you using this in many other places? Could probably just do away with this class.

But also, if you're going to make a class like this you probably want to make use of Content:

diff --git a/osu.Game.Tests/Visual/SongSelectV2/SongSelectComponentsTestScene.cs b/osu.Game.Tests/Visual/SongSelectV2/SongSelectComponentsTestScene.cs
index d984a3a11a..1583d229c5 100644
--- a/osu.Game.Tests/Visual/SongSelectV2/SongSelectComponentsTestScene.cs
+++ b/osu.Game.Tests/Visual/SongSelectV2/SongSelectComponentsTestScene.cs
@@ -12,14 +12,19 @@ namespace osu.Game.Tests.Visual.SongSelectV2
 {
     public abstract partial class SongSelectComponentsTestScene : OsuTestScene
     {
-        protected Container ComponentContainer = null!;
+        [Cached]
+        protected readonly OverlayColourProvider ColourProvider = new OverlayColourProvider(OverlayColourScheme.Aquamarine);
+
+        protected override Container<Drawable> Content { get; } = new Container
+        {
+            RelativeSizeAxes = Axes.X,
+            AutoSizeAxes = Axes.Y,
+            Padding = new MarginPadding(10),
+        };
 
         private Container? resizeContainer;
         private float relativeWidth;
 
-        [Cached]
-        protected readonly OverlayColourProvider ColourProvider = new OverlayColourProvider(OverlayColourScheme.Aquamarine);
-
         [BackgroundDependencyLoader]
         private void load()
         {
@@ -41,9 +46,9 @@ public virtual void SetUpSteps()
                 SelectedMods.SetDefault();
             });
 
-            AddStep("set content", () =>
+            AddStep("setup content", () =>
             {
-                Child = resizeContainer = new Container
+                base.Content.Add(resizeContainer = new Container
                 {
                     RelativeSizeAxes = Axes.X,
                     AutoSizeAxes = Axes.Y,
@@ -56,14 +61,9 @@ public virtual void SetUpSteps()
                             RelativeSizeAxes = Axes.Both,
                             Colour = ColourProvider.Background5,
                         },
-                        ComponentContainer = new Container
-                        {
-                            RelativeSizeAxes = Axes.X,
-                            AutoSizeAxes = Axes.Y,
-                            Padding = new MarginPadding(10),
-                        }
+                        Content
                     }
-                };
+                });
             });
         }
     }
diff --git a/osu.Game.Tests/Visual/SongSelectV2/TestSceneDifficultyNameContent.cs b/osu.Game.Tests/Visual/SongSelectV2/TestSceneDifficultyNameContent.cs
index e32d6ddb80..49e7e2bc1a 100644
--- a/osu.Game.Tests/Visual/SongSelectV2/TestSceneDifficultyNameContent.cs
+++ b/osu.Game.Tests/Visual/SongSelectV2/TestSceneDifficultyNameContent.cs
@@ -19,7 +19,7 @@ public partial class TestSceneDifficultyNameContent : SongSelectComponentsTestSc
         [Test]
         public void TestLocalBeatmap()
         {
-            AddStep("set component", () => ComponentContainer.Child = difficultyNameContent = new LocalDifficultyNameContent());
+            AddStep("set component", () => Child = difficultyNameContent = new LocalDifficultyNameContent());
 
             AddAssert("difficulty name is not set", () => LocalisableString.IsNullOrEmpty(difficultyNameContent.ChildrenOfType<TruncatingSpriteText>().Single().Text));
             AddAssert("author is not set", () => LocalisableString.IsNullOrEmpty(difficultyNameContent.ChildrenOfType<OsuHoverContainer>().Single().ChildrenOfType<OsuSpriteText>().Single().Text));

Copy link
Member Author

@Joehuu Joehuu Aug 14, 2024

Choose a reason for hiding this comment

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

Yes, it will be used for all the individual components that the wedge has. Just wanted to share the resizing and background logic so it can help find layout issues with limited width.

Totally forgot the Content thing.

@peppy peppy self-requested a review August 14, 2024 09:48
@peppy peppy merged commit 9dc496d into ppy:master Aug 15, 2024
7 of 12 checks passed
@Joehuu Joehuu deleted the difficulty-name-content branch August 15, 2024 17:33
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.

2 participants