Added UiLanguageChanged event to ILocalizationManager and cleaned up code by tombogle · Pull Request #129 · sillsdev/l10nsharp

tombogle · 2026-02-04T20:57:37Z

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

…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

github-actions · 2026-02-04T21:00:49Z

Test Results

    6 files ±0   84 suites ±0 24s ⏱️ -13s
166 tests +1 161 ✔️ +1   5 💤 ±0 0 ❌ ±0
498 runs +3 483 ✔️ +3 15 💤 ±0 0 ❌ ±0

Results for commit 6a02ff7. ± Comparison against base commit a404a2e.

♻️ This comment has been updated with latest results.

andrew-polk

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

@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";

___ *[`src/L10NSharp/LocalizationManager.cs` line 11 at r1](https://reviewable.io/reviews/sillsdev/l10nsharp/129#-Oko5v2a-p6S-9hKU0Ud:-Oko5v2a-p6S-9hKU0Ue:b334109) ([raw file](https://github.com/sillsdev/l10nsharp/blob/02959a322482fd82cdaae3405d7997fb4c51ea8a/src/L10NSharp/LocalizationManager.cs#L11)):* > ```Smalltalk > using System.Reflection; > using System.Threading; > using JetBrains.Annotations; > ```

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);

___ *[`src/SampleApp/Form1.cs` line 28 at r1](https://reviewable.io/reviews/sillsdev/l10nsharp/129#-Oko6kW83CKbXDVzJBy6:-Oko6kW83CKbXDVzJBy7:bo6dqhx) ([raw file](https://github.com/sillsdev/l10nsharp/blob/02959a322482fd82cdaae3405d7997fb4c51ea8a/src/SampleApp/Form1.cs#L28)):* > ```Smalltalk > $"The {nameof(sender)} should have been the primary localization manager on which we subscribed to handle the {nameof(ILocalizationManager.UiLanguageChanged)} event."); > > label2.Text = string.Format(label2.Text, DateTime.Now.ToShortTimeString(), DateTime.Now.ToShortDateString()); > ```

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 UiLanguageChanged event fires
HandleFormLocalized is 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());
}

… but at least one real (minor) bug fix in `GetDynamicString`. Plus a couple potentially useful improvements to deal with unusual edge cases.

tombogle

@tombogle made 6 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @andrew-polk and @aror92).

src/L10NSharp/ILocalizationManager.cs line 13 at r1 (raw file):