Repository navigation
feat(reader): add manga OCR and translation - #1252
Conversation
everpcpc
left a comment
There was a problem hiding this comment.
Self-review pass: 4 standards reviewers (Core/storage, services, views/settings, reader integration) + 1 spec reconciliation against the PR description.
Verdict: hard rules clean (no unsafe hatches / force casts / object-style environment; GRDB, RawCodableStore, and localization discipline all pass) and the spec checks out end-to-end — page images never leave the device (provider payloads carry text segments only). 4 blockers inline to fix before merge.
Spec-axis gaps (description, not code):
- This PR removes "shake to open Live Text" entirely (
ShakeGestureHandler.swift+ toggles inDivinaPreferencesView/ReaderSettingsSheet+AppConfig.shakeToOpenLiveText). The deletion is clean (zero references remain; superseded by the floating button) and the Live Text ↔ manga translation mutual exclusion inAppConfig.setLiveTextEnabled/setMangaTranslationEnabledis deliberate, but neither behavior change is mentioned in the description — please document them. LanguageCodeHelper.displayName(for:)now useslocalizedString(forIdentifier:), which changes language names app-wide (e.g.en-USnow shows the region full name,zh-Hantrenders as "Chinese (Traditional)") — worth a line in the description.- Memory-warning cleanup is iOS/tvOS-only (
MangaModelMemoryWarningObserveris#if os(iOS) || os(tvOS)); macOS has no public memory-warning API, but the description states the behavior without the platform qualifier. - The catalog UI does not pre-filter incompatible models —
validateSupportonly rejects at download time. Acceptable, but worth noting.
Minor notes (not worth inline threads):
MainApp.swift:79: the startup Live Text mutex can never fire (all write paths already enforce exclusivity) — harmless, keep or drop.ModelArchive.swift:80isHexDigitaccepts full-width hex digits; the digest comparison is fail-closed, harmless.AppConfig.modelLibraryread-modify-write races (catalog fetch vs register/select/remove) can overwrite each other; consistent with existing AppConfig patterns, low probability.MangaOverlayViewiOS/macOS branches duplicate ~100 lines of layout logic — follows the existingNativePageItem_iOS/_macOSprecedent, noted for the record.SettingsSection.swift:86: mangaTranslation reuses.blue, same as the adjacent reading section.
CI: tvOS passed; iOS/macOS still pending at review time.
|
Verified 7abf75f against all 29 review threads — every blocker and nit is addressed. Blockers
Nits — all resolved, notably: corrupt packages self-heal (delete + re-download) and The PR description now documents the shake-to-open Live Text removal and the Live Text ↔ translation mutual exclusion — spec gap closed. CI green on iOS/macOS/tvOS. One leftover observation, non-blocking: the LGTM. |
What
manga_ocrarchives and compatibility is validated before download; model binaries remain outside the app bundle, and loaded models are released on memory warning on iOS/tvOS and when the reader closes.Why
Bring manga translation to the reader without uploading page images, while keeping OCR models replaceable and giving users control over their translation provider and on-device resources.
Validation
make format— passed.make build— passed on iOS, macOS, and tvOS.make localizeand./misc/translate.py list— no missing translations.make run-ios-sim— installed and launched on the iPhone 18 Pro simulator for manual testing.git diff --check— passed.