Closed tombogle closed 5 years ago
I further enhanced the fix. I could wrap some of that logic in a testable method. Obviously, we wouldn't want the tests to actually call Bing, which is where the real issues could lie. But the tests could at least try to anticipate some of the garbage that Bing could possibly throw back at us. So that might be worthwhile.
src/L10NSharp/UI/LanguageChoosingDialog.cs, line 31 at r1 (raw file):
Based on the old and following code, it appears that we expect _originalMessageTemplate to contain something like {0} ({1}), where (in English) {0} is the English name of the language and {1} is the native name, like German (Deutsch). We want to give Bing German ({1}) and get back Deutsch ({1}) and THEN change {1} to "German". I would expect this to fail because the string.Format() will choke on a string containing {1} when only given a single argument. You'd need to pass "{1}" as a second argument (or I think you could pass "{0}" as the second argument, then Bing will get German ({0}), hopefully return Deutsch ({0}, and you are all ready to insert the English). Even if I'm wrong and string.Format gives back German ({1}) and Bing translates this to Deutsch ({1}), I think this needs more explanation. The trailing comment on the line above, particularly, was just confusing... I think it would be good to document what it's acceptable for _originalMessageTemplate to contain, what we want to pass to Bing for each such case, what we hope to get back, and then be clearer about happy and unhappy paths for what we do next and how we tell the difference.
I did consider doing the replacement as part of the initial call to Format. (I've done that before in other apps, but it kind of feels confusing either way.) I'm doing some refactoring to bring this code under unit tests. I'll think it through and try to make it intelligible.
src/L10NSharp/UI/LanguageChoosingDialog.cs, line 31 at r1 (raw file):
I did consider doing the replacement as part of the initial call to Format. (I've done that before in other apps, but it kind of feels confusing either way.) I'm doing some refactoring to bring this code under unit tests. I'll think it through and try to make it intelligible.
And, yes, you were right.
src/L10NSharp/UI/LanguageChoosingDialog.cs, line 31 at r1 (raw file):
Catch (Exception) is dangerous!
Maybe so. That is existing code (in a new file). It's probably overkill, but since the only purpose of this code is to attempt to replace the English version of the message with a language-specific one, any unhandled error is only going to make things worse. Since it's calling out to a third-party web-based delegate to get the translation it might be difficult to anticipate everything that could possibly go wrong. I had actually considered whether it might be better to display the translated message in addition to the original English one, just in case we get something back that is garbage (and also because if someone needs help, it could be hard for a support person to make sense of the problem. Searching for the translated string in our code and TMX files won't result in a hit.) In the end, though, I figured that could be left for a rainier day.
src/L10NSharp/UI/LanguageChoosingDialogViewModel.cs, line 18 at r3 (raw file):
_messageLabelFormat = messageLabelFormat; AcceptButtonText = _acceptButtonText = acceptButtonText; WindowTitle = _windowTitle = windowTitle;
What's the purpose of having an auto property WindowTitle
(and AcceptButtonText
) and a separate field _windowTitle
? Wouldn't the auto property be sufficient?
Fix for https://github.com/sillsdev/l10nsharp/issues/66
This change is