WildAid / o-fish-ios

iOS app for the Officer's Fishery Information Sharing Hub (O-FISH). The mobile app allows fisheries officers to document and share critical information gathered during a routine vessel inspection.
Apache License 2.0
33 stars 15 forks source link

Localization Management system for iOS #339

Closed Sheeri closed 3 years ago

Sheeri commented 3 years ago

We need a localization management system for iOS.

Requirements:

Nice to have:

bladebunny commented 3 years ago

I can help with this but would suggest splitting into two (or more tasks).
1: add localization support within app (does not require a library - IMHO) 2: Audit app / and automate detection of non-loc'd strings -- not sure if there is a lib or app for this but it could be a separate task from 1 above

Sheeri commented 3 years ago

@bladebunny Great suggestion! The app has already had localization support added; the file locations can be seen in LOCALIZATION.md in the top-level directory of the codebase (e.g. where README.md and CONTRIBUTING.md are)

To be clear, the requirements are:

And the nice to have's:

bladebunny commented 3 years ago

I'll take a look. At my last job we had a Python script that checked on build but I didn't write it or recall the details. FYI - there's also a target run option that displays unlocalized strings in the app in UPPERCASE. Not as elegant as the script but for a quick visual inspection.

What about my suggestion about namespacing the strings themselves? Take a look at my PR for #347 for an example.

Sample screenshot attached: Simulator Screen Shot - iPhone 11 - 2020-10-10 at 14 45 50

Sheeri commented 3 years ago

Hi @bladebunny - is the build flag you're talking about similar to https://medium.com/@pinmadhon/finding-non-nslocalized-strings-in-xcode-8-in-swift-3-or-objc-589ee279a166 ?

Right now we're not using NSLocalizedString, we're just using regular text. e.g., these are the translations for the string "Board Vessel": Localization/uk.lproj/Localizable.strings:"Board Vessel" = "Прибути на судно"; Localization/es-419.lproj/Localizable.strings:"Board Vessel" = "Embarcación a bordo"; Localization/fr.lproj/Localizable.strings:"Board Vessel" = "Inspecter Vaisseau";

And there are 2 files in the code where "Board Vessel" appears, neither one of which uses "NSLocalizedString":

o-fish-ios/Views/Components/BoardVesselButton.swift: CallToActionButton(title: "Board Vessel", action: boardVesselButtonClicked) o-fish-ios/Views/Components/PatrolBoatView/BoardVesselButtonView.swift: Text("Board Vessel")

Now, it may be that we SHOULD be using NSLocalizedString for our translations, but that's something for @am-MongoDB to determine. Can we assume that anything in quotes currently is a string?

bladebunny commented 3 years ago

Yes. You can either set the option on the build target using the switch or you can add a flag - as the article discusses. As I understand it, Text() now includes the ability to detect whether the string is localized and if so, use the localization. But I feel that is not a good general practice for a localized app because it's hard to know without auditing your strings files whether you've provided a loc for every such string. I prefer not having the string key be in English (which means I add an English strings file too).

"Board Vessel" = "Inspecter Vaisseau";

Because the localized key above looks like a normal string.

Text("Board Vessel")

or

var someString = "Board Vessel"

It's not obvious that that string is a loc key. Instead, one approach I've used, which I admit has a bit more upfront effort, is to use semantic keys that are unmistakably keys. I also extend NSLocalizedString (see my PR) to also set the key as the comment (just one param - the key) so that if the string is not localized, the app will show the key - which will not be mistaken for a string.

Something like the following in the loc file:

"boarding.labels.boardvessel" = "Board Vessel"; "boarding.labels.name.format" = "Captain %@";

Text("boarding.labels.boardvessel") or Text(NSLocalizedString("boarding.labels.boardvessel"))

Those will make it clear when the key isn't localized. I then take it a step further by wrapping those into a struct - so that I am never using raw strings in my view - unless debug but even there I don't like them. e.g.

struct AppStrings {
    struct Boarding {
         static let boardVesselLabel = NSLocalizedString("boarding.labels.boardvessel")
    }
}

...then at the point of use....

Text(AppStrings.Boarding.boardVesselLabel)

...nice and clean. No raw strings. And if you forget a label, the control will show: boarding.labels.boardvessel in the UI - which sticks out like a sore thumb. It seems like a bit more hassle but it greatly reduces loc issues.

Just my two cents...

bladebunny commented 3 years ago

BTW - i have been playing with a Python script to audit the files. Right now, my script can: 1) Detect the developer language for the base language 2) Extra all the string keys 2) Find unmatched strings in the loc files (ie cases where a translation file is either missing or has an extra string) from the base language 3) Scan swift files for raw strings 4) TBD - match raw strings against string keys to see which are missing

3 & 4 above turn out to be an issue because there are lots of strings in your average app and its hard to determine which ones are meant to be localized and which aren't programmatically. I suspect this is why there aren't more apps or scripts to do this because its very hard to do well.

I am looking into how to at least declutter the false positives by matching only where the strings are contained in NSLocalizedString or Text function calls - and optionally inside print() to help find all the print cases - which should not be in a production app (IMO).

I'll probably add something in the next few days. Last thing will be where we add it as build script or just run it manually on the side and have it spit out the results. This can also be done as a build script and emit to the stdio - which will show in Xocde -- but might really clutter up builds by showing lots of false positives.

Thoughts?

Sheeri commented 3 years ago

You said: But I feel that is not a good general practice for a localized app because it's hard to know without auditing your strings files whether you've provided a loc for every such string. I prefer not having the string key be in English (which means I add an English strings file too).

And I absolutely 100% agree. I'm not sure if there's a reason previous devs didn't use NSLocalizedString or Text. Looping @am-MongoDB into this.....

bladebunny commented 3 years ago

If you look at my commit for the dark mode stuff there’s an example of what I am talking about. I can make a start if you want to give me a task / issue for it. I can use my script to find unlocalized strings but

don’t necessarily want to take on an app-wide audit. But I can make a dent in it.

On October 12, 2020, Tim Condon notifications@github.com wrote:

You said:  But I feel that is not a good general practice for a localized app because it's hard to know without auditing your strings files whether

you've provided a loc for every such string. I prefer not having the

string key be in English (which means I add an English strings file too).

And I absolutely 100% agree. I'm not sure if there's a reason previous

devs didn't use NSLocalizedString or Text. Looping @am-MongoDB https://github.com/am-MongoDB into this.....

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/WildAid/o-fish-ios/issues/339#issuecomment- 707252083, or unsubscribe https://github.com/notifications/unsubscribe- auth/AALHW54S5CYRHW7XQ5ISMPLSKM4SDANCNFSM4SAPNUAQ.

Sheeri commented 3 years ago

OK, so after some discussion, we want to keep the code simple.

https://www.hackingwithswift.com/example-code/uikit/how-to-localize-your-ios-app points to using the 'genstrings' command to do this: find ./ -name "*.swift" -print0 | xargs -0 genstrings -a -SwiftUI -o OUTPUT_DIR This isn't perfect - it doesn't get everything (see the errors that come out), but it's not a bad approximation at least.

A python script is OK, but it feels like reinventing the wheel - we're not the only folks to run across this.

Would something like this work? https://github.com/SwiftGen/SwiftGen It feels like there are a lot of tools out there, and that we would be better served using one of those instead of making something up from scratch - we'd still need the code to look at the list of strings to be localized vs. what is already localized and spit out what isn't localized in which languages.

bladebunny commented 3 years ago

To be honest, I haven't used genstrings in my own apps. I prefer my own approach because I don't like using plain text as the string keys.

But it's really up to what you guys want. I'll have to see what that command produces because the current project doesn't have a Base.lproj directory or an en.lproj directory for the English strings as you would usually have from the default settings/config. But if it works, that's fine.

I thought the task was to try to audit for missing strings? I'll have to see what the behavior of genstrings is when there's an existing loc file. e.g Does it overwrite or append? Assuming it appends, it might be a far easier task to just diff the two versions of each loc file to show what's changed.

bladebunny commented 3 years ago

Just tested. It looks like the GenStrings command does just generate a delta. I could automate re-generating it on build but would need to know what you want to do with the output. i.e. Do you want to take the delta and automatically add it to the bottom of the existing strings files? Or just produce a sanitized version you can give to translators? Any number of possibilities....

Sheeri commented 3 years ago

I like where you're going with this line of thinking!

Here's my thought process: We want the delta put somewhere - regenerating on build isn't strictly necessary....but there's no harm in it. If we put the delta at the bottom of the localization files, I worry that they will stay there, instead of in the logical groups we already have in the file. I don't think there's a way to automate that, and it's OK not to have it automated. So maybe something like having a line in the localization files that says:

---- Below this line are localizations that need to be done. Please keep this line even if there are no lines below this one. ----- Or similar - so that anyone who wants to work on localizations can easily see what needs to be done at any time. Or someone making localization issues in Github can go to the files and easily see "We need French translations for these 7 phrases and Ukranian translations for these 5 phrases."

Would it be difficult to do that? make a script that is intended to be run on build, and adds the translations to the bottom of the files? I think that's the simplest thing right now - I don't want the builds to fail if there are translations, but I don't want to lose track of what needs to be localized.

bladebunny commented 3 years ago

Thanks. No, I don't think it'll be that difficult. I think the script would. 1) Copy or just rename loc files 2) Run genstrings and generate the deltas 3) Insert deltas at the bottom of the old loc files with your suggested comment line 4) Restore and clean up

Honestly, not hard. What language do you guys prefer to have it in? Python, Ruby, Swift?
I'd prefer not to do it in Bash.

Sheeri commented 3 years ago

If you do it in Swift, would it require anything new? I'm thinking of when we can eventually fold this into a build process; if we used python or ruby we'd have to make sure python/ruby are installed in whatever container we use for testing. If Swift already has what we need, that's one less dependency to break things....

bladebunny commented 3 years ago

I'll try with Swift. TBH have not done much scripting with Swift yet - but this shouldn't require anything crazy. Just not sure whether it's integrated with STDIO. Assuming it is. Python probably the best alternative.

Sheeri commented 3 years ago

@bladebunny - Great! If things start to get complicated with Swift, then please go ahead and use python. Thanks!

bladebunny commented 3 years ago

I added a PR with an initial implementation using Swift. A bit more code than the comparable Python would probably be but not bad. Adds an edit line to the bottom of localizations as you mention above. Somewhat limited in what strings this can detect based on matching uses of Text() or NSLocalizedString() -- it's a genstrings limitation.

One way to allow this script to do more work for you would be to start just wrapping strings, outside of Text(), in NSLocalizedString so that they can be detected. I try to trim false positives but you may see some "trash" keys like ":" or whatever in the output.

If needed, the script can be tweaked to add "ignore" or black listed keys that won't match. But, if it were me, I'd leave them and use the script to detect them so they can be cleaned up in situ.

Sheeri commented 3 years ago

No worries! We can do a one-off cleanup run of "show me everything in single or double quotes" and change to Text or NSLocalizedString as appropriate.

(and then maybe for the future, a script that checks changes to files for things in quotes, in case a developer forgot to use Text or NSLocalized String....but I'm getting ahead of myself here)

I don't mind the ":" since it's legitimately inside Text or NSLocalized String. We'll just keep it in the translation files as ":"