ReviewOS

also looking at this

craft-native/craft

feat(android): the location-recording controls in Zig — ratchet 65 → 61

#181
Merged glennmichael123 wants to merge feat/android-recording-controls into main
7 files +568 -22
9 days ago

The other half of the store. #177 took the two reads; these four are the writes — a preferences edit, sometimes a file truncation, and for two of them a foreground service to start or stop.

What stays with the shim is the sampling: the service holds a LocationCallback, a Java interface Zig cannot implement. So Zig owns the record of a recording and Kotlin owns the thing that fills it.

Permission denied is a different object from never having recorded

startLocationRecording's denied path builds a fresh JSONObject:

put("id", JSONObject.NULL)   // explicit null — the key STAYS

Every other reply goes through state():

put("id", values.getString("id", null))   // Kotlin null — the key is REMOVED

So the two objects differ in exactly one way, and a page can only tell "you did not grant location" from "nothing has ever been recorded" by whether id is present at all.

Reproduced, with both shapes asserted side by side in one test — everything else about them agrees, which is what makes the one difference easy to lose. The mutation that collapses renderDenied into the state() shape fails that test.

The service Intent is built by Kotlin

LocationRecordingService::class.java names a class whose package is — templated per app, so FindClass on the native side has no name to look up.

Zig could assemble one from getPackageName(). That is the trap the widget broadcast action already documents (#167): an applicationIdSuffix moves the runtime package and leaves the class where it was. So CraftNative.startRecordingService / stopRecordingService do it with their own class reference, and one side decides.

Declining after a write is safe here

stopLocationRecording writes the store, reads the state, then stops the service. If the read declines, Zig returns null and the shim runs the whole method — which is correct precisely because stop is idempotent, so the service still gets stopped by the half that took over. Worth stating, because "declined after a side effect" is normally the dangerous case.

Also

UUID.randomUUID().toString() goes through Java: the id is the shim's to define, and UUID.toString is a specific hyphenated format a page may be matching on. locations is emitted last and only for stopLocationRecording, because the store's if (includeLocations) put(...) runs after every other put — so a pause reply has no locations key rather than an empty one.

Testing

zig build test:android passes with the ratchet at 61. Both libcraft.so targets still build with no NDK.

7 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.