Closed Bug 1261431 Opened 8 years ago Closed 8 years ago

String grouping

Categories

(Firefox for iOS :: Localization, defect)

All
iOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fxios-v4.0 --- fixed

People

(Reporter: bnicholson, Assigned: bnicholson)

References

Details

Attachments

(1 file)

Since we're switching to NSLocalizedString with keys, we should name the IDs consistently across the app. We can make this easier if group all strings into a single class so they're all lined up together.

An added benefit is that we can also share strings in the cases where it makes sense to do so (e.g., as a settings item, and then as the title for that setting's subscreen).
flod pointed out that we may still want separate strings for settings items and subsection titles in https://github.com/mozilla/firefox-ios/pull/1683, so ignore point 2 above.
Would like to get strings figured out before the 4.0 freeze. Here's one approach -- curious to hear everyone's thoughts!
Attachment #8737967 - Flags: review?(sleroux)
Attachment #8737967 - Flags: review?(sarentz)
Attachment #8737967 - Flags: review?(jhugman)
Attachment #8737967 - Flags: review?(etoop)
Attachment #8737967 - Flags: review?(bmunar)
Comment on attachment 8737967 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1686

I personally think this is a good idea because of the reasons you said. It makes bookkeeping the strings a lot cleaner (as long as we label the strings per section (e.g. Top Sites, History Panel, etc.) and maintain a consistent format). It does take a little more work to add a new string, like you said, but I think it's worth it than just throwing a bunch of NSLocalizedStrings into classes, some assigned to a variable and some not, etc. The work it takes to maintain the cleanliness is worth it in the long run! (and good and helpful for the l10n team too, I presume)
Attachment #8737967 - Flags: review?(bmunar) → review+
Comment on attachment 8737967 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1686

Yup - I like it! Only thing I would add is when we start moving strings over to the Strings.swift file we can 'group' them using extensions to clean it up a bit instead of comments.
Attachment #8737967 - Flags: review?(sleroux) → review+
(In reply to Stephan Leroux [:sleroux] from comment #4)
> Yup - I like it! Only thing I would add is when we start moving strings over
> to the Strings.swift file we can 'group' them using extensions to clean it
> up a bit instead of comments.

Good idea! Updated the PR.

Merging so that we can use String.swift for incoming PRs with string changes. I think the most important thing is that we agree on a format for l10n string IDs; we can always change the Swift code later if we want.

https://github.com/mozilla/firefox-ios/commit/7f7b334dd6201679ec10afc6c80a6de037b852dd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.0
Comment on attachment 8737967 [details] [review]
Link to Github pull-request: https://github.com/mozilla/firefox-ios/pull/1686

I'll also update the README with details about using NSLocalizedString with value and string IDs.
Attachment #8737967 - Flags: review?(sarentz)
Attachment #8737967 - Flags: review?(jhugman)
Attachment #8737967 - Flags: review?(etoop)
NI on Stefan in case he's the one doing the export for v4.0

As a consequence of this PR, a lot of strings move around, because an entire file moves from 'Client/LightweightThemes.strings' to 'Shared/Supporting Files/LightweightThemes.strings'.

You can get an idea by looking at my diff between v3 and v4
https://github.com/flodolo/fxios-italian/commit/771feca59351c2e8defc7cc49fafa54a6fe425b9

Two issues:
* If these strings are actively used in v3.0, once we export the strings for v4.0 we basically lose them for any v3.x
* If we do this, we need to run a script that rename the file for all locales, and only after that we add new strings. Otherwise, the update script will lose all of the existing translations (the script stores translation by file:id).
Flags: needinfo?(sarentz)
Depends on: 1262945
Thanks for catching this. Going to move these strings back in bug 1262945.
Flags: needinfo?(sarentz)
You need to log in before you can comment on or make changes to this bug.