Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Android: Implement Window::theme and WindowEvent::ThemeChanged. #3995

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Android: Implement Window::theme and WindowEvent::ThemeChanged. #3995

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 32 additions & 7 deletions src/platform_impl/android/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use android_activity::input::{InputEvent, KeyAction, Keycode, MotionAction};
use android_activity::{
AndroidApp, AndroidAppWaker, ConfigurationRef, InputStatus, MainEvent, Rect,
};
use ndk::configuration::UiModeNight;
use tracing::{debug, trace, warn};

use crate::application::ApplicationHandler;
Expand Down Expand Up @@ -101,6 +102,7 @@ pub struct KeyEventExtra {}

pub struct EventLoop {
pub(crate) android_app: AndroidApp,
config: ConfigurationRef,
window_target: ActiveEventLoop,
redraw_flag: SharedFlag,
loop_running: bool, // Dispatched `NewEvents<Init>`
Expand Down Expand Up @@ -141,6 +143,7 @@ impl EventLoop {

Ok(Self {
android_app: android_app.clone(),
config: android_app.config(),
primary_pointer: None,
window_target: ActiveEventLoop {
app: android_app.clone(),
Expand Down Expand Up @@ -202,8 +205,11 @@ impl EventLoop {
app.window_event(&self.window_target, GLOBAL_WINDOW, event);
},
MainEvent::ConfigChanged { .. } => {
let old_scale_factor = scale_factor(&self.android_app);
let scale_factor = scale_factor(&self.android_app);
let old_config = self.config.clone();
self.config = self.android_app.config();
Comment on lines +208 to +209
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

except that at least with NativeActivity we don't get a new Configuration at runtime even if the theme changes (yes, with uiMode in android:configChanges).

Are you saying that at least MainEvent::ConfigChanged is being raised? It is expected that the config doesn't change, because android-activity's ConfigurationRef that you're also storing in winit here is actually an Arc<RwLock>:

https://github.com/rust-mobile/android-activity/blob/0d299300f4120821ae1fcaaf0276129c512c2c96/android-activity/src/config.rs#L9-L19

And when that MainEvent::ConfigChanged event is raised, its internal ndk::configuration::Configuration is replaced, but the surrounding ConfigurationRef remains the same so all the clones you're making here exactly to check for specific changes are thrown out of the window:

https://github.com/rust-mobile/android-activity/blob/0d299300f4120821ae1fcaaf0276129c512c2c96/android-activity/src/native_activity/glue.rs#L599-L606

I believe this footgun is at the very least very poorly documented in intent as highlighted in rust-mobile/android-activity#164 (comment). That PR changes some of it but I believe we can do even better to highlight this mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic lol. I threw this up before even running it, and tried it once. I'll see if I can figure that bit out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this is the only issue though, because I made sure to write my example to run Window::theme every time it draws, which means that the configuration is also stale.

Copy link
Member

@MarijnS95 MarijnS95 Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds familiar. I experimented with android:configChanges="screenLayout" once to turn a full activity recreate (which you know isn't functional in Rust until I submit my branches) into a simple ConfigChanged event whenever an activity changes between full-screen, split-screen, and freefloating. Despite raising a ConfigChanged event, none of the properties actually changed 😅

Copy link
Contributor Author

@xorgy xorgy Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... so I think I'll just push the Window::theme implementation into its own branch for now, and add a documentation caveat until we can figure out why we're not getting a fresh config. I understand in Java land we would have to reload all the resources to get the config, so I guess that's what's not happening. Probably a nice thing to put in the ConfigChanged event would be a copy of both the old and new configs; then we don't need to store anything.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I'd have loved for the events to be more stateful, both in the public API as well as internal to android-activity.


let old_scale_factor = scale_factor(&old_config);
let scale_factor = scale_factor(&self.config);
if (scale_factor - old_scale_factor).abs() < f64::EPSILON {
let new_surface_size = Arc::new(Mutex::new(screen_size(&self.android_app)));
let event = event::WindowEvent::ScaleFactorChanged {
Expand All @@ -215,6 +221,15 @@ impl EventLoop {

app.window_event(&self.window_target, GLOBAL_WINDOW, event);
}

let old_theme = config_theme(&old_config);
let theme = config_theme(&self.config);
if old_theme != theme {
if let Some(theme) = theme {
let event = event::WindowEvent::ThemeChanged(theme);
app.window_event(&self.window_target, GLOBAL_WINDOW, event);
}
}
},
MainEvent::LowMemory => {
app.memory_warning(&self.window_target);
Expand Down Expand Up @@ -693,7 +708,7 @@ impl RootActiveEventLoop for ActiveEventLoop {
}

fn system_theme(&self) -> Option<Theme> {
None
config_theme(&self.app.config())
}

fn listen_device_events(&self, _allowed: DeviceEvents) {}
Expand Down Expand Up @@ -822,7 +837,7 @@ impl CoreWindow for Window {
}

fn scale_factor(&self) -> f64 {
scale_factor(&self.app)
scale_factor(&self.app.config())
}

fn request_redraw(&self) {
Expand Down Expand Up @@ -959,7 +974,7 @@ impl CoreWindow for Window {
fn set_theme(&self, _theme: Option<Theme>) {}

fn theme(&self) -> Option<Theme> {
None
config_theme(&self.app.config())
}

fn set_content_protected(&self, _protected: bool) {}
Expand Down Expand Up @@ -1047,6 +1062,16 @@ fn screen_size(app: &AndroidApp) -> PhysicalSize<u32> {
}
}

fn scale_factor(app: &AndroidApp) -> f64 {
app.config().density().map(|dpi| dpi as f64 / 160.0).unwrap_or(1.0)
fn scale_factor(cfg: &ConfigurationRef) -> f64 {
cfg.density().map(|dpi| dpi as f64 / 160.0).unwrap_or(1.0)
}

fn config_theme(cfg: &ConfigurationRef) -> Option<Theme> {
use UiModeNight::*;
match cfg.ui_mode_night() {
No => Some(Theme::Light),
Yes => Some(Theme::Dark),
Any => None,
_ => None,
}
}