ReviewOS

also looking at this

craft-native/craft

feat(android): the notification cancels, and the flashlight pair deferred

#151
Merged chris wants to merge feat/android-notification-cancels into main
6 files +303 -4
9 days ago

Ratchet 89 → 87, plus two deferrals with real reasons.

The id is hashed by Java, not by Zig

NotificationManager.cancel takes an int and the page sends a string, so the Kotlin cancels id.hashCode(). Getting that number wrong cancels a different notification, or none — silently either way, since cancelling an id that doesn't exist succeeds.

Zig could reimplement it: h = 31*h + c, wrapping at 32 bits. It doesn't, and the reason isn't effort.

Java hashes UTF-16 code units. An id carrying an emoji is two surrogate units there and four bytes in the UTF-8 this bridge otherwise works in, and the two produce different numbers. Reproducing that means converting back to UTF-16 first — reimplementing the encoding in order to reimplement the hash.

Calling hashCode() on the jstring the JVM already holds cannot diverge. Same judgement bridge_android_securestore.zig makes about the prefs object: where the platform already has the thing, ask it rather than rebuild it.

The cost is that the hash isn't host-testable, and that's the honest trade — a test of a hash this file doesn't compute would be testing a second implementation that doesn't ship.

What is tested

The wiring: that hashCode()'s answer is what reaches cancel, and that a null id is refused rather than cancelling whatever notification hashes to zero.

mutationresult
.i = hash + 1expected -1234567, found -1234566
.i = 0 (drop the hash)doesn't compile — unused local constant hash
null id check removedthe null test fails

The second is worth noting: the compiler is a stronger guard than a test here, and that's what unused local constant buys. I initially misread it as "not caught" because my grep was matching test-failure format and missed a compile error.

Two deferrals, recorded

setFlashlight writes isFlashlightOn (:2011) and toggleFlashlight reads it to decide what to flip to (:2026).

Serving setFlashlight from Zig means the Kotlin body never runs, so the field stops tracking the torch — and the next toggleFlashlight inverts a stale value, turning the light on when it's already on.

Zig can't update the field either: the natives are registered on CraftNative, which has no reference to the CraftBridge instance that owns it. Migrating the pair would mean moving the state, which is a change to the shim rather than a migration away from it.

zig build test:android → 102 tests, 11/11 steps · both .so build · generator 10/10.

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

Sign in to comment on this pull request.