From bb60a2e8912b09a38881ff5c4881cb669248ba10 Mon Sep 17 00:00:00 2001 From: lorenzomicheli Date: Mon, 12 Oct 2026 08:00:52 +0000 Subject: [PATCH] gpui-mobile: pin the #29 review follow-up, draft the reply Pins Bombatomica64/gpui-mobile main at 2456b8e (fork PR #11), which merges the review fixes for longbridge/gpui-mobile#29. The reply to the review is drafted in docs/upstream/replies/gpui-mobile-29.md. Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 2 +- Cargo.toml | 2 +- MOBILE_PATCHES.md | 4 ++- docs/upstream/replies/gpui-mobile-29.md | 35 +++++++++++++++++++++++++ 4 files changed, 40 insertions(+), 3 deletions(-) create mode 100644 docs/upstream/replies/gpui-mobile-29.md diff --git a/Cargo.lock b/Cargo.lock index d910a42..b9c8ef5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2706,7 +2706,7 @@ dependencies = [ [[package]] name = "gpui-pre-mobile" version = "0.1.0" -source = "git+https://github.com/Bombatomica64/gpui-mobile?rev=dc07d18a7e5590293b4f0759b4b7e290cca5f3c2#dc07d18a7e5590293b4f0759b4b7e290cca5f3c2" +source = "git+https://github.com/Bombatomica64/gpui-mobile?rev=2456b8e91ca98f37cea00c8e6a1a3e1b0f8a2410#2456b8e91ca98f37cea00c8e6a1a3e1b0f8a2410" dependencies = [ "accesskit_android", "android-activity", diff --git a/Cargo.toml b/Cargo.toml index b51a932..8e5fdc9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,7 +15,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 = "d47dd0bde03345977db286f9bc8224bf1199cb60", default-features = false, features = ["component", "assets"] } -gpui-mobile = { package = "gpui-pre-mobile", git = "https://github.com/Bombatomica64/gpui-mobile", rev = "dc07d18a7e5590293b4f0759b4b7e290cca5f3c2", default-features = false, features = [ +gpui-mobile = { package = "gpui-pre-mobile", git = "https://github.com/Bombatomica64/gpui-mobile", rev = "2456b8e91ca98f37cea00c8e6a1a3e1b0f8a2410", 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 3612933..ee21c8a 100644 --- a/MOBILE_PATCHES.md +++ b/MOBILE_PATCHES.md @@ -57,7 +57,9 @@ the new `host::configuration_changed()`, which updates every host's window. Android 13+'s COLRv1 emoji font cannot be drawn by swash, and the bundled CBDT fallback was only read through `android-activity`'s AssetManager, so emoji were empty boxes. The first `set_host_activity` keeps the -application's AssetManager for the life of the process. +application's AssetManager for the life of the process; if the platform +started before it, the font is loaded then. Review changes requested; the +reply draft is in [docs/upstream/replies](docs/upstream/replies/gpui-mobile-29.md). Both were one branch, `android-host-appearance-emoji`, until an adversarial review: night mode came from whichever Activity registered last, and the diff --git a/docs/upstream/replies/gpui-mobile-29.md b/docs/upstream/replies/gpui-mobile-29.md new file mode 100644 index 0000000..19a9842 --- /dev/null +++ b/docs/upstream/replies/gpui-mobile-29.md @@ -0,0 +1,35 @@ +# Reply to huacnlee's review of longbridge/gpui-mobile#29 + +Draft, to be posted by hand. All changes are in +[773e5ad](https://github.com/longbridge/gpui-mobile/pull/29/commits/773e5ade2353399a3bac9f848c8dfd4b4c9a1e19). + +--- + +Thanks for the careful review! All three points are addressed in 773e5ad: + +1. **Call order / load once.** I went with the lazy option, plus the other two. + If `AndroidPlatform::new` finds no AssetManager, it logs a `warn!` and keeps + its text system pending; the next `set_host_activity` that gets an + AssetManager loads the font into it. This covers `start` running first and a + failed `getApplicationContext()`/`getAssets()` on the first `onCreate`. A + mutex is held across the check so a concurrent `set_host_activity` can't slip + in between. The "What the host has to do" section in `host.rs` now says + `set_host_activity` comes before `start`, and what happens if it doesn't. +2. **Local refs.** `host_assets` now runs inside `env.with_local_frame(2, …)`, + so `context` and `assets` are freed even if the caller stays in native code. +3. **Pending exceptions.** `set_host_activity` is now a thin wrapper that clears + any pending exception whenever the inner function returns `Err`, so the + `new_global_ref` / `get_java_vm` paths behave like the asset branch. + +On the suggestions: + +- **Night mode:** #30 covers this. It reads `uiMode` from the application + context's `Configuration` over JNI on the host path, so it doesn't depend on + this PR. I'd rather keep the two PRs independent; once both are in, switching + it to `with_asset_manager(Configuration::from_asset_manager)` is a small + follow-up if you prefer that. +- **`to_vec()` copy:** done here, since it was small: `load_asset_bytes` now + reads the asset into one `Vec::with_capacity(asset.length())` instead of + mapping it and copying. + +Checked with `cargo clippy --target aarch64-linux-android` (no new warnings).