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

Use more correct localised string source for "sign out" text #24068

Merged
merged 2 commits into from
Jul 2, 2023

Conversation

peppy
Copy link
Member

@peppy peppy commented Jun 28, 2023

@bdach
Copy link
Collaborator

bdach commented Jun 28, 2023

Thoughts on making the rest of the dropdown's items match the Title Casing? Could either change the source localisations. Or do

diff --git a/osu.Game/Overlays/Login/UserDropdown.cs b/osu.Game/Overlays/Login/UserDropdown.cs
index f2a12f9a1e..b885a5fc3d 100644
--- a/osu.Game/Overlays/Login/UserDropdown.cs
+++ b/osu.Game/Overlays/Login/UserDropdown.cs
@@ -1,8 +1,10 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using osu.Framework.Extensions.LocalisationExtensions;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.UserInterface;
+using osu.Framework.Localisation;
 using osu.Game.Graphics.UserInterface;
 using osu.Game.Users.Drawables;
 using osuTK;
@@ -16,6 +18,11 @@ public partial class UserDropdown : OsuEnumDropdown<UserAction>
 
         protected override DropdownMenu CreateMenu() => new UserDropdownMenu();
 
+        protected override LocalisableString GenerateItemText(UserAction item)
+        {
+            return base.GenerateItemText(item).ToTitle();
+        }
+
         public Color4 StatusColour
         {
             set

I guess.

@peppy
Copy link
Member Author

peppy commented Jun 30, 2023

Yeah I dunno. checking other dropdowns we're all over the place

osu! 2023-06-30 at 05 32 36

I'm not sure how well ToTitle will work on other languages, is the plan there to only make it do anything on languages which support it?

@bdach
Copy link
Collaborator

bdach commented Jun 30, 2023

I'm not sure how well ToTitle will work on other languages

Fair point, but the other option of changing other localisations to match still stands. We control LoginPanelStrings so we can do this here.

@peppy
Copy link
Member Author

peppy commented Jun 30, 2023

For sure, just not sure if we want title case or not at this point.

@bdach
Copy link
Collaborator

bdach commented Jun 30, 2023

I'm... not sure I have a well-formed opinion about that, I'd just probably match the swapped-out string and do title case. @Joehuu might have other opinions?

@Joehuu
Copy link
Member

Joehuu commented Jul 1, 2023

It shouldn't be title-cased as the majority of other cases use sentence-cased (e.g. LoginPanel, UserActivity [still not localised but is sentence-cased], and most dropdown items when you look at it [only saw 3 title-cased, including the one mentioned above]). With the above diff:

osu!_C9mLcrf13G

I would use ToSentence() just to fix the one case of Sign Out, but that method needs to ToLower() everything first:
https://github.com/ppy/osu-framework/blob/19cbb8f4a681b4e8cb7838127ddcf91b71870cea/osu.Framework/Localisation/CaseTransformableString.cs#L60-L68

@peppy
Copy link
Member Author

peppy commented Jul 1, 2023

At this point we should either fix osu-web side, or duplicate the string.

@bdach
Copy link
Collaborator

bdach commented Jul 1, 2023

I'd say duplicate.

@bdach bdach merged commit 7b3cb5b into ppy:master Jul 2, 2023
@peppy peppy deleted the fix-sign-out-string branch July 3, 2023 03:32
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.

3 participants