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 missing mania tooltip overlay for 4k and 7k #31084

Merged
merged 8 commits into from
Dec 16, 2024

Conversation

NicholasChin28
Copy link
Contributor

@NicholasChin28 NicholasChin28 commented Dec 11, 2024

Attempts to fix #30827 by adding osu!mania 4K/7K rank breakdowns to profile overlay tooltips, matching the behavior on the osu! website.

Changes:

  • Added FourKey and SevenKey to CommonStrings for localization support
  • Implemented Variant mapping in UserStatistics to properly handle 4K/7K data from the API
  • Added variant rank display logic in MainDetails for both global and country rank tooltips

The implementation follows the web version's data structure, using the variants field from the API response to show individual rankings for each key mode.

Testing:

  • Hover over global rank in osu!mania profile to see 4K/7K breakdown
  • Hover over country rank to see corresponding 4K/7K country rankings

osu.Game/Localisation/CommonStrings.cs Outdated Show resolved Hide resolved
osu.Game/Users/UserStatistics.cs Outdated Show resolved Hide resolved
…ariant display

Use existing localisation strings from BeatmapsStrings instead of CommonStrings for consistent localisation handling
@pull-request-size pull-request-size bot added size/M and removed size/L labels Dec 12, 2024
@NicholasChin28
Copy link
Contributor Author

Hi, I have made the requested changes.

  • Removed unnecessary string additions in CommonStrings
  • Updated to use LocalisableDescription attribute in the enum members with GetLocalisableDescription()

Copy link
Member

@Joehuu Joehuu left a comment

Choose a reason for hiding this comment

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

Forgot to mention this, but contributors are expected to fix the code quality issues listed below in the checks.

The anchor doesn't seem to match web's tooltip. Can probably apply this to all tooltips:

diff --git a/osu.Game/Graphics/Cursor/OsuTooltipContainer.cs b/osu.Game/Graphics/Cursor/OsuTooltipContainer.cs
index 0d36cc1d08..4180825a8d 100644
--- a/osu.Game/Graphics/Cursor/OsuTooltipContainer.cs
+++ b/osu.Game/Graphics/Cursor/OsuTooltipContainer.cs
@@ -80,6 +80,7 @@ public OsuTooltip()
                         Margin = new MarginPadding(5),
                         AutoSizeAxes = Axes.Both,
                         MaximumSize = new Vector2(max_width, float.PositiveInfinity),
+                        TextAnchor = Anchor.TopCentre,
                     }
                 };
             }

as web also defaults to TopCentre everywhere.
Screenshot 2024-12-12 at 1 03 15 PM

@NicholasChin28
Copy link
Contributor Author

NicholasChin28 commented Dec 13, 2024

Got it, thanks. I have added the text anchor for the tooltip.

@bdach bdach self-requested a review December 16, 2024 03:46
- Properly annotate things as nullable
- Remove weird passthrough property (more on that later)
Localisable strings cannot be plainly interpolated or joined. That is a
lossy operation that loses data.
This reverts commit c0b6e78.

The change affects editor and other stuff and I'm not sure it's correct.
It's not like client needs to match the appearance really. It already
doesn't in many places.
@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 16, 2024
@bdach
Copy link
Collaborator

bdach commented Dec 16, 2024

I've done things to get this in. Notably the model was wonky (some things nullable that shouldn't have been, and some things non-nullable that should have been) and the localisation was broken.

I've also reverted the tooltip change as I find it disputable and I don't think it matters enough anyway because the client design is already not matching web in several respects.

@bdach bdach merged commit d72a0b0 into ppy:master Dec 16, 2024
7 of 10 checks passed
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.

osu!mania 4k/7k ranking / pp values are not present on profile overlay
3 participants