-
-
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
Add custom hue support to user profile overlay #28849
Conversation
something might be wrong considering I've set my hue to |
They say it's not a bug, it's a feature...I dunno maybe later.
232c153
to
43d08f7
Compare
…ird UX when scrolled away
Last commit resolves the following bug: CleanShot.2024-07-14.at.15.21.05.mp4 |
loadingLayer.Hide(); | ||
} | ||
|
||
private void recreateBaseContent() |
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.
It's a bit unfortunate that we have to recreate content to make this happen. While it's maybe okay for display purposes (although it obliterates any animations and allocation savings) it won't scale to if we want a live preview of the colour changing when the user edits it, like osu-web does.
I think this could be fixed with quite minimal work by making the Hue
a bindable in OverlayColourProvider
, or if we don't want to use bindables, a HueUpdated
event. (I'd argue bindable is the way to go for auto-cleanup).
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.
Making Hue
a bindable or providing any event-based flow by OverlayColourProvider
sounds like the natural way to go about things, but I can't imagine myself living with a codebase where every implementation that uses OverlayColourProvider
should be accompanied by an event subscription. We use OverlayColourProvider
almost everywhere now, making every implementation make sure to be subscribed to an event feels awkward to me readability-wise.
I intentionally did not bring this up as it doesn't need to right now considering that everything is being recreated every user fetch anyway (minus the header, which I'm manually recreating now).
I would propose rewriting the way we provide overlay colours to components, such that the part of giving a colour and updating it is handled automatically by a local component added in each implementation. Something like this frenzibyte@6ae2f46, with the diff for each component becoming like:
diff --git a/osu.Game/Overlays/Notifications/ProgressNotification.cs b/osu.Game/Overlays/Notifications/ProgressNotification.cs
index 2362cb11f6..a5e93e6eba 100644
--- a/osu.Game/Overlays/Notifications/ProgressNotification.cs
+++ b/osu.Game/Overlays/Notifications/ProgressNotification.cs
@@ -74,9 +74,6 @@ public float Progress
protected override IconUsage CloseButtonIcon => FontAwesome.Solid.Times;
- [Resolved]
- private OverlayColourProvider colourProvider { get; set; } = null!;
-
protected override void LoadComplete()
{
base.LoadComplete();
@@ -231,6 +228,9 @@ public ProgressNotification()
[BackgroundDependencyLoader]
private void load(OsuColour colours, AudioManager audioManager)
{
+ OverlayColourProvider colourProvider;
+ AddInternal(colourProvider = new OverlayColourProvider());
+
colourQueued = colours.YellowDark;
colourActive = colours.Blue;
colourCancelled = colours.Red;
@@ -240,9 +240,8 @@ private void load(OsuColour colours, AudioManager audioManager)
new Box
{
RelativeSizeAxes = Axes.Both,
- Colour = colourProvider.Background5,
Depth = float.MaxValue,
- },
+ }.WithOverlayColour(colourProvider, OverlayColourShade.Background5),
loadingSpinner = new LoadingSpinner
{
Size = new Vector2(loading_spinner_size),
diff --git a/osu.Game/Overlays/Notifications/SimpleNotification.cs b/osu.Game/Overlays/Notifications/SimpleNotification.cs
index 109b31ff71..177ed9111a 100644
--- a/osu.Game/Overlays/Notifications/SimpleNotification.cs
+++ b/osu.Game/Overlays/Notifications/SimpleNotification.cs
@@ -53,8 +53,11 @@ public ColourInfo IconColour
private SpriteIcon? iconDrawable;
[BackgroundDependencyLoader]
- private void load(OsuColour colours, OverlayColourProvider colourProvider)
+ private void load(OsuColour colours)
{
+ OverlayColourProvider colourProvider;
+ AddInternal(colourProvider = new OverlayColourProvider());
+
Light.Colour = colours.Green;
IconContent.AddRange(new Drawable[]
@@ -62,8 +65,7 @@ private void load(OsuColour colours, OverlayColourProvider colourProvider)
new Box
{
RelativeSizeAxes = Axes.Both,
- Colour = colourProvider.Background5,
- },
+ }.WithOverlayColour(colourProvider, OverlayColourShade.Background5),
iconDrawable = new SpriteIcon
{
Anchor = Anchor.Centre,
diff --git a/osu.Game/Overlays/Notifications/UserAvatarNotification.cs b/osu.Game/Overlays/Notifications/UserAvatarNotification.cs
index 5a9241a2a1..29230e42fc 100644
--- a/osu.Game/Overlays/Notifications/UserAvatarNotification.cs
+++ b/osu.Game/Overlays/Notifications/UserAvatarNotification.cs
@@ -42,8 +42,11 @@ public UserAvatarNotification(APIUser user, LocalisableString text)
protected override IconUsage CloseButtonIcon => FontAwesome.Solid.Times;
[BackgroundDependencyLoader]
- private void load(OsuColour colours, OverlayColourProvider colourProvider)
+ private void load(OsuColour colours)
{
+ OverlayColourProvider colourProvider;
+ AddInternal(colourProvider = new OverlayColourProvider());
+
Light.Colour = colours.Orange2;
Content.Add(textDrawable = new OsuTextFlowContainer(t => t.Font = t.Font.With(size: 14, weight: FontWeight.Medium))
@@ -60,9 +63,8 @@ private void load(OsuColour colours, OverlayColourProvider colourProvider)
{
new Box
{
- RelativeSizeAxes = Axes.Both,
- Colour = colourProvider.Background5,
- },
+ RelativeSizeAxes = Axes.Both
+ }.WithOverlayColour(colourProvider, OverlayColourShade.Background5)
});
LoadComponentAsync(new DrawableAvatar(user)
Of course there would need to be further considerations for applying FadeColour
transforms or adding support for use cases where the colour is provided to a non-Drawable.Colour
property, but we'll cross that bridge when we get there I believe.
I'm also wondering about the possibility of refactoring o!f into supporting the idea of dynamically-changing colours or specifically colours with variable hue degree, but I'm not confident enough to suggest a specific flow in there, nor am I 100% serious in making o!f any more complicated than it already is.
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.
cc @ppy/team-client
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.
Sure, the helper method looks like a good way of handling it with minimal developer overheads.
I'm also wondering about the possibility of refactoring o!f into supporting the idea of dynamically-changing colours or specifically colours with variable hue degree
Nah.
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.
I don't like the helper method proposal. It's too inflexible. Can guarantee that it won't work with something.
If the desire is for the user profile overlay to respond dynamically to colour scheme changes then that is what will have to be done. And yes that means components handling this locally IMO. I don't see a way out of that. I would not make Hue
bindable, I would provide a Bindable<OverlayColourProvider>
to the user overlay and the user overlay only and do the required legwork in components to actually support that.
But all this is academic at this point since content recreation is already happening in master. So I say we ship this and worry about "regeneration of content" never later.
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.
I'm fine with going with things as they are until we need more flexibility, sure.
Ordered with express delivery; I took advantage over the fact that 99% of the user profile overlay is recreated each time a new user is loaded, so I don't have to fiddle too much with updating colour of all user profile components (I didn't realise this at first and I spent some time exploring through the concept of dynamically changing colours...and it wasn't worth it).
Preview:
CleanShot.2024-07-13.at.18.37.24-converted.mp4