Repository navigation
Conversation
jatezzz
left a comment
jatezzz
left a comment
There was a problem hiding this comment.
Some of these files are quite long. Please consider splitting them into smaller, more focused files to improve readability and adhere to the Single Responsibility Principle (SRP).
@jatezzz thanks for reviewing. I already worked with Claude to do a couple of cohesion and coupling passes. Broke up many things and made them more testable. Looks like I missed one file...BboxFragmentPicker needs help. And I'll try to break up the RegionManagerFragment too...that popped up as a borderline judgement call last time. There's probably a better way to organize the wizard steps. Anything else you spotted? Also is there a guideline for how this team applies SRP? It's one of the more vague rules and I'm curious how you and the team apply it. The more specific we can be, the more likely it is that agents can plan for and build the right thing on the first try. |
|
@fryanpan Thanks for the open response and for doing those passes with Claude! Regarding the files: You make a fantastic point about SRP being vague, and I completely agree that being specific helps both humans and AI agents. Here is how we generally look at SRP pragmatically on this team:
I love your point about agents planning better with specific rules. We should probably document these exact heuristics in a guide file so the AI tools can pick them up automatically as context. Let me know how the refactoring goes with those two fragments! |
…(ADFA-2436) Responds to the review on PR #22. Dispatchers.IO (3 inline comments). Every entry point in data/ is now `suspend` + `withContext(Dispatchers.IO)` — ActiveRegionStore, RegionCache and FirstRegionAutoActivator (the three called out), plus RegionDownloader, RegionInstaller, ProjectRegionCoordinator, MapFontExtractor, RegionSizeEstimator and ReachabilityProbe. The dispatch is pushed down into the data layer rather than wrapped at the Fragment call sites, so a caller can't do file I/O on Main. SRP / long files. Business logic pulled out of the Fragments: - domain/ (pure Kotlin, JVM-unit-tested): AutoShrinkBbox, ZoomFit, ZoomLabel, EstimateDisplay, IiabStyleBuilder, RegionWizardStateMachine - data/: RegionSizeEstimator, ProjectRegionCoordinator, ReachabilityProbe - ui/: BboxSelectionModel, MapLocationController, BboxPickerArgs - slicer/: TileMath extracted from PmtilesRegionSlicer The download wizard is now per-step Fragments (SourcePicker, DownloadProgress, Step3Save) orchestrated by RegionWizardStateMachine, per the reviewer's suggestion. Line counts, code only (comments + blanks stripped): BboxPickerFragment 1320 -> 762 raw / 446 code RegionManagerFragment 647 -> 542 raw / 401 code PmtilesRegionSlicer 643 -> 566 raw / 349 code Also in this pass: dark-mode correctness, and dead branding tokens stripped from PluginTheme (plugins take the host's Material colors — they can't re-theme host-loaded Material widgets through plugin theme attrs).
Re: SRPTried to address SRP comments by splitting up UI vs. logic and break out responsibilities more sensibly:
Re: DocumentationWill try to get more about SRP guidelines into a draft PR on the plugin-examples side, and also CoGo side. |
How this was testedAutomated
On device — Samsung A56 (Android 16), plugin installed from a clean state, both template languages:
Recordings
|
There was a problem hiding this comment.
Code Review
Overview
A new headless-plus-UI plugin that adds a Maps bottom-sheet tab and an Offline OSM Map project template. The interesting engineering is range-slicing a bbox out of IIAB's ~87 GB global PMTiles archive rather than downloading it, then bundling the slice into a generated MapLibre app that serves its own tiles over a loopback HTTP server. Region cache is shared across projects at /sdcard/CodeOnTheGo/maps/<id>/.
This is high-quality work. The comments explain why consistently (the native.code permission, the pmtiles://http:// scheme constraint, the ConnectivityReceiver.setConnected(true) hack, Pebble's newline trimming) — that's the kind of documentation that saves the next person a device round-trip. Day/night theming follows the reference pattern exactly (values-night/, themedPluginInflater + onGetLayoutInflater on all five fragments, zero hardcoded hex, zero hardcoded layout strings). Path-traversal guards are layered (regex validation plus canonical containment) in every path that resolves under the cache root.
Blocking
1. Tooltip category is wrong → tab tooltip will render n/a
maps/src/main/kotlin/org/appdevforall/maps/MapsPlugin.kt:189
override fun getTooltipCategory(): String = "plugin_maps"The category must be "plugin_" + the full plugin.id, i.e. "plugin_org.appdevforall.maps". The host registers entries under the returned string but derives plugin_<pluginId> when resolving a lookup, so a short slug silently mismatches and long-press shows the literal string n/a. The build stays green and the DB row is present — only device long-press catches it.
override fun getTooltipCategory(): String = "plugin_$PLUGIN_ID"MapsPluginCoverageTest.kt:188 asserts the wrong value too, so it needs updating with the fix. Worth noting: several existing plugins in this repo (bookshelf, random-xkcd, keystore-generator) have the same short-slug shape, while compose-preview, sketch-to-ui-plugin, and apk-viewer derive from the id. If device testing showed the Maps tooltip working, then the host is more lenient than documented — say so and I'll drop this — but the documented contract and the majority-correct plugins both point at deriving from PLUGIN_ID, which is free to do either way.
2. Template tooltip tag has no registered entry
MapTemplateBuilder.kt:63,146 attach tooltipTag = "maps.template.region_map" to the template, but getTooltipEntries() (MapsPlugin.kt:191) returns exactly one entry, tagged maps.bottom_sheet.maps_tab. The New Project card's tooltip has nothing to resolve → n/a. Add a second PluginTooltipEntry for the template tag.
3. Hard-coded upstream archive date breaks every download on rotation
RegionDownloader.kt:54,327
private const val FALLBACK_VECTOR_DATE = "2026-04-01"
...
"openstreetmap-openmaptiles.$FALLBACK_VECTOR_DATE.z00-z14.pmtiles"The constant is named FALLBACK_ but it's the only path — there is no discovery. HttpRangeByteCache.clear()'s own comment says "IIAB rotates the same URL weekly"; when that rotation drops the 2026-04-01 file, every download 404s with no recovery and the plugin looks broken in the field.
The fix is already half-built: ReachabilityProbe.buildLanProbeUrl (ReachabilityProbe.kt:47) HEAD-probes /maps/extracts.json, "the canonical IIAB inventory". Read that inventory to resolve the current filename and keep the constant as a genuine fallback.
Should fix
4. Play Services dependency contradicts the template and can crash on GMS-free devices
build.gradle.kts:92 pulls play-services-location:21.3.0, and MapLocationController.kt:165 calls LocationServices.getFusedLocationProviderClient(ctx) — the one unguarded call in a file that runCatchings everything else. On a de-Googled or AOSP device (a meaningful slice of ADFA's low-end target audience), client construction can throw before either listener is attached, and it propagates out of fetchLastKnownLocation().
Meanwhile the generated app deliberately avoids GMS — app/build.gradle.kts.peb says "The map's current-location dot uses MapLibre's built-in default location engine, so no Play Services Location dependency is needed", and MapRegionActivity proves it works. The plugin's own enableLocationComponent already uses useDefaultLocationEngine(true). Drop the dependency and use the existing fallbackToLocationManager() walk as the primary path — smaller .cgp, consistent with the template, and no GMS assumption. At minimum, wrap the client creation in runCatching { … } ?: fallbackToLocationManager().
5. PmtilesHttpServer: partial skip serves wrong bytes under a correct-looking header
templates/region-map/app/src/main/java/PACKAGE_NAME/PmtilesHttpServer.kt.peb:131-137
while (skipped < rangeStart) {
val sk = fis.skip(rangeStart - skipped)
if (sk <= 0) break // <-- then serves from the wrong offset
skipped += sk
}InputStream.skip returning 0 does not imply EOF. On a short skip the loop breaks and the body is streamed from wherever the cursor landed, while the already-written Content-Range header promises rangeStart-rangeEnd. MapLibre gets structurally valid but wrong PMTiles bytes — silent tile corruption that is very hard to trace. Fail the request instead (500, or 416) rather than serving mismatched data.
Two smaller items in the same file:
- No
setSoTimeouton the accepted socket, feeding an unboundednewCachedThreadPool. A hung client pins a thread indefinitely. Loopback keeps the risk low, but this is template code students will copy. - Suffix ranges are silently ignored:
if (dash > 0)(line 95) skipsbytes=-500, which then serves the whole file under a200. MapLibre doesn't emit suffix ranges, so this is latent, not live.
Worth an explicit note in osm-tutorial.html: this server is unauthenticated and reachable by any app on the device. It's harmless here (public map tiles already world-readable inside the APK), but the pattern is teaching material and a student who points rootPrefix at something sensitive has built a local data leak. One sentence in the tutorial and a comment on the class would cover it.
6. The 206 branch lacks the size cap the 200 branch has
RangeFetcher.kt:146-154. The 200 fallback carefully refuses unknown-length or oversized bodies before buffering (lines 162-168) — good. The 206 branch calls body.bytes() unconditionally and only checks the size in a require after the whole body is in memory. A misbehaving or hostile LAN IIAB host answering 206 with a multi-GB body OOMs the process. Check body.contentLength() against length before reading, mirroring the 200 path.
Also line 149: bytes.size == length || bytes.size == (end - offset + 1).toInt() — end is defined as offset + length - 1, so the second disjunct is always identical to the first. Dead condition.
7. Rebase before merge
13 commits behind main. Both README.md (the Examples table) and .github/workflows/update-libs.yml (the MAP array) insert into regions that main has since changed, so expect conflicts in exactly those two spots. Trivial to resolve, but better done by the author than at merge time.
Polish
- Bundled Gradle wrapper.
maps/gradlew,maps/gradlew.bat,maps/gradle/wrapper/duplicate the repo-root wrapper. Repo convention is to use the root one (cd maps && ../gradlew assemblePlugin), asflutter-templatedoes. The../libs/*.jarreferences are correct — just the wrapper to delete. .cgpsize. ~24 MB of assets before the plugin's own MapLibre.so. Two concrete trims: the vendored repo ships threekotlin-stdlibversions (1.7.10, 1.8.22, 2.2.10) where resolution needs one — ~3 MB; and the template pinsabiFilters.add("arm64-v8a")with a clear rationale, but the plugin's ownbuild.gradle.ktsdoesn't, so the.cgpcarries every ABI MapLibre's AAR publishes. Applying the same filter to the plugin is consistent and free.- arm64-only generated app. Worth stating in the tutorial and template README — a student who reaches for an x86_64 emulator instead of a device gets a missing-
.sofailure, and the reason lives only in a Gradle comment. - Hardcoded English strings.
MapLocationController.kt:126-144—setTitle("Location is off for Code on the Go"),"Open Settings","Not now", and the long message. The siblingmaybeRequestLocation()(lines 107-117) correctly usesR.string.maps_location_perm_*. These are user-facing UI, so they belong instrings.xmlalongside the rest; it's the only such offender in the plugin. Bboxhas noequals/hashCode/toString. It's a plainclass(Bbox.kt:17).RegionDownloader.kt:123logsbbox=$bbox, which printsBbox@1a2b3c4— the download log line loses its most useful field. Note the contrast withRegionInfo, where the hand-rolledequals/hashCodefor theDoubleArrayfield is done carefully and correctly. MakingBboxadata classfixes the log and forecloses any future reference-equality surprise.- Delete keys off the meta-declared id.
RegionManagerFragment.kt:505callsRegionCache.delete(info.regionId), whereregionIdcomes frommeta.optString("regionId", dir.name)(RegionCache.kt:203)./sdcardis world-writable, so a plantedmeta.jsoncan name a different valid region and delete the wrong directory. Not traversal — the containment check holds — butinfo.directoryis already canonicalized and unambiguous, so passing that instead removes the class of bug. HttpRangeByteCache.gethands back the internal array.RangeFetcher.kt:73returns the cachedByteArrayby reference from a process-global cache, though the KDoc calls them "instant in-memory copies". No current caller mutates it, but a.copyOf()(or a comment) would make that safe by construction rather than by convention. Separately,put'scontainsKeyearly-return (line 80) doesn't promote in an access-order map, so re-requesting a cached range doesn't refresh its LRU position.compileSdk/targetSdk = 34. #54 is standardizing the toolchain across plugins — coordinate so this doesn't land needing an immediate follow-up.mkdirs()precedes the containment check.ProjectMapEmitter.kt:90-95creates the directory then verifies it doesn't escape.MAPS_ASSETS_SUBPATHis a constant so it can't, but ordering the check first makes the guard real rather than decorative.- Doc/code mismatch.
Bbox.heightKm()(line 35) is documented as "latitude span × 111.32"; the body callshaversineKm. - Cleartext to LAN hosts.
RegionDownloader.base()defaults tohttp://for LAN (correct — IIAB boxes rarely run TLS), but the plugin declares nonetworkSecurityConfigand rides the host's. Worth confirming CoGo permits cleartext to arbitrary LAN hosts, or LAN downloads will fail on a host that doesn't. The template correctly scopes its own config to127.0.0.1.
Testing
359 tests passing, 98.8% line / 89.3% branch on non-UI logic. The injectable-seam design (downloadInto's copyBasemap/sliceTiles/nowMillis, ProjectMapEmitter's Android-free purity, RegionCache's *FromRoot overloads) is the right way to get this testable without Robolectric, and it's applied consistently. UI at 0% with device E2E is a reasonable call and honestly labeled.
Gaps worth a follow-up: the PMTiles slicer is 4k lines of binary-format parsing whose failure mode is silently wrong tiles rather than a crash, and the tests exercise it against synthetic archives. A golden-file test — slice a known bbox from a checked-in small PMTiles and assert the output byte-for-byte against a pmtiles CLI extract — would be the cheapest guard against a regression here, and would also de-risk the "rip this out for the CLI later" option the PR description raises. I'd also add a regression test for #5 (a short-skip Range request) since the symptom is corruption rather than an exception.
Verdict
Approve after #1–#3. Those are all small, mechanical fixes, and #1/#2 are the class of defect that only device long-press catches — worth doing before this ships. #4–#6 are the ones I'd want addressed in this PR rather than deferred; the rest can be follow-ups.
I have not built or device-tested this — those findings come from reading the diff, so #1 in particular is worth a quick long-press check on the Maps tab to confirm.
🤖 Generated with Claude Code
…ile writes (+ unit tests)
…t tests) Hand-rolls what go-pmtiles' `pmtiles extract` does (bbox → minimal v3 archive), over HTTP range requests against the IIAB-hosted global archive, because we can't ship/shell-out to a per-arch Go binary on-device. The code is dense (Hilbert-curve tile-id ranges, directory walking, run-length entries) but isolated behind a small surface and heavily unit-tested — trust the tests. If it proves hard to maintain, it's swappable for the `pmtiles` Go CLI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…s), installer, stores (+ unit tests)
…pp (Kotlin + Java) (+ unit tests)
From the license-audit + code-review DoD pass (both 0-blocker): - THIRD_PARTY_LICENSES.txt: add com.github.davidmoten:hilbert-curve:0.2.3 and its transitives (guava-mini, listenablefuture) — Apache-2.0 runtime deps bundled in the .cgp that were the one un-attributed entry. - SourcePickerFragment.persistLanHost: use applicationContext (matching the off-main read) so the write shares the one process-wide SharedPreferencesImpl — a future reorder can't reintroduce a first-access disk read on Main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(ADFA-2436) Responds to the review on PR #22. Dispatchers.IO (3 inline comments). Every entry point in data/ is now `suspend` + `withContext(Dispatchers.IO)` — ActiveRegionStore, RegionCache and FirstRegionAutoActivator (the three called out), plus RegionDownloader, RegionInstaller, ProjectRegionCoordinator, MapFontExtractor, RegionSizeEstimator and ReachabilityProbe. The dispatch is pushed down into the data layer rather than wrapped at the Fragment call sites, so a caller can't do file I/O on Main. SRP / long files. Business logic pulled out of the Fragments: - domain/ (pure Kotlin, JVM-unit-tested): AutoShrinkBbox, ZoomFit, ZoomLabel, EstimateDisplay, IiabStyleBuilder, RegionWizardStateMachine - data/: RegionSizeEstimator, ProjectRegionCoordinator, ReachabilityProbe - ui/: BboxSelectionModel, MapLocationController, BboxPickerArgs - slicer/: TileMath extracted from PmtilesRegionSlicer The download wizard is now per-step Fragments (SourcePicker, DownloadProgress, Step3Save) orchestrated by RegionWizardStateMachine, per the reviewer's suggestion. Line counts, code only (comments + blanks stripped): BboxPickerFragment 1320 -> 762 raw / 446 code RegionManagerFragment 647 -> 542 raw / 401 code PmtilesRegionSlicer 643 -> 566 raw / 349 code Also in this pass: dark-mode correctness, and dead branding tokens stripped from PluginTheme (plugins take the host's Material colors — they can't re-theme host-loaded Material widgets through plugin theme attrs).
main moved every plugin under plugins/<MixedCase>/ and gates PRs on `addons check`. This moves maps/ there, drops the bundled Gradle wrapper for the repo-root one (John's review, polish item 1), points the jar references at ../../libs, renames the artifact to maps.cgp, adds addon.json, fixes the page title and spells out Code on the Go, and adds the README Examples row. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018DEPcb7gdHpRHRHXVqNnbf
… SDK 36) (ADFA-2436) check-toolchain.sh on main requires the toolchain Code on the Go ships on-device, and #54's standardization landed while this PR was open (John's polish item on compileSdk/targetSdk 34). AGP 9 compiles Kotlin itself, so the kotlin-android plugin goes. Two knock-on fixes: - plugin-api on main added 13 PluginContext members. Test fakes now delegate to one throwing UnusedPluginContext and override what they use. - AGP 9 writes Kotlin classes under intermediates/built_in_kotlinc, so the JaCoCo report was covering zero classes and still passing. It points at the new path and fails when it finds no classes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018DEPcb7gdHpRHRHXVqNnbf
The host registers plugin tooltip entries under getTooltipCategory() but derives "plugin_" + plugin.id when it looks one up, so "plugin_maps" never matched and long-press on the Maps tab rendered n/a. The entry tag now uses the shared TOOLTIP_TAG_MAPS_TAB constant. Review: John, blocker 1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018DEPcb7gdHpRHRHXVqNnbf
…-2436) MapTemplateBuilder tags the Offline OSM Map card with maps.template.region_map, but getTooltipEntries() only returned the tab's entry. The tag now lives in MapsPlugin beside the tab's, and both have entries. The card itself still will not find it: the host's New Project screen looks card tags up under the IDE category (TemplateListFragment -> showIdeCategoryTooltip), so this needs a host change too. The KDoc says so. Review: John, blocker 2. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018DEPcb7gdHpRHRHXVqNnbf
Every download used openstreetmap-openmaptiles.2026-04-01.z00-z14.pmtiles, named by a constant called FALLBACK_ but with no discovery in front of it. The mirror publishes a new dated archive each month (2026-10-01 is current) and nothing guarantees the old one stays. The internet source now reads data.vector["14"] from IIAB's maps-catalog.json, the inventory IIAB's own region extractor uses. The entry must be a dated z00-z14 name on iiab.switnet.org/maps/2/, the body is capped at 256 KB, and the result is memoized per process so the size estimate and the download read the same file. When the catalog cannot be read the 2026-04-01 name stays as a real fallback. The archive date is recorded as the region's source version. LAN keeps the fallback name: extracts.json lists a box's local regions, not the global archive, so it cannot supply this file name. Review: John, blocker 3. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018DEPcb7gdHpRHRHXVqNnbf
…(ADFA-2436) MapLocationController built a FusedLocationProviderClient without a guard, so on a device without Google services it could throw before either listener was attached. The generated app already avoids GMS, and the plugin's own blue dot uses MapLibre's default engine. The LocationManager walk (passive, network, gps) that was the fallback is now the only path, and the dependency and its proprietary-license entry go. Review: John, should-fix 4. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018DEPcb7gdHpRHRHXVqNnbf
…A-2436) InputStream.skip may return 0 before the end of the stream. The generated app's PmtilesHttpServer took that as a stop and streamed the body from wherever the cursor was, under a Content-Range header that promised rangeStart-rangeEnd: silently wrong PMTiles bytes. Both language variants now skip with skipFully, which reads one byte to tell a short skip from the end, and do it before writing any header so a failure answers 500. The test compiles the shipped Java template against the unit-test classpath and runs skipFully on a stream whose skip always returns 0. Review: John, should-fix 5. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018DEPcb7gdHpRHRHXVqNnbf
…ively (ADFA-2436) The .module guard compared only size and sha256, and as sets, so a stale sha1 or md5 passed, and so did one file entry going stale while the other stayed right. It now computes md5, sha1, sha256 (and sha512 once one is recorded) from the AAR on disk and checks each against every file entry. The doc test only failed on the exact old sentence "64-bit ARM phones only", so dropping the 32-bit claim or rewording the restriction passed. It now requires "64-bit and 32-bit ARM phones" and both ABI names in the README and the tutorial, and fails on any "64-bit ARM ... only" or "not install on a 32-bit" wording. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
…DFA-2436) IIAB moved its maps catalog to openstreetmap-openmaptiles.2026-10-01 on 2026-10-06. The plugin reads the current name from that catalog online, but falls back to FALLBACK_VECTOR_ARCHIVE when the catalog is unreachable and always uses it for an IIAB LAN box, so the fallback was still sending those downloads to the April build. The 2026-10-01 file answers 200 on the mirror. Two tests that prove "the catalog's name wins over the fallback" used the 2026-10-01 name, which now equals the fallback and would pass even if the catalog were ignored; they now use a 2027-04-01 name. The opt-in online slicer tests point at the new archive too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
…FA-2436) 78ed1dd moved the fallback archive to the 2026-10-01 build, and LAN downloads always asked the box for that name. A box installed before IIAB's 2026-10-06 update holds only the 2026-04-01 build, so its downloads would 404. resolveTilesUrl now range-probes the box for each known name, newest first, and uses the first it serves; a box with neither fails with both names and their status codes. The picker probes before its first estimate, so the estimate and the download read the same file. The cache root is resolved before the probe, so missing storage still fails before any request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
The two http clients are process-wide, so idle connections left by earlier tests made the "keeps its connection alive" precondition read [3, 1] instead of [1, 1] in one of three full runs. The test now evicts both pools before it opens its own connections. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
…ile read (ADFA-2436) A golden file records what go-pmtiles 1.31.2 `pmtiles extract` writes for one bbox of the bundled Natural Earth archive (z1-z4, 14 tiles): tile ids and a sha256 per tile, with the command and capture date in its header. The test slices the same bbox over HTTP, as a download does, and from a file, and compares both to it. Tile ids matched as-is. Tile bytes did not, from a file: FileRangeFetcher seeks and reads one RandomAccessFile while downloadAndSlice fetches chunks from several coroutines, so two tiles came back with another read's bytes. Production reads over HTTP, so downloads were not affected, but every file:// slicer test could flake. readRange and close are now synchronized, with a concurrency test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
…s (ADFA-2436) Rendered the template for Kotlin and for Java with the versions Code on the Go supplies (AGP 9.3.1, Kotlin 2.3.21) and resolved every :app configuration offline, with an empty Gradle home, against CoGo's bundled repo plus offline-maplibre-repo in both orders. Neither language resolves kotlin-stdlib 1.8.22, kotlin-stdlib-common 1.8.22 or kotlin-stdlib-jdk7/jdk8 1.6.21 from this repo: MapLibre 13.1.0 needs kotlin-stdlib 2.2.10, and stdlib 2.x pulls jdk7/jdk8 up to at least 1.8.0, so those versions always lose. Both projects still resolve the same files and assembleDebug offline after removal. The .cgp drops from 44.2 MB to 42.5 MB. The captured list of files the generated projects resolve is now a test fixture: one test fails if any of them is missing from the repo or its manifest, another if a kotlin-stdlib jar ships that nothing resolves. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
…(ADFA-2436) probeLanArchive accepts a plain 200 as well as a 206, because a box whose web server ignores the Range header still holds the archive; and it reports a failed request as "no answer" rather than a status. Neither path had a test, so dropping 200 from the accepted codes or mislabelling an unreachable box would have gone unnoticed. The two new cases bring non-UI branch coverage back above 90%. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
…FA-2436) The region-map template's offline repo still carried 26 androidx jars and aars (with their .pom and .module files, 71 files, 1.8 MB) that neither a generated Kotlin nor a generated Java project ever selects: older or -ktx variants that lose conflict resolution to the versions the fixture lists, such as core 1.0.0 against core 1.13.1. Re-rendered both languages at 1c028e8 with those version dirs removed and resolved every :app configuration plus assembleDebug offline, with an empty Gradle home, against CoGo's bundled repo and this repo in both orders. All four runs resolve exactly the same 1161 artifacts as before the removal, and the 38 files taken from this repo match the fixture. The .cgp drops from 42.5 MB to 41.0 MB. The stdlib-only check is now repo-wide: any jar or aar the repo ships, in the manifest or on disk, that is not in the resolved-files fixture fails MapTemplateAssetsTest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
…reens (ADFA-2436) Plugin review B1 (clause 6.7): five interactive screens had no tooltip wiring. Each now has its own PluginTooltipEntry linking to the tutorial, and a long press on the screen's main controls shows it through the 3-arg showTooltip under plugin_org.appdevforall.maps. The coverage test reads the screen sources, so a screen that stops wiring a tag, or a tag without an entry, fails the build. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
… tab (ADFA-2436) Plugin review C3 and advisory 1. The Tier 3 page moves to the name plugins/README.md asks for, and gains a "Using the Maps tab" section: the region list's active switch, Refresh and Delete, the source step, where the box starts, the size estimate, the zoom limit, region naming, and the download step. Four device screenshots ship beside the page, each under 12 KB. Each screen's tooltip links to its anchor; tests check that every linked page, anchor and embedded image ships. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
Plugin review advisory 3. Nothing resolves from it: hilbert-curve 0.2.3, the only com.github dependency, comes from Maven Central. With jitpack gone, buildEnvironment, releaseRuntimeClasspath and debugUnitTestRuntimeClasspath all resolve under --refresh-dependencies with no failures. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
Plugin review advisory 4. No dependency on any resolved classpath is a com.android.support artifact, so Jetifier has nothing to rewrite. The release build sets isMinifyEnabled = false and assemblePlugin runs no R8 task, so the full-mode switch never applied. A clean assemblePlugin builds without either AGP deprecation warning. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
…FA-2436) IIAB serves an nginx JSON autoindex of /library/www/maps/ at /maps/data/, and its own map page reads it. Read that listing first and take the newest exact z00-z14 name, so *.pmtiles.old and directories never match. The file is still read from /maps/<name>: the listing location aliases the same directory but lacks IIAB's pmtiles transfer settings. When the listing is missing, not JSON, too large or has no match, probe by name newest first across every published build (10-01, 09-01, 07-01, 04-01). A box with none gets a message that it has no zoom 0-14 archive, that IIAB installs zoom 0-8 by default, and to use the internet source, followed by everything that was tried. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
… context (ADFA-2436) maps.html said to use the "Maps" template or any project. The template is "Offline OSM Map" (MapTemplateBuilder.REGION_MAP_TEMPLATE_NAME), and apply refuses a project without its map activity (ProjectMapEmitter.apply). It also said nothing outlives dispose(); MapLibre keeps the application-scoped context MapLibreBootstrap gives it, with no teardown. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
- DownloadCompleteMessage.Spec: target @stringres at the parameter explicitly, so a future Kotlin applying it to the field too changes nothing. - BboxPickerFragment.onRequestPermissionsResult: drop the super call. Fragment's implementation in fragment 1.8.8 is an empty method (javap: a bare return), and calling it was the deprecated use the compiler flagged. compileDebugKotlin + compileReleaseKotlin with --rerun --no-build-cache: 2 warnings before, 0 after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
Both OkHttp clients are plugin-owned singletons, so dispose() already evicted their idle sockets. A blocking execute() does not see the plugin scope's cancellation, though, and ran on until its 20 s / 60 s timeout. Cancel each client's dispatcher before evicting. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
… screens (ADFA-2436)
- Save screen: the duplicate-name warning said "Save will replace it" while Save is
disabled for a duplicate. It now says to pick another name; behaviour unchanged.
- Region row: the active switch gets a content description ("Use this region in the
project") so TalkBack reads more than "switch".
- Delete and Cancel + discard: red text on the host's filled button measured 1.15:1 by day
(#BA1A1A on #555555) and 1.37:1 at night (#FFB4AB on #AAAAAA) on the A06. The host button
is dark by day and light at night, the reverse of the plugin surfaces, so a new
plugin_error_on_button token takes #FFC0B8 by day (4.79:1) and #7F0008 at night (4.74:1).
- Tab creation: getEditorTabs used a string resource for the title. The host calls it on
the main thread, and the first plugin resource read there opens the plugin APK in the
host's PluginResourceContext (StrictMode DiskReadViolation). The title is now a constant,
which also lets a JVM test cover getEditorTabs.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
…DFA-2436) BboxPickerFragment hands map driving (style load, first camera, camera listeners, projection, auto-shrink) to PickerMapController and the estimate lines to EstimateLabelBinder; its constants and the location and permission code stay where they were. RegionManagerFragment hands the row actions to RegionListActions and the post-download bookkeeping to DownloadCompletion. Both are suspend code with no Fragment references, written against a new ProjectRegions interface so the JVM tests run them over a real active.txt. Behaviour is unchanged: same snackbars, same refresh points, same tooltip wiring. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
…FA-2436) aapt2 drops an unescaped double quote from a string resource, and " counts as unescaped. The device walk showed the duplicate-name warning as 'A region named Lalibela already exists'; the empty-list hint lost the quotes round "+ Download new region" the same way. Both now use \". A JVM test reads every values*/strings.xml and fails on any quote aapt2 would drop. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
…font scale 2.0 (ADFA-2436) At 2.0 the save screen's "Source" key broke mid-word because the value row claimed the width, and the download-complete snackbar cut off "Project's active region is unchanged" at Material's 2-line default. The value now takes the spare width and wraps; snackbars allow 5 lines. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
…ts (ADFA-2436) The split in d3290e7 wrote seven new files with 2-space indents; the rest of the module uses 4. Whitespace only (git diff -w is empty). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
…-2436) An empty tile list still estimates 8 KB of header overhead, so the size line read "0.0 MB download size", Next stayed on, and the download then failed with "No tiles intersect bbox Bbox(...". The estimate now flags an empty result: the size line says there is no map data in the area and Next is disabled. If a download still hits an empty box, the failure snackbar uses a plain message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
…s the active one (ADFA-2436)
The post-download snackbars and the "Apply" confirmation showed the region id
("lalibela-2") instead of its name. Applying a re-download of the region that was
already active said "Switched active region to", which was false. Snackbars now
use the display name, and that case says the region is already active and its
map in the project is updated.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
…cker (ADFA-2436) ZoomFit used the 256-px Web Mercator constant with the map's width in physical pixels, but MapLibre's camera zoom is based on a 512-dp world. On the Galaxy A06 (density 1.875) that zoomed in 3.75 times too far, so a saved region's edges were off screen. It also fitted the width only, so a tall region could overflow. Fit in dp with MapLibre's constant, on both axes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
…ded again (ADFA-2436) FirstRegionAutoActivator returned NoOpAlreadyActive whenever active.txt named a cached region, including the region that had just been downloaded. So a re-download of the active region left the old tiles in the project and the snackbar asked the user to tap Apply. Re-apply in that case and report Updated, which the snackbar shows as "The project's map is updated" with no Apply action. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
The template's Java/Kotlin sources live under app/src/main/java/PACKAGE_NAME/, but the .cgt metadata did not list packageName among its required parameters. CoGo's ZipRecipeExecutor then logs "Missing packageName, defaulted to PACKAGE_NAME" and ends project creation with a finished-with-warnings banner. showPackageNameOption() adds the parameter, as the NDK and Python templates do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S8WVMAhapvazMLrZ7XozrL
9687ec1 to
7f5f63b
Compare
|
Re: the 2026-08-03 review, at 7f5f63b. #1 Fixed: #2: I added the missing tooltip entry ( #3 Fixed, from a different inventory: #4 Fixed: Play Services is gone; #5 Fixed in both templates: the stream is positioned before any header and a failed skip answers 500; suffix ranges work; 5 s read timeout and a fixed pool of 8; the class KDoc and #6 Fixed: a 206 must declare exactly #7 Rebased onto Wrapper: deleted; the repo-root wrapper builds it. Size: ABI filter added, to arm64-v8a and armeabi-v7a because CoGo ships both; 48.5 MB -> 41.2 MB (44.2 MB at f841489, since the generated app now carries 32-bit MapLibre too). Stdlib pruned too (cd85de1): the four kotlin-stdlib jars no generated project resolves are gone, 44.2 MB -> 42.5 MB, and a test fails if the shipped jars and the resolved set differ. Unused androidx binaries removed too (301ae37); 41.1 MB at the final head. Fresh Kotlin and Java projects built offline on a Galaxy A06. Both ABIs now: the generated app ships arm64-v8a and armeabi-v7a (+3.1 MB), checked as a 32-bit process on a Galaxy A06. Strings: the location-off dialog, the delete snackbar, the download-result snackbars and the size total now come from Bbox: now a Delete: keyed off the canonical folder now ( Cache: SDK: on the repo's standard toolchain now (AGP 9.3.1, Kotlin 2.3.21, SDK 36). Emitter: the containment check now runs before
Cleartext: CoGo permits it host-wide ( Golden-file slicer test: added (fcbc910). It slices the bundled Natural Earth basemap and matches go-pmtiles 1.31.2's extract tile for tile (14 tiles). It also caught a race in local-file reads, fixed in the same commit with an 8-thread test. Short-skip regression: added; it fails if the header goes out before the skip. |
Done. BboxPickerFragment is now 423 lines (1320 before July, 762 before this round): map setup moved to PickerMapController and the size label to EstimateLabelBinder. RegionManagerFragment is 465 (was 545): list actions moved to RegionListActions and download results to DownloadCompletion. No behaviour change; checked on a Galaxy A06 at font scale 1.0 and 2.0. On the guide file: I said I would draft it in July and did not. It is now ADFA-6840: a REVIEW.md for this repo modelled on CoGo's, with your four heuristics proposed for it. I will link the PR there. |
ADFA-2436: Add Maps (offline OSM) plugin
Ticket
ADFA-2436
Description
Adds a Maps plugin that helps developers make a working offline mapping app in a few minutes.
Lets the developer download an OpenStreetMap (OSM) map region from an Internet in A Box (IIAB) server and build a read-only mapping app (annotation left for a future ticket)
Workflow
You need internet for steps 1 and 3, but the developer can start working offline after that. And the generated app also works offline.
Details by Commit
I asked Claude to reorganize the work into commits, to try and make a big PR easier to review. Each file is in exactly one commit. Some commits could benefit from more review than others.
In the "Files Changed" tab above, you can review by commit to get a more organized view:
38255019c2f4fdmin_ide_versionpin with no max),MapsPlugin. Mostly boilerplate.d762e234dce5dcbed67ac1f10ddde2395463fa84c1d45ed3bTHIRD_PARTY_LICENSES. Check attribution + licensing is reasonable, not code.Testing
Automated checks below in the table, plus I've manually tested the flow + reviewed the code.
assemblePlugin+test)P.S. The automated checks are a bit messy and defined in the wrapper repo. I'll make a future ticket to look into cleaning them up and adding them into the plugin repo. Let me know if there are parts of this you think are especially important (or not important!)
Unit Tests
Didn't spend too much effort trying to get to 100% coverage. Did try to get over 90% on most non-UI code....but it was a bit hard to test some of the areas that required Android integration or mocking out CodeOnTheGo integration. So I left it to the E2E tests to cover these areas.
./gradlew jacocoTestReportCoverage by folder, measured 2026-06-04 (the totals in the Testing table are re-measured at 7f5f63b; full breakdown + the 8-check quality assessment in
coverage-quality-assessment-2026-06-04.md):domain/slicer/util/data/templates/maps/(MapsPlugin)ui/On-Device Automated E2E QA Walk
Was using this method regularly during development to have Claude do end-to-end testing using mobile-mcp.
The test cases cover the whole flow from installing the plugin, using the project template, downloading and managing map regions, building the templated app offline, running the templated app, and testing the offline map in the templated map. The tests also went through both Kotlin and Java project templates. Checked at font scale 1.0 and 2.0 on a Galaxy A06.
Device recordings from running the tests:
Size and network
.cgp is 41.1 MB at 7f5f63b, arm64-v8a + armeabi-v7a.
Network: the Internet source downloads tile ranges from iiab.switnet.org and IIAB's
maps-catalog.jsonfrom raw.githubusercontent.com; the LAN source talks only to the box you pick. Nothing else leaves the device.StrictMode
Was getting a few warnings about this a few days ago (e.g. warnings/errors during project creation)...did a cleanup pass. The plugin should be StrictMode clean now -- no main-thread I/O. Sockets are traffic-stats-tagged.
But creating a project still triggers StrictMode warnings that are coming from the CodeOnTheGo side. Added a comment to ADFA-4121.
Notes & Questions for Reviewers
Ease of Testing + CoverageCouple of followup items would help in the future. I'll add some followup tickets:
Decisions to Note
Future Improvements — Two nice-to-haves tracked but not blocking v1
🤖 Generated with Claude Code
https://claude.ai/code/session_014Ed1X3QzdWExhALiU221EG