-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Implement Net46 Playground Application #868
Conversation
* Fixed some crashes when starting due to differences between Net46 and UWP * Converted Net46 Dialogs to be shown as modeless to ensure execution can continue (e.g. progress dialog)
…sign data file/classes * Added ScrollViewer to RedBoxDialog
…tures since KeyDown events are not reaching RootView without making the RootView Focusable and explictly setting focus on it... * Styled the Developer Options dialog... * Fixed crash when saving application settings via the developer options dialog
…nsured the Focus was applied to it by the ReactPage OnCreate * Restore support for keyboard accelerators to bring up dev options, changed dev option accelerators to match other React Native Clients. * Fix issue in getting the correct bundle path * Added support to copy the ChakraCore DLL to the Playground Net46 output directory
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Playground.Net46", "Playground.Net46\Playground.Net46.csproj", "{7AB3A24A-D5D9-479F-8E12-22213DD40D3F}" | ||
ProjectSection(ProjectDependencies) = postProject | ||
{E02ED054-4FB2-4EEB-8835-36A5E542E368} = {E02ED054-4FB2-4EEB-8835-36A5E542E368} | ||
EndProjectSection |
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.
What is this for? #Closed
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.
I believe this is the addition of the new project for Playground.Net46 #Closed
|
||
namespace Playground.Net46 | ||
{ | ||
public sealed class PlaygroundBootstrapper : ReactNativeBootstrapper |
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.
PlaygroundBootstrapper [](start = 24, length = 22)
Can we cleanup the bootstrapper stuff into a "minimalist" setup for getting the app running.
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.
I have removed all bootstrapper related code from the Playground.Net46 and have gone with a more minimalist approach similar to Playground
// return this.ReactInstanceManager.DisposeAsync(); | ||
//} | ||
|
||
return this.ReactInstanceManager.DisposeAsync(); |
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.
The goal here was to check if the reactInstanceManager has been created before trying to Dispose it... The return statement is not correct because it means that we may be creating the object only to dispose of it (clearly not what we want)...
The commented out code is the solution to the above problem, but I'm not sure what the correct return statement is for the case when IsValueCreated is false.
The method is not marked as "async" and we are not awaiting on the DisposeAsync. Should we mark the dispose async and await on the DisposeAsync?
{ | ||
} | ||
|
||
protected override void OnStartup(StartupEventArgs e) |
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.
OnStartup [](start = 32, length = 9)
add docs to protected methods #Closed
protected override void OnActivated(EventArgs e) | ||
{ | ||
base.OnActivated(e); | ||
} |
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.
Delete this.
@@ -17,7 +17,9 @@ | |||
<OutputPath>bin\x86\Debug\</OutputPath> | |||
<DefineConstants>DEBUG;NET46</DefineConstants> | |||
<DocumentationFile>bin\x86\Debug\ReactNative.Net46.XML</DocumentationFile> | |||
<DebugType>none</DebugType> | |||
<DebugType>pdbonly</DebugType> |
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.
pdbonly [](start = 15, length = 7)
Do this for all debug builds? #Closed
{ | ||
RootView.Background = (Brush)Application.Current.Resources["ApplicationPageBackgroundThemeBrush"]; | ||
RootView.Background = SystemColors.WindowBrush; | ||
|
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.
Remove this.
{ | ||
if (!string.IsNullOrEmpty(arguments)) | ||
if (arguments == null) return; |
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.
{ } #Closed
return reactInstanceManager; | ||
}); | ||
|
||
this._rootView = new Lazy<ReactRootView>(() => |
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.
this [](start = 12, length = 4)
Remove this if you want to :-) #Closed
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.
I am going to remove all this. except the one where I'm referencing dispatcher from the Page base class where I'll use base.Dispatcher to make it clear it is using a class member not from the immediate class..
@@ -5,8 +5,43 @@ | |||
|
|||
namespace ReactNative.Bridge | |||
{ | |||
static class DispatcherHelpers | |||
internal static class DispatcherHelpers |
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.
internal by default #Closed
{ | ||
throw new InvalidOperationException("Dispatcher has not been set"); | ||
} | ||
|
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.
Take this, insert into private method called AssertDispatcherSet
, apply below.
@@ -17,12 +52,17 @@ public static void AssertOnDispatcher() | |||
|
|||
public static bool IsOnDispatcher() | |||
{ | |||
return Thread.CurrentThread == Dispatcher.CurrentDispatcher.Thread; | |||
if (CurrentDispatcher == null) | |||
{ |
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.
AssertDispatcherSet #Closed
} | ||
|
||
public static async void RunOnDispatcher(Action action) | ||
{ | ||
await Dispatcher.CurrentDispatcher.InvokeAsync(action).Task.ConfigureAwait(false); | ||
await CurrentDispatcher.InvokeAsync(action).Task.ConfigureAwait(false); |
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.
CurrentDispatcher [](start = 18, length = 17)
Use is AssertDispatcherSet #Closed
{ | ||
if (options != null && options.ContainsKey("origin")) | ||
{ | ||
throw new NotImplementedException(/* TODO: (#253) */); |
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.
Double check if you guys can implement this.
@@ -42,6 +42,8 @@ public partial class UIManagerModule : ReactContextNativeModuleBase, ILifecycleE | |||
throw new ArgumentNullException(nameof(viewManagers)); | |||
if (uiImplementation == null) | |||
throw new ArgumentNullException(nameof(uiImplementation)); | |||
if ( window == null ) |
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.
[](start = 16, length = 1)
No space after () #Closed
@@ -46,7 +46,7 @@ public void SetColor(RichTextBox view, uint? color) | |||
/// <param name="view">The view.</param> | |||
/// <param name="selectable">A flag indicating whether or not the text is selectable.</param> | |||
[ReactProp("selectable")] | |||
public void SetSelectable(RichTextBox view, bool selectable) | |||
public void SetSelectable(TextBlock view, bool selectable) | |||
{ | |||
// ToDo: Manually control selectable | |||
} |
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.
throw NotImplementedException() #Closed
public void NetInfoModule_Event() | ||
{ | ||
SetDispatcherForTest(); | ||
|
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.
Nothing about NetInfo should require a dispatcher.
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.
The NetInfo tests themselves are calling OnResume, which will assert on the dispatcher availability... Leaving code that sets the dispatcher ahead of the test execution...
@@ -175,6 +175,8 @@ public void Invoke(IReactInstance reactInstance, int methodId, JArray parameters | |||
|
|||
public void WriteModuleDescription(JsonWriter writer) | |||
{ | |||
if (Target == null) throw new NullReferenceException("Target"); | |||
|
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.
This should never be null.
@@ -125,6 +125,7 @@ public bool IsReloadOnJavaScriptChangeEnabled | |||
} | |||
} | |||
|
|||
//TODO: Implement an abstraction or static helper to save settings based on the platform... The GetSetting and SetSetting methods or their contents are replicated elsewhere... | |||
private T GetSetting<T>(string key, T defaultValue) |
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.
Can we add a GitHub issue instead of a TODO, can leave a shorter TODO with reference to GitHub issue number
var values = ConfigurationManager.AppSettings; | ||
if (values[key] == null) | ||
var configFile = ConfigurationManager.OpenExeConfiguration(ConfigurationUserLevel.None); | ||
var settings = configFile.AppSettings.Settings; |
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.
Let's just be readonly, either this can be a no-op or it can just set some internal in-memory dictionary.
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.
I have implemented a simple Dictionary to store these values in memory... The settings will not be persisted across application instances...
var values = ApplicationData.Current.LocalSettings.Values; | ||
values[key] = value; | ||
#else | ||
var configFile = ConfigurationManager.OpenExeConfiguration(ConfigurationUserLevel.None); |
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.
OpenExeConfiguration [](start = 50, length = 20)
In-memory dictionary instead of using ConfigurationManager
Implement the Net46 React Native Windows application.