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