Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve PDF annotations even when cache is reset #2965

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Core/Core/AppEnvironment/AppEnvironment.swift
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ open class AppEnvironment {
OfflineModeAssembly.make()
api = API(session)
currentSession = session
currentSession?.migrateSavedAnnotatedPDFs()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose to call this here, and do it per session so to be targeting those folders relevant to this specific feature & session. Thus we do it lazily if there are multiple sessions.
There was an attempt to do it on app launch for all sessions at once, but there was a difficulty fetching all sessions that have files saved to Documents directory. Not all of them get persisted to Keychain as I was expecting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The side effect of this is that non-migrated users will still lose their annotations in case someone hits the cache reset before all users logged in at least once, right? We could ask business if this is acceptable or not. Could we migrate all files based on just the directories and not relying on keychain? I think if a user logs out then we still keep its annotated pdfs but the session is removed from keychain.

userDefaults = SessionDefaults(sessionID: session.uniqueID)

if isSilent {
Expand Down
10 changes: 8 additions & 2 deletions Core/Core/AppEnvironment/CacheManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,12 @@ public class CacheManager {

clear()
LoginSession.clearAll()
clearDirectory(URL.Directories.documents) // Also clear documents, which we normally keep around

// Also clear documents, which we normally keep around
clearDirectory(
URL.Directories.documents,
excludingPaths: [URL.Paths.annotatedPDFs] // Keeping annotated PDFs
)
}

public static func clearIfNeeded() {
Expand Down Expand Up @@ -89,10 +94,11 @@ public class CacheManager {
try? manifest.write(to: manifestURL, options: .atomic)
}

private static func clearDirectory(_ directory: URL) {
private static func clearDirectory(_ directory: URL, excludingPaths: [String]? = nil) {
let fs = FileManager.default
let urls = (try? fs.contentsOfDirectory(at: directory, includingPropertiesForKeys: nil)) ?? []
for url in urls {
if let excludingPaths, excludingPaths.contains(url.lastPathComponent) { continue }
try? fs.removeItem(at: url)
}
}
Expand Down
7 changes: 7 additions & 0 deletions Core/Core/Extensions/URLExtensions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ public extension URL {
FileManager.default.urls(for: .documentDirectory, in: .userDomainMask)[0]
}

/// The `AnnotatedPDFs` directory in the application documents' private folder.
public static var annotatedPDFs: URL {
documents.appending(component: URL.Paths.annotatedPDFs, directoryHint: .isDirectory)
}

public static var library: URL {
FileManager.default.urls(for: .libraryDirectory, in: .userDomainMask)[0]
}
Expand Down Expand Up @@ -85,6 +90,8 @@ public extension URL {
}

enum Paths {
static let annotatedPDFs = "AnnotatedPDFs"

public enum Offline {

public static func rootURL(
Expand Down
2 changes: 1 addition & 1 deletion Core/Core/Files/Model/LocalFileURLCreator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ extension LocalFileURLCreator {
if mimeClass == "pdf" {
// If the user already downloaded and modified the file locally, we don't want to download it again.
// Instead, return the url pointing to the locally modified version.
let docsURL = URL.Directories.documents.appendingPathComponent(fileName)
let docsURL = URL.Directories.annotatedPDFs.appendingPathComponent(fileName)
vargaat marked this conversation as resolved.
Show resolved Hide resolved
if FileManager.default.fileExists(atPath: docsURL.path) { return docsURL }
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -685,7 +685,7 @@ extension FileDetailsViewController: PDFViewControllerDelegate, FlexibleToolbarC

public func pdfViewController(_ pdfController: PDFViewController, didSave document: Document, error: Error?) {
if pdfAnnotationsMutatedMoveToDocsDirectory, let filePathComponent = filePathComponent {
let to = URL.Directories.documents.appendingPathComponent(filePathComponent)
let to = URL.Directories.annotatedPDFs.appendingPathComponent(filePathComponent)
if !FileManager.default.fileExists(atPath: to.path), let from = document.fileURL {
do {
try FileManager.default.createDirectory(at: to.deletingLastPathComponent(), withIntermediateDirectories: true)
Expand Down
34 changes: 34 additions & 0 deletions Core/Core/Login/LoginSession.swift
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,40 @@ public struct LoginSession: Codable, Hashable {
)
}

// MARK: - Migrate Previously-saved annotated PDF documents

public func migrateSavedAnnotatedPDFs() {
let fileManager = FileManager.default

do {
// Make sure `AnnotatedPDFs` folder exists.
try fileManager.createDirectory(at: URL.Directories.annotatedPDFs, withIntermediateDirectories: true)

// Fetch `Documents` shallow contents
let urls = try fileManager.contentsOfDirectory(at: URL.Directories.documents, includingPropertiesForKeys: nil)

guard let folderUrl = urls.first(where: { $0.hasDirectoryPath && $0.lastPathComponent == uniqueID })
else { return }

let dest = URL.Directories
.annotatedPDFs
.appending(component: folderUrl.lastPathComponent, directoryHint: .isDirectory)

if fileManager.fileExists(atPath: dest.path()) {
try? fileManager
.contentsOfDirectory(at: folderUrl, includingPropertiesForKeys: nil)
.forEach({ content in
try fileManager.moveItem(at: content, to: dest.appending(component: content.lastPathComponent))
})
} else {
try? fileManager.moveItem(at: folderUrl, to: dest)
}

} catch {
Logger.shared.error("Failure moving previously saved PDFs to AnnotatedPDFs folder: \(error.localizedDescription)")
}
}

// MARK: - Persistence into keychain

public enum Key: String, CaseIterable {
Expand Down
2 changes: 2 additions & 0 deletions Core/CoreTests/AppEnvironment/CacheManagerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,12 @@ class CacheManagerTests: CoreTestCase {
func testResetAppNecessary() {
UserDefaults.standard.setValue(true, forKey: "reset_cache_on_next_launch")
let doc = write("doc", in: URL.Directories.documents)
let annotated = write("pdf content annotated", in: URL.Directories.annotatedPDFs)
LoginSession.add(LoginSession.make())
CacheManager.resetAppIfNecessary()
XCTAssertNil(UserDefaults.standard.dictionaryRepresentation()["reset_cache_on_next_launch"])
XCTAssertFalse(FileManager.default.fileExists(atPath: doc.path))
XCTAssertTrue(FileManager.default.fileExists(atPath: annotated.path))
XCTAssert(LoginSession.sessions.isEmpty)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ class FileDetailsViewControllerTests: CoreTestCase {

func mock(_ file: APIFile, isExistingPDFFileWithAnnotations: Bool = false) {
api.mock(controller.files, value: file)
let base = isExistingPDFFileWithAnnotations ? URL.Directories.documents : URL.Directories.temporary
let base = isExistingPDFFileWithAnnotations ? URL.Directories.annotatedPDFs : URL.Directories.temporary
let url = base.appendingPathComponent("\(currentSession.uniqueID)/1/\(file.filename)")
XCTAssertNoThrow(try FileManager.default.createDirectory(at: url.deletingLastPathComponent(), withIntermediateDirectories: true))
XCTAssertTrue(FileManager.default.createFile(atPath: url.path, contents: Data()))
Expand Down Expand Up @@ -230,15 +230,16 @@ class FileDetailsViewControllerTests: CoreTestCase {

func testPrepLocalURLWithExistingPDFFile() {
let fileName = "\(currentSession.uniqueID)/1/File.pdf"
let docsUrl = URL.Directories.documents.appendingPathComponent(fileName)
let docsUrl = URL.Directories.annotatedPDFs.appendingPathComponent(fileName)
mock(APIFile.make(filename: "File.pdf", contentType: "application/pdf", mime_class: "pdf"), isExistingPDFFileWithAnnotations: true)
controller.view.layoutIfNeeded()
let result = controller.prepareLocalURL(fileName: fileName, mimeClass: "pdf", location: URL.Directories.temporary)
print(result)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
print(result)

XCTAssertEqual(result, docsUrl)
}

func testMutatedPdfFileSavesToDocumentsDirectory() {
let expectedURL = URL.Directories.documents.appendingPathComponent("\(currentSession.uniqueID)/1/File.pdf")
let expectedURL = URL.Directories.annotatedPDFs.appendingPathComponent("\(currentSession.uniqueID)/1/File.pdf")
try? FileManager.default.removeItem(atPath: expectedURL.path)
XCTAssertFalse( FileManager.default.fileExists(atPath: expectedURL.path) )
DocViewerViewController.hasPSPDFKitLicense = true
Expand Down