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

Ensure all translations use Keys, not the original EN text as a key #17

Closed
iJungleboy opened this issue May 18, 2021 · 37 comments
Closed

Comments

@iJungleboy
Copy link
Contributor

As of now many translation keys have a long original English text, sometimes even containing HTML.

image

This makes maintenance very hard, as the keys change when Oqtane updates the original text.

Please switch everything to use pure keys and move all the English texts into resource files as well

@hishamco
Copy link
Collaborator

As of now many translation keys have a long original English text, sometimes even containing HTML

I think there's nothing wrong with that, @sbwalker your thought?

Please switch everything to use pure keys and move all the English texts into resource files as well

Why

@iJungleboy
Copy link
Contributor Author

As of now many translation keys have a long original English text, sometimes even containing HTML

  1. Long text: as this will change, it changes the key, that's terrible for maintenance of other languages to keep up.
  2. HTML: If Oqtane changes, the HTML may easily change. Could add classes or whatever. The resources shouldn't have to be synced to ensure that.
  3. Long + HTML: this makes the key is very volatile - again a big problem for other languages

Please switch everything to use pure keys and move all the English texts into resource files as well

Basically same reasoning: keeping other language packs reliable requires stable keys. Using a real text which often will change based on the needs will keep on breaking keys.

@hishamco
Copy link
Collaborator

I totally agree, I will wait for @sbwalker comments, but while we already started an Oqtane Extractor tool this will not be a big problem.

Again I never mind

@sbwalker
Copy link
Member

I actually brought this up previously when Localization was being developed: oqtane/oqtane.framework#796 (comment) - and although the current implementation is the standard approach for localization in .NET Core I would much rather use immutable keys. The downside is that this will require a lot of changes:

  • modify all framework code to use keys instead of text
  • modify all RESX files to reference the new keys rather than the text ( all existing RESX files will be broken )
  • ship the English satellite resource DLLs with the framework (otherwise all text will be rendered as the key value rather than the proper text )

This will be a painful exercise but I agree that it is a much better approach for the future.

@iJungleboy
Copy link
Contributor Author

Based on the amount of keys etc. I would say 95% could be done in an two hours, but the difficult ones (like those containing placeholders) would probably take another 2-4 hours to complete.

I guess the steps would be

  1. Start with adding the EN resx so the infrastructure is ready for anything done from now on
  2. Move all the trivial ones ASAP so people see it's best practice
  3. Move the remaining ones when possible
  4. Notify the ones who created the initial language packs to update their pack

@hishamco
Copy link
Collaborator

@iJungleboy let us defer this to 3.0.0 if it's possible, and focus to stabalize & shipping the translations first

@iJungleboy
Copy link
Contributor Author

@hishamco I wouldn't wait - as of now fixing this will only affect very few users, since i18n is so new that all that use it ATM are still assuming things change. If we wait, it will affect / hurt many more users.

@hishamco
Copy link
Collaborator

I think it's the time to ship 2.2.0 I expect 3.0.0 will be the next major release, so no need for rush

@sbwalker
Copy link
Member

sbwalker commented Jun 2, 2021

The 2.1 release is already overdue so I would prefer to not increase its scope at this time. We should plan for this change to immutable localization keys in 2.2.

@iJungleboy
Copy link
Contributor Author

@sbwalker I understand, been there - done that. No stress - one step at a time. You are all doing a really impressive job 💙

@sbwalker
Copy link
Member

sbwalker commented Jun 14, 2021

@iJungleboy @hishamco I have someone who is going to go through all of the components this week and make the immutable key changes, ensure that all static text is localizable, fully implement SharedResources, etc... As part of this change I moved the default EN resources to the main Oqtane.Framework repo so that they will be maintained on an ongoing basis as a standard part of development. However I cannot get the framework to recognize them - it seems to ignore the fact that the "en" resources exist. Do either of you have any suggestions on how to get the framework to use the values in the "en" resource files?

@leigh-pointer
Copy link
Collaborator

@sbwalker where are you installing the EN resource? They should just be at root of the Resources folder no culture folder needed.

@sbwalker
Copy link
Member

@leigh-pointer so you are saying the Oqtane.Client subfolder under English (en) in this repo should be copied to Oqtane.Framework\Oqtane.Client\Resources?

@leigh-pointer
Copy link
Collaborator

@sbwalker yes, as the are straight .resx files the translations also would be put here and they should have a diff ext e.g Login.resx and nl Login.nl-NL.resx

@leigh-pointer
Copy link
Collaborator

@sbwalker make sure you rebuild and all the dll is copied. I have known it to skip the copying to the bin.

@sbwalker
Copy link
Member

@leigh-pointer it is still not working - I moved the files to the location you suggested and it does not build the related satellite assembly

image

@iJungleboy
Copy link
Contributor Author

Just a thought: I always found it strange to have en files - all the resources I ever worked with had en-US or similar 5 character codes.

This is extremely important, as even de-CH and de-DE have different number formats and different en cultures would have different date formats.

So I'm just speculating, but it's possible that this is the problem.

For context: in 2sxc we currently implemented a workaround that en is treated as en-US since we didn't want to risk problems switching to 2-character culture codes.

@leigh-pointer
Copy link
Collaborator

leigh-pointer commented Jun 14, 2021

@sbwalker It appears if you expect a dll for en to be built Oqtane requires en extention for the resx files Login.en.resx. Leaving the files without culture compiles the resx in to the client dll.

Question: why? the "en" needs to be there anyway, its default and also should be uptodate with every releases. building other cultures, even en-US would work and a resource dll would be built.

Suggest en culture should be included in the Oqtane client solution and language expanded from there.

image

image

image

image

image

@sbwalker
Copy link
Member

@leigh-pointer so you are saying that default resource files (ie. *.resx files without any culture identifier ) are compiled into the Oqtane.Client.dll?

@leigh-pointer
Copy link
Collaborator

@sbwalker yes and it makes sense as it is default. We dont need a lang resource dll for Oqtane, we need the resx files for the framework itself and so that we can translate them. IMOP this is better because the changes (lang) will or should be uptodate, coding and resx-ing as part of the workflow.

@leigh-pointer
Copy link
Collaborator

@sbwalker here is the decompile of Oqtane.client showing resources

image

@sbwalker
Copy link
Member

sbwalker commented Jun 14, 2021

@leigh-pointer I 100% agree - and I am fine that the default ( *.resx ) are compiled into the Oqtane.Client.dll.

My problem is that when I modify the text in the *.resx and recompile, the changes are not reflected in the UI.

ie. oqtane.framework\Oqtane.Client\Resources\Oqtane.Client\Resources\Modules\Admin\Login\Index.resx:

  <data name="Login Failed. Please Remember That Passwords Are Case Sensitive And User Accounts Require Verification When They Are Initially Created So You May Wish To Check Your Email." xml:space="preserve">
    <value>XXXXXXXXXXXXXXXXXXX. Login Failed. Please Remember That Passwords Are Case Sensitive And User Accounts Require Verification When They Are Initially Created So You May Wish To Check Your Email.</value>
  </data>

oqtane.framework\Oqtane.Client\Modules\Admin\Login\Index.razor:

AddModuleMessage(Localizer["Login Failed. Please Remember That Passwords Are Case Sensitive And User Accounts Require Verification When They Are Initially Created So You May Wish To Check Your Email."], MessageType.Error);

Result in UI:

image

( XXXXXXXXXXXXXXXXXXX not displayed )

@leigh-pointer
Copy link
Collaborator

@sbwalker I have noticed this sometimes too. I found doing a rebuild instead of a build to sort the issue out. VS seems to not include the updates to the resx on build. Try Rebuild Solution

@leigh-pointer
Copy link
Collaborator

leigh-pointer commented Jun 14, 2021

@sbwalker Changed the resx REbuilt solution

image

@leigh-pointer
Copy link
Collaborator

leigh-pointer commented Jun 14, 2021

@sbwalker I think VS is not building the resx in for speed. There might be an option somewhere to do a full build on build, not sure.

https://stackoverflow.com/questions/8715804/force-visual-studio-rebuild-on-embedded-resource-changed

@sbwalker
Copy link
Member

@leigh-pointer still not working for me after a Clean Solution and Rebuild Solution. Can you confirm the paths I shared in my post above are correct?

@leigh-pointer
Copy link
Collaborator

@sbwalker incorrect path
It should be
oqtane.framework\Oqtane.Client\Resources\Modules\Admin\Login\Index.resx:

@sbwalker
Copy link
Member

Thank you @leigh-pointer !! It works. The way this repo is currently organized is very frustrating, as it is not clear how people are expected to deploy the RESX files into the Oqtane framework :( One remaining question I have is how to deal with the Oqtane.Server resources that are in the English (en) folder in this repo?

@leigh-pointer
Copy link
Collaborator

Thank you @leigh-pointer !! It works. The way this repo is currently organized is very frustrating, as it is not clear how people are expected to deploy the RESX files into the Oqtane framework :( One remaining question I have is how to deal with the Oqtane.Server resources that are in the English (en) folder in this repo?

@sbwalker pleased its working. I strongly feel that the resx (default) should be with the main solution. Translating from there will be a lot simplier.

I will have a look at the resx for the server dll now.

@leigh-pointer
Copy link
Collaborator

@sbwalker so adding resx to the resources folder compiles the resx in to the dll

image

@leigh-pointer
Copy link
Collaborator

@sbwalker
image

@sbwalker
Copy link
Member

sbwalker commented Jun 14, 2021

@leigh-pointer I believe this is a bug. Originally @hishamco thought the page names should be localized before they are saved in the database but I explained that does not make sense and would not support expected localization behavior - the page names should be translated dynamically based on the current UI culture. So he changed the implemention - but forgot to clean up the RESX he had placed in Oqtane.Server.

@leigh-pointer
Copy link
Collaborator

@leigh-pointer I believe this is a bug. Originally @hishamco thought the page names should be localized before they are saved in the database but I explained that does not make sense and would not support expected localization behavior - the page names should be translated dynamically based on the current UI culture. So he changed the implemention - but forgot to clean up the RESX he had placed in Oqtane.Server.

Ok.

@sbwalker
Copy link
Member

@leigh-pointer Actually I am confused by the current implementation. The keys for the page names are defined in the SharedResources class and are referenced in the Modules\Admin\Dashboard\Index.razor however the RESX keys seem to be defined in Oqtane.Server\Resources\Repository\SiteRepository.resx - does this make any sense to you?

@leigh-pointer
Copy link
Collaborator

does this make any sense to you?

No, I cant see how that would work as they are in differect dlls and client cant ref server

@leigh-pointer
Copy link
Collaborator

No further action

@hishamco
Copy link
Collaborator

Sorry, I was OFF the last 3 days but seems @leigh-pointer closed this. If there's any further thing please ping me

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

No branches or pull requests

4 participants