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.

Sign up for GitHub

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

Jump to bottom

Fix score breakdown tooltips appearing in other feeds #29159

Merged
merged 1 commit into from
Jul 29, 2024
Merged

Fix score breakdown tooltips appearing in other feeds #29159

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions osu.Game/Screens/OnlinePlay/DailyChallenge/DailyChallengeScoreBreakdown.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ public partial class DailyChallengeScoreBreakdown : CompositeDrawable

private FillFlowContainer<Bar> barsContainer = null!;

// we're always present so that we can update while hidden, but we don't want tooltips to be displayed, therefore directly use alpha comparison here.
public override bool PropagatePositionalInputSubTree => base.PropagatePositionalInputSubTree && Alpha > 0;
Comment on lines +30 to +31
Copy link
Collaborator

@bdach bdach Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this solution is short-sighted and generally incorrect (even if it does what it says). Why? Because if some other ever-present component that handles input gets added to the carousel, or an existing one starts to handle it, you'll have to add this hack local to that other component too.

I'd say this should be fixed at the carousel level if at all possible and not here. Via overriding ShouldBeConsideredForInput(). Something like so (quick and dirty):

diff --git a/osu.Game/Screens/OnlinePlay/DailyChallenge/DailyChallengeCarousel.cs b/osu.Game/Screens/OnlinePlay/DailyChallenge/DailyChallengeCarousel.cs
index a9f9a5cd78..bf8e403913 100644
--- a/osu.Game/Screens/OnlinePlay/DailyChallenge/DailyChallengeCarousel.cs
+++ b/osu.Game/Screens/OnlinePlay/DailyChallenge/DailyChallengeCarousel.cs
@@ -18,19 +18,18 @@ public partial class DailyChallengeCarousel : Container
     {
         private const int switch_interval = 20_500;
 
-        private readonly Container content;
+        private readonly CarouselContainer content;
         private readonly FillFlowContainer<NavigationDot> navigationFlow;
 
         protected override Container<Drawable> Content => content;
 
         private double clockStartTime;
-        private int lastDisplayed = -1;
 
         public DailyChallengeCarousel()
         {
             InternalChildren = new Drawable[]
             {
-                content = new Container
+                content = new CarouselContainer
                 {
                     RelativeSizeAxes = Axes.Both,
                     Padding = new MarginPadding { Bottom = 40 },
@@ -81,7 +80,7 @@ protected override void Update()
 
             if (content.Count == 0)
             {
-                lastDisplayed = -1;
+                content.LastDisplayed = -1;
                 return;
             }
 
@@ -95,18 +94,18 @@ protected override void Update()
             if (content.Count > 1)
                 navigationFlow[currentDisplay].Progress = (float)displayProgress;
 
-            if (currentDisplay == lastDisplayed)
+            if (currentDisplay == content.LastDisplayed)
                 return;
 
-            if (lastDisplayed >= 0)
+            if (content.LastDisplayed >= 0)
             {
-                content[lastDisplayed].FadeOutFromOne(250, Easing.OutQuint);
-                navigationFlow[lastDisplayed].Active.Value = false;
+                content[content.LastDisplayed].FadeOutFromOne(250, Easing.OutQuint);
+                navigationFlow[content.LastDisplayed].Active.Value = false;
             }
 
             content[currentDisplay].Delay(250).Then().FadeInFromZero(250, Easing.OutQuint);
 
-            lastDisplayed = currentDisplay;
+            content.LastDisplayed = currentDisplay;
         }
 
         private void onManualNavigation(NavigationDot dot)
@@ -230,5 +229,15 @@ protected override bool OnClick(ClickEvent e)
                 return true;
             }
         }
+
+        private partial class CarouselContainer : Container
+        {
+            public int LastDisplayed { get; set; } = -1;
+
+            protected override bool ShouldBeConsideredForInput(Drawable child)
+            {
+                return base.ShouldBeConsideredForInput(child) && IndexOf(child) == LastDisplayed;
+            }
+        }
     }
 }

Copy link
Member

Choose a reason for hiding this comment

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

At the point of doing this I'd probably explore making the displays update nicely when coming into view as I proposed.

I think it's optimal if they should some animation when they come back on-screen (if there's something they can animate since last time).

Copy link
Collaborator

@bdach bdach Jul 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

At the point of doing this I'd probably explore making the displays update nicely when coming into view as I proposed.

Not sure I get where this is coming from? It's not like the above is some horrible eldritch hack no? Certainly no more than what this PR currently is.

it would be cool if they played a catch-up animation after becoming visible again

I dunno if I agree with that either. Not sure how you do it with something like the breakdown without it just spamming texts.

Copy link
Member

Choose a reason for hiding this comment

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

As in, if we can avoid AlwaysPresent altogether none of this needs to exist.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure how you do it with something like the breakdown without it just spamming texts.

Can show the last n scores with a delay between each.

Copy link
Member Author

@frenzibyte frenzibyte Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Your concern is valid. I tend to avoid overcomplicating things whenever possible, but I admit I was too lenient in this case. I would've agreed on going with the diff above as alternative to the solution in my PR, but I realise now that this whole situation has already been addressed in #29187.

If we want to bring back the concept of updating screen while hidden, we can also consider turning all feeds to VisibilityContainers as an alternative option to the diff above, as that disables input handling based on Visibility state rather than the direct drawable presence. Something for later.


private const int bin_count = MultiplayerPlaylistItemStats.TOTAL_SCORE_DISTRIBUTION_BINS;
private long[] bins = new long[bin_count];

Expand Down
Loading