diff --git a/docs/demos/android-night-mode-review.png b/docs/demos/android-night-mode-review.png new file mode 100644 index 0000000..f886242 Binary files /dev/null and b/docs/demos/android-night-mode-review.png differ diff --git a/docs/upstream/replies/gpui-mobile-30.md b/docs/upstream/replies/gpui-mobile-30.md index e413b81..c10a6c4 100644 --- a/docs/upstream/replies/gpui-mobile-30.md +++ b/docs/upstream/replies/gpui-mobile-30.md @@ -1,49 +1,62 @@ # Reply to huacnlee's review of longbridge/gpui-mobile#30 Draft, to be posted by hand. The review (12 Oct, CHANGES_REQUESTED) was on -c62c318. The force-push later that day to -[ec5c89d](https://github.com/longbridge/gpui-mobile/pull/30/commits/ec5c89dfe3ab399bcc90a01f03cf89873cb750b2) -only rebased the PR onto main after #29 merged: `HOST_ASSETS` now gets its -AssetManager from the `HOST_APPLICATION` context this PR keeps, instead of -calling `getApplicationContext()` a second time. `host.rs` and `platform.rs` are -unchanged apart from line offsets. **None of the review points are addressed -yet.** So this reply says that and lays out the plan. It should be rewritten -once the fixes are pushed. +c62c318. The force-push that day (ec5c89d) was only a rebase onto #29. All +review points are addressed in +[fa0075b](https://github.com/longbridge/gpui-mobile/pull/30/commits/fa0075b137437b34daeaf6a0d8d4a33686423384), +pushed on top of ec5c89d as a separate commit. --- -Thanks for the careful review! One thing to clear up first: the force-push on -12 Oct was only a rebase onto main after #29 merged. It now shares the -application context that #29's `HOST_ASSETS` needs, and nothing else changed. -It does not address your points yet. Sorry it looked like a response. Here is -what I plan to do for each one: - -1. **android-activity path starts Light.** You're right. `window_appearance()` - now reports the real mode, but nothing on the `InitWindow` path sets the - window's own appearance, so the two disagree until a `ConfigChanged`. Fixed - by item 2. -2. **Sync in the shared code.** I'll move the initial `sync_appearance` into - `AndroidPlatform::open_window` and after `init_window`, so both entry points - go through it, and remove the call-site copies in `host.rs` that this makes - redundant. I'll keep the one in `resumed`. -3. **App-local night mode.** Agreed. I'll read `uiMode` from the current host - Activity's Resources instead of the application context, so - `AppCompatDelegate.setDefaultNightMode` is respected. `HOST_APPLICATION` - stays only for the AssetManager. -4. **Order of config changes.** I'll change the signature to - `host::configuration_changed(ui_mode: i32)` and have the host pass - `newConfig.uiMode`, so nothing re-reads a configuration that may not be - updated yet. The host docs will show the call. - -On the non-blocking points, I'll do all three in the same push: - -- `window_appearance()` will return the value last stored by `sync_appearance` - instead of querying over JNI or building an `AConfiguration` on every call. -- The `ConfigurationChanged` handler will work out the appearance once and call - `set_appearance` on each window. -- A failed `host_ui_mode` will log a `warn!` instead of quietly meaning Light. - (Today only a failed `getApplicationContext()` in `set_host_activity` logs.) - -I'll reply again with the commit once it's pushed. None of this has been -verified on a device yet. I'll test a cold start in dark mode on both paths, and -an in-app night-mode toggle, before asking for another look. +Thanks for the review, and sorry for the silent force-push on the 12th: that +was only a rebase onto #29 and did not address anything. The fixes are in +fa0075b, on top of it: + +1. **android-activity path starts Light.** Fixed by item 2. On a cold start in + dark mode, `cx.window_appearance()` and `window.appearance()` both say Dark + on the first frame now. Before, the window said Light. +2. **Sync in the shared code.** `AndroidPlatform::open_window` applies the night + mode, and so does a new `AndroidPlatform::reinit_window`, which both + `InitWindow` and the host's reattach now use instead of calling + `init_window` directly. The call-site syncs in `host.rs` are gone. Only + `resumed` still re-reads it, in case it changed while another app was in + front. +3. **App-local night mode.** `uiMode` now comes from the current host Activity's + Resources, so `AppCompatDelegate.setDefaultNightMode` counts. With the + application context no longer needed for night mode, I dropped the global + this PR had added. The AssetManager is fetched as on main again. +4. **Order of config changes.** It's now `host::configuration_changed(ui_mode: i32)`, + and the host passes `newConfig.uiMode`. Nothing re-reads a configuration in + that path. The module docs show the call. + +Non-blocking: + +- The platform stores the appearance it last applied, and + `window_appearance()` just returns it, with no JNI call or `AConfiguration`. + It has its own small lock rather than living in the platform state, because + GPUI reads it from main-thread tasks, and `flush_main_thread_tasks()` runs + those with the state locked. My first version stored it in the state and + deadlocked on the first night-mode change on the android-activity path. + The device test below caught it. +- `ConfigurationChanged` reads the night mode once and applies it to every + window (`AndroidPlatform::set_appearance`). +- A failed `uiMode` read logs a `warn!`. The read also runs in a local frame + now, since the render thread never returns to Java. + +Tested on a OnePlus with Android 16, switching with `cmd uimode night`: + +- host path (Bombatomica64/Gpui-android, Diagnostics screen): cold start in + dark mode, a live toggle to light (the Activity handles `uiMode`), back to + dark while in the background then resumed, and a light system with the + Activity forced to dark the way AppCompat does it. All four show the right + appearance. +- android-activity path (this repo's example, with a log in an + `observe_window_appearance` callback): a cold start in dark mode logs + `platform=Dark window=Dark` (ec5c89d logged `platform=Dark window=Light`), and + toggling light and back fires the observer each time with both values + agreeing. + +![Cold start dark, live toggle to light, app-local dark on a light system](https://raw.githubusercontent.com/Bombatomica64/Gpui-android/main/docs/demos/android-night-mode-review.png) + +One API change for hosts: `configuration_changed` now takes the `uiMode` +argument.