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

Improve flexibility with build-in controls localization #13773

Merged
merged 8 commits into from
Feb 27, 2024

Conversation

maxkatz6
Copy link
Member

@maxkatz6 maxkatz6 commented Nov 29, 2023

What does the pull request do?

We have a longstanding problem with hardcoded localization of our built-in controls. This PR does make it more flexible by reusing the existing infrastructure of resource dictionaries (which is the main difference from the discussion of #8741). After discussion, these resources won't affect performance in any measurable way, especially when compared with other possible solutions. Also, it doesn't introduce a dependency on IStringLocalizer nor duplicate it, while still making it possible to use one, if a developer wants.

In the future, I am going to implement something like PlatformSettigns.PreferredUILanguage so it would be possible to get the current UI language from the OS and subscribe to its changes. But it is out of the scope of this PR.

What is the current behavior?

  1. It is pretty hard to redefine hardcoded English texts of our default controls.
  2. If anybody wants to implement custom IResourceProvider, they have to inherit a whole ResourceDictionary with its own problem - developers might not need a dictionary.

What is the updated/expected behavior with this PR?

  1. It should be pretty easy to redefine locale resources.
  2. It is also possible to inherit new abstract ResourceProvider type. NOTE: It is still possible to use normal ResourceDictionary like we have in some samples - https://github.com/timunie/Tims.Avalonia.Samples/tree/main/src%2FLocalizationSample. ResourceProvider is useful mostly when developer want to use non-dictionary-based locale storage.

On the last one in more details:

public class LocaleResourcesProvider : ResourceProvider
{
    private IStringLocalizer _stringLocalizer;
    public LocaleResourcesProvider(IStringLocalizer stringLocalizer)
    {
        _stringLocalizer = stringLocalizer;
        // Doesn't really matter how you handle language changes,
        // you can let Avalonia know that this ResourceProvider has updates by calling RaiseResourcesChanged
        AppGlobals.AppLocaleChanged += () => this.RaiseResourcesChanged();
    } 

    public override bool HasResources => true;
    public override bool TryGetResource(object key, ThemeVariant? theme, out object? value)
    {
        if (!(key is string resourceName))
        {
            value = null;
            return false;
        }

        value = _stringLocalizer[resourceName];
        return value is not null;
    }
}
   thisApp.Resources.MergedDictionaries.Add(new LocaleResourcesProvider(stringLocalizer))

Fixed issues

Fixes #8741
Fixes #14706

@maxkatz6
Copy link
Member Author

if I missed any of the controls - let me know. For some reason, Rider doesn't report localizable strings warnings now, even though it always annoyed me with that before.

@maxkatz6 maxkatz6 added the backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch label Nov 29, 2023
@timunie
Copy link
Contributor

timunie commented Nov 29, 2023

/cc @robloo as you have made the ColorPicker which is one of the main controls to be translated :-)

@robloo
Copy link
Contributor

robloo commented Nov 29, 2023

I don't think this is right design for localization. We were on a better track in the old discussion. This seems like a quick and dirty hack rather than an integrated design. I suppose something is better than nothing but these types of things have a habit of staying around forever.

  • The name Locale seems pretty Avalonia specific. Probably it's better to use more standard terminology.
  • Using ResourceDictionary creates a problem with collisions or at least restricts keys apps can define themselves. That wasn't an issue in other localization systems.
  • You are hard-coding the translations in source code? Doesn't anyone do this in a large project!?
  • XML resource files have been the standard for this since WinForms. And there is a lot of tooling that supports it. It's very easy, for example, to load up all cultures in one table and see which values are missing per language. ResxManager plug-in is very widely used.
  • XML resource files support adjusting widths and things aside from just text. This system supports that as well but it's not as clear. We will really need to come up with good naming conventions and stick with it. In other systems you just said ControlName.Width or ControlName.Text.
  • What about all the accessibility names? I don't see that covered here, has it been discussed?
  • I suppose a benefit of doing it this way is there is no need to name controls.

Edit, added:

  • It was always a good idea to have culture-specific resources in folders like es-ES OR have that prefixed or suffixed on the file name itself. Then tooling pickes up the right files to use based on system or app settings.
  • Also note that existing localization systems are smart with language fallbacks. You can defined a broad group of lanaguages like es which is used for all Spanish or you can override it with a specific culture's variant es-ES for Spain.
  • What thought was given to flagging out of date translation strings? In UWP localization files had the translation as well as THE ORIGNAL string value in say English. If the current English string no longer matches what is in the translation file the system knows its out of date and flags it.

ColorPicker which is one of the main controls to be translated :-)

@timunie I can separate out strings for this as required. However, the entire control has a different design philosophy than WinUI. In WinUI they have text names for almost everything. In Avalonia's ColorPicker I specifically tried to use symbols which don't need to be localized. Even the 'R', 'G', 'B' strings are considered universal symbols for the RGB color model and don't need to be translated (PhotoShop doesn't localize these and we decided to use their trick years ago).

About the only thing that needs localization right now are the actual color names and unfortunately there are quite a few of those. In the future this will probably expand to slider tooltip text and accessibility names though.

@robloo
Copy link
Contributor

robloo commented Nov 29, 2023

PlatformSettigns.PreferredUILanguage

This should probably be named RequestedUILanugage vs. ActualUILanuage.

@maxkatz6
Copy link
Member Author

ColorPicker which is one of the main controls to be translated

Oh, right, I forgot it had localizable strings too. As robloo said it should have way less of them compared to UWP, but worth double check. However, I have doubts if I should localize color names in this PR. UWP did it IIRC.

We were on a better track in the old discussion. This seems like a quick and dirty hack rather than an integrated design. I suppose something is better than nothing but these types of things have a habit of staying around forever.

First of all, a reminder, the problem is not to provide a localization framework to the users. The problem is to make build-in controls localizable with a little effort.

The old design idea was flawed and suitable for some people while being a nuisance for other.
I dislike it for a couple of big reasons, that are hard to solve in the best way:

  1. It makes the source code of the control aware of the template. I.e. control sets localization - not the template. The same problem was common with UWP/WPF controls as well. This PR doesn't have this problem - all localization is made in the template.
  2. It would introduce new infrastructure, conflicting with existing IStringLocalizer, attempting to solve localization problems for everyone, as it would end up a public API. I hate adding something half-backed to the public API, but I also can't bake it well for everybody. This PR doesn't introduce new APIs at all. Please consider ResourceProvider class as a base building block for any IResourceProvider, detached from any specific use case.
  3. It relied on static globals a lot. Something that can be avoided technically though. Like, with AppBuilder registration of some sort of localizer factory. And some sort of way to tell Avalonia that language was changed. There is no static globals in this PR either.

The developer is not forced to do anything. If they need to override strings used in default controls - they can do it after this PR. They can use XML, or txt, or hardcode it, or redirect to IStringLocalizer. IResourceProvider is an old interface that can do anything if that can implement TryGetResource.

Problems with this PR:

  1. Hard-coding single translation in the code? Yes, that's a valid point. But at this point, we don't plan on supporting multiple languages out of the box. But if we decide to do so - a current solution can be easily changed or even replaced for better needs, without breaking literally anything.
  2. Resource collisions - true, it might happen. Since resource keys can be of any object, I can put enum there to avoid collisions with strings. But I am not sure how big this problem is. Keep in mind, that localization here is also part of the specific theme.

Not using XML?

Well, as I said you can't make it good for everybody. Also, this single language is, in fact, defined in XML (XAML). Also, it's an internal implementation detail - developers don't see it.

XML resource files support adjusting widths and things aside from just text

Okay, we will never use RESX/RESW for XAML for internal localization due to technical reasons - satellites, aot and generally an overhead. There was an idea to generate static consts from RESX files with a source generator, but it would kill the whole point. And use xaml resources also support width and heights or any other values that can be put to the resources.

What about all the accessibility names

True, it's not covered by this PR. But I also don't see how it would be a problem with the current solution. In XAML accessibility names are defined in XAML, and we also can use dynamic resources there.

The name Locale seems pretty Avalonia-specific

Yeah, a better one can be used.

What thought was given to flagging out-of-date translation strings

Same as with any other resource. When a new text resource is added, and not yet overridden by the developer, - the original one will be used.

It was always a good idea to have culture-specific resources in folders like es-ES
Also note that existing localization systems are smart with language fallbacks

As mentioned above, it can be extended as part of the same internal implementation detail, even though we don't have plans for this yet. And, at the same time, developers can use whatever they want by providing a custom IResourceProvider. For example, IStringLocalizer does support language fallbacks when used with RESX. It can be used from IResourceProvider as well.

@maxkatz6
Copy link
Member Author

This should probably be named RequestedUILanugage vs. ActualUILanuage.

Not sure. It's a language provided by the OS, since it's part of the IPlatformSettigns interface. "Requested" term normally identifies something required by the user, which might not be the case with OS language. And "Actual" language isn't fitting well either, as OS language can be ignored in the app. But yeah, it is outside of this PR's scope.

@maxkatz6
Copy link
Member Author

I suppose something is better than nothing but these types of things have a habit of staying around forever.

Yes and yes. And yes, it is expected to stay here for a while. While being easy to remove if we decide so.

@maxkatz6
Copy link
Member Author

Changed sample IResourceProvider implementation to use IStringLocalizer and avoiding hardcoded strings in the description.

@maxkatz6 maxkatz6 removed the backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch label Dec 1, 2023
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.1.999-cibuild0042481-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

# Conflicts:
#	src/Avalonia.Themes.Fluent/Controls/DatePicker.xaml
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.1.999-cibuild0043442-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.1.999-cibuild0043520-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6 maxkatz6 requested a review from a team February 3, 2024 05:51
@maxkatz6 maxkatz6 merged commit 9bb1bcc into master Feb 27, 2024
7 checks passed
@maxkatz6 maxkatz6 deleted the control-localization branch February 27, 2024 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TimePicker default text could be user set.
4 participants