Implement AppearanceModule #4622
Conversation
This change adds an implementation of AppearanceModule. This ends up being a little bit hairy, since it exposes sync methods, but on Windows we can only check app theme on the UI thread. We could have reused the model of AppTheme and asked to construct some partial state on the UI thread and pass to the module, but this ends up being somewhat cumbersome. We instead will post to the UI thread internally, and block on a result before returning initial data. This should never happen in practice. Validated we can correctly switch between light-mode and dark-mode RNTester when not using web debugging (where Appearance is disabled JS side since Chrome can't handle sync methods).
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| #pragma once |
There was a problem hiding this comment.
Not related, just noticed this was missing
| namespace react::uwp { | ||
|
|
||
| // Listens for the current theme on the UI thread, storing the most recent. Will emit JS events on Appearance change. | ||
| class AppearanceChangeListener final : public Mso::ActiveObject<> { |
There was a problem hiding this comment.
This is a very weird ActiveObject. Initially I wanted to do more, but then I discovered:
- The thread you construct
UISettingson doesn't matter in terms of where it notifies you, and it itself is apparently freethreaded, so we don't delay construction until init m_currentThemewas going to be anActiveReadableFieldset by the UI thread, but adding a mutex felt like extra overhead when we could use an atomic for an integral type, so I didn't do thatIReactInstanceis already free-threaded and didn't need protection it sounded like?
In the end, we basically just use ActiveObject to associate the UI Queue without really needing a lot of its features. I could this this making sense to just be a normal object keeping a queue instead, but don't think there would be too much of a difference.
|
Relevant to #4322 |
| m_inited.Set(); | ||
| } | ||
|
|
||
| const char *AppearanceChangeListener::GetColorScheme() noexcept { |
There was a problem hiding this comment.
[](start = 54, length = 1)
make this method const
|
|
||
| // UISettings will notify us on a background thread regardless of where we construct it or register for events. Let it | ||
| // be constructed before init, but redirect callbacks to the UI thread where we can check app theme. | ||
| m_uiSettings.ColorValuesChanged([weakThis{Mso::WeakPtr(this)}](const auto &, const auto &) noexcept { |
There was a problem hiding this comment.
I should assign this to a revoker. I kind of assumes that death of UISettings would deregister events, but this isn't super well documented so I should do the safe thing.
There was a problem hiding this comment.
Yes, it would be nice, otherwise after several reloads we may get interesting behavior.
In reply to: 409749605 [](ancestors = 409749605)
| m_inited.Set(); | ||
| } | ||
|
|
||
| const char *AppearanceChangeListener::GetColorScheme() noexcept { |
|
|
||
| const char *AppearanceChangeListener::GetColorScheme() noexcept { | ||
| // Wait until we have an initial valid value from the UI thread before returning anything | ||
| m_inited.Wait(); |
There was a problem hiding this comment.
.Wait(); [](start = 10, length = 8)
It is ugly. It would be better not to lock the JS thread. It could be a serious perf bottleneck.
A better approach to initialize it upfront as we do with other UI-aware native modules.
There was a problem hiding this comment.
The problem is that we initialize the native modules on-demand and this code is always going to cause per overhead by stopping JS thread.
Please see how I solve a similar issue with the Linking module: LinkingManagerModule.cpp
There is a static part that always has information available and to provide notifications. The native modules picks it up and subscribes to events without waiting.
In reply to: 409881033 [](ancestors = 409881033)
There was a problem hiding this comment.
I went back and forward on this, I don't think this is ever a perf issue because the UI code to initialize the listener will fire long before the theme is requested, so the lock is just a safety measure. In general I agree that I don't like locking though.
FWIW, any synchronous native module call will already block the JS thread until the native thread can pump all messages and respond. That's not strictly the same as blocking on the UI thread admittedly.
The advantage we get here is that the threadiness of this module becomes an implementation detail that consumers don't need to worry about. E.g. we won't need to do any special initialization shenanigans when refactoring all of this later.
There was a problem hiding this comment.
I thought about this more overnight. I still don't think it's likely we will ever lock, but at the point where it's not trivial to reason about, we probably shouldn't be using locks. I will change this to follow the pattern of other modules. I'm interested to see the end what we end up coming up with for this problem in native modules 2.0.
ghost
added
Needs: Author Feedback
| $(ReactNativeWindowsDir)Common; | ||
| $(ReactNativeWindowsDir)JSI\Shared; | ||
| $(ReactNativeWindowsDir)Pch; | ||
| $(ReactNativeWindowsDir)ReactUwp\Pch; |
There was a problem hiding this comment.
ReactUwp [](start = 32, length = 8)
Good catch! nit: Please keep the original folder name spelling: ReactUWP.
There was a problem hiding this comment.
Going to go ahead and merge as is since we will be nuking the project soonish anyway.
* Implement AppearanceModule This change adds an implementation of AppearanceModule. This ends up being a little bit hairy, since it exposes sync methods, but on Windows we can only check app theme on the UI thread. We could have reused the model of AppTheme and asked to construct some partial state on the UI thread and pass to the module, but this ends up being somewhat cumbersome. We instead will post to the UI thread internally, and block on a result before returning initial data. This should never happen in practice. Validated we can correctly switch between light-mode and dark-mode RNTester when not using web debugging (where Appearance is disabled JS side since Chrome can't handle sync methods). * Change files * Revert an accidental change * Fix Build For ReactUwp/Win32 Playground * Address Feedback
3 participants
This change adds an implementation of AppearanceModule. This ends up being a little bit hairy, since it exposes sync methods, but on Windows we can only check app theme on the UI thread.
We could have reused the model of AppTheme and asked to construct some partial state on the UI thread and pass to the module, but this ends up being somewhat cumbersome. We instead will post to the UI thread internally, and block on a result before returning initial data. This should never happen in practice.
Validated we can correctly switch between light-mode and dark-mode RNTester when not using web debugging (where Appearance is disabled JS side since Chrome can't handle sync methods).
Microsoft Reviewers: Open in CodeFlow