search

sillsdev / l10nsharp

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

Fixed bug: LoadedManagers has wrong AppId because it was passed in with extension during Create #98

Closed tombogle closed 2 years ago

tombogle commented 2 years ago

.

This change is Reviewable

tombogle commented 2 years ago

src/L10NSharp/LocalizationManagerInternal.cs line 62 at r1 (raw file):

Previously, andrew-polk wrote…
I don't really understand what is going on here. Maybe at this point, we should be using a variable named something other than appId? Or maybe just a comment is warranted? But you say this is a blocker, so I'm ok merging as is if you need to.

Actually, the code IS really confusing... and wrong. I think it would almost always end up working, but just because we use it in a way that doesn't fall into the hole.

tombogle commented 2 years ago

src/L10NSharp/LocalizationManagerInternal.cs line 206 at r2 (raw file):

Previously, andrew-polk wrote…
I may just be missing something, but Bloom's usage here is to pass "Bloom" for both appId and name. But it is Bloom.exe, not Bloom.dll. So it feels like something is wrong with defaulting to .dll. Though I do see that is what the XLiffLocalizationManager constructor was defaulting to... so I guess I am missing something.

The default is probably not the most sensible. Most apps will want to pass .exe. I suspect the original default originated with FW and the Palaso DLLs. Turns out that DLL is also the correct default for Transcelerator. So I figured it was safer to leave the existing default even if it is wrong 60-75% of the time.