-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add UIDispatcher property to ReactInstanceSettings and IReactContext #5007
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
Changes from all commits
eb9d064
db98d7c
22245fc
40d5e6e
122b3ab
fdecd92
f916be0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| { | ||
| "type": "prerelease", | ||
| "comment": "Add UIDispatcher property to ReactInstanceSettings and IReactContext", | ||
| "packageName": "react-native-windows", | ||
| "email": "[email protected]", | ||
| "dependentChangeType": "patch", | ||
| "date": "2020-05-25T22:56:21.049Z" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| #include "pch.h" | ||
|
|
||
| #include <winrt/Microsoft.ReactNative.h> | ||
| #include <winrt/Windows.System.h> | ||
|
|
||
| using namespace winrt; | ||
| using namespace Microsoft::ReactNative; | ||
| using namespace Windows::System; | ||
|
|
||
| namespace ReactNativeIntegrationTests { | ||
|
|
||
| TEST_CLASS (ReactInstanceSettingsTests) { | ||
| TEST_METHOD(DefaultUIDispatcher_NonUIThread) { | ||
| // In case if current thread has no ThreadDispatcher, then the | ||
| // ReactInstanceSettings::UIDispatcher is not initialized. | ||
| ReactInstanceSettings settings; | ||
| TestCheck(settings); | ||
| TestCheck(!settings.UIDispatcher()); | ||
| // UIDispatcher() is a shortcut for getting property value. | ||
| TestCheck(!settings.Properties().Get(ReactDispatcherHelper::UIDispatcherProperty())); | ||
| } | ||
|
|
||
| TEST_METHOD(DefaultUIDispatcher_UIThread) { | ||
| // ReactInstanceSettings::UIDispatcher is set to a non-null value if it is | ||
| // creates from a UI thread. We simulate the UI thread with DispatcherQueueController. | ||
| auto queueController = DispatcherQueueController::CreateOnDedicatedThread(); | ||
| auto uiDispatcher = queueController.DispatcherQueue(); | ||
| TestCheck(uiDispatcher); | ||
| uiDispatcher.TryEnqueue([]() noexcept { | ||
| ReactInstanceSettings settings; | ||
| TestCheck(settings); | ||
| TestCheck(settings.UIDispatcher()); | ||
| // UIDispatcher() is a shortcut for getting property value. | ||
| TestCheck(settings.Properties().Get(ReactDispatcherHelper::UIDispatcherProperty())); | ||
| TestCheckEqual(ReactDispatcherHelper::UIThreadDispatcher(), settings.UIDispatcher()); | ||
| TestCheckEqual( | ||
| ReactDispatcherHelper::UIThreadDispatcher(), | ||
| settings.Properties().Get(ReactDispatcherHelper::UIDispatcherProperty())); | ||
| }); | ||
| queueController.ShutdownQueueAsync().get(); | ||
| } | ||
| }; | ||
|
|
||
| } // namespace ReactNativeIntegrationTests |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,20 +29,40 @@ void ReactDispatcher::Post(ReactDispatcherCallback const &callback) noexcept { | |
| } | ||
|
|
||
| /*static*/ IReactDispatcher ReactDispatcher::UIThreadDispatcher() noexcept { | ||
| return make<ReactDispatcher>(Mso::DispatchQueue::MakeCurrentThreadUIQueue()); | ||
| static thread_local weak_ref<IReactDispatcher> *tlsWeakDispatcher{nullptr}; | ||
| IReactDispatcher dispatcher{nullptr}; | ||
| auto queue = Mso::DispatchQueue::GetCurrentUIThreadQueue(); | ||
| if (queue && queue.HasThreadAccess()) { | ||
| queue.InvokeElsePost([&queue, &dispatcher ]() noexcept { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't there an issue if this ends up posting. Wouldn't it be non-deterministic if dispatcher gets set before the end of the function or not? Should this instead be a check of HasThreadAccess? #Resolved
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this context I have got the queue from the thread. It must have the thread access. I can add the HasThreadAccess to be more obvious and a comment why it is done (to access the Queue Local values). In reply to: 430521560 [](ancestors = 430521560)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the HasThreadAccess check and comments. In reply to: 430588145 [](ancestors = 430588145,430521560) |
||
| // This code runs synchronously, but we want it to be run the queue context to | ||
| // access the queue local value where we store the weak_ref to the dispatcher. | ||
| // The queue local values are destroyed along with the queue. | ||
| // To access queue local value we temporary swap it with the thread local value. | ||
| // It must be a TLS value to ensure proper indexing of the queue local value entry. | ||
| auto tlsGuard{queue.LockLocalValue(&tlsWeakDispatcher)}; | ||
| dispatcher = tlsWeakDispatcher->get(); | ||
| if (!dispatcher) { | ||
| dispatcher = winrt::make<ReactDispatcher>(std::move(queue)); | ||
| *tlsWeakDispatcher = dispatcher; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| return dispatcher; | ||
| } | ||
|
|
||
| /*static*/ ReactPropertyId<IReactDispatcher> ReactDispatcher::UIDispatcherProperty() noexcept { | ||
| static ReactPropertyId<IReactDispatcher> uiThreadDispatcherProperty{L"ReactNative.Dispatcher", L"UIDispatcher"}; | ||
| /*static*/ IReactPropertyName ReactDispatcher::UIDispatcherProperty() noexcept { | ||
| static IReactPropertyName uiThreadDispatcherProperty{ReactPropertyBagHelper::GetName( | ||
| ReactPropertyBagHelper::GetNamespace(L"ReactNative.Dispatcher"), L"UIDispatcher")}; | ||
| return uiThreadDispatcherProperty; | ||
| } | ||
|
|
||
| /*static*/ IReactDispatcher ReactDispatcher::GetUIDispatcher(IReactPropertyBag const &properties) noexcept { | ||
| return ReactPropertyBag{properties}.Get(UIDispatcherProperty()); | ||
| return properties.Get(UIDispatcherProperty()).try_as<IReactDispatcher>(); | ||
| } | ||
|
|
||
| /*static*/ void ReactDispatcher::SetUIThreadDispatcher(IReactPropertyBag const &properties) noexcept { | ||
| ReactPropertyBag{properties}.Set(UIDispatcherProperty(), UIThreadDispatcher()); | ||
| properties.Set(UIDispatcherProperty(), UIThreadDispatcher()); | ||
| } | ||
|
|
||
| } // namespace winrt::Microsoft::ReactNative::implementation | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,6 @@ namespace Microsoft.ReactNative { | |
| static IReactDispatcher UIThreadDispatcher{ get; }; | ||
|
|
||
| // Get name of the UIDispatcher property for the IReactPropertyBag. | ||
| static IReactPropertyName UIDispatcherProperty(); | ||
| static IReactPropertyName UIDispatcherProperty { get; }; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
just checking: Is it common in WinRT to post-fix properties? Or should it be
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that this is not a real property that returns UIDispatcher. It is a property that can be used to access UIDispatcher from a property bag. This is why it has the 'Property' suffix. In reply to: 430588974 [](ancestors = 430588974) |
||
| } | ||
| } // namespace ReactNative | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
(nit) is that a common pattern? #Resolved
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.
Yes, the C++/WinRT usually puts the implementation in the winrt::Microsoft::ReactNative::implementation namespace.
Previously here I did not do it, just to make the namespace shorter, but now we have another ReactContext struct in the winrt::Microsoft::ReactNative namespace coming from the MS.RN.Cxx project and we must change the namespace here to avoid the naming conflict.
In reply to: 430532708 [](ancestors = 430532708)