Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
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
Expand Up @@ -19,10 +19,10 @@ struct MyModuleSpec : winrt::Microsoft::ReactNative::TurboModuleSpec {
SyncMethod<bool(bool) noexcept>{1, L"getBool"},
SyncMethod<double(double) noexcept>{2, L"getNumber"},
SyncMethod<std::string(std::string) noexcept>{3, L"getString"},
SyncMethod<JSValueArray(JSValueArray) noexcept>{4, L"getArray"},
SyncMethod<JSValueObject(JSValueObject) noexcept>{5, L"getObject"},
SyncMethod<JSValueObject(double, std::string, JSValueObject) noexcept>{6, L"getValue"},
Method<void(Callback<JSValue>) noexcept>{7, L"getValueWithCallback"},
SyncMethod<React::JSValueArray(React::JSValueArray) noexcept>{4, L"getArray"},
Copy link
Member

@dannyvv dannyvv May 26, 2020

Choose a reason for hiding this comment

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

Just for my sanity:
I didn't see any change to something that looks like a code generator, nor a package version update in case the codegen is part of that... Did you manually change this? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I did not. This file is updated every time I run 'yarn build'. The change is related to the recent codegen changes by Andrew Coates.


In reply to: 430597854 [](ancestors = 430597854)

SyncMethod<React::JSValueObject(React::JSValueObject) noexcept>{5, L"getObject"},
SyncMethod<React::JSValueObject(double, std::string, React::JSValueObject) noexcept>{6, L"getValue"},
Method<void(Callback<React::JSValue>) noexcept>{7, L"getValueWithCallback"},
Method<void(bool, Promise<React::JSValue>) noexcept>{8, L"getValueWithPromise"},
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ struct ReactContextStub : implements<ReactContextStub, IReactContext> {
VerifyElseCrashSz(false, "Not implemented");
}

IReactDispatcher UIDispatcher() noexcept {
VerifyElseCrashSz(false, "Not implemented");
}

void DispatchEvent(
xaml::FrameworkElement const & /*view*/,
hstring const & /*eventName*/,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,10 @@ struct ReactContextMock : implements<ReactContextMock, IReactContext> {
VerifyElseCrashSz(false, "Not implemented");
}

IReactDispatcher UIDispatcher() noexcept {
VerifyElseCrashSz(false, "Not implemented");
}

void DispatchEvent(
xaml::FrameworkElement const & /*view*/,
hstring const & /*eventName*/,
Expand Down
4 changes: 4 additions & 0 deletions vnext/Microsoft.ReactNative.Cxx/ReactContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ struct ReactContext {
return ReactNotificationService{m_handle.Notifications()};
}

ReactDispatcher UIDispatcher() const noexcept {
return ReactDispatcher{m_handle.UIDispatcher()};
}

// Call methodName JS function of module with moduleName.
// args are either function arguments or a single lambda with 'IJSValueWriter const&' argument.
template <class... TArgs>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@
</Link>
</ItemDefinitionGroup>
<ItemGroup>
<ClCompile Include="ReactInstanceSettingsTests.cpp" />
<ClCompile Include="ReactNonAbiValueTests.cpp" />
<ClCompile Include="ReactNotificationServiceTests.cpp" />
<ClCompile Include="ReactPropertyBagTests.cpp" />
Expand Down
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
Expand Up @@ -309,6 +309,8 @@ public ReactContextMock(ReactModuleBuilderMock builder)

public IReactNotificationService Notifications { get; } = ReactNotificationServiceHelper.CreateNotificationService();

public IReactDispatcher UIDispatcher => Properties.Get(ReactDispatcherHelper.UIDispatcherProperty) as IReactDispatcher;

public void DispatchEvent(FrameworkElement view, string eventName, JSValueArgWriter eventDataArgWriter)
{
throw new NotImplementedException();
Expand Down
2 changes: 1 addition & 1 deletion vnext/Microsoft.ReactNative/ABIViewManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ ABIViewManager::ABIViewManager(
m_viewManagerWithChildren{viewManager.try_as<IViewManagerWithChildren>()},
m_name{to_string(viewManager.Name())} {
if (m_viewManagerWithReactContext) {
m_viewManagerWithReactContext.ReactContext(winrt::make<ReactContext>(Mso::Copy(reactContext)));
m_viewManagerWithReactContext.ReactContext(winrt::make<implementation::ReactContext>(Mso::Copy(reactContext)));
}
if (m_viewManagerWithNativeProperties) {
m_nativeProps = m_viewManagerWithNativeProperties.NativeProps();
Expand Down
8 changes: 6 additions & 2 deletions vnext/Microsoft.ReactNative/IReactContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#include "IReactContext.h"
#include "DynamicWriter.h"

namespace winrt::Microsoft::ReactNative {
namespace winrt::Microsoft::ReactNative::implementation {

ReactContext::ReactContext(Mso::CntPtr<Mso::React::IReactContext> &&context) noexcept : m_context{std::move(context)} {}

Expand All @@ -17,6 +17,10 @@ IReactNotificationService ReactContext::Notifications() noexcept {
return m_context->Notifications();
}

IReactDispatcher ReactContext::UIDispatcher() noexcept {
return Properties().Get(ReactDispatcherHelper::UIDispatcherProperty()).try_as<IReactDispatcher>();
}

void ReactContext::DispatchEvent(
xaml::FrameworkElement const &view,
hstring const &eventName,
Expand Down Expand Up @@ -55,4 +59,4 @@ void ReactContext::EmitJSEvent(
m_context->CallJSFunction(to_string(eventEmitterName), "emit", std::move(params));
}

} // namespace winrt::Microsoft::ReactNative
} // namespace winrt::Microsoft::ReactNative::implementation
5 changes: 3 additions & 2 deletions vnext/Microsoft.ReactNative/IReactContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@
#include "ReactHost/React.h"
#include "winrt/Microsoft.ReactNative.h"

namespace winrt::Microsoft::ReactNative {
namespace winrt::Microsoft::ReactNative::implementation {
Copy link

@NikoAri NikoAri May 26, 2020

Choose a reason for hiding this comment

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

implementation [](start = 41, length = 14)

(nit) is that a common pattern? #Resolved

Copy link
Member Author

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)


struct ReactContext : winrt::implements<ReactContext, IReactContext> {
ReactContext(Mso::CntPtr<Mso::React::IReactContext> &&context) noexcept;

public: // IReactContext
IReactPropertyBag Properties() noexcept;
IReactNotificationService Notifications() noexcept;
IReactDispatcher UIDispatcher() noexcept;
void DispatchEvent(
xaml::FrameworkElement const &view,
hstring const &eventName,
Expand All @@ -31,4 +32,4 @@ struct ReactContext : winrt::implements<ReactContext, IReactContext> {
Mso::CntPtr<Mso::React::IReactContext> m_context;
};

} // namespace winrt::Microsoft::ReactNative
} // namespace winrt::Microsoft::ReactNative::implementation
3 changes: 3 additions & 0 deletions vnext/Microsoft.ReactNative/IReactContext.idl
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ namespace Microsoft.ReactNative {
// The subscriptions added to IReactInstanceSettings.Notifications kept as long as IReactInstanceSettings alive.
IReactNotificationService Notifications { get; };

// Get ReactDispatcherHelper::UIDispatcherProperty from the Properties property bag.
IReactDispatcher UIDispatcher { get; };

// Dispatch UI event. This method is to be moved to IReactViewContext.
void DispatchEvent(XAML_NAMESPACE.FrameworkElement view, String eventName, JSValueArgWriter eventDataArgWriter);

Expand Down
16 changes: 15 additions & 1 deletion vnext/Microsoft.ReactNative/IReactDispatcher.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,21 @@ 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;
Copy link
Member

@dannyvv dannyvv May 26, 2020

Choose a reason for hiding this comment

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

dispatcher [](start = 19, length = 10)

uninitalized memory if queue is null? Or did I miss a new c++/winrt feature? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

The interface-like looking types in C++/WinRT are really specialized com_ptr smart pointers. This line calls the default constructor of the IReactDispatcher that sets it to nullptr. I can make it more explicit to avoid any confusion.


In reply to: 430590699 [](ancestors = 430590699)

Copy link
Member

@dannyvv dannyvv May 26, 2020

Choose a reason for hiding this comment

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

doh, sorry... thanks!, No need. Still thinking COM, not WinRT :)


In reply to: 430592838 [](ancestors = 430592838,430590699)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added passing nullptr to constructor to make it explicit.


In reply to: 430593326 [](ancestors = 430593326,430592838,430590699)

auto queue = Mso::DispatchQueue::GetCurrentUIThreadQueue();
if (queue) {
queue.InvokeElsePost([&queue, &dispatcher ]() noexcept {
Copy link
Contributor

@acoates-ms acoates-ms May 26, 2020

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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)

Copy link
Member Author

Choose a reason for hiding this comment

The 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)

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 {
Expand Down
2 changes: 1 addition & 1 deletion vnext/Microsoft.ReactNative/IReactDispatcher.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

namespace winrt::Microsoft::ReactNative::implementation {

struct ReactDispatcher : implements<ReactDispatcher, IReactDispatcher> {
struct ReactDispatcher : implements<ReactDispatcher, winrt::default_interface<IReactDispatcher>> {
ReactDispatcher() = default;
ReactDispatcher(Mso::DispatchQueue &&queue) noexcept;

Expand Down
2 changes: 1 addition & 1 deletion vnext/Microsoft.ReactNative/IReactDispatcher.idl
Original file line number Diff line number Diff line change
Expand Up @@ -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; };
Copy link
Member

@dannyvv dannyvv May 26, 2020

Choose a reason for hiding this comment

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

UIDispatcherProperty [](start = 30, length = 20)

just checking: Is it common in WinRT to post-fix properties? Or should it be UIDispatcher now that we added a getter? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
E.g. properties.Get(ReactDispatcherHelper.UIDispatcherProperty).


In reply to: 430588974 [](ancestors = 430588974)

}
} // namespace ReactNative
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,6 @@
<ClCompile Include="..\ReactUWP\Modules\AppThemeModuleUwp.cpp">
<Filter>Modules</Filter>
</ClCompile>
<ClCompile Include="..\ReactUWP\Modules\DeviceInfoModule.cpp">
<Filter>Modules</Filter>
</ClCompile>
<ClCompile Include="..\ReactUWP\Modules\DevSupportManagerUwp.cpp">
<Filter>Modules</Filter>
</ClCompile>
Expand Down Expand Up @@ -273,9 +270,7 @@
<ClCompile Include="Modules\ClipboardModule.cpp">
<Filter>Modules</Filter>
</ClCompile>
<ClCompile Include="Modules\DeviceStateModule.cpp">
<Filter>Modules</Filter>
</ClCompile>
<ClCompile Include="Modules\DeviceInfoModule.cpp" />
<ClCompile Include="Modules\DevSettingsModule.cpp">
<Filter>Modules</Filter>
</ClCompile>
Expand Down Expand Up @@ -616,9 +611,7 @@
<ClInclude Include="Modules\ClipboardModule.h">
<Filter>Modules</Filter>
</ClInclude>
<ClInclude Include="Modules\DeviceStateModule.h">
<Filter>Modules</Filter>
</ClInclude>
<ClInclude Include="Modules\DeviceInfoModule.h" />
<ClInclude Include="Modules\DevSettingsModule.h">
<Filter>Modules</Filter>
</ClInclude>
Expand Down
2 changes: 1 addition & 1 deletion vnext/Microsoft.ReactNative/NativeModulesProvider.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ std::vector<facebook::react::NativeModuleDescription> NativeModulesProvider::Get
m_modulesWorkerQueue = react::uwp::MakeSerialQueueThread();
}

auto winrtReactContext = winrt::make<ReactContext>(Mso::Copy(reactContext));
auto winrtReactContext = winrt::make<implementation::ReactContext>(Mso::Copy(reactContext));

for (auto &entry : m_moduleProviders) {
modules.emplace_back(
Expand Down
1 change: 1 addition & 0 deletions vnext/Microsoft.ReactNative/ReactHost/ReactInstanceWin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,7 @@ void ReactInstanceWin::InitNativeMessageThread() noexcept {
void ReactInstanceWin::InitUIMessageThread() noexcept {
// Native queue was already given us in constructor.
m_uiQueue = winrt::Microsoft::ReactNative::implementation::ReactDispatcher::GetUIDispatchQueue(m_options.Properties);
VerifyElseCrashSz(m_uiQueue, "No UI Dispatcher provided");
m_uiMessageThread.Exchange(
std::make_shared<MessageDispatchQueue>(m_uiQueue, Mso::MakeWeakMemberFunctor(this, &ReactInstanceWin::OnError)));

Expand Down
17 changes: 16 additions & 1 deletion vnext/Microsoft.ReactNative/ReactInstanceSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,19 @@
#include "ReactInstanceSettings.g.cpp"
#endif

namespace winrt::Microsoft::ReactNative::implementation {} // namespace winrt::Microsoft::ReactNative::implementation
namespace winrt::Microsoft::ReactNative::implementation {

ReactInstanceSettings::ReactInstanceSettings() noexcept {
// Use current thread dispatcher as a default UI dispatcher.
m_properties.Set(ReactDispatcherHelper::UIDispatcherProperty(), ReactDispatcherHelper::UIThreadDispatcher());
}

IReactDispatcher ReactInstanceSettings::UIDispatcher() noexcept {
return m_properties.Get(ReactDispatcherHelper::UIDispatcherProperty()).try_as<IReactDispatcher>();
}

void ReactInstanceSettings::UIDispatcher(IReactDispatcher const &value) noexcept {
m_properties.Set(ReactDispatcherHelper::UIDispatcherProperty(), value);
}

} // namespace winrt::Microsoft::ReactNative::implementation
5 changes: 4 additions & 1 deletion vnext/Microsoft.ReactNative/ReactInstanceSettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
namespace winrt::Microsoft::ReactNative::implementation {

struct ReactInstanceSettings : ReactInstanceSettingsT<ReactInstanceSettings> {
ReactInstanceSettings() = default;
ReactInstanceSettings() noexcept;

IReactPropertyBag Properties() noexcept;

Expand Down Expand Up @@ -85,6 +85,9 @@ struct ReactInstanceSettings : ReactInstanceSettingsT<ReactInstanceSettings> {
IRedBoxHandler RedBoxHandler() noexcept;
void RedBoxHandler(IRedBoxHandler const &value) noexcept;

IReactDispatcher UIDispatcher() noexcept;
void UIDispatcher(IReactDispatcher const &value) noexcept;

private:
IReactPropertyBag m_properties{ReactPropertyBagHelper::CreatePropertyBag()};
IReactNotificationService m_notifications{ReactNotificationServiceHelper::CreateNotificationService()};
Expand Down
4 changes: 3 additions & 1 deletion vnext/Microsoft.ReactNative/ReactInstanceSettings.idl
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

import "RedBoxHandler.idl";
import "IReactDispatcher.idl";
import "IReactNotificationService.idl";
import "IReactPropertyBag.idl";
import "RedBoxHandler.idl";

namespace Microsoft.ReactNative {

Expand Down Expand Up @@ -33,5 +34,6 @@ namespace Microsoft.ReactNative {
String BundleRootPath { get; set; };
UInt16 DebuggerPort { get; set; };
IRedBoxHandler RedBoxHandler { get; set; };
IReactDispatcher UIDispatcher { get; set; };
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ std::shared_ptr<facebook::react::MessageQueueThread> MakeJSQueueThread() noexcep
}

std::shared_ptr<facebook::react::MessageQueueThread> MakeUIQueueThread() noexcept {
return std::make_shared<Mso::React::MessageDispatchQueue>(
Mso::DispatchQueue::MakeCurrentThreadUIQueue(), nullptr, nullptr);
Mso::DispatchQueue queue = Mso::DispatchQueue::GetCurrentUIThreadQueue();
std::shared_ptr<facebook::react::MessageQueueThread> messageThread =
queue ? std::make_shared<Mso::React::MessageDispatchQueue>(queue, nullptr, nullptr) : nullptr;
return messageThread;
}

std::shared_ptr<facebook::react::MessageQueueThread> MakeSerialQueueThread() noexcept {
Expand Down
3 changes: 2 additions & 1 deletion vnext/Microsoft.ReactNative/Views/ReactRootControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ namespace react::uwp {
//===========================================================================

ReactRootControl::ReactRootControl(XamlView const &rootView) noexcept
: m_weakRootView{rootView}, m_uiQueue(Mso::DispatchQueue::MakeCurrentThreadUIQueue()) {
: m_weakRootView{rootView}, m_uiQueue(Mso::DispatchQueue::GetCurrentUIThreadQueue()) {
VerifyElseCrashSz(m_uiQueue, "Cannot get UI dispatch queue for the current thread");
PrepareXamlRootView(rootView);
}

Expand Down
Loading