sillsdev / l10nsharp

A .NET localization library for Windows Forms applications.
3 stars 10 forks source link

[API] #109: find specific languages when generics are specified #110

Closed papeh closed 1 year ago

papeh commented 1 year ago

If a client tries to initialize a LocalizationManager with only a language code, and we have a language available with that code plus extra information, use that language without prompting the user. Previous behavior: prompted the user with a dialog in this case. Unchanged behavior: initializing with extra information will fall back to the bare language code.

This addresses https://github.com/sillsdev/l10nsharp/issues/109 This addresses https://jira.sil.org/browse/LT-21232


This change is Reviewable

github-actions[bot] commented 1 year ago

Test Results

151 tests  +3   146 :heavy_check_mark: +3   9s :stopwatch: -2s   24 suites +1       5 :zzz: ±0      1 files   ±0       0 :x: ±0 

Results for commit 297ffc95. ± Comparison against base commit 23ff95c8.

This pull request removes 14 and adds 17 tests. Note that renamed tests count towards both. ``` GetString_OverloadThatTakesListOfLanguages_WorksWithFolders_1 GetString_OverloadThatTakesListOfLanguages_WorksWithFolders_2 GetString_OverloadThatTakesListOfLanguages_WorksWithFolders_3 GetString_OverloadThatTakesListOfLanguages_WorksWithFolders_4 GetString_OverloadThatTakesListOfLanguages_WorksWithFolders_5 GetString_OverloadThatTakesListOfLanguages_WorksWithFolders_6 GetString_OverloadThatTakesListOfLanguages_WorksWithFolders_7 GetString_OverloadThatTakesListOfLanguages_Works_1 GetString_OverloadThatTakesListOfLanguages_Works_2 GetString_OverloadThatTakesListOfLanguages_Works_3 … ``` ``` TestMappingLanguageCodesToAvailable_AmbiguousOptions_PromptsUser("zh-CN") TestMappingLanguageCodesToAvailable_AmbiguousOptions_PromptsUser("zh-TW") TestMappingLanguageCodesToAvailable_FindsSpecificGivenGeneric ar,en; finds en ar,en; finds en in folders ar,fr,en; finds fr ar,fr,en; finds fr in folders en,fr; finds en en,fr; finds en in folders en; finds en … ```

:recycle: This comment has been updated with latest results.

tombogle commented 1 year ago

src/L10NSharp/XLiffUtils/XLiffLocalizedStringCache.cs line 179 at r1 (raw file):

              // Another possibility is that the lang folder is "es-ES" but the client is requesting only "es".
              // TODO: actual implementation here:
              if (langId == "zz") return;

Although there is one exception, it seems our normal standard is to put the return on a line by itself. IMHO, that makes for more readable code. It's easy to miss the return at the end of the line when scanning through the code quickly.

tombogle commented 1 year ago

src/L10NSharp/XLiffUtils/XLiffLocalizedStringCache.cs line 178 at r1 (raw file):

                  return;
              // Another possibility is that the lang folder is "es-ES" but the client is requesting only "es".
              // TODO: actual implementation here:

Can you expand on what this TODO is about? I'm guessing the idea is that someday we might know of specific language tag for which we can do something better than just give up. Is that the idea?

tombogle commented 1 year ago

src/L10NSharpTests/LocalizationManagerTestsBase.cs line 438 at r1 (raw file):

      [TestCase(new[] { "en" }, "blahInEnglishCode", "en", TestName = "en; finds en")]
      [TestCase(new[] { "en", "fr" }, "blahInEnglishCode", "en", TestName = "en,fr; finds en")]
      [TestCase(new[] { "ar", "en" }, "blahInEnglishCode", "en", TestName = "ar,en; finds en")] // our arabic doesn't have a translation of 'blah', so fall to the code's English

Not your fault, but this should say "fall back"

tombogle commented 1 year ago

src/L10NSharpTests/LocalizationManagerTestsBase.cs line 581 at r1 (raw file):

      }

      private void AddChineseChineseTranslation(string folderPath)

Not sure I understand the purpose of having Chinese twicetwice in the method name. zh-CN is Simplified Chinese as used in China. Perhaps the name should be AddSimplifiedChineseTranslation OR AddSimplifiedChineseOfChinaTranslation OR AddChineseOfChinaTranslation OR AddZhCnChineseTranslation OR AddChineseZhCnTranslation OR AddChineseTranslation.

tombogle commented 1 year ago

src/L10NSharpTests/LocalizationManagerTestsBase.cs line 987 at r1 (raw file):


              // Check asking for a specific form of the language when we have only a different specific form.
              var str = LocalizationManager.GetString("theId", ".", "", new []{ "zh" }, out var languageIdUsed);

I know you said you intended to leaved "undefined" the behavior when "zn" is requested, but multiple zn- variants are available. Specifically in the case of "zn", I'm not sure this is a good idea. I don't know when/where we expect an application to request "zn", but I think it will be very common that if there is an available "zn-" language, there will be at least two. If it is truly impossible to decide upon a defined order of precedence, then I can't think of a benefit of implementing this behavior. If a caller can't know what to expect, why would they call the method?

tombogle commented 1 year ago

src/L10NSharpTests/XLiffLocalizedStringCacheTests.cs line 86 at r1 (raw file):

      public void TryGetDocument()
      {
          //var sut = new XLiffLocalizedStringCache();

Is this unfinished work, or something that needs to be reverted?

tombogle commented 1 year ago

src/L10NSharpTests/LocalizationManagerTestsBase.cs line 987 at r1 (raw file):

Previously, papeh wrote…
(FieldWorks ships only zh-CN, because some systems (Mono and historically Windows) do not recognize `zh` on its own.

I think that seems like a reasonable implementation (at least until we can find something more reasonable to do in some case).

tombogle commented 1 year ago

CHANGELOG.md line 23 at r6 (raw file):

- `LocalizationManager.Create` methods without `TranslationMemory kind` parameter

### Fixed

Even if no public members were changed, I wonder if we want this to be regarded as a "fix" or a "change"? Not 100% sure that these changes wouldn't be in some sense "breaking", given the new behavior. Hopefully not, but it's hard to be sure.

tombogle commented 1 year ago

L10NSharp.sln.DotSettings line 7 at r6 (raw file):

  <s:Boolean x:Key="/Default/UserDictionary/Words/=Filenames/@EntryIndexedValue">True</s:Boolean>
  <s:Boolean x:Key="/Default/UserDictionary/Words/=langs/@EntryIndexedValue">True</s:Boolean>
  <s:Boolean x:Key="/Default/UserDictionary/Words/=Liff/@EntryIndexedValue">True</s:Boolean>

I had considered doing this at one point, but since Liff isn't really a word and the miscapitalization is something that probably "should" be fixed if it weren't so entrenched, I kind of wondered if it was maybe better to just let it keep complaining.