diff --git a/.claude/skills/gpui-mobile/SKILL.md b/.claude/skills/gpui-mobile/SKILL.md index c12bb0a..d63c3bd 100644 --- a/.claude/skills/gpui-mobile/SKILL.md +++ b/.claude/skills/gpui-mobile/SKILL.md @@ -108,7 +108,7 @@ These come from bugs actually hit in this stack. them from `cx.background_spawn(...)`; from GPUI's thread they return an error on purpose. 2. **Host wiring order matters.** In the host's `onCreate`, call - `set_host_activity` first, then register `host::on_open_window`, then + `host::set_activity(env, host, &activity)` first, then register `host::on_open_window`, then `host::start_with_assets(gpui_kit::assets::AllAssets, launch)`. Without `start_with_assets` Kit's icons are blank; without `on_open_window` before the first surface, the screen stays blank. diff --git a/.claude/skills/gpui-mobile/references/gotchas.md b/.claude/skills/gpui-mobile/references/gotchas.md index 6133d46..966505f 100644 --- a/.claude/skills/gpui-mobile/references/gotchas.md +++ b/.claude/skills/gpui-mobile/references/gotchas.md @@ -9,7 +9,7 @@ anything newer. |---|---| | Blank screen on start | `host::on_open_window` registered after the first `surface_created`, or not at all. | | Icons and SVGs missing | `host::start` instead of `host::start_with_assets(gpui_kit::assets::AllAssets, …)`. | -| Emoji show as boxes | Android 13+ ships a COLRv1 emoji font that swash can't draw. Bundle `assets/fonts/NotoColorEmoji.ttf` (CBDT) and call `set_host_activity` before `host::start`. Flags and ❤️ with VS16 still don't render. | +| Emoji show as boxes | Android 13+ ships a COLRv1 emoji font that swash can't draw. Bundle `assets/fonts/NotoColorEmoji.ttf` (CBDT) and call `host::set_activity` (or `set_host_activity`) before `host::start`. Flags and ❤️ with VS16 still don't render. | | Dark mode doesn't follow the system | The Activity lists `uiMode` in `configChanges` but doesn't call `host::configuration_changed()` from `onConfigurationChanged`. | | Text white on white in a dark chat bubble; shimmer invisible | Kit bugs kit#3326, kit#3327; not Android-specific. | | Bold/italic look regular | Roboto is a variable font; weights aren't applied. | diff --git a/.claude/skills/gpui-mobile/references/new-app.md b/.claude/skills/gpui-mobile/references/new-app.md index 3245a05..8c32857 100644 --- a/.claude/skills/gpui-mobile/references/new-app.md +++ b/.claude/skills/gpui-mobile/references/new-app.md @@ -94,7 +94,7 @@ API: | Call | When | |---|---| -| `jni::set_host_activity(env, &activity)` | first thing in every `onCreate`, recreation included | +| `host::set_activity(env, host, &activity)` (or `jni::set_host_activity`, which does not tell hosts apart: TalkBack then lands on the wrong Activity after Back) | first thing in every `onCreate`, recreation included | | `host::on_open_window(\|_, cx\| open_window(cx))` | before the first surface | | `host::start_with_assets(gpui_kit::assets::AllAssets, launch)` | every `onCreate`; only the first call starts the render thread | | `host::surface_created(host, NativeWindow::from_surface(env, surface), scale)` | `surfaceCreated` and `surfaceChanged` (a repeat is a resize) | diff --git a/Cargo.lock b/Cargo.lock index 7f18938..2930693 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2726,7 +2726,7 @@ dependencies = [ [[package]] name = "gpui-pre-mobile" version = "0.1.0" -source = "git+https://github.com/Bombatomica64/gpui-mobile?rev=c71a31aaa12660552ca7a772f6fdd890d9c8cf7a#c71a31aaa12660552ca7a772f6fdd890d9c8cf7a" +source = "git+https://github.com/Bombatomica64/gpui-mobile?rev=4acd1761b4b2a734f49ff2bce930c61b01cfe5be#4acd1761b4b2a734f49ff2bce930c61b01cfe5be" dependencies = [ "accesskit_android", "android-activity", diff --git a/Cargo.toml b/Cargo.toml index de9b89d..27931ea 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,7 +20,7 @@ crate-type = ["cdylib"] # (see MOBILE_PATCHES.md for what each carries on top of upstream). gpui = { package = "gpui-pre", version = "=0.3.8", default-features = false } gpui-kit = { git = "https://github.com/Bombatomica64/gpui-kit", rev = "e8b28164ac7fa7e1f7d393d441beb738c286bbc8", default-features = false, features = ["component", "assets"] } -gpui-mobile = { package = "gpui-pre-mobile", git = "https://github.com/Bombatomica64/gpui-mobile", rev = "c71a31aaa12660552ca7a772f6fdd890d9c8cf7a", default-features = false, features = [ +gpui-mobile = { package = "gpui-pre-mobile", git = "https://github.com/Bombatomica64/gpui-mobile", rev = "4acd1761b4b2a734f49ff2bce930c61b01cfe5be", default-features = false, features = [ "clipboard", "share", # Exercised by the Platform APIs screen; build.sh copies their Java helpers from this checkout. "file_selector", "image_picker", "local_auth", "permission_handler", "calendar", "contacts", diff --git a/MOBILE_PATCHES.md b/MOBILE_PATCHES.md index fcc601a..2e3345b 100644 --- a/MOBILE_PATCHES.md +++ b/MOBILE_PATCHES.md @@ -35,6 +35,19 @@ follows whichever window is active. [Bounds](docs/demos/android-a11y-bounds.png), [recording](docs/demos/android-a11y-talkback.gif). +Review changes requested; rebased onto upstream `main` and fixed in separate +commits. The reply draft is in +[docs/upstream/replies](docs/upstream/replies/gpui-mobile-25.md). The +adapter now goes on each host's own Activity: hosts call the new +`host::set_activity(env, host, activity)` (the lab does) in place of +`set_host_activity`. It is attached when the window becomes active rather than +in `onCreate`, is retried after a failure, and is dropped with its window. A +view that already has another delegate is skipped. The exception-clearing +branch below is folded in. One gap stays open: a screen reader turned off +between the GPUI-side `isEnabled()` check and `accesskit_android`'s UI-thread +runnable still throws, because only the adapter can check there +(`accesskit_android` 0.9 does, but needs `accesskit` 0.25). + ### Keep focus when the IME's Done action hides the keyboard — branch `android-ime-dismiss` Upstream (#26) stopped back from injecting an `escape` keystroke (IME event @@ -97,7 +110,7 @@ change. `safe_area_insets()` and `keyboard_height()` return them too, as on iOS. LabActivity calls it from its `OnApplyWindowInsetsListener`. Draft upstream PR: [docs/upstream/safe-area-insets.md](docs/upstream/safe-area-insets.md). -### Clear the Java exceptions the accessibility bridge leaves pending — branch `android-a11y-clear-exceptions` +### Clear the Java exceptions the accessibility bridge leaves pending — branch `android-a11y-clear-exceptions`, now part of `android-accessibility` `accesskit_android`'s jni 0.21 API leaves a thrown Java exception pending, and the bridge never cleared it, so the next JNI call on the render thread diff --git a/android/app/src/main/java/dev/gpui/mobile/lab/LabActivity.java b/android/app/src/main/java/dev/gpui/mobile/lab/LabActivity.java index 838404e..8e29a10 100644 --- a/android/app/src/main/java/dev/gpui/mobile/lab/LabActivity.java +++ b/android/app/src/main/java/dev/gpui/mobile/lab/LabActivity.java @@ -106,7 +106,7 @@ protected void onCreate(Bundle state) { return insets; }); - nativeOnCreate(this, Build.VERSION.SDK_INT); + nativeOnCreate(this, hostId, Build.VERSION.SDK_INT); forwardScreenExtra(getIntent()); } @@ -453,7 +453,7 @@ private void endEdit() { // ── Native (src/host.rs) ─────────────────────────────────────────────── - private static native void nativeOnCreate(Activity activity, int apiLevel); + private static native void nativeOnCreate(Activity activity, long host, int apiLevel); private static native void nativeOpenScreen(String screen); private static native void nativeDeepLink(String url); private static native void nativeConfigurationChanged(); diff --git a/docs/upstream/README.md b/docs/upstream/README.md index 47054d0..f3f395d 100644 --- a/docs/upstream/README.md +++ b/docs/upstream/README.md @@ -33,7 +33,7 @@ captures in [../demos](../demos) and say where a capture is missing. | 11 | mobile | [`upstream/android-ime-dismiss`](https://github.com/Bombatomica64/gpui-mobile/tree/upstream/android-ime-dismiss) | ✅ | — | after only | [draft](gpui-mobile/android-ime-dismiss.md) | | 12 | kit | [`upstream/input-tap-requests-keyboard`](https://github.com/Bombatomica64/gpui-kit/tree/upstream/input-tap-requests-keyboard) | ⏳ | #11 (otherwise it's a no-op on Android) | no | [draft](gpui-kit/input-tap-requests-keyboard.md) | | 13 | mobile | [`upstream/android-text-input-configuration`](https://github.com/Bombatomica64/gpui-mobile/tree/upstream/android-text-input-configuration) | ⏳ | #11 merged, #4 released in Kit | yes | [draft](gpui-mobile/android-text-input-configuration.md) | -| 14 | mobile | [`upstream/android-a11y-clear-exceptions`](https://github.com/Bombatomica64/gpui-mobile/tree/upstream/android-a11y-clear-exceptions) | ⏳ | open #25 (push it there, or send after #25 merges) | not possible (code audit) | [draft](gpui-mobile/android-a11y-clear-exceptions.md) | +| 14 | mobile | [`upstream/android-a11y-clear-exceptions`](https://github.com/Bombatomica64/gpui-mobile/tree/upstream/android-a11y-clear-exceptions) | ✅ | pushed to #25 (7006f50) with the review fixes | not possible (code audit) | [draft](gpui-mobile/android-a11y-clear-exceptions.md) | | 15 | kit | [`upstream/input-reveal-on-focus`](https://github.com/Bombatomica64/gpui-kit/tree/upstream/input-reveal-on-focus) | ⏳ | Zed autoscroll PR in a gpui-pre release (its own test fails until then) | no | [draft](gpui-kit/input-reveal-on-focus.md) | | 16 | mobile | [`upstream/android-text-input-purpose`](https://github.com/Bombatomica64/gpui-mobile/tree/upstream/android-text-input-purpose) | ⛔ | #13, plus Zed `TextInputConfiguration::purpose` in a gpui-pre release; doesn't build until then | yes | [draft](gpui-mobile/android-text-input-purpose.md) | | 17 | kit | `input-purpose` (fork branch only) | ⛔ | #4, plus the same GPUI change; conflicts with `main` (to rebase then) | yes | — | diff --git a/docs/upstream/replies/gpui-mobile-25.md b/docs/upstream/replies/gpui-mobile-25.md new file mode 100644 index 0000000..345ab34 --- /dev/null +++ b/docs/upstream/replies/gpui-mobile-25.md @@ -0,0 +1,61 @@ +# Reply to huacnlee's review of longbridge/gpui-mobile#25 + +Draft, to be posted by hand. The branch was rebased onto `main` (it conflicted +with #29 in `jni.rs`) and force-pushed; the fixes are separate commits on top, +head [31d79d8](https://github.com/longbridge/gpui-mobile/pull/25/commits/31d79d8f51ac71eb58bad9fa433e17758002cf96). + +--- + +Thanks for the careful review! I rebased onto `main` (it conflicted with #29) +and added one commit per point: + +1. **Wrong Activity after Back** (2b761c1). New `host::set_activity(env, host, + activity)` does what `set_host_activity` does and also remembers the Activity + per `HostId`. Each window knows its host, so the adapter goes on that host's + own Activity. Hosts that only call `set_host_activity` keep the old behaviour, + and the "What the host has to do" docs now point to `set_activity`. +2. **`isEnabled()` race** (not fully fixed, see below). I don't think + gpui-mobile can close it on its own. The runnable that raises the events is + posted by `InjectingAdapter::update_if_active` inside `accesskit_android`, + and nothing runs between it and `requestSendAccessibilityEvent`. A + `Looper`-side check could still lose to a queued `MSG_SET_STATE`. + `accesskit_android` 0.9 fixes this the way you describe: `QueuedEvents::raise` + checks `isEnabled()` on the UI thread. But 0.9 needs `accesskit` 0.25, and + gpui-pre 0.3.8 is on 0.24. What changed here: + - 7006f50 clears the Java exceptions that `accesskit_android`'s jni 0.21 + leaves pending. + - 31d79d8 makes the GPUI-thread check cheaper and documents the remaining + window in the module docs. + + The real fix is either a backported `accesskit_android` 0.7.x, which I can + ask AccessKit for, or moving to 0.9 once gpui-pre is on `accesskit` 0.25. + Would you merge with this documented, or wait for one of those? +3. **`BRIDGE` not cleared** (d0bea7c). `HostDestroyed` and `Drop for + AndroidWindow` now drop the bridge, and with it the adapter, when it belongs + to that window. The closed window's callbacks are not called. +4. **Existing AccessibilityDelegate** (34f8c96). Before creating the adapter, + `attach` reads `getAccessibilityDelegate()`. If the delegate is not an earlier + AccessKit one (whose removal is already queued ahead), it logs and skips the + view. +5. **No retry** (ea97c6d). `init` now attaches again when there is no adapter. + Since 9418278 it also does so when the window's Activity has changed. +6. **Attach before `setContentView`** (9418278). `set_host_activity` no longer + touches the adapter. It is attached whenever the window becomes active: on + resume, and again (as a cheap no-op if nothing changed) when its surface + arrives. + +Non-blocking: + +- **`isEnabled()` under the lock** (31d79d8): the `AccessibilityManager` and + the `JavaVM` are cached once, and the check runs before `BRIDGE` is taken. + Nothing takes `BRIDGE` on the UI thread any more. An + `AccessibilityStateChangeListener` would need a Java class, which this crate + doesn't ship, so I kept the per-frame call. It is a synchronized field read. +- **Two jni versions**: the second one stays as long as we use + `accesskit_android`, because `InjectingAdapter::new` takes its jni 0.21 + `JNIEnv`. The VM is no longer rebuilt per attach (31d79d8). +- **Hover before `hit_test`** (6d443bd): moved. + +Checked: `cargo check` and `cargo clippy --target aarch64-linux-android` (no +warnings in the touched files), `cargo fmt --check`, and `cargo test --lib` +(46 passed). On a device: pending — I will add TalkBack results (toggling it off mid-draw, Back from a second screen) here before posting. diff --git a/src/host.rs b/src/host.rs index 4bfd4d8..3268761 100644 --- a/src/host.rs +++ b/src/host.rs @@ -45,12 +45,14 @@ pub extern "system" fn Java_dev_gpui_mobile_lab_LabActivity_nativeOnCreate<'loca mut env: EnvUnowned<'local>, _this: JObject<'local>, activity: JObject<'local>, + host: i64, api_level: i32, ) { init_logging(); env.with_env(|env| -> jni::errors::Result<()> { - if let Err(err) = mobile_jni::set_host_activity(env, &activity) { - log::error!("set_host_activity failed: {err}"); + // Per host, so the screen reader goes on this Activity's views after Back. + if let Err(err) = host::set_activity(env, host as host::HostId, &activity) { + log::error!("set_activity failed: {err}"); } Ok(()) })