Change most ruleset-accessible string types to Localisable strings #19695

naoei · 2022-08-10T20:24:58Z

Addresses my previous concern in Many ruleset strings cannot be localised #19672

Commits of interest:

~~Revert localisation for GetDisplayNameForHitResult (1e356f6) -- Read description~~
Enable localisation for SettingSourceAttribute (7cbe2fa)

Breaking changes

Mod.Description type has been changed

    // ...
-   public override string Description => "...";
+   public override LocalisableString Description => "...";
    // ...

ResumeOverlay.Message type has been changed

    // ...
-   protected override string Message => "...";
+   protected override LocalisableString Message => "...";
    // ...

Ruleset.GetDisplayNameForHitResult(HitResult) type has been changed

    // ...
-   public override string GetDisplayNameForHitResult(HitResult result)
+   public override LocalisableString GetDisplayNameForHitResult(HitResult result)
    // ...

Ruleset.GetVariantName(int variant) type has been changed

    // ...
-   public override string GetVariantName(int variant)
+   public override LocalisableString GetVariantName(int variant)
    // ...

Came across an issue where `LeaderboardScoreTooltip` attempts to capitalize all letters for the `displayName`. Unsure if I should completely ignore it and localise it anyway.

bdach · 2022-08-14T14:34:11Z

Revert localisation for GetDisplayNameForHitResult (1e356f6) -- Read description

The argumentation for this revert is weak. Support for text case transformations on localisable strings is already in place. Please use these methods.

Enable localisation for SettingSourceAttribute (7cbe2fa)

This has zero testing. The new ctor that accepts declaringType has no usage in this PR. Please add tests.

(As an aside I'm a bit skeptical that the API there is going to fly. As I understand it, the design is such that both the label and the description must come from the same class with localisables, which I see becoming a problem down the line. But I guess this can be addressed in due time.)

bdach · 2022-08-14T14:36:28Z

osu.Game.Rulesets.Catch/Mods/CatchModFloatingFruits.cs

@@ -14,7 +15,7 @@ public class CatchModFloatingFruits : Mod, IApplicableToDrawableRuleset<CatchHit
    {
        public override string Name => "Floating Fruits";


I assume the reason why mod names aren't also being localised here is their widespread usage, correct?

That is correct. I can change it into a localisable string if preferred.

Definitely not in this diff. There are many concerns to figure out with regard to that, including but not limited to how to share translations with web, or how to adapt several dependent projects like osu-tools.

What's the issue there? I think we can still reference osu.Resources from the rulesets.

I think if we're going to break Description, we should also break Name, otherwise we'll end up breaking rulesets twice over: https://github.com/taulazer/tau/blob/master/osu.Game.Rulesets.Tau/Mods/TauModDual.cs

I'm not sure we should be localising mod names. They historically haven't been, as they are treated as proper nouns.

But you can transliterate proper nouns, as is done in Korean: https://osu.ppy.sh/wiki/ko/Gameplay/Game_modifier/Double_Time (lit. deobeul taim). I'm also seeing whisperings on the Japanese net about ハードロック and ダブルタイム.

Are you sure you want to keep these as latin characters?

It's a valid direction, but we need to decide and make sure everyone is on board with it. That shouldn't be in the PR, as I think there will be diverging opinions on it.

Currently they aren't translated in stable (or osu-web to my knowledge).

Fair enough.

bdach · 2022-08-14T14:48:53Z

osu.Game/Screens/Ranking/Statistics/StatisticItem.cs

@@ -48,7 +49,7 @@ public StatisticItem([NotNull] string name, [NotNull] Drawable content, [CanBeNu
        /// <param name="createContent">A function returning the <see cref="Drawable"/> content to be displayed.</param>
        /// <param name="requiresHitEvents">Whether this item requires hit events. If true, <see cref="CreateContent"/> will not be called if no hit events are available.</param>
        /// <param name="dimension">The <see cref="Dimension"/> of this item. This can be thought of as the column dimension of an encompassing <see cref="GridContainer"/>.</param>
-        public StatisticItem([NotNull] string name, [NotNull] Func<Drawable> createContent, bool requiresHitEvents = false, [CanBeNull] Dimension dimension = null)
+        public StatisticItem(LocalisableString name, [NotNull] Func<Drawable> createContent, bool requiresHitEvents = false, [CanBeNull] Dimension dimension = null)


Why isn't this

Suggested change

public StatisticItem(LocalisableString name, [NotNull] Func<Drawable> createContent, bool requiresHitEvents = false, [CanBeNull] Dimension dimension = null)

public StatisticItem(LocalisableString? name, [NotNull] Func<Drawable> createContent, bool requiresHitEvents = false, [CanBeNull] Dimension dimension = null)

And no, this isn't a NRT issue or anything, because LocalisableString is a struct. It's really weird to have a nullable name field, then one obsoleted ctor, and then another that receives the name, but the name isn't actually optional. It looks like a bug/misannotation.

Also, you will want to chase down all StatisticItems that pass string.Empty as the name and update them to pass null instead, because otherwise, this breakage happens:

master this PR

The fix looks ok, but this needs to be listed in the OP as a breaking change so that consumers know they also need to change empty strings to null.

naoei · 2022-08-14T19:18:11Z

I've applied the changes requested, however, I don't understand what you mean here:

(As an aside I'm a bit skeptical that the API there is going to fly. As I understand it, the design is such that both the label and the description must come from the same class with localisables, which I see becoming a problem down the line. But I guess this can be addressed in due time.)

Do you mean that mod settings label and descriptions should be separated? Why can't it be unified in something like a ModSettingStrings localization class?

bdach · 2022-08-14T19:22:32Z

SettingSourceAttribute is not mod-specific. It started as a thing for mods, but now it's used more extensively - skin editor also uses it, and I wouldn't be surprised to see new usages in other contexts.

As I said though, not a big deal. If a case ever arises where we want to pull the title from one localisation class and the description from another, the current setup can be trivially extended to support it.

bdach · 2022-08-17T20:08:35Z

Pushed one menial xmldoc cleanup (d06959e).

Diff looks fine to me now, but given it contains breaking changes, I'm not merging without a second approval. I will say that the number of files touched is a bit ridiculous (nearing on a hundred), but given they're 95% menial type changes and the diffstat altogether still has an acceptable size, I'm personally willing to look away this time.

smoogipoo · 2022-08-18T05:57:23Z

osu.Game/Configuration/SettingSourceAttribute.cs

+        public SettingSourceAttribute(Type declaringType, string label, string? description = null)
+        {
+            Label = getLocalisableStringFromMember(label) ?? string.Empty;
+            Description = getLocalisableStringFromMember(description) ?? string.Empty;
+
+            LocalisableString? getLocalisableStringFromMember(string? member)
+            {
+                if (member == null)
+                    return null;
+
+                var property = declaringType.GetMember(member, BindingFlags.Static | BindingFlags.Public).FirstOrDefault();
+
+                if (property == null)
+                    return null;
+
+                switch (property)
+                {
+                    case FieldInfo f:
+                        return (LocalisableString)f.GetValue(null).AsNonNull();
+
+                    case PropertyInfo p:
+                        return (LocalisableString)p.GetValue(null).AsNonNull();
+
+                    default:
+                        throw new InvalidOperationException($"Member \"{member}\" was not found in type {declaringType} (must be a static field or property)");
+                }
+            }
+        }


Not sure I fancy this custom implementation, but it's probably okay for now...

Yeah, I was personally thinking it would be better to share this logic with osu!framework somehow.

peppy · 2022-08-18T06:19:31Z

osu.Game/Screens/Ranking/Statistics/StatisticContainer.cs

@@ -59,7 +59,7 @@ public StatisticContainer([NotNull] StatisticItem item)

        private static Drawable createHeader(StatisticItem item)
        {
-            if (string.IsNullOrEmpty(item.Name))
+            if (item.Name == null)


Isn't this going to handle string.Empty incorrectly now?

peppy · 2022-08-18T06:20:07Z

osu.Game/Screens/Ranking/Statistics/StatisticItem.cs

@@ -18,7 +19,7 @@ public class StatisticItem
        /// <summary>
        /// The name of this item.
        /// </summary>
-        public readonly string Name;
+        public readonly LocalisableString? Name;


Does this need to be nullable? Shouldn't a user just be providing string.Empty as before rather than null?

Yeah, I was going to suggest this as well. I think there's a difference between default(LocalisableString) and (LocalisableString)string.Empty, though, and we don't provide a LocalisableString.IsNullOrEmpty() or similar method.

I would agree with this being non-null, but I think we need that method to do so.

For sure. I think a method like this should be added here or framework before this change is made.

Would it be fine if I were to add the LocalisableString.IsNullOrEmpty() within osu! then later on move it to the framework side of things?

I'm already in the process of adding it. I think this change can wait on it.

peppy · 2022-08-22T05:24:14Z

I think these changes look okay, but how does this work in practice? ie. how does a ruleset source localisation resources? Would it have to add its localisations to the global localisation stores?

Also noticed that GetVariantName wasn't updated. I'm not sure these would be localised in practice, but if there's a chance they may be then probably best to make the change to that while we're here.

osu/osu.Game/Rulesets/Ruleset.cs

Line 291 in edc4ace

public virtual string GetVariantName(int variant) => string.Empty;

smoogipoo · 2022-08-22T05:47:25Z

ie. how does a ruleset source localisation resources? Would it have to add its localisations to the global localisation stores?

I believe it doesn't need to, since ResourceManagerLocalisationStore looks through all loaded assemblies for the namespace in the prefix. It should just work. Have a look at https://github.com/LumpBloom7/sentakki/blob/master/osu.Game.Rulesets.Sentakki/Localisation/Mods/SentakkiModAutoTouchStrings.cs for reference

peppy · 2022-08-22T05:48:48Z

Crazy.

Thoughts on the variant name method? Should we just change while we're here?

smoogipoo · 2022-08-22T05:49:38Z

I'd agree with changing GetVariantName(). It's documented as used in settings.

naoei added 7 commits August 10, 2022 15:51

Change most ruleset-accessible string types to Localisable strings

8cb2e11

Change mod description type to LocalisableString

60dae70

Revert localisation for GetDisplayNameForHitResult

1e356f6

Came across an issue where `LeaderboardScoreTooltip` attempts to capitalize all letters for the `displayName`. Unsure if I should completely ignore it and localise it anyway.

Don't render statistic header if display string is null

6e13cf8

Change ruleset mod description types

3e38bac

Change message type osu resume overlay

a42b809

Enable localisation for SettingSourceAttribute

7cbe2fa

pull-request-size bot added the size/L label Aug 10, 2022

LumpBloom7 mentioned this pull request Aug 11, 2022

Add localisation support LumpBloom7/sentakki#369

Merged

bdach added area:mods type:localisation labels Aug 14, 2022

bdach requested changes Aug 14, 2022

View reviewed changes

naoei and others added 4 commits August 14, 2022 14:51

Allow StatisticItem's name param to be nullable

18ce784

Localise hit result name

45e9eda

Add test coverage for localisable setting source

784ce4d

Merge branch 'master' into ruleset-localization

c940f5a

Update incorrect xmldoc

d06959e

bdach previously approved these changes Aug 17, 2022

View reviewed changes

smoogipoo previously approved these changes Aug 18, 2022

View reviewed changes

smoogipoo requested a review from peppy August 18, 2022 06:02

peppy reviewed Aug 18, 2022

View reviewed changes

smoogipoo mentioned this pull request Aug 18, 2022

Add LocalisableString.IsNullOrEmpty and LocalisableString.IsNullOrWhiteSpace ppy/osu-framework#5364

Merged

peppy mentioned this pull request Aug 18, 2022

Test ruleset compatibility during initial startup to avoid runtime errors #19830

Merged

naoei and others added 3 commits August 20, 2022 21:39

Merge branch 'master' into ruleset-localization

189a407

Make StatisticItem.Name not nullable

9386d35

Check if StatisticItem.Name is null or empty

29ef1c8

naoei dismissed stale reviews from smoogipoo and bdach via 29ef1c8 August 21, 2022 01:49

smoogipoo previously approved these changes Aug 22, 2022

View reviewed changes

peppy self-requested a review August 22, 2022 04:45

Update GetVariantName to also support localisation

d199b3b

peppy dismissed smoogipoo’s stale review via d199b3b August 22, 2022 05:51

peppy approved these changes Aug 22, 2022

View reviewed changes

peppy merged commit 058d67f into ppy:master Aug 22, 2022

naoei deleted the ruleset-localization branch August 22, 2022 07:15

peppy mentioned this pull request Aug 22, 2022

Ruleset API breaking changes / announcements #19068

Closed

naoei mentioned this pull request Aug 22, 2022

Add new mod and ui strings taulazer/tau#318

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change most ruleset-accessible string types to Localisable strings #19695

Change most ruleset-accessible string types to Localisable strings #19695

naoei commented Aug 10, 2022 •

edited by peppy

Loading

bdach commented Aug 14, 2022

bdach Aug 14, 2022

naoei Aug 14, 2022 •

edited

Loading

bdach Aug 14, 2022

smoogipoo Aug 15, 2022

peppy Aug 15, 2022

smoogipoo Aug 15, 2022

peppy Aug 15, 2022

smoogipoo Aug 15, 2022

bdach Aug 14, 2022

bdach Aug 14, 2022

naoei commented Aug 14, 2022 •

edited

Loading

bdach commented Aug 14, 2022

bdach commented Aug 17, 2022

smoogipoo Aug 18, 2022

naoei Aug 18, 2022

peppy Aug 18, 2022

peppy Aug 18, 2022

smoogipoo Aug 18, 2022

peppy Aug 18, 2022

naoei Aug 18, 2022

smoogipoo Aug 18, 2022 •

edited

Loading

peppy commented Aug 22, 2022

smoogipoo commented Aug 22, 2022 •

edited

Loading

peppy commented Aug 22, 2022

smoogipoo commented Aug 22, 2022

		@@ -14,7 +15,7 @@ public class CatchModFloatingFruits : Mod, IApplicableToDrawableRuleset<CatchHit
		{
		public override string Name => "Floating Fruits";

	public StatisticItem(LocalisableString name, [NotNull] Func<Drawable> createContent, bool requiresHitEvents = false, [CanBeNull] Dimension dimension = null)
	public StatisticItem(LocalisableString? name, [NotNull] Func<Drawable> createContent, bool requiresHitEvents = false, [CanBeNull] Dimension dimension = null)

Change most ruleset-accessible string types to Localisable strings #19695

Change most ruleset-accessible string types to Localisable strings #19695

Conversation

naoei commented Aug 10, 2022 • edited by peppy Loading

Breaking changes

bdach commented Aug 14, 2022

Choose a reason for hiding this comment

naoei Aug 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naoei commented Aug 14, 2022 • edited Loading

bdach commented Aug 14, 2022

bdach commented Aug 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoogipoo Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

peppy commented Aug 22, 2022

smoogipoo commented Aug 22, 2022 • edited Loading

peppy commented Aug 22, 2022

smoogipoo commented Aug 22, 2022

naoei commented Aug 10, 2022 •

edited by peppy

Loading

naoei Aug 14, 2022 •

edited

Loading

naoei commented Aug 14, 2022 •

edited

Loading

smoogipoo Aug 18, 2022 •

edited

Loading

smoogipoo commented Aug 22, 2022 •

edited

Loading