You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by kalliste <gi...@git.apache.org> on 2017/02/03 21:53:37 UTC
[GitHub] cordova-android pull request #358: CB-12424: (android) Fix encoding of multi...
GitHub user kalliste opened a pull request:
https://github.com/apache/cordova-android/pull/358
CB-12424: (android) Fix encoding of multipart messages.
Currently when I try to assemble a multipart message cordova-android assembles the data in NativeToJsMessageQueue.java as [""] but if I call pMessageLoopResponse(callbackContext, 3, 5) it should generate [3, 5]
private void pMessageLoopResponse(final CallbackContext ctx, int iMsg, int iTag) {
List<PluginResult> results = new ArrayList<PluginResult>();
results.add(0, new PluginResult(PluginResult.Status.OK, iMsg));
results.add(1, new PluginResult(PluginResult.Status.OK, iTag));
PluginResult result = new PluginResult(PluginResult.Status.OK, results);
result.setKeepCallback(true);
ctx.sendPluginResult(result);
}
We split off encodeMessageAsJsMessage as its own method to support multipart messages including ones containing array buffer, binary string, or other multipart messages.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kalliste/cordova-android master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-android/pull/358.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #358
----
commit 2dd9acf2fa0c179269002e369cb0d7df598741eb
Author: Josh Jackson <jj...@kallisteconsulting.com>
Date: 2017-02-03T21:20:37Z
Fix encoding of multipart messages.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #358: CB-12424: (android) Fix encoding of multipart me...
Posted by infil00p <gi...@git.apache.org>.
Github user infil00p commented on the issue:
https://github.com/apache/cordova-android/pull/358
@kalliste This test is based on the old test framework. Can you pull down the latest and take a look at this test and see if you can fit it in here? https://github.com/apache/cordova-android/blob/master/test/app/src/test/java/org/apache/cordova/unittests/NativeToJsMessageQueueTest.java
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android pull request #358: CB-12424: (android) Fix encoding of multi...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-android/pull/358
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #358: CB-12424: (android) Fix encoding of multipart me...
Posted by kalliste <gi...@git.apache.org>.
Github user kalliste commented on the issue:
https://github.com/apache/cordova-android/pull/358
Ok - I've added a unit test for this now
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #358: CB-12424: (android) Fix encoding of multipart me...
Posted by codecov-io <gi...@git.apache.org>.
Github user codecov-io commented on the issue:
https://github.com/apache/cordova-android/pull/358
# [Codecov](https://codecov.io/gh/apache/cordova-android/pull/358?src=pr&el=h1) Report
> Merging [#358](https://codecov.io/gh/apache/cordova-android/pull/358?src=pr&el=desc) into [master](https://codecov.io/gh/apache/cordova-android/commit/5591a1a4e896ed8693abd5234cb2370ead20ed5d?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #358 +/- ##
=======================================
Coverage 35.58% 35.58%
=======================================
Files 12 12
Lines 1037 1037
Branches 173 173
=======================================
Hits 369 369
Misses 668 668
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-android/pull/358?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `\u0394 = absolute <relative> (impact)`, `� = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-android/pull/358?src=pr&el=footer). Last update [5591a1a...3e03f0d](https://codecov.io/gh/apache/cordova-android/compare/5591a1a4e896ed8693abd5234cb2370ead20ed5d...3e03f0da51c4c39a20867c804c5c7b7b4ca44a30?el=footer&src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #358: CB-12424: (android) Fix encoding of multipart me...
Posted by infil00p <gi...@git.apache.org>.
Github user infil00p commented on the issue:
https://github.com/apache/cordova-android/pull/358
Can we have better method names? encodeJsMessage and encodeMessageAsJsMessage looks confusing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #358: CB-12424: (android) Fix encoding of multipart me...
Posted by kalliste <gi...@git.apache.org>.
Github user kalliste commented on the issue:
https://github.com/apache/cordova-android/pull/358
I have now submitted an ICLA and also an updated pull request with a more reasonable method name.
> Is this due to the switch in the default bridge?
That is the impression I got.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #358: CB-12424: (android) Fix encoding of multipart me...
Posted by infil00p <gi...@git.apache.org>.
Github user infil00p commented on the issue:
https://github.com/apache/cordova-android/pull/358
@kalliste Perhaps buildJsMessage, or convertToJsMessage? You don't need to create a new PR, just add a commit to this branch, and the change should propogate through.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #358: CB-12424: (android) Fix encoding of multipart me...
Posted by shankari <gi...@git.apache.org>.
Github user shankari commented on the issue:
https://github.com/apache/cordova-android/pull/358
Note that the multi-part handling appears to be a regression.
When I was on android@5.1.1, the email composer plugin, which returns a multi-part result from `isAvailable` worked. When I upgraded to android@6.1.2, it stopped working.
https://github.com/shankari/cordova-plugin-email-composer/blob/master/src/android/EmailComposer.java#L112
```
List<PluginResult> messages = new ArrayList<PluginResult>();
messages.add(new PluginResult(PluginResult.Status.OK, available[0]));
messages.add(new PluginResult(PluginResult.Status.OK, available[1]));
PluginResult result = new PluginResult(
PluginResult.Status.OK, messages);
```
Note further that multipart messages seem to be handled just fine in `encodeAsMessageHelper` (and, by implication, in `encodeAsMessage`) but not in `encodeAsJsMessage`. Is this due to the switch in the default bridge?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #358: CB-12424: (android) Fix encoding of multipart me...
Posted by kalliste <gi...@git.apache.org>.
Github user kalliste commented on the issue:
https://github.com/apache/cordova-android/pull/358
Sure! I can edit this and submit a new pull request --- What would be least confusing to you?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #358: CB-12424: (android) Fix encoding of multipart me...
Posted by kalliste <gi...@git.apache.org>.
Github user kalliste commented on the issue:
https://github.com/apache/cordova-android/pull/358
Ok - I've pulled the new code and updated this test for it.
I have not yet found a gradle task in the new version that will show the results of the tests.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #358: CB-12424: (android) Fix encoding of multipart me...
Posted by infil00p <gi...@git.apache.org>.
Github user infil00p commented on the issue:
https://github.com/apache/cordova-android/pull/358
@kalliste Thanks for submitting the ICLA. Is it possible for you to add a Unit Test to the new Unit Testing framework for this patch? People have been complaining about this being a regression despite the fact that we have zero documentation and zero tests. (In fact the refactored bridge had absolutely no unit tests until now, which is why I haven't pulled this in yet and have been working on getting some tests up.) The test project is in tests, and it's a standard Android project using JUnit and Espresso.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-android issue #358: CB-12424: (android) Fix encoding of multipart me...
Posted by infil00p <gi...@git.apache.org>.
Github user infil00p commented on the issue:
https://github.com/apache/cordova-android/pull/358
@kalliste BTW: Have you signed the ICLA with Apache? https://www.apache.org/licenses/#clas
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org