opened
purplecabbage
username_0
Included license files, and meta.
Added Newtonsoft json dll
issue
created
sgrebnov
username_1
:+1: lgtm
comment
created
nikhilkh
username_2
@username_0 - When do you plan to merge this. There is another related issue now causing WP8 mobilespec tests to crash because this has not been fixed within the platform: https://github.com/apache/cordova-plugin-file-transfer/pull/82
comment
created
brodybits
username_3
apache/cordova-plugin-file-transfer#82 is a "quick fix" that could have been included already. Looking through the references, if this fix is accepted and released, I suspect it should be possible for [cordova-plugin-file-transfer](cordova-plugin-file-transfer) to include apache/cordova-plugin-file-transfer#54.
comment
created
purplecabbage
username_0
Today. 



>
comment
created
kim07
username_4
:+1:
comment
created
vladimir-kotikov
username_5
So the https://github.com/apache/cordova-plugin-file-transfer/pull/72 probably should be closed then...
comment
created
purplecabbage
username_0
We need to release this first, and file-transfer plugin should declare the required version.
comment
created
brodybits
username_3
then...

My point that apache/cordova-plugin-file-transfer#54 was an improvement
that was closed and rejected since we did not expect to add the Newtonsoft
JSON library. Now that we *do* include the Newtonsoft library, it is
**now** possible to include apache/cordova-plugin-file-transfer#54 (in
favor over apache/cordova-plugin-file-transfer#72 and
apache/cordova-plugin-file-transfer#82).

I hope [cordova-plugin-file-transfer](
https://github.com/apache/cordova-plugin-file-transfer) will consider
apache/cordova-plugin-file-transfer#54 again.
comment
created
purplecabbage
username_0
Yeah @username_3, will have another look at apache/cordova-plugin-file-transfer/pull/54
comment
created
brodybits
username_3
apache/cordova-plugin-file-transfer#54

:+1: thanks!
comment
created
alsorokin
username_6
It looks like these changes brought new test failures on wp8:
https://issues.apache.org/jira/browse/CB-8968
comment
created
alsorokin
username_6
@username_0 @goya could you please look at these failures?
comment
created
brodybits
username_3
@username_6 I will take a look as well. Cannot promise I will find anything.
comment
created
brodybits
username_3
Can someone describe how I would run these cordova-plugin-file tests for wp8?
comment
created
vladimir-kotikov
username_5
@username_3 follow these steps:
* clone https://github.com/apache/cordova-plugin-file-transfer.git somewhere on your disk
* Create a new application
* Add three plugins:
```
    cordova plugin add path-to-cloned-file-transfer-plugin
    cordova plugin add path-to-cloned-file-transfer-plugin/tests
    cordova plugin add https://github.com/apache/cordova-plugin-test-framework.git
```
* Update config.xml at the root of your app - change `<content src="index.html">` to `<content src="cdvtests/index.html">`
* Run application
comment
created
brodybits
username_3
If we would restore the old code in `JsonHelper.Serialize` in `template/cordovalib/JSON/JsonHelper.cs` then the problem in CB-8968 goes away. Then:

Using `JsonConvert.SerializeObject` from `Newtonsoft.Json` in cordova-plugin-file *is working OK in the test*:

```diff
--- a/src/wp/File.cs
+++ b/src/wp/File.cs
@@ -22,7 +22,9 @@
 using System.Text;
 using System.Windows;
 using System.Windows.Resources;
+
 using WPCordovaClassLib.Cordova.JSON;
+using Newtonsoft.Json;
 
 namespace WPCordovaClassLib.Cordova.Commands
 {
@@ -771,7 +773,7 @@ public void readAsText(string options)
                 // JIRA: https://issues.apache.org/jira/browse/CB-8792
                 // Need to perform additional serialization here because NativeExecution is always trying
                 // to do JSON.parse() on command result. This leads to issue when trying to read JSON files
-                var resultText = JsonHelper.Serialize(text);
+                var resultText = JsonConvert.SerializeObject(text);
                 DispatchCommandResult(new PluginResult(PluginResult.Status.OK, resultText), callbackId);
             }
             catch (Exception ex)
```

Using `JsonConvert.SerializeObject` from `Newtonsoft.Json` in `template/cordovalib/ScriptCallback.cs` is *working OK in the cordova-plugin-file test*:

```diff
--- a/template/cordovalib/ScriptCallback.cs
+++ b/template/cordovalib/ScriptCallback.cs
@@ -25,6 +25,8 @@
 using WPCordovaClassLib.Cordova.JSON;
 using System.Diagnostics;
 
+using Newtonsoft.Json;
+
 namespace WPCordovaClassLib.Cordova
 {
     /// <summary>
@@ -65,7 +67,7 @@ public ScriptCallback(string function, string id, object msg, object value)
             this.ScriptName = function;
 
             String arg = String.Format("{{\"id\": {0}, \"msg\": {1}, \"value\": {2}}}",
-                 JsonHelper.Serialize(id), JsonHelper.Serialize(msg), JsonHelper.Serialize(value));
+                 JsonConvert.SerializeObject(id), JsonConvert.SerializeObject(msg), JsonConvert.SerializeObject(value));
 
             this.Args = new string[] { arg };
         }
```

Using `JsonConvert.SerializeObject` from `Newtonsoft.Json` in `template/cordovalib/CordovaCommandCall.cs` gave me a problem with `file.spec.83` (blob test) the first time, then was passing the cordova-plugin-file tests when I tried it again (should be OK).

Using `JsonConvert.SerializeObject` from `Newtonsoft.Json` in `template/cordovalib/PluginResult.cs` gives me the errors as described in CB-8968.

Wondering if any other Apache Cordova or major third-party plugins would be affected as well.
comment
created
brodybits
username_3
The Newtonsoft JSON library does seem much easier to use and perhaps more efficient as well, but unlike the .NET JSON `DataContractJsonSerializer`, Newtonsoft does not support serialization of arbitrary types. This becomes a problem with the cordova-file-plugin when it attempts to send a PluginResult with a byte array (`byte[]`) in response to a `readAsArrayBuffer` request. This is the cause of CB-8968, and it could affect other plugins as well.

I have identified two alternative solutions to fix CB-8968:

I. Fix `File.cs` in cordova-plugin-file to send the `PluginResult` with a normal integer array:

```diff
--- a/src/wp/File.cs
+++ b/src/wp/File.cs
@@ -695,7 +695,10 @@ public void readAsArrayBuffer(string options)
                     buffer = readFileBytes(filePath, startPos, endPos, isoFile);
                 }
 
-                DispatchCommandResult(new PluginResult(PluginResult.Status.OK, buffer), callbackId);
+                int[] bufferAsIntArray = new int[buffer.Length];
+                for (int i=0; i<buffer.Length; ++i) bufferAsIntArray[i] = buffer[i];
+
+                DispatchCommandResult(new PluginResult(PluginResult.Status.OK, bufferAsIntArray), callbackId);
             }
             catch (Exception ex)
             {
```

Idea thanks to: http://stackoverflow.com/questions/15226921/how-to-serialize-byte-as-simple-json-array-and-not-as-base64-in-json-net

II. In `template/cordovalib/JSON/JsonHelper.cs` add a new function such as `public static string SerializeAny(object obj)` with what used to be the contents of `JSON.JsonHelper.Serialize`, and call this function instead of `JsonConvert.SerializeObject` in `PluginResult.cs`.

I am not sure which solution would have better performance. Solution II only goes through the data once, but solution II delegates the looping to the framework libraries, which *could* do the hard work in C/C++/assembly.

I am happy to issue a PR for either solution. Solution I would be easiest.
comment
created
purplecabbage
username_0
I think we should go with solution 1.
```
int[] bytesAsIntArray = Array.ConvertAll(buffer, c => (int)c);
```
comment
created
brodybits
username_3
Array.ConvertAll did not compile on Visual Studio Express 2013, and is apparently not supported for Metro or Silverlight. Figured out how to do this using Linq:

```diff
diff --git a/src/wp/File.cs b/src/wp/File.cs
index 941c9c8..085ef77 100644
--- a/src/wp/File.cs
+++ b/src/wp/File.cs
@@ -13,6 +13,7 @@
 */
 
 using System;
+using System.Linq;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.IO;
@@ -695,7 +696,9 @@ public void readAsArrayBuffer(string options)
                     buffer = readFileBytes(filePath, startPos, endPos, isoFile);
                 }
 
-                DispatchCommandResult(new PluginResult(PluginResult.Status.OK, buffer), callbackId);
+                int[] bufferAsIntArray = (from b in buffer select (int)b).ToArray();
+
+                DispatchCommandResult(new PluginResult(PluginResult.Status.OK, bufferAsIntArray), callbackId);
             }
             catch (Exception ex)
             {
```

Please let me know if you want me to issue a PR.
comment
created
purplecabbage
username_0
Looks good, send away.

@username_0
risingj.com
comment
created
brodybits
username_3
Issued PR to fix CB-8968 (automatic build failed, don't think it is my fault)
comment
created
purplecabbage
username_0
nope, all auto build systems are hooped right now because of an issue with
Capitalized npm package names.

@username_0
risingj.com
comment
created
nikhilkh
username_2
@username_0 I'm really concerned with this switch for WP8 as it is breaking in nature. It changes how the Cordova WP8 JSON APIs behave and will cause a fair number of plugins to break. All plugins that break will have to make updates to support the new WP8 platform, and hence can't be used with the older version (3.x) of the WP8 platform. 

I would propose we don't make this disruptive change for WP8 which is a platform with limited runway as WP8.1+ should be using the Windows platform. WP8 should ideally be on life support and not making disruptive breaking changes that affect the plugin ecosystem.

We should consider an incremental fix for file transfer and other plugins that need better support for JSON parsing instead of changing the platform itself: https://github.com/apache/cordova-plugin-file-transfer/pull/72
comment
created
purplecabbage
username_0
yes, it is a breaking change.  That is why it is a 4.0.0 change. Plugin
authors need to use engine tags, and we need to make sure they work.

Personally, if I was writing a WP8.1 app, I would use cordova-wp8 and
upgrade stuff.  Having a C# wrapper around a html/js presentation will
still be appealing to many.  I would not expect cordova-wp8 to wither and
die anytime soon.


@username_0
risingj.com
comment
created
nikhilkh
username_2
If we can avoid the breaking change it would be better and in this case we have a solution that works well to avoid it (apache/cordova-plugin-file-transfer#72). I understand Newtonsoft JSON is a better API and is more efficient.

Another alternative is for WP8 to provide new set of JSON APIs based on Newtonsoft JSON - The plugins that take dependency on these new APIs will have to use the engine tag and drop support for older versions of WP8. The older APIs will remain unchanged and a lot of plugins that are no longer maintained can continue to work.
comment
created
brodybits
username_3
In addition, it is my understanding is that cordova-wp8 is (still) needed to support Windows Phone in PhoneGap build.
comment