From 5c95773d5faa2473475c46d8d0ee1a2fc29f2505 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Fri, 7 Aug 2026 08:03:41 +0200 Subject: [PATCH] Fix what a read of the whole tree turned up, and cut the comments Correctness, across the app, the release pipeline and the scripts. The app fixes worth naming: Document reported to its delegate from UIDocument's read queue, so the first open drove UIKit off the main thread; setVCconstraints added a banner height constraint the storyboard already had, which in the Full app contradicted the zero hideBannerView sets; the one-time view setup sat in viewWillAppear and stacked another constraint set on every appearance; and the search query went into evaluateJavaScript unescaped. In the release path, the signing keychain relocked after fifteen minutes while the job now archives both apps on it, deliver ran a precheck whose verdict it then discarded, an empty build number only failed after both uploads had gone out, and paths-ignore never matched a root .md. changelog-section.py wrote its errors to stdout, which both callers redirect: a missing section failed silently, and record wrote the error into the release notes. The comments got three passes. The ones restating the next line are gone, the stale ones are gone, and what is left is the why. Both schemes build, the suite passes, format.sh --check is clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Gu9vyCGNhZbUeF4195ByUv --- .github/scripts/changelog-section.py | 43 ++--- .github/scripts/resolve-version.py | 29 ++- .github/workflows/build_test.yml | 8 +- .github/workflows/format.yml | 9 +- .github/workflows/release.yml | 61 +++--- .gitignore | 2 - CHANGELOG.md | 43 ++--- OpenDocumentReader.xcodeproj/project.pbxproj | 8 +- OpenDocumentReader/AppDelegate.swift | 4 +- OpenDocumentReader/ConfigurationManager.swift | 7 +- .../ContentViewController.swift | 19 +- OpenDocumentReader/CoreWrapper.swift | 53 ++---- OpenDocumentReader/Document.swift | 76 +++++--- .../DocumentBrowserViewController.swift | 10 +- .../DocumentViewController.swift | 176 ++++++++---------- OpenDocumentReader/NonFree/CrashManager.swift | 7 +- OpenDocumentReader/PageTabBar.swift | 30 ++- OpenDocumentReader/StoreReviewHelper.swift | 34 ++-- .../OpenDocumentReaderTests.swift | 123 ++++++------ OpenDocumentReaderTests/PageTabBarTests.swift | 19 +- .../fixtures/make-fixtures.py | 12 +- README.md | 14 +- fastlane/Fastfile | 80 +++----- scripts/format.sh | 5 +- 24 files changed, 392 insertions(+), 480 deletions(-) diff --git a/.github/scripts/changelog-section.py b/.github/scripts/changelog-section.py index 510d8d9..a11c176 100755 --- a/.github/scripts/changelog-section.py +++ b/.github/scripts/changelog-section.py @@ -2,18 +2,18 @@ # # Prints the CHANGELOG.md section for one version, and fails when there is none. # -# The release run reads it before building, so a version without release copy fails -# in seconds rather than once both apps are uploaded, and again in the record job, -# where the section becomes the body of the drafted github release. +# The release run reads it before building, so a version without release copy +# fails in seconds rather than once both apps are uploaded, and again when it +# drafts the github release, whose body the section becomes. import argparse -import os import re import sys -# `## [1.37] - 2026-08-02`, and `## [1.38]` while the date is still unknown. Not -# `###`, which belongs to whichever section it sits in -HEADING = re.compile(r"^## +\[?([^\]\s]+)\]?(?: *- *.+)?\s*$") +# `## [1.37] - 2026-08-02`, `## [1.38]`, `## Unreleased`. Whatever follows the +# version is not looked at, so an unusual date separator still closes the +# section above it. Not `###`, which belongs to whichever section it sits in +HEADING = re.compile(r"^## +\[?([^\]\s]+)") # `[1.37]: https://github.com/...compare/1.36...1.37` at the foot of the file: # inside the last section, but not release copy @@ -22,21 +22,17 @@ def section(text, version): """The body under `## []`. Raises ValueError if missing or empty.""" - # an optional v, so the workflow can hand its input straight over wanted = version.strip().removeprefix("v") found = False - collecting = False body = [] for line in text.splitlines(): heading = HEADING.match(line) if heading: - if collecting: + if found: break - if heading.group(1).removeprefix("v") == wanted: - found = collecting = True - continue - if collecting and not LINK.match(line): + found = heading.group(1).removeprefix("v") == wanted + elif found and not LINK.match(line): body.append(line) if not found: @@ -45,8 +41,8 @@ def section(text, version): f"to '## [{wanted}]' before releasing it - that copy is the release body." ) - body = "\n".join(body).strip("\n") - if not body.strip(): + body = "\n".join(body).strip() + if not body: raise ValueError( f"the '## [{wanted}]' section of CHANGELOG.md is empty. A release with " "nothing user facing in it should say so rather than say nothing." @@ -63,15 +59,16 @@ def main(argv=None): args = parser.parse_args(argv) try: - with open(args.file) as changelog: - print(section(changelog.read(), args.version)) + with open(args.file, encoding="utf-8") as changelog: + body = section(changelog.read(), args.version) except (OSError, ValueError) as reason: - # as in resolve-version.py: the annotation form only counts on stdout - if os.environ.get("GITHUB_ACTIONS"): - print(f"::error::{reason}") - else: - print(reason, file=sys.stderr) + # stderr rather than a ::error:: annotation, which the runner only reads + # off stdout - and both callers redirect stdout + print(reason, file=sys.stderr) return 1 + + sys.stdout.reconfigure(encoding="utf-8") + print(body) return 0 diff --git a/.github/scripts/resolve-version.py b/.github/scripts/resolve-version.py index bf2f8fc..14c0199 100755 --- a/.github/scripts/resolve-version.py +++ b/.github/scripts/resolve-version.py @@ -1,22 +1,19 @@ #!/usr/bin/env python3 # -# Works out which version a release run builds, and refuses the runs that cannot -# name one: +# Resolves the version a release run builds: the dispatch input, or nothing at +# all on a dry run, which builds the 0.0.0 in project.pbxproj. # -# a version input that version -# no input only a dry run, on the 0.0.0 in project.pbxproj -# -# It comes from a dispatch input rather than a tag the run was pushed on: a tag -# would be a promise made before the upload, and a version often takes more than -# one build to clear review. The workflow writes the tags afterwards instead. +# The version comes from that input rather than a tag the run was pushed on: a +# tag would be a promise made before the upload, and a version often takes more +# than one build to clear review. The workflow writes the tags afterwards. # # The shape is checked here because xcodebuild never checks it: MARKETING_VERSION # is a free-form string to the build, so a typo would only surface when App Store # Connect rejects the upload at the very end. Whether the version is above what # is live is left to the store, which is the only thing that knows. # -# Prints the resolved version and writes it to GITHUB_OUTPUT as `version`, empty -# when there is none. Run it by hand to see what a dispatch would build. +# Writes the resolved version to GITHUB_OUTPUT as `version`, empty when there is +# none. Run it by hand to see what a dispatch would build. import argparse import os @@ -39,14 +36,15 @@ def fail(message): def boolean(value): """A workflow input as it reaches a shell: the string "true" or "false".""" - if value.strip().lower() in ("true", "1"): + normalised = value.strip().lower() + if normalised in ("true", "1"): return True - if value.strip().lower() in ("false", "0", ""): + if normalised in ("false", "0", ""): return False raise ValueError(f"'{value}' is not true or false") -def resolve(given, dry_run, log=print): +def resolve(given, dry_run): """The version to build, or "" for none. Raises ValueError with the reason.""" version = given.strip() @@ -56,15 +54,16 @@ def resolve(given, dry_run, log=print): "nothing to take a version from: fill in the version input, or " "tick dry_run to build without uploading." ) - log("no version given - building the 0.0.0 in project.pbxproj") + print("no version given - building the 0.0.0 in project.pbxproj") return "" if not VERSION.match(version): raise ValueError( f"'{version}' is not a version: expected up to three numbers, like 1.36 or 1.36.1" ) + version = version.removeprefix("v") - log(f"building {version}") + print(f"building {version}") return version diff --git a/.github/workflows/build_test.yml b/.github/workflows/build_test.yml index c83a90f..c843e6f 100644 --- a/.github/workflows/build_test.yml +++ b/.github/workflows/build_test.yml @@ -8,11 +8,11 @@ on: branches: - main paths-ignore: - - '**/*.md' + - '**.md' - 'fastlane/metadata/**' pull_request: paths-ignore: - - '**/*.md' + - '**.md' - 'fastlane/metadata/**' concurrency: @@ -52,7 +52,7 @@ jobs: path: | /Users/runner/Library/Developer/Xcode/DerivedData/OpenDocumentReader-*/ - # the test job only ever builds the simulator slice of one configuration, so + # the test job only builds the simulator slice of one configuration, so # release-only and device-only breakage used to surface at deploy time build: runs-on: macos-26 @@ -71,7 +71,7 @@ jobs: xcode-version: ${{ env.xcode_version }} # signing needs secrets this workflow does not have, so this only proves - # that the device slice compiles and links + # the device slice compiles and links - name: build run: > xcodebuild diff --git a/.github/workflows/format.yml b/.github/workflows/format.yml index 28e87a2..4c996e3 100644 --- a/.github/workflows/format.yml +++ b/.github/workflows/format.yml @@ -1,9 +1,7 @@ name: format -# swift-format comes with the Xcode toolchain, so this needs no ruby and reports -# style breakage in a minute instead of after a full build. -# Unlike build_test this has no paths-ignore: every file the formatter touches -# gets checked. +# Kept apart from build_test: swift-format comes with the Xcode toolchain, so +# this reports style breakage in a minute instead of after a full build. on: workflow_dispatch: @@ -20,8 +18,7 @@ permissions: contents: read env: - # the runner image only ships the iOS platform bundle for its default Xcode; - # older ones fail in ibtool with "iOS 26.0 Platform Not Installed" + # pins swift-format, which is whatever the selected toolchain ships xcode_version: "26.5" jobs: diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 6df27b1..3e8dfd9 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -1,9 +1,7 @@ name: release -# Builds both apps once, uploads each to App Store Connect, then records what went -# out. Split in three so that "Re-run failed jobs" can repair one failed upload -# against the .ipa already built and signed. Both apps go out together and share a -# build number; tags are written afterwards, never before. See the README. +# Three jobs rather than one so that "Re-run failed jobs" repairs a failed upload +# against the .ipa already built and signed. See the README. on: workflow_dispatch: @@ -17,9 +15,8 @@ on: default: false concurrency: - # every release run, not just the ones on the same ref: the build number is a - # live query of what TestFlight has, so two overlapping runs would read the - # same one and hand the second upload a pair the store has already taken + # workflow wide, not per ref: the build number is a live query of what + # TestFlight has, so two overlapping runs would read the same one group: ${{ github.workflow }} cancel-in-progress: false @@ -43,9 +40,10 @@ jobs: SIGNING_CERTIFICATE_P12: ${{ secrets.SIGNING_CERTIFICATE_P12 }} SIGNING_CERTIFICATE_PASSWORD: ${{ secrets.SIGNING_CERTIFICATE_PASSWORD }} run: | + set -euo pipefail missing="" for name in ASC_KEY_ID ASC_ISSUER_ID ASC_KEY_CONTENT SIGNING_CERTIFICATE_P12 SIGNING_CERTIFICATE_PASSWORD; do - [ -z "${!name}" ] && missing="$missing $name" + [ -n "${!name:-}" ] || missing="$missing $name" done if [ -n "$missing" ]; then echo "::error::missing repository secrets:$missing (see README)" @@ -55,12 +53,11 @@ jobs: - name: checkout uses: actions/checkout@v7 - # up front, so a run that cannot name a version ends before the twenty - # minutes of setup and building rather than after + # before the build, so a run that cannot name a version fails in seconds - name: resolve version id: version - # through the environment rather than the run: line, where the input would - # be a shell injection + # through the environment rather than the run: line, where the input + # would be a shell injection env: given: ${{ inputs.version }} run: .github/scripts/resolve-version.py --input "$given" --dry-run "$dry_run" @@ -70,7 +67,7 @@ jobs: if: ${{ steps.version.outputs.version != '' }} env: version: ${{ steps.version.outputs.version }} - run: .github/scripts/changelog-section.py --version "$version" > /dev/null + run: .github/scripts/changelog-section.py --version "$version" - uses: ruby/setup-ruby@v1 with: @@ -86,11 +83,14 @@ jobs: SIGNING_CERTIFICATE_P12: ${{ secrets.SIGNING_CERTIFICATE_P12 }} SIGNING_CERTIFICATE_PASSWORD: ${{ secrets.SIGNING_CERTIFICATE_PASSWORD }} run: | + set -euo pipefail keychain="$RUNNER_TEMP/signing.keychain-db" password="$(uuidgen)" security create-keychain -p "$password" "$keychain" - security set-keychain-settings -lut 900 "$keychain" + # long enough to outlast both archives: a keychain that relocks mid run + # fails codesign with "User interaction is not allowed" + security set-keychain-settings -lut 21600 "$keychain" security unlock-keychain -p "$password" "$keychain" echo "$SIGNING_CERTIFICATE_P12" | base64 --decode > "$RUNNER_TEMP/certificate.p12" @@ -104,10 +104,11 @@ jobs: security set-key-partition-list -S apple-tool:,apple: -k "$password" "$keychain" > /dev/null security list-keychain -d user -s "$keychain" login.keychain-db - # this is the only certificate the build has - a development one here - # would archive fine and fail the export twenty minutes in - security find-identity -v -p codesigning "$keychain" - if ! security find-identity -v -p codesigning "$keychain" | grep -q "Apple Distribution\|iPhone Distribution"; then + # a development certificate here would archive fine and fail the export + # twenty minutes in + identities="$(security find-identity -v -p codesigning "$keychain")" + echo "$identities" + if ! grep -q "Apple Distribution\|iPhone Distribution" <<< "$identities"; then echo "::error::SIGNING_CERTIFICATE_P12 holds no Apple Distribution certificate (see README)" exit 1 fi @@ -127,21 +128,22 @@ jobs: ASC_KEY_ID: ${{ secrets.ASC_KEY_ID }} ASC_ISSUER_ID: ${{ secrets.ASC_ISSUER_ID }} ASC_KEY_CONTENT: ${{ secrets.ASC_KEY_CONTENT }} - # environment rather than lane options, which fastlane passes through - # as strings - a false would arrive as "false" and read as true ODR_VERSION: ${{ steps.version.outputs.version }} - ODR_DRY_RUN: ${{ env.dry_run }} + ODR_DRY_RUN: ${{ inputs.dry_run }} ODR_BUILD_NUMBER: ${{ steps.build_number.outputs.build_number }} - number: ${{ steps.build_number.outputs.build_number }} run: | + set -euo pipefail + # empty here means each build would ask the store for its own number + [ -n "$ODR_BUILD_NUMBER" ] || { echo "::error::resolveBuildNumber produced no build number"; exit 1; } + bundle exec fastlane ios buildPro bundle exec fastlane ios buildLite - # travels with the archives: a re-run may not repeat this job, so record - # cannot depend on its outputs - echo "$number" > build-number.txt + # travels with the archives: a re-run may skip this job, so record + # cannot read its outputs + echo "$ODR_BUILD_NUMBER" > build-number.txt - # archived on a dry run too - that is how the signing path gets exercised - - name: Artifact ipas + # on a dry run too: archiving the signed .ipa is the point of one + - name: archive the ipas uses: actions/upload-artifact@v7 with: name: ipas @@ -156,6 +158,7 @@ jobs: - name: collect distribution logs if: failure() run: | + set -euo pipefail mkdir -p distribution-logs find "${TMPDIR:-/tmp}" -maxdepth 1 -name '*.xcdistributionlogs' \ -exec cp -R {} distribution-logs/ \; @@ -191,7 +194,6 @@ jobs: with: bundler-cache: true - # back to build/, where upload_ipa looks for it - name: fetch the ipas uses: actions/download-artifact@v8 with: @@ -205,7 +207,6 @@ jobs: ASC_KEY_CONTENT: ${{ secrets.ASC_KEY_CONTENT }} run: bundle exec fastlane ios ${{ matrix.lane }} - # only once both apps are up: a half uploaded release is not recorded record: needs: upload if: ${{ !inputs.dry_run }} @@ -232,6 +233,7 @@ jobs: env: version: ${{ steps.version.outputs.version }} run: | + set -euo pipefail build_number=$(cat build-number.txt) [ -n "$build_number" ] || { echo "::error::build-number.txt is empty"; exit 1; } @@ -264,6 +266,7 @@ jobs: GH_TOKEN: ${{ github.token }} version: ${{ steps.version.outputs.version }} run: | + set -euo pipefail .github/scripts/changelog-section.py --version "$version" > "${RUNNER_TEMP}/notes.md" if gh release view "$version" > /dev/null 2>&1; then diff --git a/.gitignore b/.gitignore index d360c93..08373d5 100644 --- a/.gitignore +++ b/.gitignore @@ -62,8 +62,6 @@ fastlane/test_output .DS_Store -fastlane/report.xml - graph_info.json .venv/ __pycache__/ diff --git a/CHANGELOG.md b/CHANGELOG.md index b9c1c3f..5e92406 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,32 +1,24 @@ # Changelog -User-facing changes to OpenDocument Reader for iOS. Changes to the shared -OpenDocument core that the app picked up are listed under the release that -shipped them. - -The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - -Entries go under `Unreleased` as the change lands, in the same pull request. -This file lives on `main` and only on `main`. Nothing in the build reads it, so -`project.pbxproj` keeps its `0.0.0` and the version comes from the dispatch -input. - -The heading is cut when the release is **submitted**, in one pull request that -also writes `fastlane/metadata/en-US/changelogs/.txt`. The release run -refuses a version with no section here, and makes that section the body of the -GitHub release it drafts. - -Until the release is out the section stays open: **a second build under the same -version goes under the already cut heading, not back under `Unreleased`.** Date -the heading and point its compare link at the version tag once it is live. - -The copy that App Store Connect shows under "What's New" is a different, shorter -register. It is pasted into App Store Connect at submission time; see the README -there. +Developer-facing changes to OpenDocument Reader for iOS, in [Keep a +Changelog](https://keepachangelog.com/en/1.1.0/) format. Changes to the shared +OpenDocument core are listed under the release that shipped them. The shorter +"What's New" copy the store shows lives in +`fastlane/metadata/en-US/changelogs/`. + +Entries go under `Unreleased` in the pull request that makes the change. The +heading is cut when the release is **submitted**, in one pull request that also +writes the store copy for that version. + +A release run refuses a version with no section here, and makes that section the +body of the GitHub release it drafts. Until the release is out the section stays +open: **a second build under the same version goes under the already cut +heading, not back under `Unreleased`.** Date the heading and add its compare link +once the version tag exists. ## [Unreleased] -## [1.37] - 2026-08-02 +## [1.37] No user-facing changes. This release only changes how the app is built and submitted. @@ -86,7 +78,6 @@ submitted. - An incorrect password is now reported as such instead of a generic failure. - Page handling and decryption fixes when opening protected documents. -[Unreleased]: https://github.com/opendocument-app/OpenDocument.ios/compare/1.37...HEAD -[1.37]: https://github.com/opendocument-app/OpenDocument.ios/compare/1.36...1.37 +[Unreleased]: https://github.com/opendocument-app/OpenDocument.ios/compare/1.36...HEAD [1.36]: https://github.com/opendocument-app/OpenDocument.ios/compare/1.35...1.36 [1.35]: https://github.com/opendocument-app/OpenDocument.ios/compare/1.34...1.35 diff --git a/OpenDocumentReader.xcodeproj/project.pbxproj b/OpenDocumentReader.xcodeproj/project.pbxproj index 132e9c0..16f24ba 100644 --- a/OpenDocumentReader.xcodeproj/project.pbxproj +++ b/OpenDocumentReader.xcodeproj/project.pbxproj @@ -534,7 +534,6 @@ buildSettings = { ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon; BUNDLE_DISPLAY_NAME = "OpenDocumentReader Lite"; - BUNDLE_ID_SUFFIX = .lite; CLANG_CXX_LANGUAGE_STANDARD = "gnu++20"; CLANG_ENABLE_MODULES = YES; CODE_SIGN_STYLE = Automatic; @@ -627,7 +626,6 @@ buildSettings = { ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon; BUNDLE_DISPLAY_NAME = "OpenDocumentReader Lite"; - BUNDLE_ID_SUFFIX = .lite; CLANG_CXX_LANGUAGE_STANDARD = "gnu++20"; CLANG_ENABLE_MODULES = YES; CODE_SIGN_STYLE = Automatic; @@ -859,7 +857,6 @@ buildSettings = { ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon; BUNDLE_DISPLAY_NAME = OpenDocumentReader; - BUNDLE_ID_SUFFIX = ""; CLANG_CXX_LANGUAGE_STANDARD = "gnu++20"; CLANG_ENABLE_MODULES = YES; CODE_SIGN_STYLE = Automatic; @@ -880,7 +877,7 @@ MARKETING_VERSION = 0.0.0; MODULE_VERIFIER_SUPPORTED_LANGUAGE_STANDARDS = "gnu17 gnu++20"; ONLY_ACTIVE_ARCH = YES; - PRODUCT_BUNDLE_IDENTIFIER = "at.tomtasche.reader$(BUNDLE_ID_SUFFIX)"; + PRODUCT_BUNDLE_IDENTIFIER = at.tomtasche.reader; PRODUCT_NAME = "$(BUNDLE_DISPLAY_NAME)"; SUPPORTED_PLATFORMS = "iphoneos iphonesimulator"; SUPPORTS_MACCATALYST = NO; @@ -895,7 +892,6 @@ buildSettings = { ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon; BUNDLE_DISPLAY_NAME = OpenDocumentReader; - BUNDLE_ID_SUFFIX = ""; CLANG_CXX_LANGUAGE_STANDARD = "gnu++20"; CLANG_ENABLE_MODULES = YES; CODE_SIGN_STYLE = Automatic; @@ -914,7 +910,7 @@ ); MARKETING_VERSION = 0.0.0; MODULE_VERIFIER_SUPPORTED_LANGUAGE_STANDARDS = "gnu17 gnu++20"; - PRODUCT_BUNDLE_IDENTIFIER = "at.tomtasche.reader$(BUNDLE_ID_SUFFIX)"; + PRODUCT_BUNDLE_IDENTIFIER = at.tomtasche.reader; PRODUCT_NAME = "$(BUNDLE_DISPLAY_NAME)"; SUPPORTED_PLATFORMS = "iphoneos iphonesimulator"; SUPPORTS_MACCATALYST = NO; diff --git a/OpenDocumentReader/AppDelegate.swift b/OpenDocumentReader/AppDelegate.swift index 5e89af0..9ce438c 100644 --- a/OpenDocumentReader/AppDelegate.swift +++ b/OpenDocumentReader/AppDelegate.swift @@ -15,8 +15,8 @@ class AppDelegate: UIResponder, UIApplicationDelegate { _ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]? ) -> Bool { - // same split OpenDocument.droid uses: reporting only in the ad - // supported flavor, nothing at all in the paid one + // same split OpenDocument.droid uses: reporting in the ad supported + // flavor only let reportingEnabled = ConfigurationManager.manager.configuration == .lite AnalyticsManager.shared.setEnabled(reportingEnabled) CrashManager.shared.setEnabled(reportingEnabled) diff --git a/OpenDocumentReader/ConfigurationManager.swift b/OpenDocumentReader/ConfigurationManager.swift index 4b8933a..7694ce3 100644 --- a/OpenDocumentReader/ConfigurationManager.swift +++ b/OpenDocumentReader/ConfigurationManager.swift @@ -3,12 +3,11 @@ import Foundation struct ConfigurationManager { static let manager = ConfigurationManager() - private(set) var configuration: AppType! + let configuration: AppType private init() { + let productId = Bundle.main.bundleIdentifier?.lowercased() ?? "" - if let productId = Bundle.main.bundleIdentifier { - configuration = AppType(rawValue: productId.lowercased()) ?? .lite - } + configuration = AppType(rawValue: productId) ?? .lite } } diff --git a/OpenDocumentReader/ContentViewController.swift b/OpenDocumentReader/ContentViewController.swift index 5689c33..f961c98 100644 --- a/OpenDocumentReader/ContentViewController.swift +++ b/OpenDocumentReader/ContentViewController.swift @@ -21,6 +21,8 @@ class ContentViewController: UIViewController { var imageFile = "" var index = 0 + private var isLastPage: Bool { index == Constants.onboardingImages.count - 1 } + override func viewDidLoad() { super.viewDidLoad() @@ -36,11 +38,9 @@ class ContentViewController: UIViewController { red: 0.8039215803, green: 0.8039215803, blue: 0.8039215803, alpha: 1) } - let isLastPage = index == Constants.onboardingImages.count - 1 // only en.lproj carries these keys so far, and a missing key resolves to - // the key itself rather than to the development language. The explicit - // value keeps the other 16 localizations on the English wording the - // button hardcoded before instead of showing "intro_next". + // the key itself, so the other 16 localizations need the explicit value + // to keep showing the English wording instead of "intro_next" pageButton.setTitle( isLastPage ? NSLocalizedString("intro_start", value: "Start", comment: "onboarding button on the last page") @@ -56,14 +56,13 @@ class ContentViewController: UIViewController { } @IBAction func pageButtonPressed(_ sender: Any) { - switch index { - case 0, 1: - (parent as? PageViewController)?.nextVC(atIndex: index) - case 2: + guard !isLastPage else { dismissViewController() - default: - break + + return } + + (parent as? PageViewController)?.nextVC(atIndex: index) } @IBAction func skipButtonPressed(_ sender: UIButton) { diff --git a/OpenDocumentReader/CoreWrapper.swift b/OpenDocumentReader/CoreWrapper.swift index c09c357..0b5caea 100644 --- a/OpenDocumentReader/CoreWrapper.swift +++ b/OpenDocumentReader/CoreWrapper.swift @@ -1,12 +1,3 @@ -// -// CoreWrapper.swift -// OpenDocument Reader -// -// Replaces the ObjC++ CoreWrapper that talked to odrcore's C++ API directly. -// odrcore now arrives as the `OdrCore` Swift package, so the app links a -// prebuilt xcframework instead of building the library through conan. -// - import Foundation import OdrCore import OdrCoreObjC @@ -14,9 +5,7 @@ import OdrCoreObjC let CoreWrapperErrorDomain = "app.opendocument.CoreWrapperErrorDomain" @objc enum CoreWrapperError: Int { - /// odrcore threw something we have no specific handling for. case unknown = 1 - /// The document is encrypted and the supplied password did not open it. case wrongPassword = 2 /// Not a document odrcore can translate. PDFs land here on purpose. case unsupportedFileType = 3 @@ -37,16 +26,15 @@ private final class PageServer { private var handle: HttpServer.ServerHandle? private var translation: UInt64 = 0 - /// Bound port, or 0 when there is no server. var port: UInt32 { lock.withLock { handle?.port ?? 0 } } - /// Connects `service` under a prefix nothing has been served under before, - /// and returns the base URL for it. Nil if the socket could not be opened. + /// Connects `service` and returns the base URL its views are served under. + /// Nil if the socket could not be opened. /// - /// A fresh prefix every time because the web view caches by URL: - /// re-translating after a password or an edit has to end up at an address - /// it has not seen. - func connect(_ service: HtmlService) -> (prefix: String, base: URL)? { + /// A fresh prefix every time, because the web view caches by URL: + /// re-translating after a password or an edit has to end up at an address it + /// has not seen. + func connect(_ service: HtmlService) -> URL? { lock.lock() defer { lock.unlock() } @@ -66,7 +54,7 @@ private final class PageServer { try? server.clear() guard (try? server.connect(service, prefix: prefix)) != nil else { return nil } - return (prefix, handle.url(prefix: prefix)) + return handle.url(prefix: prefix) } } @@ -74,13 +62,9 @@ private final class PageServer { /// slide of a presentation, every page of a PDF, the entire text document. private func isCombinedView(_ view: HtmlView) -> Bool { view.name == "document" } -/// Picks the views to show as pages, the same way OpenDocument.droid does. -/// -/// Spreadsheets get one tab per sheet, because scrolling through every sheet of -/// a workbook in one page is not how anyone reads a spreadsheet. Everything else -/// gets the combined view and nothing else — a presentation would otherwise show -/// its slides twice, and a PDF would list one tab per page next to the tab that -/// already has them all. +/// The views to show as pages, the same way OpenDocument.droid picks them: a tab +/// per sheet for spreadsheets, and for everything else the combined view alone, +/// which already holds every slide or page. private func selectViews(_ views: [HtmlView], _ documentType: DocumentType) -> [HtmlView] { let isSpreadsheet = documentType == .spreadsheet let hasCombinedView = views.contains(where: isCombinedView) @@ -109,6 +93,7 @@ private func selectViews(_ views: [HtmlView], _ documentType: DocumentType) -> [ pageNames = [] pageURLs = [] + document = nil let fileTypes = (try? DecodedFile.listFileTypes(path: inputPath)) ?? [] guard !fileTypes.isEmpty else { @@ -138,7 +123,6 @@ private func selectViews(_ views: [HtmlView], _ documentType: DocumentType) -> [ let documentFile = try file.asDocumentFile() let documentType = documentFile.documentType let document = try documentFile.document() - self.document = document let config = HtmlConfig() config.editable = editable @@ -149,20 +133,21 @@ private func selectViews(_ views: [HtmlView], _ documentType: DocumentType) -> [ let service = try HtmlTranslator.translate( document: document, cachePath: cachePath, config: config) - // the views are picked before they are rendered: bringing all of them - // offline first would write out every slide of a presentation only to - // throw the files away again let views = selectViews(service.views, documentType) guard !views.isEmpty else { throw coreWrapperError(.unknown, "odrcore produced no displayable page") } - guard let connected = PageServer.shared.connect(service) else { + guard let base = PageServer.shared.connect(service) else { throw coreWrapperError(.unknown, "could not serve the translated document") } + // only once nothing can throw any more: backTranslate must not be handed + // a document whose pages were never served + self.document = document + pageNames = views.map(\.name) - pageURLs = views.map { connected.base.appendingPathComponent($0.path) } + pageURLs = views.map { base.appendingPathComponent($0.path) } } @objc func backTranslate(_ diff: String, into outputPath: String) throws { @@ -177,8 +162,8 @@ private func selectViews(_ views: [HtmlView], _ documentType: DocumentType) -> [ try document.save(to: outputPath) } - /// Whether this URL is one odrcore is serving, rather than somewhere a link - /// in the document leads. + /// Whether odrcore is serving this URL, rather than it being somewhere a + /// link in the document leads. @objc static func isServedURL(_ url: URL) -> Bool { let port = PageServer.shared.port diff --git a/OpenDocumentReader/Document.swift b/OpenDocumentReader/Document.swift index a32ff73..405eae8 100644 --- a/OpenDocumentReader/Document.swift +++ b/OpenDocumentReader/Document.swift @@ -12,7 +12,6 @@ protocol DocumentDelegate: AnyObject { enum DocumentError: Error { case getHtml - case backTranslate /// odrcore accepted the document but could not serve one of its pages. case pageNotServed } @@ -30,8 +29,8 @@ class Document: UIDocument { public var page: Int = 0 { didSet { - // every page of the document already has an address, so turning to - // one is picking a URL rather than translating the file again + // every page already has an address, so turning to one picks a URL + // rather than translating the file again showPage() } } @@ -51,31 +50,27 @@ class Document: UIDocument { public var isOdf = false private var wasPageCountAnnounced = false - override init(fileURL url: URL) { - super.init(fileURL: url) - } - override func load(fromContents contents: Any, ofType typeName: String?) throws { parse() } func parse() { - delegate?.documentLoadingStarted(self) + notify { $0.documentLoadingStarted(self) } loadProgress.completedUnitCount = 2 + isOdf = false result = nil pageURLs = nil - delegate?.documentUpdateContent(self) + notify { $0.documentUpdateContent(self) } - let cachePath = URL(fileURLWithPath: NSTemporaryDirectory()) - let outputPath = URL(fileURLWithPath: NSTemporaryDirectory()) + let temporaryDirectory = NSTemporaryDirectory() do { try coreWrapper.translate( fileURL.path, - cache: cachePath.path, - into: outputPath.path, + cache: temporaryDirectory, + into: temporaryDirectory, with: password, editable: edit ) @@ -83,11 +78,11 @@ class Document: UIDocument { where error.domain == CoreWrapperErrorDomain && error.code == CoreWrapperError.wrongPassword.rawValue { - delegate?.documentEncrypted(self) + notify { $0.documentEncrypted(self) } return } catch { - delegate?.documentLoadingError(self, error: error) + notify { $0.documentLoadingError(self, error: error) } return } @@ -103,23 +98,43 @@ class Document: UIDocument { showPage() if !wasPageCountAnnounced { - delegate?.documentPagesChanged(self) + notify { $0.documentPagesChanged(self) } wasPageCountAnnounced = true } - delegate?.documentLoadingCompleted(self) + notify { $0.documentLoadingCompleted(self) } } - /// Shows the currently selected page, clamped to what the document has: - /// switching to page five of a spreadsheet and then editing it into four - /// sheets should not walk off the end. + /// Clamped to what the document has: switching to page five of a + /// spreadsheet and then editing it into four sheets should not walk off the + /// end. private func showPage() { guard let pageURLs, !pageURLs.isEmpty else { return } result = pageURLs[min(max(page, 0), pageURLs.count - 1)] - delegate?.documentUpdateContent(self) + notify { $0.documentUpdateContent(self) } + } + + /// UIDocument reads on a background queue, so `load(fromContents:)` — and + /// with it everything `parse` reports — arrives off the main thread. Runs + /// inline when already there, so setting `page` or `edit` still updates the + /// view before returning. + private func notify(_ body: @escaping (DocumentDelegate) -> Void) { + guard !Thread.isMainThread else { + if let delegate { + body(delegate) + } + + return + } + + DispatchQueue.main.async { + if let delegate = self.delegate { + body(delegate) + } + } } override func handleError(_ error: Error, userInteractionPermitted: Bool) { @@ -131,9 +146,8 @@ class Document: UIDocument { ) throws { let diff = try generateDiff() - // CoreWrapper is guarded by @synchronized, but the document handle it - // holds is only valid together with the web view that produced the diff, - // so the call stays on the main thread + // the document handle CoreWrapper holds is only valid together with the + // web view that produced the diff, so the edit stays on the main thread try onMainThread { try coreWrapper.backTranslate(diff, into: url.path) } @@ -149,8 +163,8 @@ class Document: UIDocument { return try DispatchQueue.main.sync(execute: work) } - /// Asks the web view for the edits the user made. Runs on the main thread - /// and blocks the calling save thread until the JavaScript call comes back. + /// Blocks the calling save thread until the web view has handed back the + /// edits the user made. private func generateDiff() throws -> String { let semaphore = DispatchSemaphore(value: 0) var result: Result = .failure(DocumentError.getHtml) @@ -164,8 +178,6 @@ class Document: UIDocument { } webview.evaluateJavaScript("odr.generateDiff()") { value, error in - // signalled on every path, including the ones that used to leave - // the dispatch group unbalanced and fall into the 30s timeout defer { semaphore.signal() } if let error { @@ -195,7 +207,13 @@ class Document: UIDocument { extension Document { + /// Elided in the middle, because the interesting part of a container path is + /// at both ends and analytics only takes so much. var shortenedDocumentUrl: String { - return fileURL.absoluteString.prefix(49) + ".." + fileURL.absoluteString.suffix(49) + let url = fileURL.absoluteString + + guard url.count > 100 else { return url } + + return url.prefix(49) + ".." + url.suffix(49) } } diff --git a/OpenDocumentReader/DocumentBrowserViewController.swift b/OpenDocumentReader/DocumentBrowserViewController.swift index b908749..61db5b8 100644 --- a/OpenDocumentReader/DocumentBrowserViewController.swift +++ b/OpenDocumentReader/DocumentBrowserViewController.swift @@ -11,8 +11,6 @@ class DocumentBrowserViewController: UIDocumentBrowserViewController, UIDocument let pageViewController = "pageViewController" - var documentController: DocumentViewController? = nil - override func viewDidLoad() { super.viewDidLoad() delegate = self @@ -83,9 +81,7 @@ class DocumentBrowserViewController: UIDocumentBrowserViewController, UIDocument let storyBoard = UIStoryboard(name: "Main", bundle: nil) - if presentedViewController != nil { - presentedViewController?.dismiss(animated: false, completion: nil) - } + presentedViewController?.dismiss(animated: false, completion: nil) guard let documentViewController = @@ -96,7 +92,6 @@ class DocumentBrowserViewController: UIDocumentBrowserViewController, UIDocument return } - documentController = documentViewController documentViewController.modalPresentationCapturesStatusBarAppearance = true documentViewController.loadViewIfNeeded() @@ -110,11 +105,10 @@ class DocumentBrowserViewController: UIDocumentBrowserViewController, UIDocument documentViewController.document = doc - let shortenedDocumentUrl = documentURL.absoluteString.prefix(49) + ".." + documentURL.absoluteString.suffix(49) AnalyticsManager.shared.report( AnalyticsConstants.eventViewItem, parameters: [ - AnalyticsConstants.paramItemName: shortenedDocumentUrl + AnalyticsConstants.paramItemName: doc.shortenedDocumentUrl ]) doc.open { [weak self] success in diff --git a/OpenDocumentReader/DocumentViewController.swift b/OpenDocumentReader/DocumentViewController.swift index 04ddc34..21f51de 100644 --- a/OpenDocumentReader/DocumentViewController.swift +++ b/OpenDocumentReader/DocumentViewController.swift @@ -33,7 +33,7 @@ class DocumentViewController: UIViewController, DocumentDelegate, BannerViewDele } } - private var EXTENSION_WHITELIST = [ + private let EXTENSION_WHITELIST = [ "pdf", "doc", "docx", "xls", "xlsx", "ppt", "pptx", "rtf", "rtfd.zip", "csv", "txt", "jpg", "jpeg", "png", "gif", "svg", "pages", "pages.zip", "numbers", "numbers.zip", "key", "key.zip", "mp3", "mp4", "flv", "mkv", "3gp", "aac", "bmp", "css", "htm", "html", "js", "json", "mpeg", "oga", "ogv", "sh", "tif", "tiff", "weba", @@ -60,9 +60,7 @@ class DocumentViewController: UIViewController, DocumentDelegate, BannerViewDele public var document: Document? { didSet { - if let doc = document { - doc.delegate = self - } + document?.delegate = self } } @@ -70,21 +68,26 @@ class DocumentViewController: UIViewController, DocumentDelegate, BannerViewDele super.viewDidLoad() // once, not on every appearance: a second target would parse the - // document twice for a single tap + // document twice for a single tap, and a second set of constraints + // would fight the first pageTabBar.addTarget(self, action: #selector(pageSelected(sender:)), for: .valueChanged) - webview.navigationDelegate = self + + searchBar.delegate = self + searchBar.showsCancelButton = true + searchBarHeightWhenShown = searchBar.heightAnchor.constraint(equalToConstant: 56) + searchBarHeightWhenHidden = searchBar.heightAnchor.constraint(equalToConstant: 0) + + setVCconstraints() + hideSearchBar() + + barButtonItem.title = NSLocalizedString("back_to_documents", comment: "") } - /// odrcore renders a page when this web view asks for it, so a document - /// that only falls over halfway through translating falls over here rather - /// than in `translate`. Route that into the same handling, which shows the - /// error page or the raw file, instead of letting the server's plain text - /// "Internal Server Error" through. - /// - /// Only for the page itself: a link in the document leading somewhere that - /// answers 404, or a frame inside it doing so, is not this document failing - /// to render and must not replace it. + /// odrcore renders a page only once this web view asks for it, so a document + /// that falls over halfway through translating falls over here rather than + /// in `translate`. Only the main frame counts: a link in the document + /// answering 404 is not this document failing to render. func webView( _ webView: WKWebView, decidePolicyFor navigationResponse: WKNavigationResponse, decisionHandler: @escaping (WKNavigationResponsePolicy) -> Void @@ -102,8 +105,7 @@ class DocumentViewController: UIViewController, DocumentDelegate, BannerViewDele decisionHandler(.cancel) - // the fallback loads a file or an HTML string, neither of which comes - // back as an HTTP response, so this cannot recurse + // the fallback loads a file or an HTML string, so this cannot recurse documentLoadingError(doc, error: DocumentError.pageNotServed) } @@ -122,31 +124,22 @@ class DocumentViewController: UIViewController, DocumentDelegate, BannerViewDele override func viewWillAppear(_ animated: Bool) { super.viewWillAppear(animated) - searchBar.delegate = self - searchBar.showsCancelButton = true - - searchBarHeightWhenShown = searchBar.heightAnchor.constraint(equalToConstant: 56) - searchBarHeightWhenHidden = searchBar.heightAnchor.constraint(equalToConstant: 0) - - setVCconstraints() - hideSearchBar() + document?.webview = webview - barButtonItem.title = NSLocalizedString("back_to_documents", comment: "") + guard ConfigurationManager.manager.configuration == .lite else { + hideBannerView() - document?.webview = self.webview + return + } - if ConfigurationManager.manager.configuration == .lite { - bannerView.delegate = self - bannerView.adUnitID = "ca-app-pub-8161473686436957/8123543897" - bannerView.rootViewController = self + bannerView.delegate = self + bannerView.adUnitID = "ca-app-pub-8161473686436957/8123543897" + bannerView.rootViewController = self - ATTrackingManager.requestTrackingAuthorization(completionHandler: { _ in - DispatchQueue.main.async { - self.loadBannerAd() - } - }) - } else { - hideBannerView() + ATTrackingManager.requestTrackingAuthorization { [weak self] _ in + DispatchQueue.main.async { + self?.loadBannerAd() + } } } @@ -163,7 +156,8 @@ class DocumentViewController: UIViewController, DocumentDelegate, BannerViewDele bannerView.leadingAnchor.constraint(equalTo: view.leadingAnchor).isActive = true bannerView.trailingAnchor.constraint(equalTo: view.trailingAnchor).isActive = true bannerView.topAnchor.constraint(equalTo: searchBar.bottomAnchor).isActive = true - bannerView.heightAnchor.constraint(equalToConstant: 50).isActive = true + // no height here: that is bannerViewHeight from the storyboard, which + // hideBannerView zeroes, and a second one would fight it pageTabBar.topAnchor.constraint(equalTo: bannerView.bottomAnchor).isActive = true pageTabBar.leadingAnchor.constraint(equalTo: view.leadingAnchor).isActive = true @@ -192,10 +186,6 @@ class DocumentViewController: UIViewController, DocumentDelegate, BannerViewDele hideBannerView() } - override func viewDidAppear(_ animated: Bool) { - super.viewDidAppear(animated) - } - override func viewDidDisappear(_ animated: Bool) { super.viewDidDisappear(animated) @@ -217,15 +207,9 @@ class DocumentViewController: UIViewController, DocumentDelegate, BannerViewDele } func toggleFullscreen() { - isFullscreen = !isFullscreen + isFullscreen.toggle() - let event: String - if isFullscreen { - event = "menu_fullscreen_enter" - } else { - event = "menu_fullscreen_leave" - } - AnalyticsManager.shared.report(event) + AnalyticsManager.shared.report(isFullscreen ? "menu_fullscreen_enter" : "menu_fullscreen_leave") setNeedsStatusBarAppearanceUpdate() } @@ -269,23 +253,26 @@ class DocumentViewController: UIViewController, DocumentDelegate, BannerViewDele } private func findNext(searchText: String) { - webview?.evaluateJavaScript( - "odr.searchNext(\"" + searchText + "\")", - completionHandler: { (value: Any!, error: Error!) -> Void in - if error != nil { - CrashManager.shared.log(error) - } - }) + callSearch("odr.searchNext", with: searchText) } private func findAll(searchText: String) { - webview?.evaluateJavaScript( - "odr.search(\"" + searchText + "\")", - completionHandler: { (value: Any!, error: Error!) -> Void in - if error != nil { - CrashManager.shared.log(error) - } - }) + callSearch("odr.search", with: searchText) + } + + private func callSearch(_ function: String, with searchText: String) { + // an unescaped quote or backslash in the query would break the call + // apart rather than search for itself + let escaped = + searchText + .replacingOccurrences(of: "\\", with: "\\\\") + .replacingOccurrences(of: "\"", with: "\\\"") + + webview?.evaluateJavaScript("\(function)(\"\(escaped)\")") { _, error in + if let error { + CrashManager.shared.log(error) + } + } } @IBAction func returnToDocuments(_ sender: Any) { @@ -305,7 +292,7 @@ class DocumentViewController: UIViewController, DocumentDelegate, BannerViewDele handler: { (_) in AnalyticsManager.shared.report("alert_unsaved_changes_no") - self.discardChanges() + // nothing was written, so closing is the discard self.closeCurrentDocument() })) alert.addAction( @@ -329,15 +316,19 @@ class DocumentViewController: UIViewController, DocumentDelegate, BannerViewDele } } + /// Also reached through viewDidDisappear, so the document is dropped rather + /// than closed a second time on the way out. func closeCurrentDocument() { document?.close() + document = nil + self.dismiss(animated: true, completion: nil) } @IBAction func showMenu(_ sender: Any) { let alert = UIAlertController(title: nil, message: nil, preferredStyle: .actionSheet) - if document?.isOdf ?? false && !(document?.edit ?? false) { + if (document?.isOdf ?? false) && !(document?.edit ?? false) { alert.addAction( UIAlertAction( title: NSLocalizedString("menu_edit", comment: ""), style: .default, @@ -422,13 +413,9 @@ class DocumentViewController: UIViewController, DocumentDelegate, BannerViewDele controller: UIViewController, message: String, seconds: Double, color: UIColor? = .gray, completion: (() -> Void)? = nil ) { - let alert: UIAlertController! - - if UIDevice.current.userInterfaceIdiom == .pad { - alert = UIAlertController(title: nil, message: message, preferredStyle: .alert) - } else { - alert = UIAlertController(title: nil, message: message, preferredStyle: .actionSheet) - } + let alert = UIAlertController( + title: nil, message: message, + preferredStyle: UIDevice.current.userInterfaceIdiom == .pad ? .alert : .actionSheet) alert.view.backgroundColor = color alert.view.layer.cornerRadius = 15 @@ -464,15 +451,15 @@ class DocumentViewController: UIViewController, DocumentDelegate, BannerViewDele } func documentUpdateContent(_ doc: Document) { - guard let url = document?.result else { + guard let url = doc.result else { self.webview.loadHTMLString( "

\(NSLocalizedString("loading", comment: ""))

", baseURL: nil) return } - // odrcore serves the pages over loopback; the file variant is what the - // app falls back to when that server could not be brought up + // pages come off the loopback server; a file URL needs read access + // granted along with it if url.isFileURL { self.webview.loadFileURL(url, allowingReadAccessTo: url) } else { @@ -481,12 +468,11 @@ class DocumentViewController: UIViewController, DocumentDelegate, BannerViewDele } func documentEncrypted(_ doc: Document) { - // self.webview.loadHTMLString("

Error

Failed to load given document because it is encrypted. Feel free to contact us via tomtasche@gmail.com for further questions.", baseURL: nil) - + // the document is opened before this controller is presented, so the + // first attempt has nothing to present the prompt on if viewIfLoaded?.window == nil { - // delay because ViewController might not be visible yet - DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { - self.documentEncrypted(doc) + DispatchQueue.main.asyncAfter(deadline: .now() + 0.1) { [weak self] in + self?.documentEncrypted(doc) } return @@ -495,38 +481,36 @@ class DocumentViewController: UIViewController, DocumentDelegate, BannerViewDele let alert = UIAlertController( title: NSLocalizedString("toast_error_password_protected", comment: ""), message: "", preferredStyle: .alert ) - alert.addTextField { (textField) in + alert.addTextField { textField in textField.text = "" } alert.addAction( UIAlertAction( title: NSLocalizedString("cancel", comment: ""), style: .cancel, - handler: { [] (_) in - self.returnToDocuments("nil" as Any) + handler: { [weak self] action in + self?.returnToDocuments(action) })) alert.addAction( UIAlertAction( title: NSLocalizedString("ok", comment: ""), style: .default, - handler: { [weak alert] (_) in - self.document?.password = alert?.textFields?.first?.text ?? "" + handler: { [weak self, weak alert] _ in + self?.document?.password = alert?.textFields?.first?.text ?? "" })) self.present(alert, animated: true, completion: nil) } func documentLoadingError(_ doc: Document, error: Error) { + progressBar.isHidden = true + // attention: wrong for extensions like ".pages.zip" let fileType = doc.fileURL.pathExtension.lowercased() let fileName = doc.fileURL.absoluteString.lowercased() - for type in EXTENSION_WHITELIST { - if !fileName.hasSuffix(type) { - continue - } - + if EXTENSION_WHITELIST.contains(where: fileName.hasSuffix) { + // not odrcore's to render, but the web view knows the format self.webview.loadFileURL(doc.fileURL, allowingReadAccessTo: doc.fileURL) - progressBar.isHidden = true searchButton.isEnabled = false AnalyticsManager.shared.report( @@ -555,6 +539,10 @@ class DocumentViewController: UIViewController, DocumentDelegate, BannerViewDele func documentLoadingStarted(_ doc: Document) { progressBar.isHidden = false progressBar.observedProgress = doc.loadProgress + + // only what odrcore translated is searchable, and a later parse — after + // a password, say — may well get there + searchButton.isEnabled = true } func documentLoadingCompleted(_ doc: Document) { @@ -584,7 +572,7 @@ class DocumentViewController: UIViewController, DocumentDelegate, BannerViewDele } /// The tab bar scales with the text size, so its height is whatever it - /// currently needs rather than a fixed number. + /// currently needs. private func updatePageTabBarHeight() { pageTabBarHeight.constant = pageTabBar.isHidden ? 0 : pageTabBar.preferredHeight } diff --git a/OpenDocumentReader/NonFree/CrashManager.swift b/OpenDocumentReader/NonFree/CrashManager.swift index 31e3152..294a4d7 100644 --- a/OpenDocumentReader/NonFree/CrashManager.swift +++ b/OpenDocumentReader/NonFree/CrashManager.swift @@ -17,8 +17,7 @@ final class CrashManager { self.enabled = enabled } - /// Context attached to everything reported afterwards, the way custom keys - /// used to be attached to a Crashlytics report. + /// Context attached to everything reported afterwards. func setCustomValue(_ value: String, forKey key: String) { customValues[key] = value } @@ -30,8 +29,8 @@ final class CrashManager { } func log(_ error: Error) { - // unconditionally, so a debug session shows the failure even with - // reporting turned off + // unconditional, unlike log(String): a failure is worth seeing in a + // debug session with reporting turned off logger.error("\(String(describing: error), privacy: .public) \(self.describedContext(), privacy: .private)") } diff --git a/OpenDocumentReader/PageTabBar.swift b/OpenDocumentReader/PageTabBar.swift index 443e330..433d127 100644 --- a/OpenDocumentReader/PageTabBar.swift +++ b/OpenDocumentReader/PageTabBar.swift @@ -2,11 +2,10 @@ import UIKit /// A horizontally scrollable row of text tabs, one per document page. /// -/// This is what the document view uses to switch between the pages or sheets of -/// a document. It replaces the vendored ScrollableSegmentedControl, whose -/// upstream was archived in 2022, and covers only what the document view asked -/// of it: equal-width text tabs that share the available width while they fit, -/// scroll once they do not, and underline the selected one. +/// Replaces the vendored ScrollableSegmentedControl, whose upstream was archived +/// in 2022, and covers only what the document view asked of it: text tabs that +/// share the available width while they fit, scroll once they do not, and +/// underline the selected one. final class PageTabBar: UIControl { static let titlePadding: CGFloat = 8 static let underlineHeight: CGFloat = 4 @@ -15,9 +14,8 @@ final class PageTabBar: UIControl { /// What the tabs took before they scaled with the text size. private static let minimumHeight: CGFloat = 40 - /// How tall the row has to be for the title not to be clipped at the - /// current text size. Works out to the familiar 40 points at the default - /// size and grows from there. + /// Tall enough not to clip the title at the current text size — the familiar + /// 40 points by default, growing from there. var preferredHeight: CGFloat { let lineHeight = UIFont.preferredFont(forTextStyle: Self.titleTextStyle, compatibleWith: traitCollection) .lineHeight @@ -36,8 +34,6 @@ final class PageTabBar: UIControl { } } - /// The selected tab, or nil when nothing is selected. - /// /// Following `UISegmentedControl`, setting this does not send /// `.valueChanged` — only a tap does. var selectedIndex: Int? { @@ -85,9 +81,8 @@ final class PageTabBar: UIControl { override func layoutSubviews() { super.layoutSubviews() - // Tab widths depend on how much room there is, so a resize has to go - // through the flow layout again. Only on an actual change, or the - // invalidation would bounce back here forever. + // tab widths depend on how much room there is. only on an actual change, + // or the invalidation would bounce back here forever guard bounds.size != laidOutSize else { return } laidOutSize = bounds.size @@ -119,11 +114,10 @@ final class PageTabBar: UIControl { resizeTabs() } - /// Every tab gets an even share of the row while that is wide enough for - /// the longest title. Failing that they keep their own widths, sharing out - /// whatever is left over — so a single long sheet name is neither truncated - /// nor allowed to dictate the width of the short ones. Once they no longer - /// fit at all, the row scrolls. + /// Every tab gets an even share of the row while that is wide enough for the + /// longest title. Failing that they keep their own widths and share out + /// whatever is left over, so a single long sheet name is neither truncated + /// nor allowed to dictate the width of the short ones. private func resizeTabs() { guard let widestTitle = titleWidths.max() else { tabWidths = [] diff --git a/OpenDocumentReader/StoreReviewHelper.swift b/OpenDocumentReader/StoreReviewHelper.swift index 378359c..ef0cc2c 100644 --- a/OpenDocumentReader/StoreReviewHelper.swift +++ b/OpenDocumentReader/StoreReviewHelper.swift @@ -17,34 +17,22 @@ struct StoreReviewHelper { static let Defaults = UserDefaults.standard - static func incrementAppOpenedCount() { // called from appdelegate didfinishLaunchingWithOptions: - guard var appOpenCount = Defaults.value(forKey: UserDefaultsKeys.APP_OPENED_COUNT) as? Int else { - Defaults.set(1, forKey: UserDefaultsKeys.APP_OPENED_COUNT) - return - } - appOpenCount += 1 - Defaults.set(appOpenCount, forKey: UserDefaultsKeys.APP_OPENED_COUNT) + static func incrementAppOpenedCount() { + let key = UserDefaultsKeys.APP_OPENED_COUNT + + Defaults.set(Defaults.integer(forKey: key) + 1, forKey: key) } - static func checkAndAskForReview() { // call this whenever appropriate - // this will not be shown everytime. Apple has some internal logic on how to show this. - guard let appOpenCount = Defaults.value(forKey: UserDefaultsKeys.APP_OPENED_COUNT) as? Int else { - Defaults.set(1, forKey: UserDefaultsKeys.APP_OPENED_COUNT) - return - } + static func checkAndAskForReview() { + let appOpenCount = Defaults.integer(forKey: UserDefaultsKeys.APP_OPENED_COUNT) - switch appOpenCount { - case 3: - StoreReviewHelper().requestReview() - case _ where appOpenCount % 10 == 0: - StoreReviewHelper().requestReview() - default: - print("App run count is : \(appOpenCount)") - break - } + // asking is not the same as showing: StoreKit rate-limits the prompt + guard appOpenCount == 3 || (appOpenCount > 0 && appOpenCount % 10 == 0) else { return } + + requestReview() } - fileprivate func requestReview() { + private static func requestReview() { AnalyticsManager.shared.report("rating_show") if let scene = UIApplication.shared.connectedScenes.first(where: { $0.activationState == .foregroundActive }) diff --git a/OpenDocumentReaderTests/OpenDocumentReaderTests.swift b/OpenDocumentReaderTests/OpenDocumentReaderTests.swift index 2a7f9cc..0709b92 100644 --- a/OpenDocumentReaderTests/OpenDocumentReaderTests.swift +++ b/OpenDocumentReaderTests/OpenDocumentReaderTests.swift @@ -12,14 +12,15 @@ import XCTest @testable import OpenDocumentReader class OpenDocumentReaderTests: XCTestCase { - private var saveURL: URL! + private let temporaryDirectory = NSTemporaryDirectory() + private var documentURL: URL! override func setUpWithError() throws { - saveURL = try copyFixture(ofType: "odt") + documentURL = try copyFixture(ofType: "odt") } - /// Copies a bundled fixture next to the documents directory, because - /// translating writes its cache and output beside the input. + /// Out of the read-only test bundle, and away from the temporary directory + /// translating uses for its cache and output. private func copyFixture(ofType pathExtension: String) throws -> URL { let documentsURL = try FileManager.default.url( for: .documentDirectory, @@ -28,10 +29,7 @@ class OpenDocumentReaderTests: XCTestCase { create: false) let url = documentsURL.appendingPathComponent("test." + pathExtension) - - if FileManager.default.fileExists(atPath: url.path) { - try FileManager.default.removeItem(at: url) - } + try? FileManager.default.removeItem(at: url) let bundlePath = try XCTUnwrap( Bundle(for: Self.self).path(forResource: "test", ofType: pathExtension)) @@ -40,16 +38,11 @@ class OpenDocumentReaderTests: XCTestCase { return url } - private func makeWrapper() -> (wrapper: CoreWrapper, cache: String, output: String) { - let temporaryDirectory = NSTemporaryDirectory() - - return (CoreWrapper(), temporaryDirectory, temporaryDirectory) - } - func testTranslatesDocumentIntoPages() throws { - let (wrapper, cache, output) = makeWrapper() + let wrapper = CoreWrapper() - try wrapper.translate(saveURL.path, cache: cache, into: output, with: nil, editable: true) + try wrapper.translate( + documentURL.path, cache: temporaryDirectory, into: temporaryDirectory, with: nil, editable: true) XCTAssertFalse(wrapper.pageURLs.isEmpty) XCTAssertEqual(wrapper.pageURLs.count, wrapper.pageNames.count) @@ -57,42 +50,48 @@ class OpenDocumentReaderTests: XCTestCase { /// A text document has nothing but its combined view. func testTextDocumentIsASinglePage() throws { - let (wrapper, cache, output) = makeWrapper() + let wrapper = CoreWrapper() - try wrapper.translate(saveURL.path, cache: cache, into: output, with: nil, editable: false) + try wrapper.translate( + documentURL.path, cache: temporaryDirectory, into: temporaryDirectory, with: nil, editable: false) XCTAssertEqual(wrapper.pageNames, ["document"]) } - /// Spreadsheets are the one format that drops the combined view: a tab per - /// sheet is how a workbook is read. + /// The one format that drops the combined view: a workbook is read a sheet + /// at a time. func testSpreadsheetBecomesOnePagePerSheet() throws { - let (wrapper, cache, output) = makeWrapper() + let wrapper = CoreWrapper() let url = try copyFixture(ofType: "ods") - try wrapper.translate(url.path, cache: cache, into: output, with: nil, editable: false) + try wrapper.translate( + url.path, cache: temporaryDirectory, into: temporaryDirectory, with: nil, editable: false) XCTAssertEqual(wrapper.pageNames, ["Alpha", "Beta", "Gamma"]) } - /// The combined view of a presentation already holds every slide, so - /// listing the slides next to it would show each of them twice. + /// The combined view already holds every slide, so listing the slides next + /// to it would show each of them twice. func testPresentationKeepsOnlyTheCombinedPage() throws { - let (wrapper, cache, output) = makeWrapper() + let wrapper = CoreWrapper() let url = try copyFixture(ofType: "odp") - try wrapper.translate(url.path, cache: cache, into: output, with: nil, editable: false) + try wrapper.translate( + url.path, cache: temporaryDirectory, into: temporaryDirectory, with: nil, editable: false) XCTAssertEqual(wrapper.pageNames, ["document"]) } - /// Nothing is rendered up front any more, so the pages have to come back - /// from the loopback server odrcore is serving them on. + /// Nothing is rendered to disk up front any more, so the pages have to come + /// back off the loopback server odrcore is serving them on. func testPagesAreServedOverLoopback() throws { - let (wrapper, cache, output) = makeWrapper() + let wrapper = CoreWrapper() let url = try copyFixture(ofType: "ods") - try wrapper.translate(url.path, cache: cache, into: output, with: nil, editable: false) + try wrapper.translate( + url.path, cache: temporaryDirectory, into: temporaryDirectory, with: nil, editable: false) + + XCTAssertFalse(wrapper.pageURLs.isEmpty) for pageURL in wrapper.pageURLs { XCTAssertEqual(pageURL.scheme, "http") @@ -104,27 +103,29 @@ class OpenDocumentReaderTests: XCTestCase { } } - /// Translating again has to produce addresses the web view has not cached - /// yet - the same URL would come back out of its cache with the pages the - /// document had before the password or the edit. + /// The same URL would come back out of the web view's cache holding the + /// pages the document had before the password or the edit. func testRetranslatingMovesThePagesToNewAddresses() throws { - let (wrapper, cache, output) = makeWrapper() + let wrapper = CoreWrapper() - try wrapper.translate(saveURL.path, cache: cache, into: output, with: nil, editable: false) + try wrapper.translate( + documentURL.path, cache: temporaryDirectory, into: temporaryDirectory, with: nil, editable: false) let before = wrapper.pageURLs - try wrapper.translate(saveURL.path, cache: cache, into: output, with: nil, editable: true) + try wrapper.translate( + documentURL.path, cache: temporaryDirectory, into: temporaryDirectory, with: nil, editable: true) XCTAssertNotEqual(before, wrapper.pageURLs) } - /// The web view is the actual consumer of those URLs, and the one App - /// Transport Security applies to. A missing `NSAllowsLocalNetworking` would - /// show up here and nowhere else - `URLSession` above would still be happy. + /// A missing `NSAllowsLocalNetworking` shows up here and nowhere else: App + /// Transport Security applies to the web view, not to the `URLSession` + /// above. func testTheWebViewLoadsAServedPage() throws { - let (wrapper, cache, output) = makeWrapper() + let wrapper = CoreWrapper() - try wrapper.translate(saveURL.path, cache: cache, into: output, with: nil, editable: false) + try wrapper.translate( + documentURL.path, cache: temporaryDirectory, into: temporaryDirectory, with: nil, editable: false) let url = try XCTUnwrap(wrapper.pageURLs.first) let recorder = NavigationRecorder(finished: expectation(description: "loaded \(url)")) @@ -137,19 +138,17 @@ class OpenDocumentReaderTests: XCTestCase { XCTAssertNil(recorder.error) } - /// A failing page is treated as the document failing to render, so what - /// counts as one of our pages has to be narrow: a link in the document that - /// answers 404 must not take the document down with it. + /// A failing page is treated as the document failing to render, so a link + /// in the document that answers 404 must not take the document down too. func testOnlyTheServersOwnURLsCountAsPages() throws { - let (wrapper, cache, output) = makeWrapper() + let wrapper = CoreWrapper() - try wrapper.translate(saveURL.path, cache: cache, into: output, with: nil, editable: false) + try wrapper.translate( + documentURL.path, cache: temporaryDirectory, into: temporaryDirectory, with: nil, editable: false) let page = try XCTUnwrap(wrapper.pageURLs.first) XCTAssertTrue(CoreWrapper.isServedURL(page)) - // the port is whichever one was free, so the near misses are spelled - // relative to the one we actually got rather than a hardcoded number let port = try XCTUnwrap(page.port) for other in [ @@ -183,11 +182,12 @@ class OpenDocumentReaderTests: XCTestCase { } func testBackTranslateWritesEditedDocument() throws { - let (wrapper, cache, output) = makeWrapper() + let wrapper = CoreWrapper() - try wrapper.translate(saveURL.path, cache: cache, into: output, with: nil, editable: true) + try wrapper.translate( + documentURL.path, cache: temporaryDirectory, into: temporaryDirectory, with: nil, editable: true) - let editedURL = URL(fileURLWithPath: NSTemporaryDirectory()) + let editedURL = URL(fileURLWithPath: temporaryDirectory) .appendingPathComponent("test-edited.odt") try? FileManager.default.removeItem(at: editedURL) @@ -203,9 +203,9 @@ class OpenDocumentReaderTests: XCTestCase { /// backTranslate used to dereference an empty std::optional when nothing had /// been translated yet. func testBackTranslateWithoutTranslateFails() { - let (wrapper, _, _) = makeWrapper() + let wrapper = CoreWrapper() - let editedURL = URL(fileURLWithPath: NSTemporaryDirectory()) + let editedURL = URL(fileURLWithPath: temporaryDirectory) .appendingPathComponent("never-translated.odt") XCTAssertThrowsError(try wrapper.backTranslate("{}", into: editedURL.path)) { error in @@ -214,14 +214,15 @@ class OpenDocumentReaderTests: XCTestCase { } func testUnsupportedFileTypeReportsTypedError() throws { - let (wrapper, cache, output) = makeWrapper() + let wrapper = CoreWrapper() - let notADocument = URL(fileURLWithPath: NSTemporaryDirectory()) + let notADocument = URL(fileURLWithPath: temporaryDirectory) .appendingPathComponent("not-a-document.odt") try "definitely not an office document".write(to: notADocument, atomically: true, encoding: .utf8) XCTAssertThrowsError( - try wrapper.translate(notADocument.path, cache: cache, into: output, with: nil, editable: false) + try wrapper.translate( + notADocument.path, cache: temporaryDirectory, into: temporaryDirectory, with: nil, editable: false) ) { error in let error = error as NSError XCTAssertEqual(error.domain, CoreWrapperErrorDomain) @@ -230,10 +231,16 @@ class OpenDocumentReaderTests: XCTestCase { } func testTranslatePerformance() throws { - let (wrapper, cache, output) = makeWrapper() + let wrapper = CoreWrapper() + let path = documentURL.path + let directory = temporaryDirectory measure { - try? wrapper.translate(saveURL.path, cache: cache, into: output, with: nil, editable: true) + do { + try wrapper.translate(path, cache: directory, into: directory, with: nil, editable: true) + } catch { + XCTFail("translate threw \(error)") + } } } } diff --git a/OpenDocumentReaderTests/PageTabBarTests.swift b/OpenDocumentReaderTests/PageTabBarTests.swift index 8cabfa0..b0a46a7 100644 --- a/OpenDocumentReaderTests/PageTabBarTests.swift +++ b/OpenDocumentReaderTests/PageTabBarTests.swift @@ -11,8 +11,6 @@ class PageTabBarTests: XCTestCase { tabBar = PageTabBar(frame: CGRect(x: 0, y: 0, width: 320, height: 40)) tabBar.addTarget(self, action: #selector(valueChanged), for: .valueChanged) - - valueChangedCount = 0 } @objc private func valueChanged() { @@ -59,8 +57,8 @@ class PageTabBarTests: XCTestCase { XCTAssertNil(tabBar.selectedIndex) } - /// The document view selects the first page itself once a document is - /// loaded, and must not have that echoed back as a page change. + /// The document view selects the first page itself once a document loads, + /// and must not have that echoed back as a page change. func testProgrammaticSelectionDoesNotNotify() { tabBar.titles = ["Alpha", "Beta"] @@ -108,9 +106,8 @@ class PageTabBarTests: XCTestCase { } /// An even share is only fair while it is wide enough for the longest - /// title. "Q4 Revenue Forecast" next to "A" and "B" fits the bar - /// comfortably, but an even third of it would truncate the long one for no - /// reason. + /// title: all three of these fit the bar, but an even third would truncate + /// the long one for no reason. func testALongTitleKeepsItsWidthWhileTheyAllStillFit() throws { tabBar.titles = ["A", "B", "Q4 Revenue Forecast"] @@ -159,12 +156,8 @@ class DocumentViewControllerPageTabsTests: XCTestCase { as? DocumentViewController) viewController.loadViewIfNeeded() - // selecting a tab parses the document, so it has to be a real one - documentURL = URL(fileURLWithPath: NSTemporaryDirectory()).appendingPathComponent("page-tabs.odt") - try? FileManager.default.removeItem(at: documentURL) - - let bundlePath = try XCTUnwrap(Bundle(for: type(of: self)).path(forResource: "test", ofType: "odt")) - try FileManager.default.copyItem(at: URL(fileURLWithPath: bundlePath), to: documentURL) + documentURL = URL( + fileURLWithPath: try XCTUnwrap(Bundle(for: Self.self).path(forResource: "test", ofType: "odt"))) } private func makeDocument(pageNames: [String]) -> Document { diff --git a/OpenDocumentReaderTests/fixtures/make-fixtures.py b/OpenDocumentReaderTests/fixtures/make-fixtures.py index c0edaa6..8e09181 100644 --- a/OpenDocumentReaderTests/fixtures/make-fixtures.py +++ b/OpenDocumentReaderTests/fixtures/make-fixtures.py @@ -1,10 +1,9 @@ #!/usr/bin/env python3 """Builds the minimal ODF packages the page selection tests translate. -The sample documents we have lying around are megabytes of third party -material; these are a few hundred bytes each and exist only so a test can ask -"how many pages does a two sheet spreadsheet turn into". Run this from the -repository root when a fixture needs another sheet or slide: +A few hundred bytes each, rather than the megabytes of third party material our +sample documents are, because all a test asks of them is how many pages a two +sheet spreadsheet turns into. Rerun when a fixture needs another sheet or slide: python3 OpenDocumentReaderTests/fixtures/make-fixtures.py """ @@ -84,8 +83,7 @@ def presentation(slides: list[str]) -> str: def write(path: Path, mimetype: str, content_xml: str) -> None: with zipfile.ZipFile(path, "w", zipfile.ZIP_DEFLATED) as package: - # the mimetype entry has to come first and stay uncompressed for the - # package to be recognised without sniffing the contents + # first and uncompressed, or the package is only recognised by sniffing package.writestr( zipfile.ZipInfo("mimetype"), mimetype, compress_type=zipfile.ZIP_STORED ) @@ -96,7 +94,7 @@ def write(path: Path, mimetype: str, content_xml: str) -> None: def main() -> None: - here = Path(__file__).parent + here = Path(__file__).resolve().parent write( here.parent / "test.ods", diff --git a/README.md b/README.md index c41621c..98d9fdf 100644 --- a/README.md +++ b/README.md @@ -39,15 +39,15 @@ translation. The `` changes on every translation, because the web view caches by URL and a document re-translated after a password or an edit has to land on an address it has not seen. -`kCoreWrapperServesOverHttp` in `CoreWrapper.mm` switches back to writing the -pages out as files, which is also what happens automatically if the socket -cannot be opened at all. It is there to keep that path working while the -migration finishes, not as a runtime option. +There is no file-writing fallback: a socket that cannot be opened fails the +translate, and the document is reported as failed. The web view still loads +`file:` URLs, but only for the formats odrcore does not handle at all, which +`DocumentViewController` hands it directly. -Nothing about this needs a capability or prompts the user. A listening socket on +None of this needs a capability or prompts the user. A listening socket on loopback takes no entitlement, and the local network permission introduced in -iOS 14 covers the local subnet and multicast, not `127.0.0.1`. The one thing it -does need is an App Transport Security exception, since ATS blocks plain HTTP: +iOS 14 covers the local subnet and multicast, not `127.0.0.1`. It does need an +App Transport Security exception, since ATS blocks plain HTTP: `NSAllowsLocalNetworking` in both `Info.plist`s, which is the narrow one for local addresses and — unlike `NSAllowsArbitraryLoads` — needs no justification in App Store review. diff --git a/fastlane/Fastfile b/fastlane/Fastfile index 7715905..0d45116 100644 --- a/fastlane/Fastfile +++ b/fastlane/Fastfile @@ -1,15 +1,3 @@ -# This file contains the fastlane.tools configuration -# You can find the documentation at https://docs.fastlane.tools -# -# For a list of all available actions, check out -# -# https://docs.fastlane.tools/actions -# -# For a list of all available plugins, check out -# -# https://docs.fastlane.tools/plugins/available-plugins -# - require "base64" require "fileutils" require "shellwords" @@ -22,13 +10,9 @@ APPS = { lite: { name: "lite", scheme: "ODR Lite", app_identifier: "at.tomtasche.reader.lite1" }, }.freeze -# gitignored, and where the release workflow archives the .ipa from. -# -# Absolute, because the two halves of a Fastfile disagree about the working -# directory: fastlane parses this file and runs every lane body in fastlane/, -# and only chdirs up to the project root around an action. A bare "build" is -# therefore build/ to gym and fastlane/build/ to a plain File.exist? one line -# above it. Resolved here, while that directory is still known. +# Absolute, because lane bodies run in fastlane/ while actions run one directory +# up: a bare "build" would be fastlane/build/ to File.exist? and build/ to gym. +# Resolved at parse time, which fastlane also does from fastlane/. IPA_DIR = File.expand_path("../build").freeze def dry_run? @@ -87,8 +71,6 @@ platform :ios do end lane :tests do - # no clear_derived_data here: CI runners start clean anyway and locally it - # only throws away the incremental build of a C++ heavy project run_tests( project: "OpenDocumentReader.xcodeproj", scheme: "ODR Full", @@ -96,13 +78,9 @@ platform :ios do ) end - # An App Store Connect API key replaces the interactive Apple ID login, which - # is what kept releases tied to one person's machine. Supply it through - # ASC_KEY_ID / ASC_ISSUER_ID / ASC_KEY_CONTENT, the last being the base64 of - # the .p8 file. - # - # It is written to a private temporary file because that is the shape - # app_store_connect_api_key takes it in; every caller removes it again. + # ASC_KEY_CONTENT is the base64 of the .p8. It is written to a private + # temporary file because that is the shape app_store_connect_api_key takes it + # in; every caller removes it again. private_lane :api_key_file do path = File.join(Dir.mktmpdir("asc-api-key"), "AuthKey_#{ENV.fetch('ASC_KEY_ID')}.p8") File.write(path, Base64.decode64(ENV.fetch("ASC_KEY_CONTENT"))) @@ -120,9 +98,8 @@ platform :ios do ) end - # One above the highest either app has: they share a number, so one - # (version, build) pair names one commit in both listings. App Store Connect only - # requires the number to increase, not to be contiguous. + # One above the highest either app has, so one (version, build) pair names one + # commit in both listings. The store only requires the number to increase. private_lane :next_build_number do |options| APPS.values.map { |app| latest_testflight_build_number( @@ -133,12 +110,10 @@ platform :ios do }.max + 1 end - # ODR_VERSION is the marketing version - the dispatch input in CI, since the - # repository has no real one. ODR_BUILD_NUMBER is the number resolved once for the - # whole release; unset, this asks App Store Connect itself, as a hand run does. - # - # Both come from the environment rather than lane options, which fastlane passes - # through as strings: `dry_run:false` would arrive as "false" and read as true. + # ODR_VERSION and ODR_BUILD_NUMBER come from the environment rather than lane + # options, which fastlane passes through as strings: `dry_run:false` would + # arrive as "false" and read as true. An unset ODR_BUILD_NUMBER asks App Store + # Connect itself, as a hand run does. private_lane :build_ipa do |options| version = ENV["ODR_VERSION"].to_s.strip @@ -163,14 +138,11 @@ platform :ios do UI.message("building #{options[:scheme]} #{version.empty? ? '(unversioned)' : version} as build #{build_number}") - # Signing is manual from here on. Automatic signing has xcodebuild mint - # distribution assets of its own -- cloud signing, which only an Admin key - # may do, so the export failed with "Cloud signing permission error" while - # the imported certificate went unused. sigh downloads the App Store - # profile matching that certificate instead, and both xcodebuild passes - # are handed it. Downloading one is something any key may do; creating the - # profile, which this also does when the account has none, wants an Admin - # key - and fails here, before the build, rather than after it. + # Manual, because automatic signing has xcodebuild mint distribution assets + # of its own - cloud signing, which only an Admin key may do, and which + # left the imported certificate unused. sigh wants an Admin key only when + # the account has no profile yet and it has to create one; that fails here, + # before the build, rather than after it. profile_dir = Dir.mktmpdir("provisioning-profile") get_provisioning_profile( api_key: key, @@ -182,17 +154,14 @@ platform :ios do UI.user_error!("sigh returned no profile name") if profile_name.to_s.empty? xcargs = [ - # the archive is signed the same way the export re-signs it "CODE_SIGN_STYLE=Manual", "CODE_SIGN_IDENTITY=#{Shellwords.escape('Apple Distribution')}", "PROVISIONING_PROFILE_SPECIFIER=#{Shellwords.escape(profile_name)}", - # passed on the command line rather than written into project.pbxproj, - # so a release never leaves the working tree dirty + # on the command line, so a release never leaves the working tree dirty "CURRENT_PROJECT_VERSION=#{build_number}", ] - # left out when there is none rather than passed as 0.0.0, which is what - # project.pbxproj already says - xcargs << "MARKETING_VERSION=#{version}" unless version.empty? + # left out rather than passed as the 0.0.0 project.pbxproj already says + xcargs << "MARKETING_VERSION=#{Shellwords.escape(version)}" unless version.empty? build_app( project: "OpenDocumentReader.xcodeproj", @@ -214,9 +183,8 @@ platform :ios do end end - # Takes the .ipa built earlier rather than building one, so a failed upload can be - # retried against the same bytes. Needs no keychain and no profile: the archive is - # already signed. + # Takes the .ipa built earlier rather than building one, so a failed upload can + # be retried against the same bytes. private_lane :upload_ipa do |options| ipa = File.join(IPA_DIR, "#{options[:name]}.ipa") UI.user_error!("no #{ipa} to upload - run the matching build lane first") unless File.exist?(ipa) @@ -233,7 +201,9 @@ platform :ios do # deliberate step in App Store Connect. deliver's option is # submit_for_review; skip_submission is pilot's and is rejected here. submit_for_review: false, - precheck_include_in_app_purchases: false + # deliver runs precheck even when it is not submitting, and precheck + # refuses to run at all against in-app purchases with an API key + run_precheck_before_submit: false ) ensure FileUtils.remove_entry(File.dirname(key_path), true) diff --git a/scripts/format.sh b/scripts/format.sh index 4f759c4..8a07f15 100755 --- a/scripts/format.sh +++ b/scripts/format.sh @@ -5,9 +5,8 @@ # scripts/format.sh rewrites the sources in place # scripts/format.sh --check only reports what would change (used by CI) # -# swift-format is taken from the active Xcode toolchain, so the formatter -# version follows the Xcode version CI pins instead of being installed -# separately. +# swift-format comes from the active Xcode toolchain, so the formatter version +# follows the Xcode version CI pins. set -euo pipefail