-
-
Notifications
You must be signed in to change notification settings - Fork 10
Added UiLanguageChanged event to ILocalizationManager and cleaned up code #129
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
base: master
Are you sure you want to change the base?
Conversation
…which is invoked when UI language changes Removed a lot of cruft (mostly left over from decision to jettison the localization dialog Removed the version of LocalizationManagerWinforms.Create that did not take an icon. Switched to C#8 Replaced deprecated code in GoogleTranslator with a supported approach
andrew-polk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't made it very far in this review yet. Posting what I've done.
@andrew-polk reviewed 17 files and all commit messages, and made 2 comments.
Reviewable status: 17 of 79 files reviewed, 1 unresolved discussion (waiting on @aror92 and @tombogle).
src/L10NSharp/ILocalizationManager.cs line 13 at r1 (raw file):
public interface ILocalizationManager: IDisposable { event EventHandler UiLanguageChanged;
Probably deserves a comment.
andrew-polk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrew-polk reviewed 62 files and made 5 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @aror92 and @tombogle).
src/L10NSharp/L10NCultureInfo.cs line 49 at r1 (raw file):
// 2. Check if the three-letter language name is set to default // Source: https://stackoverflow.com/a/71388328/1964319 var isFullyUnknown = RawCultureInfo.CultureTypes.HasFlag(CultureTypes.UserCustomCulture) && RawCultureInfo.ThreeLetterWindowsLanguageName == "ZZZ";
devinreview found this existing issue. I'll let you decide if you want to do anything with it
src/L10NSharp/L10NCultureInfo.cs:R49
NullReferenceException when checking if culture is fully unknown
In L10NCultureInfo.cs:49, the code accesses RawCultureInfo.CultureTypes and RawCultureInfo.ThreeLetterWindowsLanguageName without checking if RawCultureInfo is null. However, RawCultureInfo can be set to null on line 41 in the catch block when CultureNotFoundException is thrown.
Impact and fix
When a culture is not found, line 41 sets RawCultureInfo = null, then line 49 tries to access properties on it:
var isFullyUnknown = RawCultureInfo.CultureTypes.HasFlag(CultureTypes.UserCustomCulture) && RawCultureInfo.ThreeLetterWindowsLanguageName == "ZZZ";This will throw a NullReferenceException. The fix should check for null first:
var isFullyUnknown = RawCultureInfo != null && RawCultureInfo.CultureTypes.HasFlag(CultureTypes.UserCustomCulture) && RawCultureInfo.ThreeLetterWindowsLanguageName == "ZZZ";I'm slightly hesitant about adding the PublicApi annotations.
What benefit do they really provide? Does this mean that we need to keep them up to date? Are devs now going to assume that everything which isn't annotated as such is not public API? Should we add something to the readme?
src/L10NSharp/LocalizationManager.cs line 142 at r1 (raw file):
} [CanBeNull]
devinreview says
src/L10NSharp/LocalizationManager.cs:R142-144
Incorrect [CanBeNull] attribute on bool return type
At LocalizationManager.cs:142, the [CanBeNull] attribute is applied to TrySetUILanguage which returns bool. Value types like bool cannot be null in C#, so this attribute is meaningless and incorrect. This appears to be a copy-paste error or misunderstanding of the attribute's purpose. While this won't cause runtime issues, it represents incorrect metadata that could confuse developers or static analysis tools.
src/L10NSharp/LocalizationManager.cs line 506 at r1 (raw file):
englishText); } }
Not a new issue, but devinreview points out the below. I'll leave it to you if you want to fix it here or separately or create an issue.
src/L10NSharp/LocalizationManager.cs:R496-505
Comment parameter accepted but never used in GetDynamicString
In LocalizationManager.cs:496-505, the GetDynamicString method accepts a comment parameter but never passes it to the underlying implementation. The method signature includes string comment (line 497), but when calling LocalizationManagerInternal<XLiffDocument>.GetDynamicString (line 503), only appId, id, englishText are passed - the comment is dropped.
Impact
When callers provide a comment to help translators understand context, that comment is silently ignored and never stored in the translation files. This means translators won't get the intended guidance.
Compare with LocalizationManagerInternal.cs:465 which correctly calls GetDynamicStringOrEnglish(appId, id, englishText, comment, LocalizationManager.UILanguageId) passing all parameters including comment.
The fix should pass the comment parameter:
return LocalizationManagerInternal<XLiffDocument>.GetDynamicString(appId, id, englishText, comment);devinreview says
src/SampleApp/Form1.cs:R28
Format string reuse bug causes incorrect display on subsequent UI language changes
In SampleApp/Form1.cs:28, the code calls string.Format(label2.Text, ...) where label2.Text is expected to be a format string. However, HandleFormLocalized is called every time the UI language changes via the UiLanguageChanged event (line 18). On the first call, label2.Text contains "The UI language was last changed at {0} on {1}." and formatting works correctly. On subsequent calls, label2.Text already contains the formatted result (e.g., "The UI language was last changed at 5:30 PM on 2/4/2026."), which no longer has the placeholders {0} and {1}.
Impact and expected behavior
When the user changes the UI language a second time:
- The
UiLanguageChangedevent fires HandleFormLocalizedis called again- Line 28 tries to format the already-formatted string
- This will either throw a
FormatException(if the formatted string doesn't happen to have valid format placeholders) or produce incorrect output
The fix should retrieve the localized format string each time:
private void HandleFormLocalized(object sender, EventArgs e)
{
if (sender != Program.PrimaryLocalizationManager)
throw new InvalidOperationException(...);
var formatString = LocalizationManager.GetString("TheSampleForm.Form1.label2",
"The UI language was last changed at {0} on {1}.");
label2.Text = string.Format(formatString, DateTime.Now.ToShortTimeString(), DateTime.Now.ToShortDateString());
}
The UiLanguageChanged event is invoked when UI language changes. This provides a way for clients to deal with changes, thus restoring functionality that was lost when we got rid of LocalizeItemDlg.StringsLocalized.
Removed a lot of cruft (mostly left over from the decision to jettison the localization dialog
Removed the version of LocalizationManagerWinforms.Create that did not take an icon.
Switched to C version 8.0
Replaced deprecated code in GoogleTranslator with a supported approach.
This change is