From 8cab0661870711b3c0c60e7e460cb9a8e33f26d1 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 16 Aug 2026 09:15:17 +0200 Subject: [PATCH] Turn the pencil into a save button while editing Editing emptied the bar and moved saving into the menu. The pencil now stays and becomes a save button, as on Android. Discarding stays in the menu and now leaves edit mode. EditWorkflowTests drives the round trip through the view controller. One of its checks is an expected failure: tapping a document sets no cursor, which is odrcore's to fix, and the test will ask to be updated once it is. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UNugiHVRHVc1CGHgVGW7Dz --- CHANGELOG.md | 9 + OpenDocumentReader.xcodeproj/project.pbxproj | 4 + .../DocumentViewController.swift | 53 +++- OpenDocumentReader/Main.storyboard | 2 +- .../EditWorkflowTests.swift | 284 ++++++++++++++++++ fastlane/metadata/en-US/changelogs/1.41.txt | 1 + 6 files changed, 336 insertions(+), 17 deletions(-) create mode 100644 OpenDocumentReaderTests/EditWorkflowTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index c948b3a..23bdbee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,15 @@ once the version tag exists. for a pdf page as it does for a document. - The search button leaves the tool bar when the page cannot be searched, rather than greying out - the same as the edit button. +- The pencil turns into a save button while editing, as on Android. Saving from + the bar ends the edit and shows what was written; a save that failed stays in + it. Saving has left the menu; discarding is still there and now leaves edit + mode. + +### Known issues + +- Tapping a document sets no cursor, so edit mode cannot be typed into. The + cause is in odrcore; `EditWorkflowTests` pins it until the fix ships. ### Fixed diff --git a/OpenDocumentReader.xcodeproj/project.pbxproj b/OpenDocumentReader.xcodeproj/project.pbxproj index 2554ff5..9a3c92e 100644 --- a/OpenDocumentReader.xcodeproj/project.pbxproj +++ b/OpenDocumentReader.xcodeproj/project.pbxproj @@ -27,6 +27,7 @@ E26C39392250DC6E009C484A /* WebKit.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = E26C39382250DC6E009C484A /* WebKit.framework */; }; E2A17B0400000000000000A4 /* PageTabBarTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E2A17B0300000000000000A3 /* PageTabBarTests.swift */; }; E2A17B0500000000000000A5 /* DeclaredDocumentTypesTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E2A17B0600000000000000A6 /* DeclaredDocumentTypesTests.swift */; }; + E2A17B0700000000000000A7 /* EditWorkflowTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = E2A17B0800000000000000A8 /* EditWorkflowTests.swift */; }; E2A17B1000000000000000B0 /* test.ods in Resources */ = {isa = PBXBuildFile; fileRef = E2A17B1200000000000000B2 /* test.ods */; }; E2A17B1100000000000000B1 /* test.odp in Resources */ = {isa = PBXBuildFile; fileRef = E2A17B1300000000000000B3 /* test.odp */; }; E2A17B1400000000000000B4 /* test.csv in Resources */ = {isa = PBXBuildFile; fileRef = E2A17B1500000000000000B5 /* test.csv */; }; @@ -66,6 +67,7 @@ E26C39382250DC6E009C484A /* WebKit.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = WebKit.framework; path = System/Library/Frameworks/WebKit.framework; sourceTree = SDKROOT; }; E2A17B0300000000000000A3 /* PageTabBarTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PageTabBarTests.swift; sourceTree = ""; }; E2A17B0600000000000000A6 /* DeclaredDocumentTypesTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DeclaredDocumentTypesTests.swift; sourceTree = ""; }; + E2A17B0800000000000000A8 /* EditWorkflowTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = EditWorkflowTests.swift; sourceTree = ""; }; E2A17B1200000000000000B2 /* test.ods */ = {isa = PBXFileReference; lastKnownFileType = file; path = test.ods; sourceTree = ""; }; E2A17B1300000000000000B3 /* test.odp */ = {isa = PBXFileReference; lastKnownFileType = file; path = test.odp; sourceTree = ""; }; E2A17B1500000000000000B5 /* test.csv */ = {isa = PBXFileReference; lastKnownFileType = text; path = test.csv; sourceTree = ""; }; @@ -174,6 +176,7 @@ E22B252E2557F0E2001D0C52 /* OpenDocumentReaderTests.swift */, E2A17B0300000000000000A3 /* PageTabBarTests.swift */, E2A17B0600000000000000A6 /* DeclaredDocumentTypesTests.swift */, + E2A17B0800000000000000A8 /* EditWorkflowTests.swift */, E22B25302557F0E2001D0C52 /* Info.plist */, ); path = OpenDocumentReaderTests; @@ -392,6 +395,7 @@ E22B252F2557F0E2001D0C52 /* OpenDocumentReaderTests.swift in Sources */, E2A17B0400000000000000A4 /* PageTabBarTests.swift in Sources */, E2A17B0500000000000000A5 /* DeclaredDocumentTypesTests.swift in Sources */, + E2A17B0700000000000000A7 /* EditWorkflowTests.swift in Sources */, ); runOnlyForDeploymentPostprocessing = 0; }; diff --git a/OpenDocumentReader/DocumentViewController.swift b/OpenDocumentReader/DocumentViewController.swift index 995e560..6aefc72 100644 --- a/OpenDocumentReader/DocumentViewController.swift +++ b/OpenDocumentReader/DocumentViewController.swift @@ -67,6 +67,10 @@ class DocumentViewController: UIViewController, DocumentDelegate, UISearchBarDel /// Whether the document on screen can be edited and searched. Neither button /// stays in the bar when it cannot be used. private var canEdit = false { didSet { updateToolBar() } } + /// The same slot the pencil sits in, showing the way out of the edit it + /// started — as on OpenDocument.droid, where edit mode replaces the bar + /// rather than emptying it. + private var isEditingDocument = false { didSet { updateEditButtonRole() } } private var canSearch = false { didSet { updateToolBar() @@ -176,7 +180,7 @@ class DocumentViewController: UIViewController, DocumentDelegate, UISearchBarDel // the chevron says where it goes; the words are for VoiceOver, which is // the one reader a glyph is no shorter for barButtonItem.accessibilityLabel = NSLocalizedString("back_to_documents", comment: "") - editButton.accessibilityLabel = NSLocalizedString("menu_edit", comment: "") + updateEditButtonRole() // nothing is editable or searchable until a page says so updateToolBar() @@ -413,8 +417,21 @@ class DocumentViewController: UIViewController, DocumentDelegate, UISearchBarDel findAll(searchText: searchText) } - @IBAction func editButton(_ sender: UIBarButtonItem) { - editDocument() + /// One button, both ways: the pencil starts an edit and the save glyph ends + /// it. See ``updateEditButtonRole()``. + @IBAction func editOrSave(_ sender: UIBarButtonItem) { + if isEditingDocument { + // the file holds the edit once it is written, so leaving edit mode + // reads back what was saved. A save that failed stays in the edit, + // which is the only place that text still exists. + saveContent { success in + guard success else { return } + + self.document?.edit = false + } + } else { + editDocument() + } } private func updateToolBar() { @@ -430,10 +447,19 @@ class DocumentViewController: UIViewController, DocumentDelegate, UISearchBarDel } } - /// Offered for the documents that can be edited, and only while they are not - /// being edited already. + /// Offered for the documents that can be edited, whether or not one is being + /// edited right now — the button is the way both into an edit and out of it. private func updateEditButton() { - canEdit = (document?.isEditable ?? false) && !(document?.edit ?? false) + canEdit = document?.isEditable ?? false + isEditingDocument = document?.edit ?? false + } + + /// A pencil to start an edit, and the save glyph to write one. The label goes + /// with it: VoiceOver reads that, not the glyph. + private func updateEditButtonRole() { + editButton.image = UIImage(systemName: isEditingDocument ? "square.and.arrow.down" : "pencil") + editButton.accessibilityLabel = NSLocalizedString( + isEditingDocument ? "action_edit_save" : "menu_edit", comment: "") } /// Asked of the page rather than guessed from the format: odrcore writes the @@ -545,19 +571,12 @@ class DocumentViewController: UIViewController, DocumentDelegate, UISearchBarDel @IBAction func showMenu(_ sender: Any) { let alert = UIAlertController(title: nil, message: nil, preferredStyle: .actionSheet) - // editing is not in here: it is the pencil in the bar + // neither editing nor saving is in here: both are the one button in the bar if document?.edit ?? false { alert.addAction( UIAlertAction( - title: NSLocalizedString("action_edit_save", comment: ""), style: .default, - handler: { (_) in - self.saveContent(completion: nil) - })) - - alert.addAction( - UIAlertAction( - title: NSLocalizedString("menu_discard_changes", comment: ""), style: .default, + title: NSLocalizedString("menu_discard_changes", comment: ""), style: .destructive, handler: { (_) in self.discardChanges() })) @@ -587,10 +606,12 @@ class DocumentViewController: UIViewController, DocumentDelegate, UISearchBarDel self.present(alert, animated: true, completion: nil) } + /// Reads the document off disk again, which drops the edit, and leaves edit + /// mode with it — the only way back to reading without saving. func discardChanges() { AnalyticsManager.shared.report("menu_edit_discard") - document?.edit = true + document?.edit = false } func saveContent(completion: ((Bool) -> Void)?) { diff --git a/OpenDocumentReader/Main.storyboard b/OpenDocumentReader/Main.storyboard index 9b589da..c25899e 100644 --- a/OpenDocumentReader/Main.storyboard +++ b/OpenDocumentReader/Main.storyboard @@ -46,7 +46,7 @@ - + diff --git a/OpenDocumentReaderTests/EditWorkflowTests.swift b/OpenDocumentReaderTests/EditWorkflowTests.swift new file mode 100644 index 0000000..d094097 --- /dev/null +++ b/OpenDocumentReaderTests/EditWorkflowTests.swift @@ -0,0 +1,284 @@ +import WebKit +import XCTest + +@testable import OpenDocumentReader + +/// The whole round trip, driven through the real view controller rather than +/// through ``CoreWrapper`` alone: the bar is half of what makes editing work, +/// and it is the half that broke. +class EditWorkflowTests: XCTestCase { + private var documentURL: URL! + private var window: UIWindow! + private var controller: DocumentViewController! + private var document: Document! + + private static let editedText = "Edited by the test" + + override func setUpWithError() throws { + documentURL = try copyFixture(ofType: "odt") + try present(documentURL) + } + + override func tearDown() { + window?.isHidden = true + window = nil + controller = nil + document = nil + } + + /// 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, in: .userDomainMask, appropriateFor: nil, create: false) + + let url = documentsURL.appendingPathComponent("edit-workflow." + pathExtension) + try? FileManager.default.removeItem(at: url) + + let bundlePath = try XCTUnwrap( + Bundle(for: Self.self).path(forResource: "test", ofType: pathExtension)) + try FileManager.default.copyItem(at: URL(fileURLWithPath: bundlePath), to: url) + + return url + } + + /// On a key window, because the controller hands the web view to the + /// document as it appears and the page has to be laid out to be tapped. + private func present(_ url: URL) throws { + let storyboard = UIStoryboard(name: "Main", bundle: Bundle(for: DocumentViewController.self)) + controller = try XCTUnwrap( + storyboard.instantiateViewController(withIdentifier: "TextDocumentViewController") + as? DocumentViewController) + + document = Document(fileURL: url) + controller.document = document + + window = UIWindow(frame: CGRect(x: 0, y: 0, width: 390, height: 844)) + window.rootViewController = controller + window.makeKeyAndVisible() + + controller.view.layoutIfNeeded() + } + + // MARK: - the bar + + func testAnEditableDocumentOffersThePencil() throws { + openDocument() + + XCTAssertTrue(barContains(controller.editButton)) + XCTAssertEqual(controller.editButton.image, UIImage(systemName: "pencil")) + } + + /// What used to happen instead: the button left the bar, and saving was only + /// reachable through the menu. + func testThePencilBecomesASaveButtonWhileEditing() throws { + openDocument() + + controller.editOrSave(controller.editButton) + waitForEditablePage() + + XCTAssertTrue(document.edit) + XCTAssertTrue(barContains(controller.editButton)) + XCTAssertEqual(controller.editButton.image, UIImage(systemName: "square.and.arrow.down")) + } + + /// A document nothing can be written back to keeps the room for itself. + func testACsvOffersNoEditButton() throws { + try present(try copyFixture(ofType: "csv")) + openDocument() + + XCTAssertFalse(controller.document?.isEditable ?? true) + XCTAssertFalse(barContains(controller.editButton)) + } + + // MARK: - the page + + /// The one thing edit mode is for. odrcore marks the text runs + /// `contenteditable`, but a tap has to reach one for the caret to be set and + /// the keyboard to unfold. + func testTappingTheTextReachesTheEditableRun() throws { + openDocument() + + controller.editOrSave(controller.editButton) + waitForEditablePage() + + let tapped = + evaluate( + """ + (function () { + var run = document.querySelector('[contenteditable]'); + var box = run.getBoundingClientRect(); + var hit = document.elementFromPoint(box.left + box.width / 2, box.top + box.height / 2); + + return hit ? hit.tagName : 'none'; + })() + """) as? String + + // odrcore 6.6.0 draws `.odr-page-outer` at `z-index:-1000`, which paints + // the page behind the column holding it; hit testing reads paint order, + // so every tap lands on that column. Delete this expectation - and it + // will insist on being deleted - once we ship a core without the rule. + XCTExpectFailure("the page is painted behind .odr-pages, which swallows the tap") { + XCTAssertEqual(tapped, "X-S") + } + } + + /// Programmatic focus is not what the user does, but it proves the run is + /// editable and that only reaching it is the problem. + func testAFocusedRunTakesTheEdit() throws { + openDocument() + + controller.editOrSave(controller.editButton) + waitForEditablePage() + + typeIntoTheFirstRun() + + XCTAssertEqual(evaluate("document.querySelector('[contenteditable]').innerText") as? String, Self.editedText) + } + + // MARK: - the save + + func testSavingWritesTheEditToTheFile() throws { + openDocument() + + controller.editOrSave(controller.editButton) + waitForEditablePage() + typeIntoTheFirstRun() + + let saved = expectation(description: "saved") + controller.saveContent { success in + XCTAssertTrue(success) + saved.fulfill() + } + wait(for: [saved], timeout: 60) + + XCTAssertTrue(try reopenedText().contains(Self.editedText)) + } + + /// The save button is the way out of the edit as well as the way to write + /// it: a page left editable after a successful save has no button left to + /// end it. + func testSavingFromTheBarLeavesEditMode() throws { + openDocument() + + controller.editOrSave(controller.editButton) + waitForEditablePage() + typeIntoTheFirstRun() + + controller.editOrSave(controller.editButton) + waitForPage(where: "document.querySelectorAll('[contenteditable]').length === 0") + + XCTAssertFalse(document.edit) + XCTAssertEqual(controller.editButton.image, UIImage(systemName: "pencil")) + XCTAssertTrue(try reopenedText().contains(Self.editedText)) + } + + /// The way back to reading without saving, and the only one besides leaving + /// the document altogether. + func testDiscardingChangesLeavesEditModeAndTheFileAlone() throws { + openDocument() + + controller.editOrSave(controller.editButton) + waitForEditablePage() + typeIntoTheFirstRun() + + controller.discardChanges() + waitForPage(where: "document.querySelectorAll('x-s').length > 0") + + XCTAssertFalse(document.edit) + XCTAssertEqual(controller.editButton.image, UIImage(systemName: "pencil")) + XCTAssertFalse(try reopenedText().contains(Self.editedText)) + } + + // MARK: - helpers + + private func barContains(_ item: UIBarButtonItem) -> Bool { + (controller.toolBar.items ?? []).contains { $0 === item } + } + + private func openDocument() { + let opened = expectation(description: "opened") + document.open { success in + XCTAssertTrue(success) + opened.fulfill() + } + wait(for: [opened], timeout: 60) + + waitForPage(where: "document.querySelectorAll('x-s').length > 0") + } + + private func waitForEditablePage() { + waitForPage(where: "document.querySelectorAll('[contenteditable]').length > 0") + } + + /// The controller is its own navigation delegate — taking that away is what + /// tells the tool bar what the page can do — so the test waits on the page + /// itself rather than on `didFinish`. + private func waitForPage( + where condition: String, file: StaticString = #filePath, line: UInt = #line + ) { + let deadline = Date().addingTimeInterval(60) + + while Date() < deadline { + if evaluate(condition) as? Bool == true { return } + + _ = XCTWaiter.wait(for: [expectation(description: "a turn of the run loop")], timeout: 0.1) + } + + XCTFail("timed out waiting for \(condition)", file: file, line: line) + } + + /// A change to the text node, which is what typing amounts to: odrcore's + /// script watches for `characterData` and notes the run it belongs to. + private func typeIntoTheFirstRun() { + _ = evaluate( + """ + (function () { + var run = document.querySelector('[contenteditable]'); + run.focus(); + run.firstChild.data = '\(Self.editedText)'; + })() + """) + + // the mutation is reported in a microtask, so the diff is only complete + // on the next turn + _ = XCTWaiter.wait(for: [expectation(description: "the observer to run")], timeout: 0.5) + } + + /// Errors are swallowed: a page that is not there yet is what the polling + /// above is waiting for. + @discardableResult + private func evaluate(_ script: String) -> Any? { + let done = expectation(description: "evaluated") + var result: Any? + + controller.webview.evaluateJavaScript(script) { value, _ in + result = value + done.fulfill() + } + wait(for: [done], timeout: 30) + + return result + } + + /// The file as it now stands on disk, translated afresh. + private func reopenedText() throws -> String { + let wrapper = CoreWrapper() + let temporaryDirectory = NSTemporaryDirectory() + + try wrapper.translate( + documentURL.path, cache: temporaryDirectory, into: temporaryDirectory, with: nil, editable: false) + + let url = try XCTUnwrap(wrapper.pageURLs.first) + + var html = "" + let fetched = expectation(description: "fetched") + URLSession.shared.dataTask(with: url) { data, _, _ in + html = String(decoding: data ?? Data(), as: UTF8.self) + fetched.fulfill() + }.resume() + wait(for: [fetched], timeout: 30) + + return html + } +} diff --git a/fastlane/metadata/en-US/changelogs/1.41.txt b/fastlane/metadata/en-US/changelogs/1.41.txt index 59af5f0..1986e18 100644 --- a/fastlane/metadata/en-US/changelogs/1.41.txt +++ b/fastlane/metadata/en-US/changelogs/1.41.txt @@ -1,3 +1,4 @@ - PDFs are drawn by the app's own engine, so they fit the screen and behave like every other document - The search and edit buttons are there only for documents that can be searched or edited - LibreOffice's flat XML documents, master document templates and Excel templates can be opened from the document browser instead of being greyed out +- Editing a document turns the pencil into a save button, so saving is one tap away