-
Notifications
You must be signed in to change notification settings - Fork 920
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
base: master
Are you sure you want to change the base?
Conversation
So I've tried this on device now, and the |
let old_config = self.config.clone(); | ||
self.config = self.android_app.config(); |
There was a problem hiding this comment.
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 newConfiguration
at runtime even if the theme changes (yes, withuiMode
inandroid: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>
:
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:
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
changelog
module if knowledge of this change could be valuable to users