diff --git a/src/L10NSharp.Tests/XLiffLocalizationManagerTests.cs b/src/L10NSharp.Tests/XLiffLocalizationManagerTests.cs index f439045..038f275 100644 --- a/src/L10NSharp.Tests/XLiffLocalizationManagerTests.cs +++ b/src/L10NSharp.Tests/XLiffLocalizationManagerTests.cs @@ -280,6 +280,21 @@ public void AddTransUnit_NullSourceNullTarget_DoesNotThrow() Assert.DoesNotThrow(() => body.AddTransUnit(tu)); } + [Test] + public void AddTransUnit_NoId_GeneratesIdAndIndexesTranslation() + { + var body = new XLiffBody(); + var tu = new XLiffTransUnit { + Source = new XLiffTransUnitVariant { Lang = "en", Value = "Text." } + }; + + Assert.That(body.AddTransUnit(tu), Is.True); + + // AddTransUnit relies on the Id generated for an Id-less unit. + Assert.That(tu.Id, Is.Not.Null.And.Not.Empty); + Assert.That(body.TranslationsById[tu.Id], Is.EqualTo("Text.")); + } + [Test] public void MergeXliffDocuments_BaselineUnitHasNullSource_DoesNotThrow() { diff --git a/src/L10NSharp/LocalizationManagerInternal.cs b/src/L10NSharp/LocalizationManagerInternal.cs index 0f7a721..e53c5bc 100644 --- a/src/L10NSharp/LocalizationManagerInternal.cs +++ b/src/L10NSharp/LocalizationManagerInternal.cs @@ -566,8 +566,8 @@ public static string GetDynamicStringOrEnglish(string appId, string id, string? [return: NotNullIfNotNull("langId")] internal static string? MapToExistingLanguageIfPossible(string? langId) { - if (langId is null || string.IsNullOrEmpty(langId)) - return null; + if (langId is null || langId.Length == 0) + return langId; // It's a concurrent dictionary, so we can (for performance) try this without a lock. if (MapToExistingLanguage.TryGetValue(langId, out var realId)) return realId; diff --git a/src/L10NSharp/XLiffUtils/XLiffBody.cs b/src/L10NSharp/XLiffUtils/XLiffBody.cs index caf7483..99e031b 100644 --- a/src/L10NSharp/XLiffUtils/XLiffBody.cs +++ b/src/L10NSharp/XLiffUtils/XLiffBody.cs @@ -93,8 +93,9 @@ public ListWrapper TransUnitsForXml /// ------------------------------------------------------------------------------------ internal XLiffTransUnit? GetTransUnitForId(string? id) { - if (id == null) - return null; + if (id == null) + return null; + _transUnitDict.TryGetValue(id, out XLiffTransUnit? result); return result; } @@ -124,15 +125,16 @@ public ListWrapper TransUnitsForXml /// Adds the specified translation unit. /// /// The translation unit. - /// true if the translation unit was successfully added. Otherwise, false. + /// The id under which the translation unit was added, or null if it was not + /// added. /// ------------------------------------------------------------------------------------ - internal bool AddTransUnitRaw(XLiffTransUnit? tu) + internal string? AddTransUnitRaw(XLiffTransUnit? tu) { if (tu == null || tu.IsEmpty) - return false; + return null; - bool lockTaken = false; string key; + bool lockTaken = false; try { _transUnitIdLock.Enter(ref lockTaken); @@ -141,18 +143,18 @@ internal bool AddTransUnitRaw(XLiffTransUnit? tu) // it into the dictionary. This assumes nothing else modifies IDs once they // are in this system: once our locked code has given the TU an ID, any other // thread will see that it is non-empty. - key = tu.Id; - if (string.IsNullOrEmpty(key)) + key = tu.Id ?? ""; + if (key.Length == 0) { - tu.Id = (++_transUnitId).ToString(); - key = tu.Id; + key = (++_transUnitId).ToString(); + tu.Id = key; } // If a translation unit with the specified id already exists, then quit here. // This check and the dictionary write must both happen inside the lock to avoid // a TOCTOU race where two threads with the same ID both pass the check. if (GetTransUnitForId(key) != null) - return false; + return null; _transUnitDict[key] = tu; } finally @@ -160,19 +162,21 @@ internal bool AddTransUnitRaw(XLiffTransUnit? tu) if (lockTaken) _transUnitIdLock.Exit(false); } - return true; + return key; } public bool AddTransUnit(XLiffTransUnit tu) { - if (!AddTransUnitRaw(tu)) + // Use inserted key (not tu.Id), so TranslationsById and _transUnitDict can't drift. + var id = AddTransUnitRaw(tu); + if (id == null) return false; // If the target exists, store its value in the dictionary lookup. Otherwise, store // the source value there. if (tu.Target?.Value != null) - TranslationsById[tu.Id] = tu.Target.Value; + TranslationsById[id] = tu.Target.Value; else if (tu.Source?.Value != null) - TranslationsById[tu.Id] = tu.Source.Value; + TranslationsById[id] = tu.Source.Value; return true; } @@ -187,7 +191,7 @@ internal void AddTransUnitOrVariantFromExisting(XLiffTransUnit tu, string langId { var variantToAdd = tu.GetVariantForLang(langId); - if (variantToAdd == null || AddTransUnit(tu)) + if (variantToAdd == null || AddTransUnit(tu) || tu.Id == null) return; var existingTu = GetTransUnitForId(tu.Id); diff --git a/src/L10NSharp/XLiffUtils/XLiffTransUnit.cs b/src/L10NSharp/XLiffUtils/XLiffTransUnit.cs index 9d58a02..57b8087 100644 --- a/src/L10NSharp/XLiffUtils/XLiffTransUnit.cs +++ b/src/L10NSharp/XLiffUtils/XLiffTransUnit.cs @@ -1,5 +1,4 @@ using System.Collections.Generic; -using System.Diagnostics.CodeAnalysis; using System.Xml.Serialization; using static System.String; @@ -114,7 +113,6 @@ public List Notes /// /// ------------------------------------------------------------------------------------ [XmlIgnore] - [MemberNotNullWhen(false, nameof(Id), nameof(Source), nameof(Target))] public bool IsEmpty => IsNullOrEmpty(Id) && Notes.Count == 0 && Source == null && Target == null; @@ -206,7 +204,7 @@ public void RemoveVariant(string? langId) /// ------------------------------------------------------------------------------------ public override string ToString() { - return IsEmpty ? "Empty" : Id; + return IsNullOrEmpty(Id) ? "Empty" : Id!; } #endregion diff --git a/src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs b/src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs index 81f068b..fa0ebf2 100644 --- a/src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs +++ b/src/L10NSharp/XLiffUtils/XliffLocalizationManager.cs @@ -22,6 +22,7 @@ internal class XliffLocalizationManager : ILocalizationManagerInternal? _stringCache; public Dictionary ComponentCache { get; } = new Dictionary(); @@ -104,7 +105,7 @@ internal XliffLocalizationManager(string appId, string origExtension, string app if (string.IsNullOrEmpty(_customXliffFileFolder)) _customXliffFileFolder = null; - StringCache = new XliffLocalizedStringCache(this); + _stringCache = new XliffLocalizedStringCache(this); } /// @@ -181,8 +182,8 @@ private void CreateOrUpdateDefaultXliffFileIfNecessary( Console.WriteLine("WARNING - L10NSharp Update deleted corrupted {0}", DefaultStringFilePath); } if (verAttribute != null && - Version.TryParse(verAttribute.Value, out var existingVer) && - existingVer >= _appVersion) + Version.TryParse(verAttribute.Value, out var existingVer) && + existingVer >= _appVersion) { return; } @@ -204,10 +205,10 @@ private void CreateOrUpdateDefaultXliffFileIfNecessary( else { stringCache.UpdateLocalizedInfo(new LocalizingInfo("_dummyEntryToGetValidFile") - { - LangId = "en", - Text = "No strings were collected. This entry prevents an invalid, zero-length file. Delete this file to try regenerating it." - } + { + LangId = "en", + Text = "No strings were collected. This entry prevents an invalid, zero-length file. Delete this file to try regenerating it." + } ); } } @@ -298,16 +299,21 @@ public void Dispose() /// /// Full file name and path to the default string file (i.e. English strings). /// + /// Empty for instances created by the minimal constructor. /// ------------------------------------------------------------------------------------ - internal string DefaultStringFilePath { get; } + internal string DefaultStringFilePath { get; } = string.Empty; internal string DefaultInstalledStringFilePath => - Path.Combine(_installedXliffFileFolder, + Path.Combine( + _installedXliffFileFolder ?? throw new InvalidOperationException( + $"{nameof(DefaultInstalledStringFilePath)} is not available on a localization manager created without a folder of installed XLIFF files."), LocalizationManager.GetTranslationFileNameForLanguage(Id, LocalizationManager.kDefaultLang)); /// ------------------------------------------------------------------------------------ - public ILocalizedStringCache StringCache { get; } = null!; + public ILocalizedStringCache StringCache => + _stringCache ?? throw new InvalidOperationException( + "There is no string cache on a localization manager created by the minimal constructor."); /// ------------------------------------------------------------------------------------ @@ -600,15 +606,20 @@ internal static XLiffDocument MergeXliffDocuments(XLiffDocument xliffNew, XLiffD foreach (var tu in xliffNew.File.Body.TransUnitsUnordered) { xliffOutput.File.Body.AddTransUnit(tu); + var id = tu.Id; + // AddTransUnit assigns an Id unless the unit is entirely empty. + if (id == null) + continue; + if (tu.Dynamic) ++newDynamicCount; if (xliffOld != null) { - var tuOld = xliffOld.File.Body.GetTransUnitForId(tu.Id!); + var tuOld = xliffOld.File.Body.GetTransUnitForId(id); if (tuOld == null) { ++newStringCount; - newStringIds.Add(tu.Id!); + newStringIds.Add(id); } else { @@ -637,14 +648,14 @@ internal static XLiffDocument MergeXliffDocuments(XLiffDocument xliffNew, XLiffD if (tu.Source?.Value != tuOld.Source?.Value) { ++changedStringCount; - changedStringIds.Add(tu.Id!); + changedStringIds.Add(id); if (!string.IsNullOrWhiteSpace(tuOld.Source?.Value)) tu.AddNote("en", $"OLD TEXT (before {xliffNew.File.ProductVersion}): {tuOld.Source!.Value}"); } if (tuOld.Dynamic && !tu.Dynamic) { ++wrongDynamicFlagCount; - wrongDynamicStringIds.Add(tu.Id!); + wrongDynamicStringIds.Add(id); tu.AddNote("en", $"Not dynamic: found in static scan of compiled code (version {xliffNew.File.ProductVersion})"); } } @@ -657,14 +668,19 @@ internal static XLiffDocument MergeXliffDocuments(XLiffDocument xliffNew, XLiffD { foreach (var tu in xliffOld.File.Body.TransUnitsUnordered) { - var tuNew = xliffNew.File.Body.GetTransUnitForId(tu.Id!); + var tuNew = xliffNew.File.Body.GetTransUnitForId(tu.Id); if (tuNew == null) { xliffOutput.File.Body.AddTransUnit(tu); + var id = tu.Id; + // AddTransUnit assigns an Id unless the unit is entirely empty. + if (id == null) + continue; + if (tu.Dynamic) { ++missingDynamicStringCount; - missingDynamicStringIds.Add(tu.Id!); + missingDynamicStringIds.Add(id); if (newDynamicCount > 0) // note only if attempt made to collect dynamic strings { tu.Notes.RemoveAll(n => n.Text != null && n.Text.StartsWith("Not found")); @@ -674,7 +690,7 @@ internal static XLiffDocument MergeXliffDocuments(XLiffDocument xliffNew, XLiffD else { ++missingStringCount; - missingStringIds.Add(tu.Id!); + missingStringIds.Add(id); tu.Notes.RemoveAll(n => n.Text != null && n.Text.StartsWith("Not found")); tu.AddNote("en", $"Not found in static scan of compiled code (version {xliffNew.File.ProductVersion})"); }