search

Automattic / Gravatar-SDK-iOS

Gravatar SDK is a Swift library that allows you to integrate Gravatar features into your own iOS applications.
https://gravatar.com
Mozilla Public License 2.0
36 stars 1 forks source link

Setup Legacy Localization Support #356

Closed andrewdmontgomery closed 3 weeks ago

andrewdmontgomery commented 1 month ago

Closes #339

Description

This sets up localization of the SDK and Demo apps:

For this PR, I would like to agree on the key names. Once we've agreed on those, I can upload our .strings to GlotPress and set up the automation for (eventually) downloading translations.

Feedback on the "comments" is very welcome. But we can make changes to those up until we submit strings for localization.

Note: this is just basic support for localization

This only introduces support for localization. Notably:

Testing Steps

Required for this PR:

Nice to have:

Update an SDK source file and generate strings

  1. Find a source file with a localizable string
  2. Modify it:
    • Add or modify a comment
    • Modify a key
    • Modify a value
      1. Update the Localizable.strings file using the command line: make generate-strings
    • [ ] OBSERVE: you should see your change appear in Sources/GavatarUI/Resources/en.lproj/Localizable.strings

Update a Demo source file and generate strings

Right now, only one string is localizable. It's the Text("Email:") view in DemoAvatarPickerView. For this test, you can either modify that one, or (if you're familiar) add localization support to one of other views.

  1. Find a source file with a localizable string
  2. Modify it:
    • Add or modify a comment
    • Modify a key
    • Modify a value
      1. Update the Localizable.strings file using the command line: make generate-strings
    • [ ] OBSERVE: you should see your change appear in Sources/GavatarUI/Resources/en.lproj/Localizable.strings
wpmobilebot commented 1 month ago
Gravatar SwiftUI Prototype BuildšŸ“² You can test the changes from this Pull Request in Gravatar SwiftUI Prototype Build by scanning the QR code below to install the corresponding build.
App NameGravatar SwiftUI Prototype Build Gravatar SwiftUI Prototype Build
Build Number1142
Version1.0
Bundle IDcom.automattic.gravatar-sdk-demo-swiftui.prototype-build
Commitc5c550a263b8206722640f23a32515771859cfe8
App Center BuildGravatar SDK Demo - SwiftUI #66
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.
wpmobilebot commented 1 month ago
Gravatar UIKit Prototype BuildšŸ“² You can test the changes from this Pull Request in Gravatar UIKit Prototype Build by scanning the QR code below to install the corresponding build.
App NameGravatar UIKit Prototype Build Gravatar UIKit Prototype Build
Build Number1142
Version1.0
Bundle IDcom.automattic.gravatar-sdk-demo-uikit.prototype-build
Commitc5c550a263b8206722640f23a32515771859cfe8
App Center BuildGravatar SDK Demo - UIKit #67
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.
andrewdmontgomery commented 3 weeks ago

I want to call out one change especially:

Sources/GravatarUI/SwiftUI/AvatarPicker/AvatarPickerView.swift

Right now, the private enum Localized {...} uses some nested enum to structure these string references. I also tried this with a flat enum Localized, too, with entries like:

And then calling:

But with nesting, I found it a bit easier to keep track of everything:

I find that the calling site is easier to read this way:

And it's easier to scan the enum:

private enum Localized {
    enum Header {
        static let title = ...
        static let subtext = ...
    }

    enum ContentLoading {
        enum Success {
            static let title = ...
            static let subtext = ...
        }

        enum Failure {
            // etc.
        }
    }
}

Thoughts?

cc: @AliSoftware

pinarol commented 3 weeks ago

Some keys have too much nesting for my taste at the first look but it all makes sense after reading the code a bit. So LGTM. šŸ‘

andrewdmontgomery commented 3 weeks ago

Some keys have too much nesting for my taste at the first look but it all makes sense after reading the code a bit. So LGTM. šŸ‘

Yeah, that's how I felt, too.

andrewdmontgomery commented 3 weeks ago

@AliSoftware do you have any thoughts about how/when to generate the en source Localizable.string? Does it make sense to have the PR's that make changes to localizable strings also commit the changes within the PR? In this case, we'd want a check for uncommitted strings. We could even (potentially) have a githook.

AliSoftware commented 3 weeks ago

@AliSoftware do you have any thoughts about how/when to generate the en source Localizable.string? Does it make sense to have the PR's that make changes to localizable strings also commit the changes within the PR? In this case, we'd want a check for uncommitted strings. We could even (potentially) have a githook.

Ah, good questionā€¦ usually for mobile apps we do that at the moment of code freeze, which then allows those generated en.lproj strings to be frozen for the duration of the beta-testing phase as well as sending those generated English copies for translation (to GlotPress or whatever platform we use), and then download the translations at the end of the beta-testing phase (typically ā‰ˆ5 days later) just before doing the final release.

But to my understanding, for Gravatar SDK, you don't plan to follow a "code-freeze + beta-testing phase" release process, but instead just create a tag whenever you feel ready to release a new version?

If so, the best options I could think of from the top of my head right now could be:

Option 1: As a precommit hook (as you mention)

The main drawback of this approach is that git hooks are local. So developers and contributors would have to install the hook on their development machine for the process to work well as expected.

Option 2: make CI push the updated strings file

Alternatively, we could decide to have CI do the generation of the file and pushing the changes itself.

But tbh I'm not a fan of having CI do git pushes. First because this involves security considerations (CI has to have a token that is allowed to push, and we should thus make sure this token is super secure).

Also, because that could easily create conflictsā€”especially if a developer do quick iterations on their PRs while the CI has already started a build on a preceding commit and that CI build is about to push the commit with updated strings at the end of that buildā€¦ but the developer end up pushing a new commit in-betweenā€¦

Finally, if we were to go with that approach, that would beg the question if CI should do such commit to regenerate strings files (if diff for it is non-empty):

1a: On each commit of PR branches

2b: Only on trunk, once the PR has landed

2c: Only on tags, at the time the release is created

TBH Non of those options feel perfect to me.

The last option would be for you to consider introducing a release process for the SDK, i.e. not just "when I want to do a release I just create a GitHub Release and git tag and CI will publish it and that's done", but a process more like "code-freeze", where you'd at least:

That would make the release process of the SDK a bit more convolutedā€¦ but would provide better flexibility (not just for localizations, but also release notes, potential beta-testingā€¦)

andrewdmontgomery commented 3 weeks ago

But tbh I'm not a fan of having CI do git pushes.

Fully agree. And for so many reasons.

The main drawback of [using a githook] is that git hooks are local

Yeah. And then we're left with the question of what to do when that doesn't happen. Does CI fail and block a PR? (don't love that.) Does CI start auto-committing? (again, don't love that.)

The last option would be for you to consider introducing a release process for the SDK

I think this is the answer. The fact of the matter is, since the SDK will be localized, we already have a release process, in the sense that there is a process, with steps that block a release:

  1. We need to submit strings for localizing
  2. We need to wait for those strings to be localized
  3. We need to integrate those localizations

That's a process, and it blocks a release. We should understand the implications of that process and make decisions about how we want it to progress. And then we use that to automate the actions that are needed to complete that process consistently.

We also try to sync up our releases (roughly) with the Android SDK. Yet another reason to make this a formal step in the release process.

Ok, I'll work on what that might look like. And thanks for your roughed out idea of how that might work.

andrewdmontgomery commented 3 weeks ago

btw, we already have a release process: https://lanternp2.wordpress.com/sdk-releases/ios-sdk-release-manual/

AliSoftware commented 3 weeks ago

btw, we already have a release process: Pfnfe0-d4-p2

Neat!

šŸ’” Once the process is more settled (as I assume that checklist might soon change slightly to account for those questions around L10n and translations integration), we should probably consider migrating that checklist from a P2 page to our internal ReleaseV2 tool in MC at some point in the future. Even if the first iteration of that would still only be a checklist of manual stepsā€”and we only automate those steps (via buttons on the release scenario of ReleasesV2 calling appropriate fastlane lanes in CI) laterā€”that could already be beneficial e.g. to give ability to track the progress of such checklists within the tool, allow release managers to leave notes and breadcrumbs on each checklist item, etcā€¦

Though I guess you'll probably want to run a couple of releases firstā€”to test and consolidate the current process (and account for changes in it like for L10n)ā€”and officialize it more in ReleaseV2 only when it's considered "stable enough"ā€¦?

andrewdmontgomery commented 3 weeks ago

Whoa, that's nifty. I was unaware of this tool. This looks lovely.

Though I guess you'll probably want to run a couple of releases firstā€”to test and consolidate the current process (and account for changes in it like for L10n)ā€”and officialize it more in ReleaseV2 only when it's considered "stable enough"ā€¦?

Yeah, the P2 page (and the spreadsheet that supports it) is here to be low-lift, and to give us the flexibility to discover our processes before we start stamping that process into a tool.

I like this goal.