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 tooltips to truncated text #23829

Merged
merged 6 commits into from
Jun 12, 2023
Merged

Conversation

Joehuu
Copy link
Member

@Joehuu Joehuu commented Jun 9, 2023

osu Game Tests_ejYwxMSqYJ

Made the changes said in ppy/osu-framework#5636 (comment) + simplifying the tooltip IsTruncated ? Text : string.Empty to just Text as it wasn't needed (i.e. the HandlePositionalInput override already disables tooltip showing).

Looking again, TruncatingSpriteText may be better suited for framework. OsuSpriteText is already a simple class with just two lines, so inheritance isn't a problem and could be TruncatingSpriteText : SpriteText for framework and OsuTruncatingSpriteText : TruncatingSpriteText for osu!-side.


I also tried centralising the MaxWidth and RelativeSizeAxes = Axes.X assigning to TruncatingSpriteText (Joehuu@dd8e1ba), but not sure about the solution as I'm working around a framework limitation and probably should be done more directly in SpriteText.

@Joehuu Joehuu self-assigned this Jun 9, 2023
@peppy
Copy link
Sponsor Member

peppy commented Jun 9, 2023

I'm just wondering if we don't want to add this implementation to OsuSpriteText to provide a default tooltip in cases of truncation, rather than making a specific class for it. Feels like we're going to end up manually adding it in places it gets forgotten over time.

@bdach
Copy link
Collaborator

bdach commented Jun 9, 2023

We've been over this in the framework pull: ppy/osu-framework#5636 (comment)

@frenzibyte
Copy link
Member

I was also hoping this could be done in OsuSpriteText where it makes sense, but as mentioned above, that causes all sprite texts to be considered as candidates for input events, which would lead to a performance hit.

In my opinion, rather than overthinking this further, I would be totally fine with this approach as long as the Truncate property is completely obsoleted and points at the new TruncatingSpriteText class. Along the lines of:

diff --git a/osu.Game/Graphics/Sprites/OsuSpriteText.cs b/osu.Game/Graphics/Sprites/OsuSpriteText.cs
index e149e0abfb..afbec0eab4 100644
--- a/osu.Game/Graphics/Sprites/OsuSpriteText.cs
+++ b/osu.Game/Graphics/Sprites/OsuSpriteText.cs
@@ -3,12 +3,19 @@
 
 #nullable disable
 
+using System;
 using osu.Framework.Graphics.Sprites;
 
 namespace osu.Game.Graphics.Sprites
 {
     public partial class OsuSpriteText : SpriteText
     {
+        [Obsolete("Use TruncatingSpriteText instead.")]
+        public new bool Truncate
+        {
+            set => throw new InvalidOperationException($"Use {nameof(TruncatingSpriteText)} instead.");
+        }
+
         public OsuSpriteText()
         {
             Shadow = true;
diff --git a/osu.Game/Graphics/Sprites/TruncatingSpriteText.cs b/osu.Game/Graphics/Sprites/TruncatingSpriteText.cs
index da0dbd49d2..229fad29f9 100644
--- a/osu.Game/Graphics/Sprites/TruncatingSpriteText.cs
+++ b/osu.Game/Graphics/Sprites/TruncatingSpriteText.cs
@@ -2,6 +2,7 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using osu.Framework.Graphics.Cursor;
+using osu.Framework.Graphics.Sprites;
 using osu.Framework.Localisation;
 
 namespace osu.Game.Graphics.Sprites
@@ -14,7 +15,7 @@ public sealed partial class TruncatingSpriteText : OsuSpriteText, IHasTooltip
 
         public TruncatingSpriteText()
         {
-            Truncate = true;
+            ((SpriteText)this).Truncate = true;
         }
     }
 }
diff --git a/osu.Game/Overlays/Mods/ModSelectPanel.cs b/osu.Game/Overlays/Mods/ModSelectPanel.cs
index 81285833bd..6179f31637 100644
--- a/osu.Game/Overlays/Mods/ModSelectPanel.cs
+++ b/osu.Game/Overlays/Mods/ModSelectPanel.cs
@@ -118,22 +118,20 @@ protected ModSelectPanel()
                                 Direction = FillDirection.Vertical,
                                 Children = new[]
                                 {
-                                    titleText = new OsuSpriteText
+                                    titleText = new TruncatingSpriteText
                                     {
                                         Font = OsuFont.TorusAlternate.With(size: 18, weight: FontWeight.SemiBold),
                                         RelativeSizeAxes = Axes.X,
-                                        Truncate = true,
                                         Shear = new Vector2(-ShearedOverlayContainer.SHEAR, 0),
                                         Margin = new MarginPadding
                                         {
                                             Left = -18 * ShearedOverlayContainer.SHEAR
                                         }
                                     },
-                                    descriptionText = new OsuSpriteText
+                                    descriptionText = new TruncatingSpriteText
                                     {
                                         Font = OsuFont.Default.With(size: 12),
                                         RelativeSizeAxes = Axes.X,
-                                        Truncate = true,
                                         Shear = new Vector2(-ShearedOverlayContainer.SHEAR, 0)
                                     }
                                 }

@Joehuu
Copy link
Member Author

Joehuu commented Jun 9, 2023

ModSelectPanels were intentionally omitted as IncompatibilityDisplayingModPanel has a custom tooltip implemented which conflicts:

osu!_KURWPKcjkd

Kind of a weird case, because the freemods overlay will actually benefit from this as it doesn't show the custom incompatibility tooltip:
osu!_Rgts6Eh7tt

@frenzibyte
Copy link
Member

That should definitely be handled very explicitly. Either as...a truncating sprite text that doesn't show a tooltip...or move the tooltip provider to a drawable that would take hover away from the sprite text.

Joehuu and others added 2 commits June 10, 2023 11:53
Co-Authored-By: Salman Ahmed <frenzibyte@gmail.com>
Going simple with a bool instead of making `TooltipText` init-able, as the current cases will just init `string.Empty`. And not sure if we want custom tooltip text in the future.
@peppy peppy self-requested a review June 12, 2023 06:22
@peppy
Copy link
Sponsor Member

peppy commented Jun 12, 2023

Kind of a weird case, because the freemods overlay will actually benefit from this as it doesn't show the custom incompatibility tooltip: osu!_Rgts6Eh7tt

I was going to go in and fix this, but the mod column classes are a bit hard to work with (I got lost). Specifically, ModButtonTooltip exists as a base class but is never constructed itself, which seems very weird.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Jun 12, 2023
@peppy
Copy link
Sponsor Member

peppy commented Jun 12, 2023

This is probably fine, but requesting one more approval.

@frenzibyte
Copy link
Member

I'll abstain from approval here since part of the changes in this PR were proposed by me above.

@frenzibyte frenzibyte requested a review from a team June 12, 2023 08:21
Copy link
Member

@Susko3 Susko3 left a comment

Choose a reason for hiding this comment

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

Looks fine. On first glance I thought TruncatingSpriteText was a framework class as it doesn't have Osu in it.

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

i have a weird feeling that this is overly constrained and is going to backfire on us but there's enough approvals that let's give it a go i guess

@bdach bdach merged commit af3fbdb into ppy:master Jun 12, 2023
15 of 17 checks passed
@Joehuu Joehuu deleted the truncating-text-tooltips branch June 12, 2023 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Truncated text should have tooltips of full text
5 participants