Skip to content

ADFA-2436: Add Maps (offline OSM) plugin#22

Open
fryanpan wants to merge 10 commits into
mainfrom
feature/ADFA-2436-maps-plugin
Open

ADFA-2436: Add Maps (offline OSM) plugin#22
fryanpan wants to merge 10 commits into
mainfrom
feature/ADFA-2436-maps-plugin

Conversation

@fryanpan

@fryanpan fryanpan commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

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

  1. User installs the Maps plugin in CodeOnTheGo
  2. Creates a New Project using the "Offline OSM Map" template (template provided by the plugin)
  3. In the Maps tab in the bottom sheet, they can download one or more regions and make this region activeThis downloads map data either from an IIAB server either on the internet, or local LAN
  4. Build the project
  5. Run the new app!

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:

Commit What it is LoC Review Notes
5049a68 README + HTML tutorial + plugin registration ~500 read Intro and docs. The tutorial could probably use another edit pass at some point -- lmk if there's anything to change here.
7981fa8 Scaffold + build config + entry point ~1,200 skim Gradle, JaCoCo + test deps, manifest (incl. the min_ide_version pin with no max), MapsPlugin. Mostly boilerplate.
c044a6a domain + util (+ tests) ~1,400 read The readable, correctness-critical pure logic — bbox/zoom math, region-id validation, atomic file writes. No Android deps, ~100% unit-tested.
44ce879 slicer — PMTiles bbox extraction (+ tests) ~4,100 skim This is the logic to parse the 87GB single PMTiles file in IIAB's OpenStreetMap of the whole world. I let Claude implement this...but if it gets too hard to maintain/is buggy, we can bring in the pmtiles CLI that IIAB uses (will increase plugin size a bit)
c30e5ca data layer (+ tests) ~2,700 read Region cache, downloader (refactored with injectable IO seams), installer, stores. Path-traversal + atomic-write guards.
b3b53f2 UI — tab, region manager, bbox picker, wizard ~4,700 read UI code. Tested in E2E, not fully unit-tested.
9c5acb4 project template + emitter (+ tests) ~2,600 read The emitter + the Kotlin and Java template sources for the generated region-map app. Includes a simple local HTTP for serving tiles (see notes below)
87d2746 offline-build support (vendored MapLibre repo) ~27,800 (199 files) skim briefly Vendored Maven repo (AARs + POMs). Verify licenses + the artifact set, not line-by-line. The reversible decision (alternative: require the user to be online for the first build).
36c9985 bundled assets ~180 skim Natural Earth basemap (CC0), Noto fonts (OFL), day/night icons (CC0), THIRD_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.

Gate Status
Build clean (assemblePlugin + test) ✅ green — 359 tests, 0 failures
≥90% non-UI coverage + quality ✅ 98.8% line / 89.3% branch (pure non-UI logic); 8-check quality assessment
android-qa device walk ✅ test-cases.md reviewed; device-verified; StrictMode noise traced to CoGo host (ADFA-4121), not the plugin
UX review ✅ on the working e2e flow (2026-06-01); no UX changes this session
License audit ✅ 0 blockers; hilbert-curve attribution gap fixed
Code review ✅ 0 blockers; ran code review with Claude and Codex CLI
New plugin review skill ✅ GREEN-LIGHT — 7/7 rubric clauses Pass, security clean
Completeness sweep ✅ clean (naming / metadata / icons / help / deploy)
E2E Video Recording ✅ See links below in E2E section

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 jacocoTestReport

Coverage by folder (full breakdown + the 8-check quality assessment in coverage-quality-assessment-2026-06-04.md):

Folder Line Branch
domain/ 100% 98%
slicer/ 100% 88%
util/ 96% 100%
data/ 77% 79%
templates/ 68% 56%
maps/ (MapsPlugin) 62% 29%
ui/ 0% (device-tested)
Pure non-UI logic (excl. the 4 Android-host-bound classes) 98.8% 89.3%

On-Device Automated E2E QA Walk

Was using this method regularly during development to have Claude do end-to-end testing using mobile-mcp.

Link to the test cases here which 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.

Device recordings from running the tests:

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:

    • Adding Kaspresso testing to the plugin repo (installs some version of CoGo and then installs + tests plugin) would helpThis would cover 80-90% of the E2E tests I ran
    • Adding mobile-mcp testing + the android QA skill from wrapper repo would also help with the tests that are a bit harder to arrange in Kaspresso (e.g. that require building an app and then starting the new app)
  • Decisions to Note

    • Offline Build Support: Decided it might be worth having in ~15 MB of vendored MapLibre libraries so a developer can build the templated map app offline. It may also be okay to drop that 15 MB download and ask the user to build once while they still have internet.
    • PMTiles SlicerClaude made a case that it could do this quickly and would save 5+ MB of CLI download of the pmtiles Go CLI that IIAB uses. So Claude was quite fast...and the code seems to work. But it also ended up being 4k lines! It might be better in the long run to rip this out and switch to the CLI to stay similar to IIAB.
    • Local HTTP Server: Ran into an issue getting MapLibre to read local-filesystem map tiles. In the MapLibre forums, running a simple local HTTP server seemed like an acceptable workaround. Maybe with more digging (or a future MapLibre version) it won't be needed.
  • Future Improvements — Two nice-to-haves tracked but not blocking v1

    • GPS location permission in CoGo's manifest so the bbox picker can show current location (needs a host change)
    • Let the user download their first region during New Project creation for a smoother one-step flow (complex — requires async work inside the Pebble wizard; current two-step create→Maps-tab→download is good enough for v1).

fryanpan and others added 10 commits June 4, 2026 20:59
…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>
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>
@fryanpan fryanpan marked this pull request as ready for review June 5, 2026 16:13

@jatezzz jatezzz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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).

@@ -0,0 +1,93 @@
package org.appdevforall.maps.data

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please ensure file operations runs on Dispatchers.IO.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks...no need to identify more of these examples. I'll go through and review all file operations.

@@ -0,0 +1,95 @@
package org.appdevforall.maps.data

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please ensure file operations runs on Dispatchers.IO.

@@ -0,0 +1,280 @@
package org.appdevforall.maps.data

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please ensure file operations runs on Dispatchers.IO.

@fryanpan

Copy link
Copy Markdown
Contributor Author

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.

@jatezzz

jatezzz commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

@fryanpan Thanks for the open response and for doing those passes with Claude!

Regarding the files: BboxFragmentPicker and RegionManagerFragment are definitely the main ones. For the wizard, splitting it into separate smaller fragments (or custom views) orchestrated by a single shared ViewModel or a state machine usually keeps things much cleaner. I didn't spot any other major offenders, but those two are a great place to start!

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:

  1. Separation of Concerns (The "UI vs. Logic" rule): Fragments/Activities should strictly handle OS lifecycle, UI bindings, and user input routing. If a Fragment is formatting data, making decisions about business rules, or directly touching file I/O/Network (which also ties into the Dispatchers.IO comment), it's violating SRP. That logic belongs in a ViewModel or a UseCase/Domain class.
  2. The "And" Heuristic: If you describe what a class does and have to use the word "AND" (e.g., "It picks a bounding box AND formats the coordinates AND saves them to disk"), it probably has too many responsibilities.
  3. Dependency Bloat: If a class or ViewModel requires injecting more than 4-5 repositories or services to do its job, it’s usually a sign that it’s orchestrating too much and should be broken down into smaller, focused UseCases.
  4. Line Count (Soft limit): While not a hard rule, if a file starts crossing the 300-400 line mark, it's usually a strong smell that it's accumulating responsibilities.

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants