Skip to content
This repository was archived by the owner on May 1, 2024. It is now read-only.

[Enhancement] Support for more than one resource manager #1047

Closed
wants to merge 56 commits into from

Conversation

maxkoshevoi
Copy link
Contributor

@maxkoshevoi maxkoshevoi commented Mar 7, 2021

Description of Change

We use two classes to get localized values at runtime: TranslateExtesion and LocalizedString.

  • LocalizedString doesn't store or reference any ResourceManager, so it basically already supports more then one.
  • TranslateExtesion Always uses ResourceManager from LocalizationResourceManager, so this needs to change.

What was done

  • Added new optional ResourceManager parameter to TranslateExtesion, so that user could specify ResourceManager for each string. If none is specified, LocalizationResourceManager.DefaultResourceManager is used.
  • Renamed resourceManager in LocalizationResourceManager to DefaultResourceManager and made it readonly and internal (so that TranslateExtesion could read it).
  • Removed indexer and GetString from LocalizationResourceManager (as suggested by @brminnick here) They were only needed to be used in TranslateExtesion, and now it no longer uses them.

Alternative implementations

  • DefaultResourceManager can be moved to TranslateExtesion as nothing else uses it. But then users will need to initialize TranslateExtesion as well as LRM

Bugs Fixed

API Changes

Added:

  • ResourceManager TranslateExtension.ResourceManager { get; set; } = LocalizationResourceManager.Current.DefaultResourceManager
  • void LocalizationResourceManager.Init(CultureInfo initialCulture); (since passing resource manager is no longer mandatory)

Changed:

  • void LocalizationResourceManager.Init(ResourceManager resource) => void LocalizationResourceManager.Init(ResourceManager defaultResourceManager) (renamed parameter)

Removed:

  • string LocalizationResourceManager.GetValue(string text)
  • string LocalizationResourceManager.this[string text]
  • LocalizedString(LocalizationResourceManager localizationManager, Func<string> generator = null) (user cannot create new instances of LRM, so this constructor doesn't make sense)

Behavioral Changes

User can now pass any resource manager in TranslateExstension

Breaking Changes

  • Removed indexer and GetValue from LRM, but they shouldn't be used anyway. And I can bring them back if needed. Their removal is not critical to this PR.
  • Renamed resource parameter to defaultResourceManager.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard

@maxkoshevoi maxkoshevoi changed the title [Enhancement] TranslateExtension to support for more than one resource manager [Enhancement] Support for more than one resource manager Mar 7, 2021
@maxkoshevoi
Copy link
Contributor Author

@pictos @brminnick @jfversluis Could you please take a look at this?

@maxkoshevoi
Copy link
Contributor Author

maxkoshevoi commented Mar 7, 2021

@brminnick I know, you wanted to remove backing field for culture from LRM as well, but your solution:

public CultureInfo CurrentCulture
{
	get => CultureInfo.DefaultThreadCurrentUICulture ?? CultureInfo.DefaultThreadCurrentCulture;
	set
	{
		if (CurrentCulture == value)
		{
			return;
		}

		CultureInfo.DefaultThreadCurrentUICulture = value;
		CultureInfo.DefaultThreadCurrentCulture = value;
		OnPropertyChanged(nameof(CurrentCulture));
	}
}

Works on Android, but doesn't work on UWP (cannot test on iOS). Looks like resource managers in UWP get their default culture from some other place than CultureInfo.DefaultThreadCurrentUICulture

@pictos
Copy link
Contributor

pictos commented Mar 7, 2021

@maxkoshevoi right now we are focused in um merge the nullable PR asap. So we will review this one right after the nullable is merged (:

@maxkoshevoi
Copy link
Contributor Author

@pictos Sounds great! I just had some spare time, so implemented this one.

@pictos
Copy link
Contributor

pictos commented Mar 12, 2021

@maxkoshevoi, can you rebase your PR against develop?

@maxkoshevoi
Copy link
Contributor Author

maxkoshevoi commented Mar 12, 2021

@pictos It's already against develop (I merged nullable PR in a0db18c).
CI failed because of a timeout (created an issue for that #1065), I'll rerun it. There are no conflicts and all tests are passing.

Is there something else that needs to be done?

@maxkoshevoi
Copy link
Contributor Author

@AndreiMisiukevich Could you please assign @brminnick as a reviewer here?

@AndreiMisiukevich
Copy link
Contributor

@maxkoshevoi done)

@AndreiMisiukevich AndreiMisiukevich requested a review from pictos April 1, 2021 08:24
@TheCodeTraveler TheCodeTraveler added the ❗ Breaking Change This issue/PR described a breaking change! Proceed with caution and never merge to current stable! label Apr 3, 2021
@TheCodeTraveler
Copy link
Contributor

TheCodeTraveler commented Apr 3, 2021

FYI - this introduces a breaking change by modifying the constructor of LocalizedString.

This should be merged into a major release, e.g. v2.0.0

@maxkoshevoi
Copy link
Contributor Author

@brminnick It seems like there won't be 2.0.0 considering XCT stops accepting new features in September 2021. Should this PR be merged in 1.3.0 (if approved), transferred to CommunityToolkit.Maui, or maybe something else?

@TheCodeTraveler
Copy link
Contributor

Thanks! However, we are no longer adding new features to Xamarin Community Toolkit, focusing on the .NET MAUI Community Toolkit.

I've posted more information about the Future Of Xamarin Community Toolkit here: https://devblogs.microsoft.com/xamarin/the-future-of-xamarin-community-toolkit/?WT.mc_id=mobile-0000-bramin

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
❗ Breaking Change This issue/PR described a breaking change! Proceed with caution and never merge to current stable!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants