feat(Shared): UIManager Migration by kevinvangelder · Pull Request #831 · microsoft/react-native-windows

kevinvangelder · 2016-10-26T21:21:00Z

initial work migrating Network module
Progress on Networking Module
more progress on Network module
fix csproj
migrating more devsupport
migrating more devsupport
Migrate DevOptionDialog
migrated Progress Dialog
migrated RedBoxDialog
migrated WebSocketJavaScriptExecutor
migrate DevSupportManager
saved changes from last commit -- oops.
implement feedback from @rozele
a little more cleanup
switched to in-memory settings strategy
tweaks to try to fix AppVeyor build
fixed build path for Facebook.CSSLayout
remove unwanted comment
migrating easy UIManager files
migrating moderate UIManager files
refactored Networking module to shared per @rozele feedback
starting migration of pointers in touch
Touch basics needs circular references
update csproj
migrated difficult UIManager files
implemented more feedback from @rozele
fix(Linking): Adds getInitialUrl and url events to LauncherModule (fix(Linking): Adds getInitialUrl and url events to LauncherModule #798)
Using the ProtocolActivatedEventArgs, this changeset adds the getInitialUrl API implementation for the Linking module (aka the LauncherModule in native code) and the url event, which is called each time the app is activated with a new protocol URI.
Fixes Implement Linking getInitialURL API #301
Fixes Implement Linking #687
imlemented more @rozele feedback
implement @rozele feedback
implemented feedback from @matthargett
removed another unnecessary if/def
small tweaks to wrap up UIModule migration
migrated uimanager tests - all but 2 pass
Revert "Merge pull request feat(Portable): Migrate network module #787 from infinitered/shared-project-network"
This reverts commit 4b78183, reversing
changes made to 8da1824.
Revert "merge upstream"
This reverts commit 1fd5fdf, reversing
changes made to 249c6f7.
tweaks for tests
tweak I18N setting read
fix spacing changes
adding touch handling
diverge BaseViewManager for UWP/WPF
implemented feedback from @rozele
added reactprop to no-op todo comments
allow ReactShadowNode dirty override for Net46
more tweaks per feedback
added missing files
add csproj change
abstracted platform specific window finding code from UIManagerModule
cleanup
implemented @rozele feedback for touch

@rozele

* initial work migrating Network module * Progress on Networking Module * more progress on Network module * fix csproj * migrating more devsupport * migrating more devsupport * Migrate DevOptionDialog * migrated Progress Dialog * migrated RedBoxDialog * migrated WebSocketJavaScriptExecutor * migrate DevSupportManager * saved changes from last commit -- oops. * implement feedback from @rozele * a little more cleanup * switched to in-memory settings strategy * tweaks to try to fix AppVeyor build * fixed build path for Facebook.CSSLayout * remove unwanted comment * migrating easy UIManager files * migrating moderate UIManager files * refactored Networking module to shared per @rozele feedback * starting migration of pointers in touch * Touch basics needs circular references * update csproj * migrated difficult UIManager files * implemented more feedback from @rozele * fix(Linking): Adds `getInitialUrl` and `url` events to LauncherModule (microsoft#798) Using the ProtocolActivatedEventArgs, this changeset adds the `getInitialUrl` API implementation for the Linking module (aka the LauncherModule in native code) and the `url` event, which is called each time the app is activated with a new protocol URI. Fixes microsoft#301 Fixes microsoft#687 * imlemented more @rozele feedback * implement @rozele feedback * implemented feedback from @matthargett * removed another unnecessary if/def * small tweaks to wrap up UIModule migration * migrated uimanager tests - all but 2 pass * Revert "Merge pull request microsoft#787 from infinitered/shared-project-network" This reverts commit 4b78183, reversing changes made to 8da1824. * Revert "merge upstream" This reverts commit 1fd5fdf, reversing changes made to 249c6f7. * tweaks for tests * tweak I18N setting read * fix spacing changes * adding touch handling * diverge BaseViewManager for UWP/WPF * implemented feedback from @rozele * added reactprop to no-op todo comments * allow ReactShadowNode dirty override for Net46 * more tweaks per feedback * added missing files * add csproj change * abstracted platform specific window finding code from UIManagerModule * cleanup * implemented @rozele feedback for touch

rozele · 2016-10-26T22:19:36Z

ReactWindows/ReactNative.Net46/Tracing/NullLoggingActivityBuilder.cs


-        class NullDisposable : IDisposable
+        public class NullDisposable : IDisposable
        {


Actually, can you just use Disposable.Empty from System.Reactive?

rozele · 2016-10-26T22:21:18Z

ReactWindows/ReactNative.Net46.Tests/Bridge/Queue/MessageQueueThreadTests.cs


-        [SetUp]
+        // ToDo: Conflicts with UIManagerModule tests [SetUp]
        public void SetUp()


SetUp [](start = 20, length = 5)

Why don't you just create a singleton class for initializing the Application and expose a method called Initialize that is idempotent?

rozele · 2016-10-26T22:22:03Z

ReactWindows/ReactNative.Net46.Tests/Internal/DispatcherHelpers.cs

        }

+        [Apartment(ApartmentState.STA)]
        public static async Task<T> CallOnDispatcherAsync<T>(Func<T> func)


What does this do? Can you leave a comment about it's purpose?

rozele · 2016-10-26T22:22:51Z

ReactWindows/ReactNative.Net46.Tests/UIManager/AppRegistryTests.cs

@@ -0,0 +1,38 @@
+using NMock;
+using NUnit.Framework;


Where are you using NMock in here?

rozele · 2016-10-26T22:24:09Z

ReactWindows/ReactNative.Net46.Tests/UIManager/DependencyObjectExtensionsTests.cs

+
+namespace ReactNative.Tests.UIManager
+{
+    [TestFixture, RequiresSTA]


RequiresSTA [](start = 18, length = 11)

This smells of something being very wrong. I've never seen a need for an attribute like this in unit tests...

rozele · 2016-10-26T22:26:33Z

ReactWindows/ReactNative.Net46.Tests/UIManager/PropertySetterTests.cs

@@ -0,0 +1,486 @@
+using NMock;
+using NUnit.Framework;


I'm seeing the reference to NMock in a few files but not quite sure where it's being used.

rozele · 2016-10-26T22:28:26Z

ReactWindows/ReactNative.Shared/Modules/I18N/I18NUtil.cs

+                var result = default(bool);
+                var parsed = bool.TryParse(ConfigurationManager.AppSettings[AllowRTL], out result);
+                return (bool)(object)(parsed && result);
+#endif


Make sense to abstract this into a helper? These getter/setter patterns are used in a few places in this file.

I started to do that but then didn't because it was going to make the file larger.

I'd rather make it larger and have the #ifs isolated to a smaller number of locations.

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

rozele · 2016-10-26T22:30:40Z

ReactWindows/ReactNative.Shared/UIManager/NativeViewHierarchyManager.cs

            var target = VisualTreeHelper.FindElementsInHostCoordinates(new Point(touchX, touchY), uiElement)
+#else
+            var sources = new List<DependencyObject>();
+            VisualTreeHelper.HitTest(


Might be useful to put a TODO here to consider a pooled data structure. You'll be allocating a lot of lists here in case of a touch heavy application

Honestly, since you're doing FirstOrDefault() below anyway, I would separate the UWP and .NET 4.6 behaviors a bit more. With the HitTest approach, you can inline the HasTag() test in the HitTest delegate body.

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

rozele · 2016-10-26T22:32:59Z

ReactWindows/ReactNative.Shared/UIManager/NativeViewHierarchyManager.cs

+                uiElement,
+                null,
+                new HitTestResultCallback(
+                    (HitTestResult hit) =>


You should just use type inference here, remove the new HitTestResultCallback constructor wrapper and also remove the (HitTestResult hit) parameter type from the lambda parameters (i.e., just write hit =>

rozele · 2016-10-26T22:34:13Z

ReactWindows/ReactNative.Shared/UIManager/UIManagerModule.cs

 using ReactNative.UIManager.Events;
 using System;
 using System.Collections.Generic;
+using System.Windows;


Is System.Windows a valid namespace in WINDOWS_UWP? Rather, is it actually being used in UWP?

VS wasn't complaining, but it wasn't being used anyways, so I'll remove it.

Or rather, VS was lying to me... it was being used. I'll just move it into the if/def to be sure.

rozele · 2016-10-26T22:35:38Z

ReactWindows/ReactNative.Shared/UIManager/UIManagerModule.cs

        /// <param name="viewManagers">The view managers.</param>
        /// <param name="uiImplementation">The UI implementation.</param>
+        /// <param name="window">The ApplicationView/Framework Element.</param>
        public UIManagerModule(


Can we have a separate comments for each platform?

rozele · 2016-10-26T22:37:40Z

ReactWindows/ReactNative.Shared/UIManager/UIManagerModule.cs

+            ApplicationView window)
+#else
+            FrameworkElement window)
+#endif


Can't we just pass change the constructor signature for .NET 4.6 only? I really don't want to have to think through all the implications of not grabbing the ApplicationView when it's used. Basically, this would be better if you parameterized constructor on the window parameter only for .NET 4.6

We could, but that's going to make this file a lot more messy.

I'd prefer not to change the behavior of UWP. Previously, each time the bounds changed, we grabbed the instance from the APplicationView singleton method, now we cache it, so we could hae failure cases that would occur in different ways if these callbacks somehow end up on the wrong thread.

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

What about passing the dimensions into the UIManager module and managing the size changed behavior somewhere else where we have a ReactContext available?

In reply to: 85242918 [](ancestors = 85242918,85238878)

This is the most centralized way to decouple this class from the concrete type, and eliminate a global instance access. This approach will allow for parallelization of all tests being run without the worry of conflicts in global mutable state.

rozele · 2016-10-26T22:39:36Z

ReactWindows/ReactNative.Tests/UIManager/UIManagerModuleTests.cs

    public class UIManagerModuleTests
    {
-        [TestMethod]
+        [TestMethod, STAThread]


STAThread [](start = 21, length = 9)

I'd rather not use this attribute until I understand it more. It shouldn't be needed if you take the suggestion in UIManager to only parameterize for .NET 4.6 (since we can get the application view at any time from UWP).

rozele · 2016-10-26T23:44:35Z

ReactWindows/ReactNative.Shared/Modules/I18N/I18NUtil.cs

+using System;
+using System.Configuration;
+using static System.FormattableString;
+#endif


Where is FormattableString being used? #Closed

rozele · 2016-10-27T00:32:57Z

ReactWindows/ReactNative.Net46/Tracing/NullLoggingActivityBuilder.cs

-            {

-            }
+        public IDisposable Create()


Add a doc comment for this public API

rozele · 2016-10-28T00:11:09Z

ReactWindows/ReactNative/CoreModulesPackage.cs

 using System;
 using System.Collections.Generic;
+using Windows.UI.ViewManagement;



Delete this namespace.

rozele · 2016-10-28T00:11:50Z

ReactWindows/ReactNative/UIManager/UIManagerModule.cs

@@ -5,6 +5,7 @@
 using ReactNative.UIManager.Events;
 using System;
 using System.Collections.Generic;


Whitespace below should be removed.

rozele · 2016-10-28T00:12:43Z

ReactWindows/ReactNative.Tests/UIManager/UIManagerModuleTests.cs

        }

-        [TestMethod]
+        [TestMethod, STAThread]


STAThread [](start = 21, length = 9)

Remove STAThread

rozele · 2016-10-28T00:12:53Z

ReactWindows/ReactNative.Tests/UIManager/UIManagerModuleTests.cs

        }

-        [TestMethod]
+        [TestMethod, STAThread]


STAThread [](start = 21, length = 9)

Remove STAThread

rozele · 2016-10-28T00:13:01Z

ReactWindows/ReactNative.Tests/UIManager/UIManagerModuleTests.cs

 using System.Collections.Generic;
 using System.Threading.Tasks;
+using Windows.UI.ViewManagement;



Remove unused namespace.

matthargett

looks like all @rozele feedback implemented. 👍

kevinvangelder mentioned this pull request Oct 26, 2016

feat(Portable): Migrated Touch, UIManager, and I18N Module #804

Closed

rozele reviewed Oct 26, 2016

View reviewed changes

kevinvangelder added 2 commits October 26, 2016 16:07

changes per @rozele feedback

3170c9a

cleanup NativeViewHierarchyManager

fa2909b

rozele reviewed Oct 26, 2016

View reviewed changes

cleanup I18N imports

fdc8017

rozele reviewed Oct 27, 2016

View reviewed changes

kevinvangelder added 2 commits October 26, 2016 17:39

added doc comments for new NullLoggingActivityBuilder method

1d8749c

diverge UWP and Net46 UIManagerModule

b9c0662

rozele reviewed Oct 28, 2016

View reviewed changes

cleanup

0a1e53c

matthargett reviewed Oct 28, 2016

View reviewed changes

matthargett approved these changes Oct 28, 2016

View reviewed changes

rozele merged commit 8c47f40 into microsoft:master Oct 28, 2016

Conversation

kevinvangelder commented Oct 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rozele Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rozele Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rozele Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthargett left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rozele Oct 26, 2016 •

edited

Loading

rozele Oct 26, 2016 •

edited

Loading

rozele Oct 26, 2016 •

edited

Loading