From 2540b9ce137f2a9b7f05ed1c3387bccc687013e4 Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Sat, 31 Oct 2026 21:12:13 +0100 Subject: [PATCH 1/2] Scan the file chooser's directories off the UI thread listFiles() plus a stat on every entry ran on the UI thread, so a large or slow directory held it past the ANR budget. Play reports one such ANR on vc107, a Pixel 9a parked in UnixFileSystem.list0 under FileChooserActivity.refreshDir. DirScanner does the listing on a worker thread and hands the result back through the activity's handler. Each scan takes a ticket, so a scan the next tap has superseded neither lists nor draws: a slow listing started earlier would otherwise draw over the directory that tap opened. The parent entry no longer compares the parent's path with the directory's. getParentFile() answers null at the root, which is the only case that comparison caught. Co-authored-by: Claude Opus 5 (1M context) Signed-off-by: Xavier Roche --- .../java/com/httrack/android/DirScanner.java | 104 ++++++++++ .../httrack/android/FileChooserActivity.java | 138 +++++++------ .../com/httrack/android/DirScannerTest.java | 188 ++++++++++++++++++ 3 files changed, 364 insertions(+), 66 deletions(-) create mode 100644 app/src/main/java/com/httrack/android/DirScanner.java create mode 100644 app/src/test/java/com/httrack/android/DirScannerTest.java diff --git a/app/src/main/java/com/httrack/android/DirScanner.java b/app/src/main/java/com/httrack/android/DirScanner.java new file mode 100644 index 00000000..300b39f2 --- /dev/null +++ b/app/src/main/java/com/httrack/android/DirScanner.java @@ -0,0 +1,104 @@ +package com.httrack.android; + +import java.io.File; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.Executor; +import java.util.concurrent.atomic.AtomicInteger; + +/** + * Lists directories for the file chooser away from the UI thread. Listing one and stating each of + * its entries blows past the ANR budget on a large or slow directory, and the newest scan is the + * only one worth drawing, because a slower one started earlier would otherwise draw over it. + */ +class DirScanner { + /** What one scan found. */ + static final class Listing { + /** Where the "up one level" entry leads, the filesystem root when there is nothing above. */ + final File parent; + + /** The visible sub-directories, in the order the filesystem listed them. */ + final List subdirectories; + + Listing(final File parent, final List subdirectories) { + this.parent = parent; + this.subdirectories = subdirectories; + } + } + + /** Takes the listing of the newest scan, on the UI executor. */ + interface Sink { + void onListing(Listing listing); + } + + private final Executor worker; + private final Executor ui; + + /** Counts the scans asked for, so a scan can tell whether it is still the newest. */ + private final AtomicInteger asked = new AtomicInteger(); + + DirScanner(final Executor worker, final Executor ui) { + this.worker = worker; + this.ui = ui; + } + + /** + * Scan a directory and hand the result to a sink. + * + * @param dir the directory to list, as the chooser currently has it + * @param fallback listed instead when dir is gone + * @param sink called on the UI executor, and never for a scan a later one has superseded + */ + void scan(final File dir, final File fallback, final Sink sink) { + final int ticket = asked.incrementAndGet(); + worker.execute(new Runnable() { + @Override + public void run() { + if (ticket != asked.get()) { + return; + } + final Listing listing = list(dir, fallback); + ui.execute(new Runnable() { + @Override + public void run() { + if (ticket == asked.get()) { + sink.onListing(listing); + } + } + }); + } + }); + } + + /** + * Read the parent and the visible sub-directories of a directory. + * + * @param dir the directory to list, which may be null or gone + * @param fallback listed instead when dir is gone, which leaves the parent at the root + * @return what the chooser must show + */ + Listing list(final File dir, final File fallback) { + final boolean usable = dir != null && dir.exists(); + return new Listing(parentOf(usable ? dir : null), subdirectoriesOf(usable ? dir : fallback)); + } + + /** Returns the directory above dir, or the filesystem root when there is nothing above it. */ + private static File parentOf(final File dir) { + final File parent = dir != null ? dir.getParentFile() : null; + return parent != null ? parent : new File("/"); + } + + /** Returns the sub-directories of dir the chooser offers, which excludes the hidden ones. */ + private static List subdirectoriesOf(final File dir) { + final List dirs = new ArrayList(); + final File[] list = dir != null ? dir.listFiles() : null; + if (list != null) { + for (final File file : list) { + if (file.isDirectory() && !file.isHidden()) { + dirs.add(file); + } + } + } + return dirs; + } +} diff --git a/app/src/main/java/com/httrack/android/FileChooserActivity.java b/app/src/main/java/com/httrack/android/FileChooserActivity.java index 2583289b..f49993d6 100755 --- a/app/src/main/java/com/httrack/android/FileChooserActivity.java +++ b/app/src/main/java/com/httrack/android/FileChooserActivity.java @@ -3,6 +3,7 @@ import java.io.File; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.Executor; import android.os.Bundle; import android.os.Handler; @@ -21,7 +22,7 @@ import android.widget.TextView; public class FileChooserActivity extends Activity implements - View.OnClickListener { + View.OnClickListener, DirScanner.Sink { // Borderless buttons ? protected static final boolean borderless = android.os.Build.VERSION.SDK_INT >= 11; @@ -33,6 +34,19 @@ public class FileChooserActivity extends Activity implements // Handler to execute code in UI thread private final Handler handlerUI = new Handler(); + /** Reads the directories to offer, off the UI thread. */ + private final DirScanner scanner = new DirScanner(new Executor() { + @Override + public void execute(final Runnable command) { + new Thread(command).start(); + } + }, new Executor() { + @Override + public void execute(final Runnable command) { + handlerUI.post(command); + } + }); + /* * Convert dp (Density-independent Pixels) to px (pixel) */ @@ -51,84 +65,76 @@ private void setFile(final File file) { } /* - * Create all tabs for main options menu. + * Ask for the current directory's listing. onListing draws it once it arrives. */ private void refreshDir() { - // Clear list - files.clear(); + scanner.scan(projectRootFile, defaultRootFile, this); + } - // Find parent - final File parent = projectRootFile != null && projectRootFile.exists() ? projectRootFile.getParentFile() : null; - if (parent != null && projectRootFile != null && !parent.getAbsolutePath().equals(projectRootFile.getAbsolutePath())) { - files.add(new Pair(getString(R.string.ellipsis), parent)); - Log.d("httrack", "found parent of " + projectRootFile.getAbsolutePath() + ": " + parent.getAbsolutePath()); - } else { - files.add(new Pair(getString(R.string.ellipsis), new File("/"))); - Log.d("httrack", "could not find found parent of " + projectRootFile.getAbsolutePath() + ", using /"); + /** Draws the listing as one button per entry: create all tabs for main options menu. */ + @Override + public void onListing(final DirScanner.Listing listing) { + if (isDestroyed()) { + return; } - // List directory - final File[] list = projectRootFile != null && projectRootFile.exists() ? projectRootFile - .listFiles() : defaultRootFile.listFiles(); - if (list != null) { - for (final File file : list) { - if (file.isDirectory() && !file.isHidden() /* && file.canWrite() */) { - files.add(new Pair(file.getName(), file)); - } - } + // Clear list + files.clear(); + // Up one level, then the sub-directories + files.add(new Pair(getString(R.string.ellipsis), listing.parent)); + for (final File dir : listing.subdirectories) { + files.add(new Pair(dir.getName(), dir)); } - if (files != null) { - // Create tabs - final LinearLayout scroll = LinearLayout.class - .cast(findViewById(R.id.layout)); - scroll.removeAllViews(); - - // Add all buttons linking to options - for (int i = 0; i < files.size(); i++) { - // Add separator - if (borderless && i != 0) { - final View line = new View(this, null, R.style.DividerLineHorizontal); - - // FIXME TODO: why in hell isn't my shiny style NOT working ? - final LinearLayout.MarginLayoutParams layout = new LinearLayout.MarginLayoutParams( - LayoutParams.FILL_PARENT, dpToPx(1)); - layout.bottomMargin = dpToPx(8); - layout.topMargin = dpToPx(8); - line.setLayoutParams(layout); - line.setBackgroundColor(getResources().getColor(R.color.black)); - - // Add line - scroll.addView(line); - } + // Create tabs + final LinearLayout scroll = LinearLayout.class + .cast(findViewById(R.id.layout)); + scroll.removeAllViews(); + + // Add all buttons linking to options + for (int i = 0; i < files.size(); i++) { + // Add separator + if (borderless && i != 0) { + final View line = new View(this, null, R.style.DividerLineHorizontal); + + // FIXME TODO: why in hell isn't my shiny style NOT working ? + final LinearLayout.MarginLayoutParams layout = new LinearLayout.MarginLayoutParams( + LayoutParams.FILL_PARENT, dpToPx(1)); + layout.bottomMargin = dpToPx(8); + layout.topMargin = dpToPx(8); + line.setLayoutParams(layout); + line.setBackgroundColor(getResources().getColor(R.color.black)); + + // Add line + scroll.addView(line); + } - // Next tab class - final Pair entry = files.get(i); - final String title = entry.first; - - // Create a flat button ; see - // - // Added in API level 11 - final Button button = borderless ? new Button(this, null, - android.R.attr.borderlessButtonStyle) : new Button(this); - if (borderless) { - // Left-aligned text - button.setGravity(Gravity.LEFT | Gravity.CENTER_VERTICAL); - } + // Next tab class + final Pair entry = files.get(i); + final String title = entry.first; + + // Create a flat button ; see + // + // Added in API level 11 + final Button button = borderless ? new Button(this, null, + android.R.attr.borderlessButtonStyle) : new Button(this); + if (borderless) { + // Left-aligned text + button.setGravity(Gravity.LEFT | Gravity.CENTER_VERTICAL); + } - // Title - button.setText(title); + // Title + button.setText(title); - // Set listener - button.setOnClickListener(this); + // Set listener + button.setOnClickListener(this); - // Set tag to our index - button.setTag(i); + // Set tag to our index + button.setTag(i); - // And finally add button - scroll.addView(button); - } + // And finally add button + scroll.addView(button); } } diff --git a/app/src/test/java/com/httrack/android/DirScannerTest.java b/app/src/test/java/com/httrack/android/DirScannerTest.java new file mode 100644 index 00000000..a84e9f73 --- /dev/null +++ b/app/src/test/java/com/httrack/android/DirScannerTest.java @@ -0,0 +1,188 @@ +package com.httrack.android; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.io.File; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.concurrent.Executor; +import java.util.concurrent.atomic.AtomicInteger; + +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +/** Exercises what the file chooser offers, and the rule that only the newest scan is drawn. */ +public class DirScannerTest { + @Rule + public final TemporaryFolder tmp = new TemporaryFolder(); + + /** Queues what it is handed, standing in for the worker thread and for the looper. */ + private static final class Deferred implements Executor { + private final List queued = new ArrayList(); + + @Override + public void execute(final Runnable command) { + queued.add(command); + } + + void runAll() { + final List batch = new ArrayList(queued); + queued.clear(); + for (final Runnable command : batch) { + command.run(); + } + } + } + + /** Counts the scans that really list, which a superseded one must not do. */ + private static final class CountingScanner extends DirScanner { + final AtomicInteger scans = new AtomicInteger(); + + CountingScanner(final Executor worker, final Executor ui) { + super(worker, ui); + } + + @Override + Listing list(final File dir, final File fallback) { + scans.incrementAndGet(); + return super.list(dir, fallback); + } + } + + private static final class Recorder implements DirScanner.Sink { + final List listings = new ArrayList(); + + @Override + public void onListing(final DirScanner.Listing listing) { + listings.add(listing); + } + } + + private static List names(final List files) { + final List names = new ArrayList(); + for (final File file : files) { + names.add(file.getName()); + } + Collections.sort(names); + return names; + } + + private static final Executor DIRECT = new Executor() { + @Override + public void execute(final Runnable command) { + command.run(); + } + }; + + private File folder(final String name, final String... children) throws Exception { + final File dir = tmp.newFolder(name); + for (final String child : children) { + assertTrue(new File(dir, child).mkdir()); + } + return dir; + } + + @Test + public void offersTheVisibleSubdirectoriesOnly() throws Exception { + final File dir = folder("base", "sub", ".hidden"); + assertTrue(new File(dir, "page.html").createNewFile()); + + final DirScanner.Listing listing = new DirScanner(DIRECT, DIRECT).list(dir, tmp.getRoot()); + assertEquals(Arrays.asList("sub"), names(listing.subdirectories)); + assertEquals(dir.getParentFile().getAbsolutePath(), listing.parent.getAbsolutePath()); + } + + @Test + public void theRootLeadsBackToItself() throws Exception { + final DirScanner.Listing listing = new DirScanner(DIRECT, DIRECT) + .list(new File("/"), tmp.getRoot()); + assertEquals("/", listing.parent.getAbsolutePath()); + } + + @Test + public void aDirectoryThatIsGoneFallsBackToTheDefault() throws Exception { + final File fallback = folder("default", "websites"); + final File gone = tmp.newFolder("gone"); + assertTrue(gone.delete()); + + final DirScanner.Listing listing = new DirScanner(DIRECT, DIRECT).list(gone, fallback); + assertEquals(Arrays.asList("websites"), names(listing.subdirectories)); + // Not the fallback's parent: a path that no longer exists says nothing about where it sat. + assertEquals("/", listing.parent.getAbsolutePath()); + } + + /** listFiles() answers null on anything but a directory, and that is not an empty listing. */ + @Test + public void aPlainFileOffersNothing() throws Exception { + final File file = tmp.newFile("note.txt"); + final DirScanner.Listing listing = new DirScanner(DIRECT, DIRECT).list(file, tmp.getRoot()); + assertTrue(listing.subdirectories.isEmpty()); + } + + /** The listing reads a directory created after scan() returned, so it cannot have run inline. */ + @Test + public void scanReadsTheDirectoryAfterItReturns() throws Exception { + final Deferred worker = new Deferred(); + final Deferred ui = new Deferred(); + final Recorder recorder = new Recorder(); + final File dir = folder("base"); + + new DirScanner(worker, ui).scan(dir, dir, recorder); + assertTrue(recorder.listings.isEmpty()); + assertTrue(new File(dir, "late").mkdir()); + worker.runAll(); + ui.runAll(); + + assertEquals(Arrays.asList("late"), names(recorder.listings.get(0).subdirectories)); + } + + /** A listing finished before the next tap must not draw over the directory that tap opened. */ + @Test + public void aFinishedScanIsDroppedOnceAnotherIsAsked() throws Exception { + final Deferred worker = new Deferred(); + final Deferred ui = new Deferred(); + final Recorder recorder = new Recorder(); + final DirScanner scanner = new DirScanner(worker, ui); + final File first = folder("first", "in-first"); + final File second = folder("second", "in-second"); + + scanner.scan(first, first, recorder); + worker.runAll(); + scanner.scan(second, second, recorder); + worker.runAll(); + ui.runAll(); + + assertEquals(1, recorder.listings.size()); + assertEquals(Arrays.asList("in-second"), names(recorder.listings.get(0).subdirectories)); + } + + /** A scan still queued when the next tap arrives must not list at all. */ + @Test + public void aQueuedScanIsNeverRunOnceAnotherIsAsked() throws Exception { + final Deferred worker = new Deferred(); + final Deferred ui = new Deferred(); + final Recorder recorder = new Recorder(); + final CountingScanner scanner = new CountingScanner(worker, ui); + final File first = folder("first", "in-first"); + final File second = folder("second", "in-second"); + + scanner.scan(first, first, recorder); + scanner.scan(second, second, recorder); + worker.runAll(); + ui.runAll(); + + assertEquals(1, scanner.scans.get()); + assertEquals(Arrays.asList("in-second"), names(recorder.listings.get(0).subdirectories)); + } + + @Test + public void theChooserNeverListsOnItsOwn() throws Exception { + assertFalse("listing belongs to DirScanner, which runs off the UI thread", + TestSources.javaSource("FileChooserActivity").contains("listFiles(")); + } +} From ab426bc60233385b90e1bdf6df2f9beb54af2cb1 Mon Sep 17 00:00:00 2001 From: Xavier Roche Date: Sat, 31 Oct 2026 21:27:47 +0100 Subject: [PATCH 2/2] Make a UI-thread listing impossible rather than merely untested The review found theChooserNeverListsOnItsOwn vacuous. It greps the activity for listFiles(. A refreshDir body of showDir(scanner.list(...)) passes that grep and lists on the UI thread again, which is the bug this PR exists to remove. So list() is private now, and that mutant stops at the compiler. The tests reach a listing through scan() with a direct executor instead. The counting seam moves with it. No test subclasses DirScanner any more, so the class is final like its peers here. A CountedDir over java.io.File counts the listings a superseded scan never does. It also fixes what one listing returns, so a test can pin the order the chooser offers. Three smaller findings. The two null guards no caller can reach are gone. The root fallback is one named constant. The draw is private, because a public onListing let any class in the package drive the views past the ticket check. Co-authored-by: Claude Opus 5 (1M context) Signed-off-by: Xavier Roche --- .../java/com/httrack/android/DirScanner.java | 46 ++++---- .../httrack/android/FileChooserActivity.java | 18 +-- .../com/httrack/android/DirScannerTest.java | 111 ++++++++++++------ 3 files changed, 110 insertions(+), 65 deletions(-) diff --git a/app/src/main/java/com/httrack/android/DirScanner.java b/app/src/main/java/com/httrack/android/DirScanner.java index 300b39f2..764d9d39 100644 --- a/app/src/main/java/com/httrack/android/DirScanner.java +++ b/app/src/main/java/com/httrack/android/DirScanner.java @@ -7,12 +7,14 @@ import java.util.concurrent.atomic.AtomicInteger; /** - * Lists directories for the file chooser away from the UI thread. Listing one and stating each of - * its entries blows past the ANR budget on a large or slow directory, and the newest scan is the - * only one worth drawing, because a slower one started earlier would otherwise draw over it. + * Lists directories for the file chooser off the UI thread. A stat() per entry blows past the ANR + * budget on a large or slow directory. Only the newest scan is drawn, because a slower one started + * earlier would otherwise draw over it. */ -class DirScanner { - /** What one scan found. */ +final class DirScanner { + private static final File ROOT = new File("/"); + + /** Holds what one scan found. */ static final class Listing { /** Where the "up one level" entry leads, the filesystem root when there is nothing above. */ final File parent; @@ -35,7 +37,7 @@ interface Sink { private final Executor ui; /** Counts the scans asked for, so a scan can tell whether it is still the newest. */ - private final AtomicInteger asked = new AtomicInteger(); + private final AtomicInteger newest = new AtomicInteger(); DirScanner(final Executor worker, final Executor ui) { this.worker = worker; @@ -45,23 +47,23 @@ interface Sink { /** * Scan a directory and hand the result to a sink. * - * @param dir the directory to list, as the chooser currently has it - * @param fallback listed instead when dir is gone - * @param sink called on the UI executor, and never for a scan a later one has superseded + * @param dir the directory to list, as the chooser currently has it, never null + * @param fallback listed instead when dir is gone, never null + * @param sink called on the UI executor, unless a later scan has superseded this one */ void scan(final File dir, final File fallback, final Sink sink) { - final int ticket = asked.incrementAndGet(); + final int ticket = newest.incrementAndGet(); worker.execute(new Runnable() { @Override public void run() { - if (ticket != asked.get()) { + if (ticket != newest.get()) { return; } final Listing listing = list(dir, fallback); ui.execute(new Runnable() { @Override public void run() { - if (ticket == asked.get()) { + if (ticket == newest.get()) { sink.onListing(listing); } } @@ -71,27 +73,29 @@ public void run() { } /** - * Read the parent and the visible sub-directories of a directory. + * Read the parent and the visible sub-directories of a directory. Private, so that the UI thread + * can reach a listing only through scan(). * - * @param dir the directory to list, which may be null or gone - * @param fallback listed instead when dir is gone, which leaves the parent at the root + * @param dir the directory to list, which may be gone + * @param fallback listed instead when dir is gone, which leaves the parent at the root, because + * a path that no longer exists says nothing about where it sat * @return what the chooser must show */ - Listing list(final File dir, final File fallback) { - final boolean usable = dir != null && dir.exists(); - return new Listing(parentOf(usable ? dir : null), subdirectoriesOf(usable ? dir : fallback)); + private static Listing list(final File dir, final File fallback) { + final boolean usable = dir.exists(); + return new Listing(usable ? parentOf(dir) : ROOT, subdirectoriesOf(usable ? dir : fallback)); } /** Returns the directory above dir, or the filesystem root when there is nothing above it. */ private static File parentOf(final File dir) { - final File parent = dir != null ? dir.getParentFile() : null; - return parent != null ? parent : new File("/"); + final File parent = dir.getParentFile(); + return parent != null ? parent : ROOT; } /** Returns the sub-directories of dir the chooser offers, which excludes the hidden ones. */ private static List subdirectoriesOf(final File dir) { final List dirs = new ArrayList(); - final File[] list = dir != null ? dir.listFiles() : null; + final File[] list = dir.listFiles(); if (list != null) { for (final File file : list) { if (file.isDirectory() && !file.isHidden()) { diff --git a/app/src/main/java/com/httrack/android/FileChooserActivity.java b/app/src/main/java/com/httrack/android/FileChooserActivity.java index f49993d6..861b90af 100755 --- a/app/src/main/java/com/httrack/android/FileChooserActivity.java +++ b/app/src/main/java/com/httrack/android/FileChooserActivity.java @@ -22,7 +22,7 @@ import android.widget.TextView; public class FileChooserActivity extends Activity implements - View.OnClickListener, DirScanner.Sink { + View.OnClickListener { // Borderless buttons ? protected static final boolean borderless = android.os.Build.VERSION.SDK_INT >= 11; @@ -34,7 +34,7 @@ public class FileChooserActivity extends Activity implements // Handler to execute code in UI thread private final Handler handlerUI = new Handler(); - /** Reads the directories to offer, off the UI thread. */ + /** Lists the directories to offer on a worker thread, then draws them through handlerUI. */ private final DirScanner scanner = new DirScanner(new Executor() { @Override public void execute(final Runnable command) { @@ -65,15 +65,19 @@ private void setFile(final File file) { } /* - * Ask for the current directory's listing. onListing draws it once it arrives. + * Ask for the current directory's listing. showDir draws it once it arrives. */ private void refreshDir() { - scanner.scan(projectRootFile, defaultRootFile, this); + scanner.scan(projectRootFile, defaultRootFile, new DirScanner.Sink() { + @Override + public void onListing(final DirScanner.Listing listing) { + showDir(listing); + } + }); } - /** Draws the listing as one button per entry: create all tabs for main options menu. */ - @Override - public void onListing(final DirScanner.Listing listing) { + /** Draws the listing as one button per entry. */ + private void showDir(final DirScanner.Listing listing) { if (isDestroyed()) { return; } diff --git a/app/src/test/java/com/httrack/android/DirScannerTest.java b/app/src/test/java/com/httrack/android/DirScannerTest.java index a84e9f73..dddbffdb 100644 --- a/app/src/test/java/com/httrack/android/DirScannerTest.java +++ b/app/src/test/java/com/httrack/android/DirScannerTest.java @@ -21,6 +21,13 @@ public class DirScannerTest { @Rule public final TemporaryFolder tmp = new TemporaryFolder(); + private static final Executor DIRECT = new Executor() { + @Override + public void execute(final Runnable command) { + command.run(); + } + }; + /** Queues what it is handed, standing in for the worker thread and for the looper. */ private static final class Deferred implements Executor { private final List queued = new ArrayList(); @@ -39,18 +46,29 @@ void runAll() { } } - /** Counts the scans that really list, which a superseded one must not do. */ - private static final class CountingScanner extends DirScanner { - final AtomicInteger scans = new AtomicInteger(); + /** A directory whose listing the test controls: it counts the calls and can fix what they give. */ + private static final class CountedDir extends File { + private static final long serialVersionUID = 1L; + + final AtomicInteger listings = new AtomicInteger(); + + private final File[] contents; - CountingScanner(final Executor worker, final Executor ui) { - super(worker, ui); + /** Lists the real directory, and counts. */ + CountedDir(final File dir) { + this(dir, null); + } + + /** Lists exactly these, in this order. */ + CountedDir(final File dir, final File[] contents) { + super(dir.getPath()); + this.contents = contents; } @Override - Listing list(final File dir, final File fallback) { - scans.incrementAndGet(); - return super.list(dir, fallback); + public File[] listFiles() { + listings.incrementAndGet(); + return contents != null ? contents : super.listFiles(); } } @@ -63,21 +81,26 @@ public void onListing(final DirScanner.Listing listing) { } } + /** Scans with no thread and no looper, and returns what the sink was handed. */ + private static DirScanner.Listing listed(final File dir, final File fallback) { + final Recorder recorder = new Recorder(); + new DirScanner(DIRECT, DIRECT).scan(dir, fallback, recorder); + return recorder.listings.get(0); + } + private static List names(final List files) { final List names = new ArrayList(); for (final File file : files) { names.add(file.getName()); } - Collections.sort(names); return names; } - private static final Executor DIRECT = new Executor() { - @Override - public void execute(final Runnable command) { - command.run(); - } - }; + private static List sortedNames(final List files) { + final List names = names(files); + Collections.sort(names); + return names; + } private File folder(final String name, final String... children) throws Exception { final File dir = tmp.newFolder(name); @@ -92,36 +115,45 @@ public void offersTheVisibleSubdirectoriesOnly() throws Exception { final File dir = folder("base", "sub", ".hidden"); assertTrue(new File(dir, "page.html").createNewFile()); - final DirScanner.Listing listing = new DirScanner(DIRECT, DIRECT).list(dir, tmp.getRoot()); - assertEquals(Arrays.asList("sub"), names(listing.subdirectories)); + final DirScanner.Listing listing = listed(dir, tmp.getRoot()); + assertEquals(Arrays.asList("sub"), sortedNames(listing.subdirectories)); assertEquals(dir.getParentFile().getAbsolutePath(), listing.parent.getAbsolutePath()); } + /** listFiles() answers in no particular order, and the chooser offers that order unchanged. */ + @Test + public void keepsTheOrderTheFilesystemGave() throws Exception { + final File dir = folder("base", "a", "b", "c"); + final File[] reversed = { new File(dir, "c"), new File(dir, "b"), new File(dir, "a") }; + + final DirScanner.Listing listing = listed(new CountedDir(dir, reversed), tmp.getRoot()); + assertEquals(Arrays.asList("c", "b", "a"), names(listing.subdirectories)); + } + + /** Nothing above the directory means the root, both at "/" and for a directory that is gone. */ @Test - public void theRootLeadsBackToItself() throws Exception { - final DirScanner.Listing listing = new DirScanner(DIRECT, DIRECT) - .list(new File("/"), tmp.getRoot()); - assertEquals("/", listing.parent.getAbsolutePath()); + public void theParentEntryFallsBackToTheRoot() throws Exception { + final File gone = tmp.newFolder("gone"); + assertTrue(gone.delete()); + + assertEquals("/", listed(new File("/"), tmp.getRoot()).parent.getAbsolutePath()); + assertEquals("/", listed(gone, tmp.getRoot()).parent.getAbsolutePath()); } @Test - public void aDirectoryThatIsGoneFallsBackToTheDefault() throws Exception { + public void aDirectoryThatIsGoneIsListedFromTheFallback() throws Exception { final File fallback = folder("default", "websites"); final File gone = tmp.newFolder("gone"); assertTrue(gone.delete()); - final DirScanner.Listing listing = new DirScanner(DIRECT, DIRECT).list(gone, fallback); - assertEquals(Arrays.asList("websites"), names(listing.subdirectories)); - // Not the fallback's parent: a path that no longer exists says nothing about where it sat. - assertEquals("/", listing.parent.getAbsolutePath()); + assertEquals(Arrays.asList("websites"), sortedNames(listed(gone, fallback).subdirectories)); } /** listFiles() answers null on anything but a directory, and that is not an empty listing. */ @Test public void aPlainFileOffersNothing() throws Exception { final File file = tmp.newFile("note.txt"); - final DirScanner.Listing listing = new DirScanner(DIRECT, DIRECT).list(file, tmp.getRoot()); - assertTrue(listing.subdirectories.isEmpty()); + assertTrue(listed(file, tmp.getRoot()).subdirectories.isEmpty()); } /** The listing reads a directory created after scan() returned, so it cannot have run inline. */ @@ -138,7 +170,7 @@ public void scanReadsTheDirectoryAfterItReturns() throws Exception { worker.runAll(); ui.runAll(); - assertEquals(Arrays.asList("late"), names(recorder.listings.get(0).subdirectories)); + assertEquals(Arrays.asList("late"), sortedNames(recorder.listings.get(0).subdirectories)); } /** A listing finished before the next tap must not draw over the directory that tap opened. */ @@ -158,17 +190,17 @@ public void aFinishedScanIsDroppedOnceAnotherIsAsked() throws Exception { ui.runAll(); assertEquals(1, recorder.listings.size()); - assertEquals(Arrays.asList("in-second"), names(recorder.listings.get(0).subdirectories)); + assertEquals(Arrays.asList("in-second"), sortedNames(recorder.listings.get(0).subdirectories)); } - /** A scan still queued when the next tap arrives must not list at all. */ + /** A scan still queued when the next tap arrives must not read the filesystem at all. */ @Test public void aQueuedScanIsNeverRunOnceAnotherIsAsked() throws Exception { final Deferred worker = new Deferred(); final Deferred ui = new Deferred(); final Recorder recorder = new Recorder(); - final CountingScanner scanner = new CountingScanner(worker, ui); - final File first = folder("first", "in-first"); + final DirScanner scanner = new DirScanner(worker, ui); + final CountedDir first = new CountedDir(folder("first", "in-first")); final File second = folder("second", "in-second"); scanner.scan(first, first, recorder); @@ -176,13 +208,18 @@ public void aQueuedScanIsNeverRunOnceAnotherIsAsked() throws Exception { worker.runAll(); ui.runAll(); - assertEquals(1, scanner.scans.get()); - assertEquals(Arrays.asList("in-second"), names(recorder.listings.get(0).subdirectories)); + assertEquals(0, first.listings.get()); + assertEquals(Arrays.asList("in-second"), sortedNames(recorder.listings.get(0).subdirectories)); } + /** The activity owns no listing of its own, and never draws one into a destroyed window. */ @Test - public void theChooserNeverListsOnItsOwn() throws Exception { + public void theChooserOnlyDrawsWhatTheScannerHandsIt() throws Exception { + final String source = TestSources.javaSource("FileChooserActivity"); assertFalse("listing belongs to DirScanner, which runs off the UI thread", - TestSources.javaSource("FileChooserActivity").contains("listFiles(")); + source.contains("listFiles(") || source.contains(".list()")); + assertTrue("a listing can land after onDestroy, and the views are gone by then", + TestSources.between(source, "private void showDir", "files.clear()") + .contains("isDestroyed()")); } }