Fix NSLocalizedString and Add Localization Tests #2824

MSFTFox · 2017-12-14T18:25:45Z

Add automated tests using NSLocalizedStringFromTableInBundle and a fabricated bundle. Also add a page in WoCCatalog for testing various system languages and NSLocalizedString from the main bundle.

Fixes #2791

This change is

DHowett-MSFT · 2017-12-14T19:17:39Z

Frameworks/CoreFoundation/PlugIn.subproj/CFBundle_Locale.c

@@ -531,6 +536,10 @@ CF_PRIVATE CFArrayRef _CFBundleCopyUserLanguages() {
    static CFArrayRef _CFBundleUserLanguages = NULL;
    static dispatch_once_t once = 0;
    dispatch_once(&once, ^{
+// WINOBJC: __CFAppleLanguages does not exist on Windows
+#if DEPLOYMENT_TARGET_WINDOWS
+        _CFBundleUserLanguages = EnumerateUserPreferredLanguages();


Per offline suggestion, I'd prefer we use CFLocaleGetPreferredUserLanguages or whatever it's called. That way we don't need to use our WinRT Enumerate directly everywhere.

minor: should we be transforming this list from - to _ on creation? it could save us the step of converting the - to a _ for every lookup.

we could do it inside Enumerate if CF prefers _ locale names

Windows locales are set up to use -. If somebody is setting up localization, they may think to use the natural windows syntax in their bundle names. I think it makes sense to support both - and _ in that case.

DHowett-MSFT · 2017-12-14T19:18:51Z

Frameworks/CoreFoundation/PlugIn.subproj/CFBundle_Locale.c

@@ -663,8 +673,66 @@ static CFStringRef _CFBundleCopyLanguageFoundInLocalizations(CFArrayRef localiza
    return NULL;
 }

+// WINOBJC: Helper functions for workaround in _CFBundleCreateMutableArrayOfFallbackLanguages
+static CFStringRef _copyStringTruncated(CFStringRef localization, CFRange cutoff) {
+    CFMutableStringRef truncatedString = CFStringCreateMutableCopy(NULL, 0, localization);


CFStringCreateWithSubstring

DHowett-MSFT · 2017-12-14T19:19:17Z

Frameworks/CoreFoundation/PlugIn.subproj/CFBundle_Locale.c

 // Given a list of localizations (e.g., provided as argument to API, or present as .lproj directories), return a mutable array of localizations in preferred order. Returns NULL if nothing is found.
 static CFMutableArrayRef _CFBundleCreateMutableArrayOfFallbackLanguages(CFArrayRef availableLocalizations, CFArrayRef preferredLocalizations) {
+// WINOBJC: The API that performs the work described below does not exist in our 3rd party libraries


nit: add note that ualoc_ is an apple-specific ICU API suite

DHowett-MSFT · 2017-12-14T19:20:11Z

Frameworks/CoreFoundation/PlugIn.subproj/CFBundle_Locale.c

+    // We know the user languages are in preferred order, add to this list in this order
+    // Prefer the full language locale, attempt to convert any hyphens to underscores as
+    // Windows language settings are retrieved with hyphens while underscores are commonly used for localization.
+    // Finally, attempt to truncate any underscores from the language to find a base localization.


nit: give examples of these. "The user's preferred language list comes in as en-US and de-DE, ..."

DHowett-MSFT · 2017-12-14T19:22:02Z

Frameworks/CoreFoundation/PlugIn.subproj/CFBundle_Locale.c

+
+    for (CFIndex i = 0, count = CFArrayGetCount(preferredLocalizations); i < count; i++) {
+        CFStringRef preferredLocalization = (CFStringRef)CFArrayGetValueAtIndex(preferredLocalizations, i);
+        for (CFIndex j = 0, count = CFArrayGetCount(availableLocalizations); j < count; j++) {


these multiple scope-nested counts scare me

DHowett-MSFT · 2017-12-14T19:29:03Z

tests/UnitTests/Foundation/WindowsOnly/BundleInternalTests.mm

+    NSString* helloWorldInGerman = NSLocalizedStringFromTableInBundle(helloWorld, nil, bundle, nil);
+    NSString* codingInChinese = NSLocalizedStringFromTableInBundle(coding, nil, bundle, nil);
+    ASSERT_OBJCEQ(@"Hallo Welt", helloWorldInGerman);
+    // Cannot use OBJCEQ as these unicode characters will cause a crash. isEqualToString is a code point literal comparison.


i'm sorry? what? a crash?

Yup, unicode characters in ASSERT_OBJCEQ was just like... ¯_(ツ)_/¯ crash.

Compile time? Runtime? Either way, please file it

Run time. Will do!

DHowett-MSFT · 2017-12-14T19:29:55Z

tests/UnitTests/Foundation/WindowsOnly/BundleInternalTests.mm

+        NSString* _subDirectory = @"en.lproj";
+        NSString* localizationResourceName = @"Localizable.strings";
+        // Create a unique test directory
+        NSString* tempDir = [[@"./tmp_TestFoundation" stringByAppendingString:[NSUUID UUID].UUIDString] stringByAppendingString:@"/"];


agh stringByAppendingPathComponent:

DHowett-MSFT · 2017-12-14T19:30:25Z

tests/UnitTests/Foundation/WindowsOnly/BundleInternalTests.mm

+        ASSERT_EQ(nil, error);
+
+        // Make a flat bundle in the playground
+        NSString* bundlePath = [tempDir stringByAppendingString:_bundleName];


DHowett-MSFT · 2017-12-14T19:30:47Z

tests/UnitTests/Foundation/WindowsOnly/BundleInternalTests.mm

+        [[NSFileManager defaultManager] createDirectoryAtPath:localizationPath
+                                  withIntermediateDirectories:false
+                                                   attributes:nil
+                                                        error:&error];


just flatten these all into one path, but set withIntermediateDirectories:true.

DHowett-MSFT · 2017-12-14T19:31:24Z

tests/UnitTests/Foundation/WindowsOnly/BundleInternalTests.mm

+
+protected:
+    virtual void SetUp() {
+        NSString* _subDirectory = @"en.lproj";


?? you're only testing looking strings up in the single lproj that exists, which usually matches the system locale.

The point was to always find a localization we have. Since we know we can use English as a fallback the test doesn't care about what locale or language settings the machine has. This way we can ensure we're testing the localization changes.

by testing the base localization, we aren't testing anything related to 1. localization or 2. localization changes though.

I mean, we're certainly testing the NSLocalizedString path. Grabbing a particular locale is not the goal here, nor is that something I think we can rely on with a unit test. The system can only have one locale setting, so it doesn't seem to matter to me which locale that is. Since English is a known fallback is that not acceptable?

DHowett-MSFT

seems fine now!

DHowett-MSFT · 2017-12-15T19:56:52Z

Frameworks/CoreFoundation/PlugIn.subproj/CFBundle_Locale.c

-    CFStringDelete(truncatedString, CFRangeMake(cutoff.location, CFStringGetLength(truncatedString) - cutoff.location));
-    return truncatedString;
+    return CFStringCreateWithSubstring(NULL, localization, CFRangeMake(0, cutoff.location));
+    //CFMutableStringRef truncatedString = CFStringCreateMutableCopy(NULL, 0, localization);


Oh woops. I wanted to keep that around in case I goofed anything in the substring call.

DHowett-MSFT · 2017-12-15T19:57:34Z

samples/WOCCatalog/WOCCatalog/LocalizationViewController.m

@@ -28,10 +28,10 @@ @implementation LocalizationViewController {
    UILabel* _pasteTextLabel;
 }
 - (void)viewDidLoad {


nit: you may need [super viewDidLoad] here for completeness' sake

DHowett-MSFT · 2017-12-15T19:58:03Z

tests/UnitTests/Foundation/WindowsOnly/BundleInternalTests.mm

@@ -30,20 +30,13 @@ virtual void SetUp() {
        NSString* _subDirectory = @"en.lproj";
        NSString* localizationResourceName = @"Localizable.strings";
        // Create a unique test directory
-        NSString* tempDir = [[@"./tmp_TestFoundation" stringByAppendingString:[NSUUID UUID].UUIDString] stringByAppendingString:@"/"];
-
+        NSString* tempDir = [[@"./tmp_TestFoundation" stringByAppendingPathComponent:[NSUUID UUID].UUIDString] stringByAppendingPathComponent:@"/"];


/ is not a path component, so you can leave this call out.

DHowett-MSFT · 2017-12-15T19:59:31Z

tests/UnitTests/Foundation/WindowsOnly/BundleInternalTests.mm

-        NSString* tempDir = [[@"./tmp_TestFoundation" stringByAppendingString:[NSUUID UUID].UUIDString] stringByAppendingString:@"/"];
-
+        NSString* tempDir = [[@"./tmp_TestFoundation" stringByAppendingPathComponent:[NSUUID UUID].UUIDString] stringByAppendingPathComponent:@"/"];
+        NSString* bundlePath = [tempDir stringByAppendingPathComponent:_bundleName];


funny question: why not just expose bundlePath in the fixture and use that instead of re-combining _playground and _bundleName later?

Not funny at all. That seems like a good idea to avoid extra string work. 👍

DHowett-MSFT · 2017-12-15T21:46:51Z

tests/UnitTests/Foundation/WindowsOnly/BundleInternalTests.mm

@@ -61,14 +57,15 @@ virtual void TearDown() {
    }

    StrongId<NSString> _playground;
+    StrongId<NSString> bundlePath;


DHowett-MSFT · 2017-12-15T21:47:14Z

Frameworks/CoreFoundation/PlugIn.subproj/CFBundle_Locale.c

@@ -25,6 +25,11 @@
 #endif
 #include <ctype.h>

+// WINOBJC: Helper for retrieving list of preferred languages
+#if DEPLOYMENT_TARGET_WINDOWS
+#include "_CFLocaleInternal.h"


is this still necessary?

DHowett-MSFT · 2017-12-16T01:52:23Z

you don't need to take all of my feedback; you are free to reject some.

triplef · 2017-12-16T06:50:01Z

Thank you for this fix!

Just to clarify, does this only support getting localized strings or does it enable getting any kind of localized bundle resource?

MSFTFox · 2017-12-18T04:04:33Z

It should have fixed a fundamental problem in lproj lookup. The problem was that an API for looking up localized resources did not have support for retrieving user preferred languages. This has been solved.

If you have some other particular APIs you would like us to take a look at, please let us know. This fix was really focused on ensuring localized strings were functional.

triplef · 2017-12-18T09:27:07Z

Sounds great, thanks!

* Fix NSLocalizedString and add new tests * Address CR Feedback. * Forgot to change to StringByAppendingPath * Nevermind. ASSERT_OBJCEQ does indeed work. * Address feedback. * Address feedback.

Fix NSLocalizedString and add new tests

efe8879

MSFTFox requested review from DHowett-MSFT and rajsesh December 14, 2017 18:26

DHowett-MSFT suggested changes Dec 14, 2017

View reviewed changes

MSFTFox added 3 commits December 14, 2017 16:15

Address CR Feedback.

36021c6

Forgot to change to StringByAppendingPath

645f392

Nevermind. ASSERT_OBJCEQ does indeed work.

ac31e7d

DHowett-MSFT approved these changes Dec 15, 2017

View reviewed changes

Address feedback.

e1bad38

DHowett-MSFT approved these changes Dec 15, 2017

View reviewed changes

DHowett-MSFT reviewed Dec 15, 2017

View reviewed changes

Address feedback.

86f1a13

MSFTFox merged commit 22fb297 into microsoft:develop Dec 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix NSLocalizedString and Add Localization Tests #2824

Fix NSLocalizedString and Add Localization Tests #2824

MSFTFox commented Dec 14, 2017 •

edited by rajsesh

Loading

DHowett-MSFT Dec 14, 2017

DHowett-MSFT Dec 14, 2017

DHowett-MSFT Dec 14, 2017

MSFTFox Dec 14, 2017

DHowett-MSFT Dec 14, 2017

DHowett-MSFT Dec 14, 2017

DHowett-MSFT Dec 14, 2017

DHowett-MSFT Dec 14, 2017

DHowett-MSFT Dec 14, 2017

MSFTFox Dec 14, 2017

DHowett-MSFT Dec 14, 2017

MSFTFox Dec 14, 2017

DHowett-MSFT Dec 14, 2017

DHowett-MSFT Dec 14, 2017

DHowett-MSFT Dec 14, 2017

DHowett-MSFT Dec 14, 2017

MSFTFox Dec 14, 2017

DHowett-MSFT Dec 14, 2017

MSFTFox Dec 15, 2017

DHowett-MSFT left a comment

DHowett-MSFT Dec 15, 2017

MSFTFox Dec 15, 2017

DHowett-MSFT Dec 15, 2017

DHowett-MSFT Dec 15, 2017

DHowett-MSFT Dec 15, 2017

MSFTFox Dec 15, 2017

DHowett-MSFT Dec 15, 2017

DHowett-MSFT Dec 15, 2017

DHowett-MSFT commented Dec 16, 2017

triplef commented Dec 16, 2017

MSFTFox commented Dec 18, 2017

triplef commented Dec 18, 2017

Fix NSLocalizedString and Add Localization Tests #2824

Fix NSLocalizedString and Add Localization Tests #2824

Conversation

MSFTFox commented Dec 14, 2017 • edited by rajsesh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT commented Dec 16, 2017

triplef commented Dec 16, 2017

MSFTFox commented Dec 18, 2017

triplef commented Dec 18, 2017

MSFTFox commented Dec 14, 2017 •

edited by rajsesh

Loading