Import legacy mirrors through the storage-access framework (makes #11 safe to ship)#15
Merged
Conversation
Scoped storage leaves exactly one external location writable without a permission, and this moves the mirror root to it. From API 30 on, WRITE_EXTERNAL_STORAGE grants nothing for shared storage and requestLegacyExternalStorage is ignored, so the old <external>/HTTrack/Websites root simply stops working; there is no flag that brings it back. The permission and its request go with it, being a no-op at 30 and auto-denied at 33. The engine is unaffected. Every path it sees is injected from Java (initRootPath, -O, buildTopIndex), so this is a Java-side choice; the one hardcoded outside-app path in the JNI glue is a dead pre-KitKat fallback. A persisted BasePath naming a public directory is now dropped rather than carried: mkdirs() there would merely return false and fall through to the default, which reads as the app forgetting its setting. The same check rejects one arriving from the file chooser. This does NOT migrate anything. A user with mirrors under the old public root keeps them on disk but loses sight of them; no legal read path exists in this configuration, since MediaStore does not index HTML. The import flow is a separate change, and the all-files build for sideload and F-Droid is another. FileProvider gains external-cache-path, without which browsing the help or licence pages throws "Failed to find configured root" as soon as the file:// reflection hack stops working at targetSdk 29. external-path goes, mapping storage we can no longer read, and files-path was never reachable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Review of the first commit found the getFilesDir() fallback half-wired: it was added as a root, taught to isWritableProjectPath, then denied a FileProvider entry, so browsing a mirror on the internal fallback threw "Failed to find configured root". files-path comes back. The same check also conflated "outside our storage" with "cannot tell". An ejected volume hides getExternalFilesDir(), which made a perfectly good BasePath look foreign and erased it for good, a remount not being enough to bring it back. It now answers null when it cannot decide, and only a decided no drops the setting; an undecided one falls back for the run and keeps it. Since the answer decides whether a user's setting survives, the path math moves to StoragePaths, away from the Context lookups, where it can be tested off-device: sibling directories sharing a prefix, a neighbour package, .. traversal, a symlink out of the root, and the unmounted case. Dropping the separator from either side of the comparison makes files2 match files, and the suite says so. These are the repo's first tests, so the assemble job gains testDebugUnitTest and uploads the report. Also gone: a stale comment left stranded above the new method, and the sdcardHTTrackPath extra the chooser still parsed after this stopped sending it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Signed-off-by: Xavier Roche <roche@httrack.com> # Conflicts: # .github/workflows/android.yml
Signed-off-by: Xavier Roche <roche@httrack.com> # Conflicts: # app/src/main/java/com/httrack/android/HTTrackActivity.java
The move to app-specific storage put the old public HTTrack/Websites tree out of reach, so this lets the user hand it back: a menu entry and a one-time first-launch offer open the storage-access picker, and the chosen folder is copied in. It needs no permission, which is why the framework picker is the route rather than direct file access. The copy never touches the source, so an interrupted or partial run loses nothing and can be repeated; existing destination files are skipped, and each file lands via a .part rename so a half-written one is never mistaken for complete. A picked "HTTrack" folder is descended into its "Websites" child so project directories keep their depth. The copy walk is written against a small Source interface with a DocumentFile adapter, so its fidelity, idempotency, and per-file error tolerance are unit tested off-device (the first tests this repo has for this path). The picker, the threading, and the dialogs still want a device to confirm end to end. Stacked on the app-specific-storage change; the offer is what makes shipping that safe for users with existing mirrors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Xavier Roche <roche@httrack.com>
Adversarial review found real gaps, and two of my own tests that could not
fail. Addressed:
- A second import while one runs started a second thread onto the same .part
files, racing to corrupt them. An in-flight guard turns the re-entry into a
no-op with the "importing" notice.
- fromTreeUri and the findFile("Websites") walk ran on the UI thread, one IPC
per entry: an ANR waiting to happen on a large tree. All of it moves to the
worker now.
- The result toast was posted to the activity that launched the pick, so a
rotation during a long copy swallowed it. It now posts through a main-thread
handler with the application context, and only refreshes the project list
when an activity is still there to hold it.
- The free-space check the helper carried totalSize() for was never wired; it
now runs before the copy, so a tree that would not fit is refused up front
rather than filling storage.
- A source file whose name already exists as a destination directory was
counted as "skipped", silently dropping it; it is now reported.
- The first-launch offer no longer stacks on top of the native-load failure
dialog.
Tests: the .part atomicity and the entry-name guard were both covered by
tests that passed whether or not the mechanism was present. Replaced with
ones that fail under the corresponding mutation -- a copy that writes straight
to the final name is caught by observing the file mid-write, and a guard
shrunk to only reject slashes is caught by a null entry name (which
DocumentFile.getName may return) crashing the walk.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Xavier Roche <roche@httrack.com>
Signed-off-by: Xavier Roche <roche@httrack.com> # Conflicts: # app/build.gradle
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #11. The move to app-specific storage stranded the old public
HTTrack/Websitestree, and this is the remedy that makes shipping #11 safe: the user hands the folder back through the storage-access framework and its contents are copied in.ACTION_OPEN_DOCUMENT_TREEneeds no permission, which is why it is the route rather than direct file access we no longer have.There is a menu entry ("Import existing mirrors") and a one-time first-launch offer. There is no reliable way to detect the old folder once the storage permission is gone, so the offer asks rather than asserts, and "Not now" leaves it to return while "Import" and "Don't ask again" settle it.
The copy is strictly read-only on the source, so the failure mode is benign: an interrupted or partial run loses nothing and is simply re-run. Existing destination files are skipped (idempotent, resumable), and each file is written to a
.partsibling and renamed only once whole, so a half-written file from a killed run is never mistaken for complete. If the user picks theHTTrackfolder, itsWebsiteschild is descended into so project directories keep their depth. A free-space check refuses a tree that would not fit before any copying starts.The whole operation — the tree discovery as well as the copy — runs on a worker thread, since walking a
DocumentFiletree is one IPC per entry; a second import while one is running is a no-op rather than two threads racing on the same temp files; and the result is reported through a main-thread handler with the application context, so a rotation mid-copy does not swallow it.The copy walk runs against a small
Sourceinterface with aDocumentFileadapter, so the part where correctness matters — fidelity, idempotency, per-file error tolerance, path-escape refusal, and the.partatomicity — is unit tested off-device against an in-memory tree. The two safety-critical tests were written to fail under the matching mutation (a direct-to-final-name copy, and a guard that only rejects slashes), not merely to pass. These join theStoragePathstests from #11.Still not device-verified. The picker, the actual
DocumentFilecopy against a real provider, and the dialogs rest on the API contract; no emulator is available here. The copy core is tested; the framework wiring is not. The one-time offer shows for every fresh install, since nothing can tell a new user from an upgrader without a permission we dropped — a detection signal would let it be gated, and is noted as a follow-up.Base is
storage-app-specific(#11), not master, so the diff is the import alone; retarget to master once #11 lands.