Give the odrcore wrapper typed errors and stop crashing on purpose - #114
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a50c161555
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
87d7b89 to
2562a58
Compare
a50c161 to
8288c9d
Compare
|
Correct, fixed in 30864e6. A key missing from a table resolves to the key itself rather than falling back to the development language, so the other 16 localizations would have rendered Went with the explicit-value option rather than seeding 16 tables with untranslated English: NSLocalizedString("intro_start", value: "Start", comment: "onboarding button on the last page")Verified the fallback actually resolves to One loose end from the same commit, out of scope here: |
2562a58 to
e6a0f6e
Compare
CoreWrapper reported failures through an NSNumber errorCode carrying -2, -3 and -5, decoded again in Document.swift and a third time in DocumentViewController. It also assigned 0 to an object pointer to mean success. It now uses a proper NSError domain with an NS_ERROR_ENUM, so Swift sees throwing methods and the magic numbers are gone. Crashes fixed along the way: - parse() indexed pagePaths[page] unchecked. The page filter drops every page for some documents, so an empty array was reachable and indexing it killed the app. translate now fails with an error instead of reporting success with nothing to show, and the index is clamped. - backTranslate dereferenced an empty std::optional when it ran before any translate. Now a typed error. - writeContents entered a DispatchGroup inside the completion handler and only left it on the happy path. A failing generateDiff left it unbalanced and every save waited out the full 30s timeout. It also called fatalError from that handler. Replaced with a semaphore signalled on every path. - eight fatalError calls used as error handling, for states a user can actually reach: no document, failed import, missing storyboard entry, root controller of an unexpected type. All either recover or report. - three as! casts on storyboard lookups. - the password alert force unwrapped textFields![0] and .text!. - documentEncrypted rescheduled itself when the view was not in a window yet, then presented the alert anyway. Other cleanups: - DocumentDelegate: class -> AnyObject - @UIApplicationMain -> @main - UIApplication.shared.openURL -> open(_:), deprecated since iOS 10 - UserDefaults.synchronize(), a no-op since iOS 12 - the progress bar observed UIDocument's own progress, not the loadProgress that parse() actually advances - the onboarding buttons were hardcoded English in Constants while the app ships 17 localizations. They are NSLocalizedString now, with the keys added to en.lproj for translation. The rest of Constants was dead. Tests go from one measure block asserting a nil error code to five cases covering page output, back translation, both new error paths and the performance measurement.
Moving the button titles out of Constants into NSLocalizedString added the keys to en.lproj only. A key missing from a table does not fall back to the development language, it resolves to the key itself, so all 16 other localizations would have shown "intro_next" / "intro_start" on the button where they used to show the hardcoded English. Passing value: keeps that English wording until the strings are translated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011MhKU2kWm1cPW4GBq9gon5
30864e6 to
d44e7c3
Compare
Stacked on #113.
CoreWrapperreported failures through anNSNumber errorCodecarrying-2,-3and-5, decoded again inDocument.swiftand a third time inDocumentViewController. It also assigned0to an object pointer to mean success, soXCTAssertNil(coreWrapper.errorCode)was passing for the wrong reason. It now uses anNSErrorDomain+NS_ERROR_ENUM, which Swift imports as throwing methods.Crashes fixed
pagePaths[page]unchecked. The page filter drops every page for some document shapes (a spreadsheet whose only page is nameddocument), so an empty array was reachable and indexing it killed the app.translatenow fails with an error rather than reporting success with nothing to show, and the index is clamped.backTranslatedereferenced an emptystd::optionalwhen called before anytranslate. Now a typed error — covered by a new test.writeContentsleaked its DispatchGroup.enter()happened inside the JS completion handler andleave()only on the happy path, so a failinggenerateDiffleft it unbalanced and every save burned the full 30s timeout before throwing. It also calledfatalErrorfrom that handler. Replaced with a semaphore signalled on every path, plus a main-thread guard so hopping to main can't deadlock.fatalErrorcalls used as error handling, for states users actually reach: no document, failed import, missing storyboard identifier, root controller of an unexpected type. Each now recovers or reports.as!casts on storyboard lookups, and the password alert'stextFields![0]/.text!.documentEncryptedrescheduled itself when the view wasn't in a window yet — and then presented the alert anyway.Other cleanups
DocumentDelegate: class→AnyObject@UIApplicationMain→@mainUIApplication.shared.openURL→open(_:)(deprecated since iOS 10)UserDefaults.synchronize()(no-op since iOS 12)UIDocument.progress, not theloadProgressthatparse()actually advances — so it never moved.Constantswhile the app ships 17 localizations. NowNSLocalizedStringwithintro_next/intro_skip/intro_startadded toen.lproj; other languages fall back to English exactly as they do today, but are now translatable via Crowdin. The rest ofConstantswas dead code.Verification
measureblock asserting a nil error code) to 5, all passing: page output, back translation,backTranslatebeforetranslate, unsupported file type, and the performance measurement.Deliberately not in this PR
The
actor+async/awaitrestructure ofCoreWrapper.parse()still runs the C++ translate synchronously on whatever thread thedidSetfired on. That change alters threading semantics, and with no integration coverage over document loading I would be changing behavior I cannot verify. It wants its own PR with tests written first — say the word and I will do it next.