Skip to content

Conversation

@pre10der89
Copy link
Contributor

Implement the Net46 React Native Windows application.

pre10der89 and others added 28 commits October 31, 2016 09:49
* 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
Copy link
Contributor

@rozele rozele Nov 9, 2016

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

Copy link
Contributor Author

@pre10der89 pre10der89 Nov 10, 2016

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
Copy link
Contributor

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.

Copy link
Contributor Author

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();
Copy link
Contributor Author

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)
Copy link
Contributor

@rozele rozele Nov 11, 2016

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);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this.

<DefineConstants>DEBUG;NET46</DefineConstants>
<DocumentationFile>bin\x86\Debug\ReactNative.Net46.XML</DocumentationFile>
<DebugType>none</DebugType>
<DebugType>pdbonly</DebugType>
Copy link
Contributor

@rozele rozele Nov 11, 2016

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;

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this.

private void ApplyArguments(string[] arguments)
{
if (!string.IsNullOrEmpty(arguments))
if (arguments == null) return;
Copy link
Contributor

@rozele rozele Nov 11, 2016

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>(() =>
Copy link
Contributor

@rozele rozele Nov 11, 2016

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

Copy link
Contributor Author

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..

namespace ReactNative.Bridge
{
static class DispatcherHelpers
internal static class DispatcherHelpers
Copy link
Contributor

@rozele rozele Nov 11, 2016

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");
}

Copy link
Contributor

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.

{
return Thread.CurrentThread == Dispatcher.CurrentDispatcher.Thread;
if (CurrentDispatcher == null)
{
Copy link
Contributor

@rozele rozele Nov 11, 2016

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);
Copy link
Contributor

@rozele rozele Nov 11, 2016

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) */);
Copy link
Contributor

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.

throw new ArgumentNullException(nameof(viewManagers));
if (uiImplementation == null)
throw new ArgumentNullException(nameof(uiImplementation));
if ( window == null )
Copy link
Contributor

@rozele rozele Nov 11, 2016

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

public void SetSelectable(TextBlock view, bool selectable)
{
// ToDo: Manually control selectable
}
Copy link
Contributor

@rozele rozele Nov 11, 2016

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();

Copy link
Contributor

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.

Copy link
Contributor Author

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...

public void WriteModuleDescription(JsonWriter writer)
{
if (Target == null) throw new NullReferenceException("Target");

Copy link
Contributor

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.

}

//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)
Copy link
Contributor

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;
Copy link
Contributor

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.

Copy link
Contributor Author

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);
Copy link
Contributor

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

@rozele
Copy link
Contributor

rozele commented Nov 15, 2016

:shipit:

@rozele rozele merged commit 8bf64e1 into microsoft:master Nov 15, 2016
@pre10der89 pre10der89 deleted the playground-net46 branch November 15, 2016 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants