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.

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

Conversation

xorgy
Copy link
Contributor

@xorgy xorgy commented Nov 12, 2024

  • Tested on all platforms changed
  • Added an entry to the changelog module if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

@xorgy
Copy link
Contributor Author

xorgy commented Nov 12, 2024

So I've tried this on device now, and the Window::theme implementation works, 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).

Comment on lines +208 to +209
let old_config = self.config.clone();
self.config = self.android_app.config();
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

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants