Introducing .NET version of css-layout using P/Invoke #220

mattpodwysocki · 2016-09-20T18:58:24Z

This version of the css-layout project includes support for .NET projects. Up in the air is how many configurations of .NET projects we allow for, such as Portable Class Libraries, Universal Windows Platform, .NET Core, etc. Still needs integration with Buck.

ghost · 2016-09-20T18:59:00Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

ghost · 2016-09-20T19:12:27Z

@mattpodwysocki updated the pull request - view changes

emilsjolander

Could you remove the non-native version and its code dependencies. You don't need to mirror the java version which has a an interface backed by both a java implementation and a native implementation, that is being removed very soon.

emilsjolander · 2016-09-20T19:43:26Z

csharp/.gitignore

@@ -0,0 +1,265 @@
+## Ignore Visual Studio temporary files, build results, and


please mirror this in a corresponding .hgignore

@emilsjolander ok, will do

@emilsjolander done

emilsjolander · 2016-09-20T19:44:14Z

.gitignore

@@ -6,3 +6,6 @@
 /.buckd
 /lib/gtest/googletest-*/
 /gentest/test.html
+
+# Visual studio code


Please mirror this in .hgignore

@emilsjolander done

emilsjolander · 2016-09-20T19:45:36Z

csharp/CSSLayout/CSSLayout-win.h

@@ -0,0 +1,206 @@
+/**


This seems annoying to maintain. Can we make this use the same .h file as the C version?

@emilsjolander we could if you want pragma comments for checking if Windows, sure

Or some macro magic?

@emilsjolander should we update it as such? https://gist.github.com/mattpodwysocki/c48da3dd668708ee227263b2ae382815

@emilsjolander removed as per request

emilsjolander · 2016-09-20T19:47:23Z

csharp/Facebook.CSSLayout/CSSCachedMeasurement.cs

@@ -0,0 +1,12 @@
+namespace Facebook.CSSLayout
+{
+    public class CSSCachedMeasurement


This class should not be needed as it is not part of the public api

@emilsjolander removed as per request

emilsjolander · 2016-09-20T19:47:57Z

csharp/Facebook.CSSLayout/CSSLayout.cs

+namespace Facebook.CSSLayout
+{
+    public class CSSLayout
+    {


This class should not be needed as it is handled in the C code.

emilsjolander · 2016-09-20T19:48:20Z

csharp/Facebook.CSSLayout/CSSLayoutContext.cs

@@ -0,0 +1,8 @@
+namespace Facebook.CSSLayout
+{
+    public class CSSLayoutContext


This class should not be needed as the C version does not have it.

@emilsjolander is the intent then not to implement the regular CSSNode class and only implement the native class?

@emilsjolander removed as per request

ghost · 2016-09-20T20:46:56Z

@mattpodwysocki updated the pull request - view changes

ghost · 2016-09-21T18:34:45Z

@mattpodwysocki updated the pull request - view changes

facebook-github-bot · 2016-09-21T18:41:23Z

@mattpodwysocki updated the pull request - view changes

facebook-github-bot · 2016-09-21T18:57:17Z

@mattpodwysocki updated the pull request - view changes

ghost · 2016-09-21T21:01:20Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

emilsjolander · 2016-09-21T21:31:48Z

Everything is looking great. Thanks for being so responsive! Only thing I see missing is integrating this with buck. You can find more info on building csharp with buck here https://buckbuild.com/rule/csharp_library.html

ghost · 2016-09-21T22:33:59Z

@mattpodwysocki updated the pull request - view changes

emilsjolander · 2016-09-22T18:17:20Z

I chatted with @mattpodwysocki and there are a couple things that are blocking proper buck support for this so we will skip getting this working on this pull request and instead we plan on fixing this afterwards.

ghost · 2016-09-22T20:08:43Z

@mattpodwysocki updated the pull request - view changes

emilsjolander · 2016-09-22T20:12:27Z

Looking great! Thanks for the super quick response times. I'll try to merge this today.

ghost · 2016-09-22T20:13:23Z

Thanks for importing. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: This version of the css-layout project includes support for .NET projects. Up in the air is how many configurations of .NET projects we allow for, such as Portable Class Libraries, Universal Windows Platform, .NET Core, etc. Still needs integration with Buck. Closes #220 Reviewed By: lucasr Differential Revision: D3909367 Pulled By: emilsjolander fbshipit-source-id: aaaa9c4ff2d3452649f256c3268cf873cf33a0b9

langpavel · 2016-09-23T02:09:10Z

Can you clarify goodness? It's nothing in code, only confused me more..

emilsjolander · 2016-09-23T02:17:27Z

Pavel, what are you referring to?
On Thu, 22 Sep 2016 at 19:09, Pavel Lang [email protected] wrote:

Can you clarify goodness? It's nothing in code, only confused me more..

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#220 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdIpDEqTbY277VyS-UPE37U0lrukASCks5qszTKgaJpZM4KB_sA
.

Daniel15 · 2016-10-04T00:28:12Z

The C stuff looks a bit Windows-specific; it would be good to get a build working on Linux too so this could be used on .NET Core or on Mono 😄

Daniel15 · 2016-10-04T00:21:56Z

csharp/.hgignore

@@ -0,0 +1,265 @@
+## Ignore Visual Studio temporary files, build results, and


I wonder if .hgignore could just be a symlink to .gitignore?

Yeah probably

Daniel15 · 2016-10-04T00:24:04Z

csharp/Facebook.CSSLayout/CSSLayoutContext.cs

+{
+    public class CSSLayoutContext
+    {
+        public MeasureOutput measureOutput { get; } = new MeasureOutput();


Uppercase first character of property name (measureOutput -> MeasureOutput)?

Daniel15 · 2016-10-04T00:24:45Z

csharp/Facebook.CSSLayout/CSSNode.cs

+        {
+            if (_isDisposed)
+            {
+                throw new ObjectDisposedException("CSSNode");


nameof(CSSNode) assuming you're using C# 6.0 😄

Daniel15 · 2016-10-04T00:25:58Z

csharp/Facebook.CSSLayout/CSSNode.cs

+                CheckDisposed();
+                Native.CSSNodeStyleSetFlex(_cssNode, value);
+            }
+        }


All these properties feel like a lot of boilerplate, I wonder if they could be autogenerated?

Daniel15 · 2016-10-04T00:27:15Z

csharp/Facebook.CSSLayout/Native.cs

+    {
+        private const string DllName = "CSSLayout.dll";
+
+        [DllImport(DllName)]


I think this should omit the .dll suffix so it could potentially work on Linux in the future.

emilsjolander · 2016-10-04T11:09:38Z

@Daniel15 Feel free to open a pull request with further platform support 👍

mattpodwysocki added 5 commits September 16, 2016 14:58

initial commit 🚀

2f1f363

Updating to latest CSSNode

012e65c

Updating to latest CSSNode

d013551

Another update 🚀

2e145c5

Updating CSSNode 🚀

31befa1

🚀 CSSStyle

678d228

emilsjolander reviewed Sep 20, 2016

View reviewed changes

🚀 changing dllexport

e3b1710

🚀 removing unnecessary files

5247d1e

🚀 adding extra ignores

9b059b3

🚀 removing all extra classes

df024ef

ghost added the CLA Signed label Sep 21, 2016

🚀 initial BUCK

ef0639e

ghost added the CLA Signed label Sep 21, 2016

ghost added the CLA Signed label Sep 22, 2016

🚀 adding headers and virtual Dirty

2841da4

ghost added the CLA Signed label Sep 22, 2016

ghost closed this in facebook/react-native@2da0888 Sep 22, 2016

Daniel15 reviewed Oct 4, 2016

View reviewed changes

This pull request was closed.

		@@ -0,0 +1,265 @@
		## Ignore Visual Studio temporary files, build results, and

Introducing .NET version of css-layout using P/Invoke #220

Introducing .NET version of css-layout using P/Invoke #220

Conversation

mattpodwysocki commented Sep 20, 2016

ghost commented Sep 20, 2016

ghost commented Sep 20, 2016

emilsjolander left a comment

Choose a reason for hiding this comment

emilsjolander Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 20, 2016

ghost commented Sep 21, 2016

facebook-github-bot commented Sep 21, 2016

facebook-github-bot commented Sep 21, 2016

ghost commented Sep 21, 2016

emilsjolander commented Sep 21, 2016

ghost commented Sep 21, 2016

emilsjolander commented Sep 22, 2016

ghost commented Sep 22, 2016

emilsjolander commented Sep 22, 2016

ghost commented Sep 22, 2016

langpavel commented Sep 23, 2016

emilsjolander commented Sep 23, 2016

Daniel15 commented Oct 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daniel15 Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

Daniel15 Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilsjolander commented Oct 4, 2016

emilsjolander Sep 20, 2016 •

edited

Loading

Daniel15 Oct 4, 2016 •

edited

Loading

Daniel15 Oct 4, 2016 •

edited

Loading