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

Codebase Suggestion: Make Resource management and Multilingual support mechanisms consistent with WinUI #202

Closed
ShankarBUS opened this issue Nov 6, 2020 · 13 comments · Fixed by #225
Labels
enhancement New feature or request
Milestone

Comments

@ShankarBUS
Copy link
Contributor

ShankarBUS commented Nov 6, 2020

Currently this library supports localization by using the RESX localization present in .NET out-of-the-box. (all .NET projects support RESX natively).

While UWP projects use another file format called RESW (which is a highly restricted sibling of RESX).
The RESX comes with auto-code generation, eliminating the need to do manual fetching (storing the resource name and adding a method to fetch it) of the resources.

RESW only supports string resources whereas RESX supports images, icon and many more.

In the sense of WinUI, it makes use of RESW only for string resources and has its own method of fetching the resource (storing the resource name in a const within a static class like this const string SR_RESOURCENAME = "RESOURCENAME" and fetches the value by using some helpers).

We can do the exact same!
It will eliminate the need for us to manually copy paste the resources from WinUI into the ModernWpf resource files (manual labour may result in errors).
It will give us the support for all the languages that WinUI supports! No extra work! (just have to change %1!s! to {0})

We can clone the *.resw files from WinUI to ModernWpf as *.resx files using an automation tool.
Strikingly, the RESW & RESX formats look exactly the same (even xml comments are same!).

Benefits:

  • Increased consistency with WinUI codebase and resource management.
  • No need for manual copy pasting strings.
  • Decreased error in translations.
  • String resources can be placed in a directory adjacent to their owner control just like in WinUI codebase! (RESX resources can be placed anywhere.
  • Each controls' resources will be kept adjacent to themselves thus eliminating mixing of their resources!
  • A automation tool could be made to clone the resources from WinUI, rename *.resw to *.resx and replace C++ specific string formatting characters (such as %1!s! to {0}, %1!u! to {0:d} & etc) to .NET specifc ones.

Please consider this @Kinnara.

Just give me the permission, I'll do all the work (automation tool, converting existing resources).

@ShankarBUS ShankarBUS changed the title Codebase Suggestion: Multilingual support Codebase Suggestion: Make Resource management and Multilingual support mechanisms consistent with WinUI Nov 6, 2020
@Kinnara
Copy link
Owner

Kinnara commented Nov 16, 2020

Great suggestion! Permission granted 😄

@Kinnara Kinnara added the enhancement New feature or request label Nov 16, 2020
@ShankarBUS
Copy link
Contributor Author

Hey @Kinnara, I've successfully created the automation tool (not automation TBH. "Converter" maybe?) in my fork's ResourceManagement branch.

These are the drawbacks with the current version of this tool:

  • You must have the WinUI repo cloned locally on your PC. (You need to do it manually; I had a thought about automating this one too. But meh, is it that necessary?)
  • You have to type the source/destination folder paths by yourself for now. There's no folder picker in WPF. But I will add it in the next commit.

Now let's get to the convertion procedure:

  • Run the WinUIResourcesConverter app.
  • A window will open up like this
    image
  • Copy paste the path of the control's Strings folder [that you want to convert the resources for] into the "Source directory" textbox
    (the path could be easily copied from file explorer's path textbox)
    image
  • Now click the "Load resources" button. You will get a list of available multilingual resources and their count.
    image
  • Type in the destination folder to place the generated "*.resx" files (D:\Files\ModernWpf\ModernWpf.Controls\PersonPicture\Strings in my case) and hit "Convert"
    image

It has a progress bar to show the convertion progress and each converted resources will get a check mark next to them.
image


I did my best 😁. If have any suggestions, please tell them to me.
If you prefer a CLI to GUI, I can make it a console app instead.
I will pause further progress till you check the tool's functionality and reliability and give it an approval.

@Kinnara
Copy link
Owner

Kinnara commented Dec 2, 2020

Looks brilliant! Thanks a lot. I'll give it a try soon.

@ShankarBUS
Copy link
Contributor Author

Ooooooook. So, I just found out how big of a mess this change would introduce.

Let me explain why.

  • WinUI repo doesn't contain resources for all inbox controls. It doesn't contain the source code for some controls such as ToggleSwitch and CommandBar (I just realized one thing, how did you port them if the source code is not open? What kind of sorcery is this?). Thus, the localization resources are not provided there.
  • Some controls such as TextContextMenu and ProgressBar are present in the ModernWpf assembly rather than the ModernWpf.Controls assembly.
  • Thus, I couldn't completely replace all the existing resources in the Strings.resx file and delete it.
  • This leads to another problem. WinUI has a different list of supported langs and regions. This library has it own. zh-CN vs zh-Hans and zh-TW vs zh-Hant is a great example. The resources are scattered and leaves a H U G E M E S S.

We have to

  • either find a way to replace the resources in the Strings.resx with updated ones and make them compatible with the new method
  • or live on with the current technique. (i.e. less no. of supported langs, manual copy-pasting of resources to modify them, all resources are present in a single place and not adjacent to the target control)

@JVimes
Copy link

JVimes commented Dec 9, 2020

Would this affect me as an app developer? Could I keep using RESX files for localization?

I like ModernWPF having WinUI's color, layout, and control concepts. But going beyond that makes me concerned about the learning curve, and diminishing benefit since WinUI for Desktop is already in preview.

I like the idea of supporting more languages, and automating how WinUI stuff is brought in.

@ShankarBUS
Copy link
Contributor Author

@JVimes,

Would this affect me as an app developer?

If done perfectly and completely, this change wouldn't affect any existing apps given that they didn't hard code the supporting assembly list (such as for MSI setups).

Could I keep using RESX files for localization?

Your app using RESX for localization is completely independent of this library.
They will have independent satellite assemblies. So, your localization method won't break. I can assure you that.

I like ModernWPF having WinUI's color, layout, and control concepts. But going beyond that makes me concerned about the learning curve, and diminishing benefit since WinUI for Desktop is already in preview.

That's why I didn't want to merge the related PR any time soon. I just wanted to improve this project a bit. I don't want to scare away developers. And also this is not "my project" so the author can reject this change and preserve the current state.

I like the idea of supporting more languages, and automating how WinUI stuff is brought in.

If the author (@Kinnara) wants me to convert the WinUI resources (for languages which are only supported currently) and merge them into existing Strings.resx instead of separate RESX for each controls, I could do that.

Thanks for sharing your opinion.

@JVimes
Copy link

JVimes commented Dec 10, 2020

Thank you @ShankarBUS. I understand now. 🙂 I appreciate the R&D work.

@Kinnara
Copy link
Owner

Kinnara commented Dec 14, 2020

Some controls such as TextContextMenu and ProgressBar are present in the ModernWpf assembly rather than the ModernWpf.Controls assembly.

Thus, I couldn't completely replace all the existing resources in the Strings.resx file and delete it.

Will moving ResourceAccessor to the ModernWpf assembly address the first point? If so, the strings for TextContextMenu (most can be found in the WinUI CommandBarFlyout) and ProgressBar can follow the WinUI way too, leaving about 6 strings in Strings.resx, which isn't too many IMO.

WinUI has a different list of supported langs and regions. This library has it own. zh-CN vs zh-Hans and zh-TW vs zh-Hant is a great example.

Good point. Basically WinUI has a superset of supported languages. I would probably start by renaming the Strings.*.resx files to use the WinUI language codes, e.g. zh-Hans to zh-CN and zh-Hant to zh-TW. Actually the existing zh-Hans and zh-Hant strings are indeed zh-CN and zh-TW, respectively, just named to match the resources in the .NET framework itself.

@ShankarBUS
Copy link
Contributor Author

ShankarBUS commented Dec 14, 2020

Will moving ResourceAccessor to the ModernWpf assembly address the first point? If so, the strings for TextContextMenu (most can be found in the WinUI CommandBarFlyout) and ProgressBar can follow the WinUI way too, leaving about 6 strings in Strings.resx, which isn't too many IMO.

That may solve it partially. But my method depends on the Assembly type in which the *.resx files are present. ModernWpf assembly doesn't have any knowledge of ModernWpf.Controls assembly.

Ref:

resourceManager = new(resourcesFilePath, typeof(ResourceAccessor).Assembly);

So, to mitigate that issue, we may need to use Assembly.GetCallingAssembly Method. But that method has some pre-requisites to be made.
I already made it prepared to be moved to the ModernWpf assembly. But I was waiting for your approval.
AFAIK, it will work perfectly. I will do some intensive testing before finalizing.

Good point. Basically WinUI has a superset of supported languages. I would probably start by renaming the Strings.*.resx files to use the WinUI language codes, e.g. zh-Hans to zh-CN and zh-Hant to zh-TW.

So, you're going to rename Strings.zh-Hans.resx to Strings.zh-CN.resx and Strings.zh-Hant.resx to Strings.zh-TW.resx?
Are you aware of the effect it may cause? It would be a breaking change for existing developers who hard coded (such as for WixToolkit. PowerToys would be affected by this) supporting assemblies (or satellite assemblies to be precise).

Actually the existing zh-Hans and zh-Hant strings are indeed zh-CN and zh-TW, respectively, just named to match the resources in the .NET framework itself.

I knew it! It should've been zh-CN and zh-TW since the beginning.
So, I was having this discussion in one of my app's repository (ModernFlyouts-Community/ModernFlyouts#250).

Some of the people were suggesting to use zh-CN and zh-TW (ModernFlyouts-Community/ModernFlyouts#250 (comment)) and some of the them were suggesting to use zh-Hans and zh-Hant instead of the others (ModernFlyouts-Community/ModernFlyouts#250 (comment)). Any opinions?

Edit:

Wait a minute! You're from China, right? Whoa! Then you must know how to solve this issue.

@Samuel12321
Copy link

Samuel12321 commented Jan 5, 2021

Any updates in this?
We have found into another bug that this may help with ModernFlyouts-Community/ModernFlyouts#333

@ShankarBUS
Copy link
Contributor Author

Well, I'm ready to finish it.

I rebased my branch, implemented all the suggestions by the owner (but not pushed yet, testing stuffs right now).

We could come to a conclusion and merge this once @Kinnara give me the green signal.

@ShankarBUS
Copy link
Contributor Author

@Kinnara,

Well, I finished my work.

  • I moved the ResourceAccessor to the ModernWpf assembly.
  • Updated resources for RatingControl.
  • Included support for pending controls (only AppBarButton and ToggleSwitch are left, TextContextMenu's Ignore menu item label too)
  • Updated all the Strings.*.resx files. (renamed files, removed un-necessary resources in them, sorted their content alphabetically)
  • In-order to prevent additional satellite assemblies I renamed some Strings.*.resx files to be in sync with the WinUI languages. Notable examples Strings.cs.resx to Strings.cs-CZ.resx and etc, Strings.fr.resx to Strings.fr-CA.resx + Strings.fr-FR.resx and Strings.es.resx to Strings.es-ES.resx + Strings.es-MX.resx and finally Strings.zh-Hans.resx to Strings.zh-CN.resx and Strings.zh-Hant.resx to Strings.zh-TW.resx.

I didn't have time to check everything. So, I'll call my friend @Samuel12321 for reviewing my work.

The PR is now completely ready to be merged. I even rebased to the latest master. Hope my PR helps 😄

@ShankarBUS
Copy link
Contributor Author

@niels9001,

If the related PR gets merged, you may need to update the satellite assemblies list in the PowerToys installer WixToolkit project.

I'm obligated to notify you this. Since PowerToys uses WixToolkit for creating an installer you have to manually specify which files go where and this change could mess with the process.

The related PR introduce a hot mess of changes.

I'm not sure if this PR would be merged but I'm just letting you know 🙂.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants