-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Conversation
…t of `DifficultyNameContent`
/// <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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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));
There was a problem hiding this comment.
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.
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 aBindable<APIBeatmap?>
and do theBindTarget
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" becomesDifficultyNameContent
and the base beingBaseDifficultyNameContent
orDifficultyNameContentBase
, 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
toLocalDifficultyNameContent
.Note that the changed files haven't been moved to
Visual/SongSelectV2
. There are other v2 components that should be moved fromVisual/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.