5d39f4b3fb3b3c10c6317c3015672791a137aec5
18 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 SoftwareApp
|
66183a8596 |
The benchmark runs in CI, and fails when the jank metric is missing
Item 5 of the queue: .github/workflows/benchmark.yml measures on an API 35 emulator (reactivecircus/android-emulator-runner, google_apis, x86_64, SwiftShader), on demand and every Monday. Not on pull requests: a run is minutes of measurement behind twenty of booting an emulator, and the gate is meant to answer in one or two. It installs both APKs and instruments directly instead of using connectedBenchmarkAndroidTest, because that task installs the app itself - which undoes the compile reset that makes the measurement a never-run baseline. tools/benchmark-report.py turns the JSON into two tables, keeping frameCount (a length) apart from frameDurationCpuMs and frameOverrunMs (speeds), and --require-frame-duration fails the job when the second table would be empty: the metric is the reason the job exists. Driving the workflow's own script against the phone first paid for itself three times. It found that cmd package compile --reset needs adbd as root - a user build refuses it with SecurityException, so the workflow now does adb root first (google_apis allows it) instead of silently measuring a compiled app. It produced the second run of the 20 000-file open (16 frames median, 32 max) and the first numbers for scroll, search and cold start at that size: search costs 319 frames, which is now the biggest number in the file. And it showed the phone reporting frameDurationCpuMs and frameOverrunMs after all, so PERFORMANCE.md's 'this phone reports counts only' was a property of those earlier runs, not of the handset - corrected, with the numbers. |
||
|
SoftwareApp
|
3d8f351d12 |
Make the benchmark fixture reusable, since building it is the expensive part
I wrote in PERFORMANCE.md that twenty thousand files are an argument for building the fixture once and keeping it, and then deleted it after the run - twice. The script now makes keeping it the default: create leaves a folder alone when it already holds at least the requested number of files, prints what it found, and still re-grants storage because a reinstall clears app-ops. --fresh rebuilds. Verified by running --count 300 twice and watching the second call reuse. |
||
|
SoftwareApp
|
4846454645 |
Ship a baseline profile, generated on the emulator because the phone cannot collect one
The profile is real: 13193 lines in app/src/main/baseline-prof.txt, 584 of them the app's own classes, bundled into the release APK as assets/dexopt/baseline.prof. It is produced by a new :baselineprofile test module that walks the first journey - roots screen, one volume opened - under the androidx.baselineprofile plugin, and :app now depends on androidx.profileinstaller so the bundled profile is applied at install time. Two device facts decided how it was made. Collection requires API 33+ (BaselineProfileRule refuses Android 12 outright), so it ran on the API 35 AVD that was already in ~/.android/avd; that emulator is software-rendered and needs androidx.benchmark.suppressErrors=EMULATOR for any macrobenchmark. And the release build needed profileable android:shell=true, which is why app/src/release/AndroidManifest.xml exists and says why: the same APK is both the thing measured and the thing shipped, so the profile's class names are the ones in the shipped dex. The effect is not measured, and PERFORMANCE.md says so: 7404 ms median without the profile against 9005 ms with it, run to run, because a TCG emulator varies by more than a profile can buy. What remains is one run on a physical API 33+ phone, with the two compile commands documented. The emulator did settle the other open question - FrameTimingMetric there reports frameDurationCpuMs and frameOverrunMs, the metrics this phone cannot produce at all. :baselineprofile is compiled by tools/verify.sh --release like :benchmark, so it cannot rot between device runs. |
||
|
SoftwareApp
|
d243e1ba7b |
Let the benchmark fixture be any size, so the scaling question can be asked
count is now a parameter: ./tools/benchmark-fixture.sh --count 20000 builds the folder the performance doc has been promising 5000 and 20000-file numbers for. The script validates the argument rather than quietly building the default folder, which is the failure mode that would otherwise waste a run. Verified with 1500 files and then removed. |
||
|
SoftwareApp
|
b72847b822 |
Record the first complete benchmark run, and what its numbers are not
With the install watcher answering MIUI's dialog, all four tests ran and three produced numbers: coldStart 574.17 ms P50 (553.95-730.19), opening a 1000-file folder 14 frames P50 (13-17), scrolling it with four swipes 98 frames P50 (93-100). The search benchmark still fails, on the app bar's search action not being in the accessibility tree after twenty seconds - the setup wait is raised from ten to twenty now that the folder render is known to be the slow step. The frame counts are labelled as frame counts and not as jank, because this phone reports no frame durations at all: the same results file contains frameCount and nothing else, so 14 frames says the interaction is short and says nothing about whether a frame was slow. The doc also keeps the measurements that explain the watcher's shape - the dialog is FLAG_SECURE, its lifetime varies between under a second and 8.5 s, and pm install from an app uid fails earlier with MIUI's XSpace SecurityException - along with the honest cost: MIUI keeps mCurrentFocus on the installer after the window is gone, so a coordinate tap can land on the home screen, which it did. The watcher no longer reuses a saved position at all and refuses to tap within two seconds of the previous tap. |
||
|
SoftwareApp
|
456884a4d0 |
Stop the install watcher from dying quietly mid-run
You called this one: the watcher was not running any more, and the reason was mine. It ran under set -euo pipefail, and two helpers ended in a pipeline to head -1; when head exits first sed takes a SIGPIPE, pipefail turns that into a failed assignment and -e turns that into an exit. So the script answered the first dialog or two and then vanished without a word, which is exactly what a run that stops after its first install looks like. It now runs without -e, the helper pipelines end in || true, and it prints a heartbeat every thirty seconds so is it still running can be answered from the log. Measured after the fix: two consecutive uninstall/install cycles both succeeded, the script stayed alive through a 75 s watch, and in a full benchmark run it was still alive at the end with two dialogs answered. That run lasted 1m29s instead of the previous 35s and one test got as far as driving the app, so the installs do sometimes get through now. One was still refused, and the run still failed overall. |
||
|
SoftwareApp
|
904b74773d |
Answer install dialogs without ever tapping a closed one
Three things went wrong in this script and are fixed here, each with the run that showed it. It remembered a button position and tapped it without re-checking the dialog was still there, so a tap landed on the Telegram icon on the home screen behind a dialog that had already closed; it refused to answer a second dialog because it compared window tokens, and every dialog gets a new one, so the third install in a run was skipped; and its idle poll was so slow that a pair of installs could not both be caught. Now: the position is only used while an installer window is focused, re-checked immediately before the tap; a newer dialog with the same installer activity is answered rather than skipped; and after every dialog the script watches at 0.4 s for the follow-up install. Verified with two consecutive uninstall/install cycles, the second answered from the measured position five seconds after the first, and by installing the release APK with only the watcher running. The benchmark's own reinstall is still refused - two dialogs answered during one run, both with the installer focused, and pm install still returned Install canceled by user, because MIUI closes the dialog when the session is over. |
||
|
SoftwareApp
|
08b48f53fc |
Watch for MIUI's install dialog and answer it only when its button is on screen
macrobenchmark installs its target before measuring it, and on this phone MIUI puts that install behind com.miui.securitycenter/.permcenter.install.AdbInstallActivity. The test cannot answer it: CompilationMode.installPackageFromPaths runs `pm install -t ...` through Shell.executeScriptCaptureStdout, so the call blocks until the child exits and no test code is running while the dialog is up. This script runs beside the Gradle run and taps the Install button instead. It never taps blindly. Every five seconds it asks who has focus; if that is the installer, it reads the screen and taps the bounds of a button whose label is an install word - or, failing that, MIUI's own button2 id - and if no such button appears within ten seconds it taps nothing and says so. The first version guessed at the dialog's layout when the screen could not be read, which is what made it click whatever happened to be at the guessed coordinates; that code is gone. Reading the screen costs three to four seconds per dialog, which is the price of never pressing the wrong thing. Measured limits, in the script's own comment: for an install started from adb the dialog appears after 1.7 s and counts down from about ten, so a five second poll leaves time to read and tap; for the install macrobenchmark starts inside the test, the window is focused for under a second and is FLAG_SECURE (a screencap of the focused dialog shows the home screen behind it), so that dialog cannot be answered by this script or by a human. The durable fix is the MIUI developer option the dialog is named after, Install via USB. |
||
|
SoftwareApp
|
cb2db9ab63 |
Make the benchmark build the app itself, and record what the runner does to it
A separate package id for the app under test is the tidier design and cannot be used on this device: a new package id needs a human to approve the install every time, and installing by tapping the APK left the package listed by pm list packages yet unknown to pm enable and invisible to PackageManager. The benchmark build type therefore keeps the app's own id and takes the release key, so it updates an installed release build and keeps its data. The trade-off is stated where it happens: this build replaces a release install, and reinstalling the release APK puts it back. Two bugs surfaced on the way and both are now commented at the line they belong to. -PbenchmarkBuild was read with providers.gradleProperty, which reported it absent, so the runner installed the debug APK and failed on it; hasProperty is what works. And the reason am instrument cannot be used for this module is now recorded: MacrobenchmarkRule uninstalls the target at the start of a run and reinstalls it from the APK paths the Gradle runner passes in, so without those the app is uninstalled and left that way - which is what produced NameNotFoundException for an app that was installed a second earlier, twice, with Benchmark: Uninstalling in logcat to prove it. coldStart keeps its measured number (532 ms median). The three interaction benchmarks remain unmeasured for a reason that is now written down: each run needs one tap on the phone for the reinstall MIUI gates. Gate green, map current. |
||
|
SoftwareApp
|
94f85bd287 |
Get a real cold-start number out of the benchmark, and record what the device demanded
coldStart measures 532 ms median (min 499, max 805) on the benchmark build - R8, not debuggable, profileable, CompilationMode.None - against 1814 ms on the debug build, which is the honest illustration of why a debug number must never be published as the app's performance. The tool also warns that it cannot clear the runtime image on this Android version, so the figure is optimistic by an unmeasured amount; that is in PERFORMANCE.md next to the number rather than in a footnote. The three interaction benchmarks are written and compile but have not produced a number, and getting that far took four fixes worth naming. The app under test cannot keep the app's package id (signature) and cannot use .benchmark (the test APK owns that id, INSTALL_FAILED_VERSION_DOWNGRADE); macrobenchmark needs package visibility for it or it reports NameNotFoundException for an app that pm list packages shows installed; UiAutomation.executeShellCommand runs as the test app's uid, so the fixture has to be made through adb instead - that one cost the first attempt a hang; and testBuildType has to be behind a flag or :app:testDebugUnitTest stops existing. The workflow that works on a device with an install gate is now documented: install the two APKs by hand once, run tools/benchmark-fixture.sh, then am instrument. The Gradle task is documented as the alternative and as the reason the app under test disappeared once - it uninstalls around the run. Gate green, map current, 386 tests unchanged. |
||
|
SoftwareApp
|
1f94d0a38a |
Add a macrobenchmark module, and find out what the build was hiding
docs/PERFORMANCE.md has been honest that three of its four numbers do not exist. They exist now as measurements, if not yet as results: benchmark/ is a macrobenchmark module with four tests - cold start with StartupTimingMetric, then opening, scrolling and searching a folder of a thousand files with FrameTimingMetric - sharing a fixture at /sdcard/AABenchmark that is created once per run and deleted in @AfterClass. Frames rather than a named trace section on purpose: what a user calls slow is jank on screen, and the alternative puts benchmarking calls into production code for the sake of a number. CompilationMode.None() is the honest cold-install baseline; Partial() plus a baseline profile is the follow-up and needs the same module. The storage grant is applied with appops in setup, which is what lets the run be unattended - a permission dialog is the one thing a test cannot answer. Three build facts had to be discovered rather than designed, and each is now written where it will be read. The app under test cannot keep its own package id: it needs a debug signature to install on a test device, so it cannot replace a release install, and an un-suffixed build is exactly that. It also cannot use the obvious suffix: `.benchmark` is already the benchmark module's own APK id, and the test APK then fails to install with INSTALL_FAILED_VERSION_DOWNGRADE - which is what the first attempt did. Hence `.perf`. And `testBuildType = "benchmark"` cannot be set unconditionally, because it makes benchmark the variant that *unit* tests run against and :app:testDebugUnitTest - the task the entire gate is built on - stops existing; it is behind -PbenchmarkBuild instead. All three were measured, not guessed, and the third would have broken CI silently. Two smaller things came with it. tools/verify.sh --release now also compiles the app's benchmark variant and the test APK, because build wiring nobody runs is wiring that rots; no device is involved. And the code map grew an EXTRA_ROOTS section so it indexes benchmark/ too - a map that silently omits a module lies by omission, which is worse than the one line it costs. The numbers themselves are still missing, and for a device reason rather than a code one: installing the new package id on this MIUI phone fails with INSTALL_FAILED_USER_RESTRICTED (the "Install via USB" gate, which wants a human and answered too late). Both APKs are pushed to /sdcard/Download/ for a manual install, and the run is one command away: `adb shell am instrument -w -e class de.softwareapp_hb.filemanager.benchmark.BrowserBenchmark de.softwareapp_hb.filemanager.benchmark/androidx.test.runner.AndroidJUnitRunner`. Gate green with --release, including the new compile step; map current; 386 tests unchanged. |
||
|
SoftwareApp
|
0bbee25ab3 |
Test the interface as a user sees it, and let a debug build tell on itself
Fifteen rendering tests across four files are the first tests in this project that compose a screen, and they cover the claims that used to be prose only: the app bar's star exists where there is a folder to star and says Add or Remove according to the state, the selection bar offers Rename to several files and Details only to one, the batch dialog spells out the names it will produce and refuses a collision with the file named, the shared name dialog refuses a slash while it is typed, and a favourite that has gone says "Folder not found" rather than showing a path that is not there. They run on the JVM through Robolectric - the Compose test artifacts plus the test manifest - so they belong to the same verify.sh gate as everything else instead of needing a device: 386 tests in total. Writing them turned up one behaviour worth recording. The batch dialog continues the folder's series by default, so a collision cannot be shown unless the start number is forced back to 1 - which is precisely why the feature works in practice, and now also why two of these tests set the field explicitly. The debug build now enables StrictMode (disk reads and writes, network, leaked closables; logged, not fatal) and attaches LeakCanary. A file manager is the worst possible app for a blocking disk read on the main thread, and a debug build is the last place where noticing it is still cheap. Logging rather than a crash is deliberate: turning a slow screen into a crash on someone's phone is the worse trade. The flag is read from the manifest, not from BuildConfig, which this build does not generate. Performance has a number for the first time, and it immediately paid for itself. tools/measure-startup.sh takes cold start with `am start -W` - median of several runs, one warm-up discarded because the first launch after an install pays for dexopt - and docs/PERFORMANCE.md records 1814 ms over five runs, in the debug build, which is the slow case by construction. StrictMode's first run then produced five violations against that number: 492 ms resolving `app.cacheDir` inside the view-model factory, 397 ms reading free space with StatFs for every volume inside refresh(), 477 ms of the platform loading its own typefaces, and two 54 ms OEM font hooks. That is about 1.5 s of main-thread disk I/O in a 1.8 s start, of which ~0.9 s is ours and mechanical to move. Neither of ours is fixed here on purpose: the numbers and the two fixes are recorded in PERFORMANCE.md, and they are the first items of the speed work. What is not measured is stated in the same file rather than left to be assumed: folder open at 1 000 / 5 000 / 20 000 files, thumbnail paint while scrolling, and keystroke-to-result all need a `:benchmark` module and a profileable build under the release package id, and this device waits for a confirmation dialog on the first install of a new package id. The module is deliberately absent instead of half-configured: build wiring nobody can run is worse than a recorded gap. Gate green (map current, detekt, 386 tests, lint), and CHANGELOG, README, AGENTS.md, ROADMAP, PERFORMANCE.md, EDGE_CASE_TESTING and the regenerated map carry it. |
||
|
SoftwareApp
|
b1bc805af0 |
Make the gate one command, and add static analysis behind a baseline
Everything this repository promised was voluntary until now: 371 tests, lint and the generated code map ran only if whoever changed the code remembered to run them. tools/verify.sh is that gate as one command - the map check, detekt, the unit tests and lint, cheapest first so a stale document fails in a second rather than after a release build - and --release adds the R8 build. The ordering is not cosmetic: the two cheapest checks are also the two that catch drift nothing else can see. The same script runs in three places: CI on every push and pull request, a nightly --release job (R8 is the one check nothing else performs, and it has hidden a real defect here once), and a pre-push hook once the clone has been told to use .githooks. The nightly also uploads the R8 mapping, which is what turns a release stack trace back into a line of Kotlin and is only worth keeping if it belongs to the build that shipped. Detekt is adopted with a baseline, which is the entire adoption strategy: everything it finds today is frozen in config/detekt/baseline.xml, so the gate fails on a new finding rather than on a backlog nobody is going to clear this week. Entries are keyed by rule, file and enclosing declaration, so a new long function is still reported inside a file that already has one - proved by writing a TODO into a file, watching the build fail on that exact line, and taking it out again. What it found first agrees with the churn numbers from the last pass: ArchiveEngine 16 findings, BrowserViewModel 11, SelectionBar 9, 86 in total over 13 rules, the largest groups being the complexity ones this was added for (cyclomatic complexity 10, nested block depth 7, too many functions 4, long methods 3, two classes over 700 lines) plus 30 over-long lines. Three findings were fixed rather than parked, because they were real: dead `menuOpen` state left in BrowserScreen when the overflow menu moved into BrowserTopBar, a missing trailing newline, and two `db` parameters that are the platform's signature rather than an oversight (suppressed with that reason written next to them). Four rules were configured rather than obeyed: PackageNaming, because the applicationId's underscore is an identity this app cannot change; FunctionNaming, for @Composable (PascalCase by convention) and for backticked test names, which is the project's specification style; MagicNumber, because sizes and limits are named constants here; and MatchingDeclarationName, because a file is named after its concept rather than after a declaration inside it. Two tooling facts cost real time and are in neither tool's documentation. Detekt runs through its CLI rather than its Gradle plugin: the plugin performs the analysis inside the build's own JVM, and detekt 1.23 cannot parse JDK 25's version string - it fails with "25.0.4.1" as the entire error message - so the task drives detekt-cli on a 21 launcher, which is also the only way to pin the JVM the analyser runs on. And a baseline has to exist before it can be read, which is why it starts as an empty document rather than being generated into place. Verified: the fast gate is green in 17 seconds, --release adds a 36-second R8 build, 371 JVM tests still pass, lint is clean and the map is current. CHANGELOG, README, AGENTS.md, ROADMAP (the gate is now a recorded decision) and the regenerated map carry it. |
||
|
SoftwareApp
|
0743bda29e |
Let a folder be starred, and put the starred ones on the home screen
The app bar of any folder now carries a star, and a starred folder appears in a Favourites section on the home screen, above the libraries. Above, because it is the user's own list - and absent entirely while empty, so someone opening the app for the first time still meets the libraries first, which is the order they were given for a reason. The star in the row is the removal, which is what a star means everywhere else: the mark that says "this is a favourite" is the thing you tap to stop it being one. That avoids a long-press as well, and with it a destructive gesture nobody can see. A favourite is a reference, not a copy: data/BookmarkStore keeps one path per row and nothing about the folder's contents, so reading the whole list is one query and no filesystem work, which is what makes it affordable on every visit to the home screen. It has its own SQLite database rather than a table in the trash index - one table and three queries do not justify Room in a build that has no annotation processor, and the two stores have nothing to say to each other. Two behaviours there are decisions rather than defaults. A folder that has gone is kept and marked (Folder not found) instead of being deleted: a card out of its slot and a folder deleted in another app are indistinguishable from here, and silently forgetting someone's list because their SD card was out is the worse of the two mistakes. And opening the row re-checks existence at that moment, because the flag is as old as the screen - so the answer is a message rather than an empty listing, which would read as an empty folder instead of a missing one. Two smaller refusals came out of the same question. There is no star in a library or on the root screen, because neither is a folder and there would be nothing to come back to: the distinction that governs the new-folder button, and one the view model enforces too rather than trusting a stale screen. And a bookmark has no undo, because Undo means "the last change to the filesystem the app made" and a favourite is not one - the star is one tap to put back. The row's label comes from the same place the breadcrumbs do, which is why a starred volume root reads as "Internal storage" rather than as "0". Sixteen tests: nine against real SQLite through Robolectric (round trip, ordering, starring twice neither duplicating nor moving a row, and the same no-drop migration guard the trash index carries), and seven through the view model (toggling, outliving the view model that added it, newest first, the missing folder kept and marked, the missing folder refusing to open, and a library refusing to be starred). 371 JVM tests, lint clean. Verified on the phone: the star appears only inside a folder and flips from "Add to favourites" to "Remove from favourites" without leaving the screen; the home screen lists the volume root above the libraries with its own label and its path underneath; the row opens it with the star already filled; and the row's star takes the whole section away with a snackbar. The documentation moved with it - CHANGELOG, ROADMAP (M15), README, COMPETITIVE_ANALYSIS (that gap closes instead of joining the candidates), EDGE_CASE_TESTING, and the generated code map. |
||
|
SoftwareApp
|
ddae70a8bc |
Turn the project's rules into tests, and generate a map of where everything lives
The first question about any change is which file it belongs in, and the three documents that
kept being asked to answer it - README (the tour), ROADMAP (what is done) and CHANGELOG (why it is
like that) - were never going to. docs/CODE_MAP.md answers it: all 74 source files with the purpose
taken from the KDoc already sitting next to them, their size, and the tests that cover them,
followed by every test name in the suite. Those names are sentences about behaviour in this project
("a batch rename is a single undo step"), so the index doubles as the specification, and it is the
cheapest way for a reader, human or otherwise, to find out whether something is already covered.
The map is generated by tools/make-code-map.py rather than written, because a hand-maintained
description of 74 files is wrong within a milestone and nobody notices. The script harvests
declarations and comments; --check fails the moment the file stops matching the tree, and it caught
this very change (350 tests to 355); and the file says so about itself in its first line. A file is
described by the KDoc of the declaration that shares its name, which is the convention this
codebase already follows, and where no declaration matches, the first KDoc is used and the file is
listed at the end - so 15 fallbacks out of 74 are visible rather than assumed. Test files are
attached by the class they declare (NameTest -> Name.kt), which is a name-based match and therefore
approximate: the one honest weakness, and worth knowing before trusting a row that says a file has
no tests.
AGENTS.md is the one page that points at the map: what the app is, the two files to read before the
source, the exact commands, the layout with its dependency direction (ui -> domain -> data -> core,
with core free of Android), the rules that have tests behind them, what to read on demand and what
not to read at all, the definition of done, and the traps that have cost real time here - MIUI's two
install gates, assembleDebug racing an adb install, R8 stripping what only a resource names, and adb
input text not delivering a slash.
The same pass made four of the project's stated rules fail instead of merely being stated, in
ProjectInvariantsTest, which reads the source tree: core/ imports no Android (the engine stays
testable without Robolectric), no Text(...) in ui/ is handed a string literal (text comes from
strings.xml, which the pseudo-locale build exercises), the manifest requests exactly the six
permissions the app is built around and never INTERNET, and separator handling lives in core/ alone.
Three of the four were already true. The fourth found a bug: createFolder checked only that the name
was not empty, so "notes/final" was resolved by StoragePath into the notes folder that happened to
exist - the dialog offered one folder in the folder on screen and the app created a different one.
Rename refused the same input, because the rule has exactly one implementation and only one of its
two callers used it.
Both callers now go through isUsableName, and the shared name dialog says why a name is refused -
"A name cannot contain a slash", while it is typed, with its button disabled - rather than appearing
to do nothing when it is pressed. The batch dialog and both single-name dialogs read the same
string, so the rule reads the same wherever a name is entered. Verified on the phone: a slash typed
into New folder shows the note with Create the only disabled node in the dump, and deleting the
slash clears the note and re-enables it.
355 JVM tests (5 added: the four invariants and the folder-name refusal), lint clean, the map
current. The source-level rules state their own limits in their comments - the literal rule catches
a literal as the whole text of a Text(...) and not one concatenated into resource text, because
seeing that would take a Kotlin parser, and a rule that pretends otherwise is worse than one that
says where it stops.
|
||
|
SoftwareApp
|
b28e9dca37 |
Screenshots: 10-inch tablet set; guard against blank captures
The 7-inch roots capture was pure app background (Compose had not composed its first frame yet, though the system bars had drawn) so it measured as uniform. made-store-screenshots.sh now measures each capture and refuses to normalise a blank one. |
||
|
SoftwareApp
|
67c2cd61db | Screenshots: demo-library generator, per-form-factor targets, listing notes | ||
|
SoftwareApp
|
d845a7646e | M10: store assets - Plain Files naming, generated icon/feature graphic, listing copy |