fix(ios)!: localize the patterns count and let hosts fall back to editor strings - #569
Conversation
XCFramework BuildThis PR's XCFramework is available for testing. Add the following to your .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/569")Built from 7b48e6d |
Improve i18n.
`EditorLocalization.localize` held the default strings inline, so host apps replaced the whole table and switched over `EditorLocalizableString` exhaustively. Every string the editor added then broke the host build, blocking adoption of unrelated changes until someone wrote a translation. Expose the defaults as `defaultLocalize` so hosts can delegate unhandled keys with a `default:` case. New strings render untranslated instead of failing to compile, matching how the editor's own JS translations and Android string resources degrade. Report the fallback at the `debug` level, but only once a host installs its own `localize`. Without an override every string comes from the default table by design, so logging each one would be noise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fallback report went through `EditorLogger`, which only reaches hosts that install a logger and lower the log level from its `error` default. Host apps do neither by default, so the message was never emitted for the integration it was meant to help. Log through `OSLog` instead, which needs no host configuration and matches how the rest of the library reports activity. Use `notice` so the message also persists to the log store and stays readable from Console.app after the fact, rather than only streaming live to an attached debugger. Cover the emission against a real `OSLogStore` with `EditorLogger` left unconfigured, matching how host apps integrate the library. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`EditorLocalization` is `@MainActor`, so `defaultLocalize` inherited that isolation. Host apps delegate to it from their own localization functions, which are ordinarily not isolated, so referencing it warned under Swift 5 and would fail to compile under the Swift 6 language mode. Declare it `nonisolated` and make it a function rather than a stored closure. It is a fixed lookup that no one replaces, and `nonisolated` cannot apply to a non-`Sendable` closure type. Guard the reporting flag it reads with a lock instead of leaving it `nonisolated(unsafe)`, so the cross-actor access is free of races rather than asserted to be. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
21d8914 to
b18641c
Compare
Reporting was gated on a flag that any assignment to `localize` tripped, which meant a host restoring the editor's own strings latched reporting on permanently. It also logged on every string read, and every call site sits in a SwiftUI `body` that re-evaluates per render pass, so scrolling the pattern list wrote a persisted `notice` per row per frame. Replace the latch with `reportsMissingTranslations`, a public opt-out that states the intent directly, and report each key at most once per process. Deduping through the lock also closes the read-then-log race the previous flag only claimed to close. The demo app opts out: it renders the editor's own strings on purpose, so every lookup falls back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The hand-written `allCases` existed only to drive a test asserting every key has a default string. The exhaustive switch in `defaultLocalize` already guarantees that at compile time, and because the list was hand-written it could not catch the case it was meant to: a new enum case nobody adds to `allCases` compiles, passes, and goes untested. It was also a public promise of completeness the type could not keep, so a host iterating it to pre-build a translation table would silently miss keys. The remaining test covers `patternsCount`, the one default that is computed rather than a literal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`defaultLocalize` reported on every call, but it has two callers that mean opposite things: a host delegating an unhandled key, and the editor reading its own default before a host installs `localize`. The second fired falsely. `EditorViewController` reads `loadingEditor` in a stored property initializer, which runs while the view controller is built — before the host's `localize` assignment is guaranteed to have landed. So WordPress-iOS saw "Missing host translation for loadingEditor" for a string it translates. Reports that name correctly-translated strings train integrators to ignore all of them. Split the lookup from the reporting: the library's own default closure calls a private `defaultString(for:)`, and the public `defaultLocalize` — which only hosts call — keeps reporting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A plain `default` warns "default will never be executed" in a host that happens to cover every case, which WordPress-iOS does today. `@unknown default` compiles clean there and behaves identically once the editor adds a string. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| /// case .showMore: NSLocalizedString("editor.blockInserter.showMore", ...) | ||
| /// // ...keys the host translates. | ||
| /// @unknown default: EditorLocalization.defaultLocalize(key) | ||
| /// } |
There was a problem hiding this comment.
Should EditorLocalization.localize allow returning a nil, which means GBK can call defaultLocalize internally, rather than requiring the app to call it?
There was a problem hiding this comment.
This approach makes sense. Thanks for the suggestion. Addressed in 42b4bb4.
| @@ -39,7 +41,81 @@ public enum EditorLocalizableString { | |||
| @MainActor | |||
There was a problem hiding this comment.
Majority of this class are nonisolated functions. Should it be bound to the main actor at all?
There was a problem hiding this comment.
I believe I refactored the class to align with your suggestion, but let me know if I did not. See b08789c.
There was a problem hiding this comment.
I think you should be able to delete the nonisolated declarations now.
There was a problem hiding this comment.
I believe the only remaining nonisolated is with reportsMissingTranslations. When removing it, I see an error: Static property 'reportsMissingTranslations' is not concurrency-safe because it is nonisolated global shared mutable state.
Should this be removable or are you referencing other declarations?
| set { reportingLock.withLock { _reportsMissingTranslations = newValue } } | ||
| } | ||
|
|
||
| private nonisolated static let reportingLock = NSLock() |
There was a problem hiding this comment.
I feel like adding this lock increases complexity. I don't think it's too much to log missing translations repeatedly on the same keys.
There was a problem hiding this comment.
I agree there is definitely increased complexity to achieve de-duping logs. I attempted simplifying it in b681e10. Let me know what you think of the state of it now.
I agree losing de-duping is likely not a big issue. The one outlier is the Patterns sheet, where a single missing translation string would result in numerous logs as you scroll the sheet. However, if you feel it's still worth dropping the de-duping entirely, let me know, I'm open to that.
`EditorLocalization.localize` now returns `String?`. Hosts return `nil` for keys they do not translate instead of calling a public `defaultLocalize`. Delegating by convention could be skipped or reimplemented: a host was free to write `default: ""` and ship blanks past the fallback. `nil` is now the only way to decline a key, so every fallback routes through the editor's defaults and its missing-translation reporting. This also retires the isolation problem that made `defaultLocalize` public. It is now a private `defaultString(for:)`, so nothing forces `nonisolated` onto host code, and reporting moves to the subscript — the one place that observes a declined key. The seven `EditorLocalization.localize(...)` call sites in the editor moved to the subscript, which they should have used already; calling the closure directly bypassed the fallback. Drops the demo app's `reportsMissingTranslations = false`. The demo installs no closure, so it never returns `nil` and never reported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comments argued against alternatives that only ever existed in this branch's history — a public `defaultLocalize` for hosts to call, and `@unknown default` in the host switch. A reader of the merged code has no such context, so the justifications read as warnings about phantom options. Keeps what the code does not say for itself: return `nil` to decline a key, and why `default: nil` beats an exhaustive switch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The example recommended a plain `default`, on the mistaken finding that Swift does not warn "default will never be executed" for an exhaustive switch. Xcode does emit that warning; the CLI check that suggested otherwise failed to reproduce the build's configuration. A host covering every case defined today — which any host adopting this version will be — hits that warning with a plain `default`, so `@unknown` is what the example should show. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`EditorLocalization` was `@MainActor`, but only two of its members need it. Everything else was annotated `nonisolated` to escape that, so the class declaration claimed an isolation the code spent five keywords undoing. The annotation now sits on `localize` and the subscript, which is where it belongs: `localize` holds a non-`Sendable` closure, and the subscript reads it. `reportsMissingTranslations`, `defaultString(for:)` and `reportMissingTranslation(for:)` drop their now-redundant `nonisolated`. The `nonisolated(unsafe)` on the two stored properties stays — that suppresses global-mutable-state checking, unrelated to the class. No source break for hosts. Reading a string off the main actor is still rejected, and `reportsMissingTranslations` is still settable from a non-isolated context; both verified against WordPress-iOS's call pattern with `swiftc -swift-version 6 -strict-concurrency=complete`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reporting state was two values sharing one `NSLock`, which forced a `_`-prefixed backing var and a manual accessor pair on a property that is otherwise a plain `Bool`. That bought atomic reads of the flag and the set together — an invariant with no consumer. The flag gates whether to report; the set answers whether a key was seen. Nothing reads them as a pair. `reportsMissingTranslations` is now a stored `nonisolated(unsafe)` var: word-sized, set once during host setup before any editor view reads a string. The key set moves to `OSAllocatedUnfairLock<Set<String>>`, so the type states what is guarded instead of leaving it to convention. Keeps the once-per-key dedup. Its call sites sit in SwiftUI `body` methods — `PatternSectionView` reads `patternsCount` per section — so logging every read would write an entry per row per frame while a list scrolls. Behavior is unchanged; the existing reporting tests pass untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`nonisolated(unsafe)` is invisible at the API boundary — a host can read or write this property from a detached task under Swift 6 strict concurrency with no diagnostic. Documentation is the only channel for the constraint. The comment explained the annotation to maintainers rather than the usage to consumers. It now leads with what a host should do, and keeps the caveat as the reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What?
Localizes the patterns count string, and lets host apps fall back to the editor's own strings for keys they do not translate.
Warning
Source-breaking for host apps.
EditorLocalization.localizenow returnsString?instead ofString, so every host that assigns it needs a signature update — independent of the newpatternsCount(Int)case, which also stops an exhaustive switch from compiling. WordPress-iOS needs a companion PR before it can adopt this version: wordpress-mobile/WordPress-iOS#25848Why?
The patterns count rendered as a hardcoded English string. Fixing that means adding a case to
EditorLocalizableString, which exposed a design problem worth fixing in the same change.Hosts override
EditorLocalization.localizeby replacing the closure, so they switch over the enum exhaustively with nodefault:. Swift requires that switch to be exhaustive, so every string the editor adds breaks the host build — and the break surfaces later, when the host bumps the dependency, not when the string is added. A host that does nothing gets a build failure rather than an untranslated string, which blocks them from adopting unrelated fixes until someone writes a translation.That penalty is disproportionate for this localization gap. Android string resources fall back through
values-ar/→values/, and gettext returns the source English when no translation exists; neither fails the build. The editor's own JS translations already work this way.How?
Declining a key —
localizereturnsString?. A host returnsnilfor keys it does not translate, and the editor supplies its own string:Once a host writes that fallback case, future keys render untranslated instead of failing to compile.
An optional return rather than a public fallback function the host calls itself, so the fallback cannot be skipped or reimplemented:
nilis the only way to decline, and it routes through the editor's defaults and reporting. Every read goes through theEditorLocalization[...]subscript, which unwraps, falls back, and reports in one place.Reporting — falling back logs each untranslated string once per process, at
noticeso it persists to the log store without the host configuringEditorLogger. Reporting once per key matters because every call site sits in a SwiftUIbodythat re-evaluates on each render pass; logging per read would write an entry per row per frame while a list scrolls. Keys are deduplicated by case name, sopatternsCount(3)andpatternsCount(7)count as one missing string.Only a
nilreturn reports. The default closure answers every key, so strings the editor reads while building views —loadingEditoramong them, which can run before the host assignslocalize— are never reported as missing.Hosts that render the editor's own strings deliberately can opt out with
EditorLocalization.reportsMissingTranslations = false.Isolation —
@MainActorsits onlocalizeand the subscript rather than the whole class.localizeholds a non-Sendableclosure and the subscript reads it; nothing else needs isolation, and no annotation is forced onto host code. Verified withswiftc -typecheck -swift-version 6 -strict-concurrency=completeagainst a file reproducing WordPress-iOS's call pattern, including that reads off the main actor are still rejected.Testing Instructions
This is not testable in the Demo app. See testing instructions in wordpress-mobile/WordPress-iOS#25848.
Accessibility Testing Instructions
No UI changes. Strings that previously rendered in English still do; the change is which code supplies them.