-
Notifications
You must be signed in to change notification settings - Fork 420
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
Introduce a pluralisable string type #4918
base: master
Are you sure you want to change the base?
Conversation
…rrent locale + add corresponding test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% on the PluralisableString : TranslatableString
inheritance. Feels it complicates code and maybe it may be easier to duplicate the pieces that need to be duplicated for it to work, dunno.
@@ -71,14 +73,14 @@ public string GetLocalised(LocalisationParameters parameters) | |||
catch (FormatException e) | |||
{ | |||
// The formatting has failed | |||
Logger.Log($"Localised format failed. Key: {Key}, culture: {parameters.Store.EffectiveCulture}, fallback format string: \"{Fallback}\", localised format string: \"{localisedFormat}\". Exception: {e}", | |||
Logger.Log($"Localised format failed. Key: {Key}, culture: {parameters.Store.EffectiveCulture}, fallback format string: \"{GetLocalisedFormat(parameters, Fallback)}\", localised format string: \"{localisedFormat}\". Exception: {e}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this call GetLocalisedFormat()
again rather than use the localisedFormat
local as previously?
@@ -90,7 +92,7 @@ public bool Equals(TranslatableString? other) | |||
&& Args.SequenceEqual(other.Args); | |||
} | |||
|
|||
public bool Equals(ILocalisableStringData? other) => other is TranslatableString translatable && Equals(translatable); | |||
public virtual bool Equals(ILocalisableStringData? other) => other is TranslatableString translatable && Equals(translatable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If other
is a PluralisableString
, this implementation is potentially wrong. The class type checking here should be done in an invariant manner (i.e. using GetType() == other.GetType()
rather than via is
- because in this case, a PluralisableString
theoretically is a TranslatableString
).
//use the last plural form available for the current locale if the requested form is missing from translated strings. | ||
return variants.ElementAtOrDefault(getPluralIndex(parameters)) ?? variants.ElementAt(variants.Length - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about using .ElementAt()
on an array. As far as I can tell it shouldn't incur allocs but you may as well write
//use the last plural form available for the current locale if the requested form is missing from translated strings. | |
return variants.ElementAtOrDefault(getPluralIndex(parameters)) ?? variants.ElementAt(variants.Length - 1); | |
// use the last plural form available for the current locale if the requested form is missing from translated strings. | |
int pluralIndex = getPluralIndex(parameters); | |
return variants[Math.Min(pluralIndex, variants.Length - 1)]; |
On a second thought, reverting the inheritance and simply copying over the format logic sounds cleaner, as that would remove the need to override the equal checks which are becoming messy. Will do that instead. |
would this allow for omitting variables (including cases with multiple ones)?
|
As far as I can tell this PR already supports that, yes. (see |
PRing this in a working state to discuss about integrating this in the localisation pipeline.
This implements a
PluralisableString
type deriving fromTranslatableString
, that can display a pluralised variant of a translation according to the plural context.I initially had the 'separator character' delimiting two variants in localised text as a ctor parameter but I guess it should be fine to have it be hardcoded as a '|' for now, which corresponds to what's used on web-side to separate plural variants altough I'm opened to make it a parameter again.
The integration should be transparent on osu!-side with required changes being on
osu-localisation-analyzer
end.Plans to do so sum up to adding heuristics to detect strings which support pluralisation (contains the
|
delimiter) and usePluralisableString
in place ofTranslatableString
in that case. I'll mention that I haven't delved into it yet but according to my memory from a previous look at the source, that should be pretty straightforward.The diffstat makes this PR looks big where in fact more than half of it comes from the locale -> plural count mapping.
Implementation references
https://github.com/laravel/framework/blob/f60d3da8f9ad236d339562cd89eb5dfde4eb7dce/src/Illuminate/Translation/MessageSelector.php#L99 for code-based plural rules
Spec at : https://unicode-org.github.io/cldr-staging/charts/latest/supplemental/language_plural_rules.html