Avoid users to create new instances of the LocalizationResourceManager

[pictos]

Description of Change

Just marked the public constructor from LocalizationResourceManager (LRM) as obsolete and ask the users to access the instance using the Current property.

Bugs Fixed

• Fixes [Bug] We can create new instances of the LocalizationManager #769

API Changes

Added:

• [Obsolete] public LocalizationResourceManager();

Behavioral Changes

Users can create new instances of LRM but this is marked as obsolete

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

• Updated documentation

[XamarinCommunityToolkitBot]

A new build for this PR has been completed. As a result a NuGet has been created which allows you to verify this fix or try out the new feature! Download here

[pictos]

Also, we can minimize the breaks by adding this workaround

               public static LocalizationResourceManager Current { get; } = new LocalizationResourceManager(null);
		
               LocalizationResourceManager(object obj)
		{
		}
		
                [Obsolete("Please use the Current property instead of create a new instance of this class")]
		[EditorBrowsable(EditorBrowsableState.Never)]
		public LocalizationResourceManager()
		{
		}

The good of the break is that we will force all devs to use it in the right way... The bad part is the break.
Thoughts?

[AndreiMisiukevich]

Or just

#pragma warning disable
public static LocalizationResourceManager Current { get; } = new LocalizationResourceManager();	
#pragma warning restore

[Obsolete("Please use the Current property instead of create a new instance of this class")]
[EditorBrowsable(EditorBrowsableState.Never)]
public LocalizationResourceManager()
{
}

[AndreiMisiukevich]

@pictos but basically do you think that many people already use this class?
Maybe we can just message all of them )

[pictos]

but basically do you think that many people already use this class?

@AndreiMisiukevich I have no idea, but we have more than 4k users, so if they need i18n on their apps, they can rely on our implementation and miss use it. If you look at this PR's issue, you can see that devs can miss understand it and create new instances per resx file. Or even wrap up this in a DI and not use the singleton for it. Here I'm trying to fix the API and protect the devs from themselves.

Yeah, your suggestion is better than mine, I'll push it.

[XamarinCommunityToolkitBot]

A new build for this PR has been completed. As a result a NuGet has been created which allows you to verify this fix or try out the new feature! Download here

[jfversluis]

So just to be clear, this doesn't really break the users, right? This just obsoletes it and directs them to use something else?

Then can you make the actual breaking change and PR that into develop @pictos ? can't do that yet, because when we merge main to develop then we're back with this change again... Let's at least make an issue so we won't forget. while we figure out how to go about this the best way

[jfversluis]

@pictos could you update the description in the first comment to reflect this change? You're now talking about the private constructor which is not accurate anymore.

Then merge away :)

[maxkoshevoi]

@pictos, @AndreiMisiukevich, @jfversluis Just to be clear, what should we do when user has multiple resource files? Like described in the issue linked to this PR. Do we just not support that scenario?

Currently Translate extension won't work with multiple LocalizationResourceManager instances (we can add another parameter to it though), but LocalizedString will (thanks to second constructor that accepts LocalizationResourceManager instance).

[jfversluis]

@maxkoshevoi do you know how that works in "normal" .NET/C# projects?

[maxkoshevoi]

@jfversluis in normal project if you have several resource files, you can reference each of them by name, and get localized resource strings from them:

So normal projects allow for multiple resource files no problem. By restricting creation of LocalizationResourceManager instances we are saying that user mast have no more than one resource file (with localized variations of it) to use our on-the-fly culture change.

[AmrAlSayed0]

@jfversluis There was a discussion here #769 (comment) you can see what I mean. BTW, I'm generating my AppResource.Designer.cs files with the PublicResXFileCodeGenerator instead of ResXFileCodeGenerator so that it is public and can be used from other assemblies/projects that reference it.

[pictos]

@AmrAlSayed0 are you able to test your project with the nugget generated from this PR?

[AmrAlSayed0]

@pictos No, I didn't see a way how I would be able to unless TranslateExtension is modified to take an instance of LocalizationResourceManager as a parameter or something (Maybe, optionally, in which case use .Current)

And by the way, I should mention that I'm bringing this up because that is MY use case. I'm in no way blocking this PR. If you feel that the LocalizationResourceManager and TranslateExtenstion are ok as they are now and you got approved, go ahead and merge it. I just won't be able to use it. I already wrote something similar in my own app and I use XCT in my app so I thought I could just give up my poorly written implementation and use XCT's but this PR won't affect me very much, so feel free to do whatever.

[pictos]

@AmrAlSayed0 if you have time to test it will be great because I can try to make it work on your scenario. I don't want to break any current user. If you are not able to test there is no problem I can try to implement something similar to test it.
Even if this PR gets merged you will be able to create a new instance, I just marked that constructor as obsolete.

[maxkoshevoi]

@pictos If TranslateExtension had optional LocalizationResourceManager parameter, we would be able to fully support multiple resource files. What do you think about adding it?

[pictos]

@maxkoshevoi that could be a thing, I need to try out that first. The downside is that will add more complexity to the lib.

[maxkoshevoi]

@pictos One optional parameter for markup extension that only has two parameters shouldn't complicate things much, but it will allow people with more than one resource file to use our culture changing flow.
Also LocalizationResourceManager constructor shouldn't be obsolete in this case.