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 a way to customise the CultureInfo and IFormatProvider used for formatting LocalisableStrings #5538

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Susko3
Copy link
Member

@Susko3 Susko3 commented Nov 22, 2022

New methods in LocalisationManager

protected virtual IFormatProvider CustomiseCultureAndFormatProvider(CultureInfo culture)
  • This can be used in by inheritors to provide a custom format provider and to customise the culture (by directly modifying it's properties).
protected void RefreshCulture()
  • Used in combination with the above, allows to re-run CustomiseCultureAndFormatProvider().
  • Example: refresh the culture when OsuSetting.Prefer24HourTime changes.

This will allow us to address ppy/osu#21157, and have 12/24 hour time preference apply globally to all LocalisableStrings.

CustomLocalisationManagerTest gives a glimpse of how this will work osu!-side.

Breaking changes

ILocalisationStore.EffectiveCulture renamed to UICulture

Renamed to better reflect how it's used.

peppy
peppy previously approved these changes Nov 23, 2022
@peppy peppy requested a review from bdach November 23, 2022 09:54
@Susko3
Copy link
Member Author

Susko3 commented Nov 23, 2022

Note that the way this PR resolves the current culture and UI culture is different from how Windows does it.

Windows will use CurrentUICulture for the language of the UI and translations, but will use CurrentCulture for formatting numbers, dates, etc. This has (to me) the unwanted side-effect that the two languages are awkwardly mixed together:

image

This PR uses CurrentUICulture same as windows -- for string lookups. But to avoid the above issue, sets the CurrentCulture so that it's a more specific version of the CurrentUICulture / ILocalisationStore.UICulture.

This avoid the problem of mixing languages, but still allows user choice. Eg. if using the en language in osu!, the date formats will be different if the user has set en-US or en-GB in windows settings. And if the user doesn't have English in their language list, it'll use the ILocalisationStore-provided en.

@bdach
Copy link
Collaborator

bdach commented Nov 23, 2022

I've read through this and to be frank the increase in complexity scares me. Even though I have my educated guesses, I'm still not 100% sure why the amount of "culture" knobs in localisation parameters has tripled and why some places will now be able to use a different knob from the other.

I will trust your judgement that introducing all this is necessary and unavoidable, but I do find myself not fully understanding it. There are a lot of words being thrown around about "culture" vs "UI culture" vs "format provider" but not much visible human-understandable justification for the split.

bdach
bdach previously approved these changes Nov 23, 2022
Comment on lines +26 to +42
/// <summary>
/// Culture that is used for culture-specific string formatting, case transformations, etc.
/// </summary>
public readonly CultureInfo Culture;

/// <summary>
/// Culture that is used for language/string lookups.
/// </summary>
/// <remarks>
/// Usually corresponds to <see cref="Store"/>.<see cref="ILocalisationStore.UICulture"/>.
/// </remarks>
public readonly CultureInfo UICulture;

/// <summary>
/// <see cref="IFormatProvider"/> that is used for culture-specific formatting of <see cref="LocalisableFormattableString"/>s.
/// </summary>
public readonly IFormatProvider FormatProvider;
Copy link
Contributor

Choose a reason for hiding this comment

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

The descriptions and names of these don't help me in understanding when each one should be used. Both mention very similar things in the descriptions and I had to look elsewhere to find out the specifics.

For others seeking knowledge, see here:

Given the above resources, maybe we should take this opportunity to separate from the "UI" naming scheme, and perhaps use something like "FormattingCulture" and "LocalisationCulture" instead?

Additionally, is there any purpose for FormatProvider to be a separate member here? My thinking is CustomiseCultureAndFormatProvider could return the new CultureInfo that would be used as the Culture member here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wholeheartedly agree with dropping the (UI)?Culture vernacular and using something, anything more descriptive. While those are canonical .NET names, they are also terrible, horrible, no good, very bad names. I guess you could mention in xmldoc which one maps to which though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given the above resources, maybe we should take this opportunity to separate from the "UI" naming scheme, and perhaps use something like "FormattingCulture" and "LocalisationCulture" instead?

Agree with the UICultureLocalisationCulture part. FormattingCulture conflicts with FormatProvider and is too narrow; it should be used anywhere where the current culture is required (eg. in CaseTransformableString – which is not strictly formatting). I think keeping Culture is appropriate for this.

LocalisationParameters.UICulture shouldn't be really used for anything, it's used privately in ILocalisationStore implementations. So it could be removed from LocalisationParameters to reduce the confusion. It's basically Store.UICulture ?? CultureInfo.InvariantCulture so there's no need for it.

Additionally, is there any purpose for FormatProvider to be a separate member here? My thinking is CustomiseCultureAndFormatProvider could return the new CultureInfo that would be used as the Culture member here.

Having a custom IFormatProvider allows for more flexibility. Have a look at CustomLocalisationManagerTest.CustomFormatProvider for an example where customizing just the culture is not enough.

Copy link
Collaborator

Choose a reason for hiding this comment

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

LocalisationParameters.UICulture shouldn't be really used for anything, it's used privately in ILocalisationStore implementations. So it could be removed from LocalisationParameters to reduce the confusion.

Removing sounds amicable to me, personally.

Having a custom IFormatProvider allows for more flexibility. Have a look at CustomLocalisationManagerTest.CustomFormatProvider for an example where customizing just the culture is not enough.

I've seen the example, but do we really need this for anything? Would using prepared CultureInfo instances not be good enough for the typical usages?

Or, in other words: Do you already foresee this being used anywhere, and if yes, then for what exactly? That particular test example doesn't really convince me much. Concrete use cases, if any are planned, are best.

Copy link
Member Author

Choose a reason for hiding this comment

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

osu! (mostly carried over from osu-web) uses a lot of slightly different date formats:

  • d MMMM yyyy
  • dd MMMM yyyy
  • d MMM yyyy
  • MMMM yyyy
  • MMM yyyy
  • yyyy-MM-dd
  • and some weird sole dd and MMM usages

I plan to combine formats which differ only in dd <->d, but I think formats that differ only in MMM <-> MMMM should stay as-is.

Here is a very wip branch that makes use of custom formats.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm... not really sure how to respond, frankly. So I guess the goal of this is to have custom osu!-specific format strings? But like, the built-in C# ones are perfectly fine?

Please don't misunderstand. My primary goal in asking these questions is to keep complexity low. And it feels like this is growing out of control into some sort of custom formatting subsystem and every question I ask is met with more code, the purpose of which isn't really clear. As far as I'm aware, the direction and rationale of these changes that would support the amount of complexity in the code is not described anywhere, so I don't even have a reference point why all of this code is here, and what was the process of arriving at this solution, and why it's all so complicated.

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.

4 participants