Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix TIMOB-23198 - Hyperloop: Android can't access fields with primitive array types #16

Merged
merged 1 commit into from
Apr 14, 2016
Merged

Fix TIMOB-23198 - Hyperloop: Android can't access fields with primitive array types #16

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 20 additions & 4 deletions android/src/hyperloop/HyperloopUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,17 @@ static Object[] wrapArguments(Class<?>[] params, Object[] args) {
* @return
*/
static Object wrap(Class<?> paramType, Object result) {
if (result == null) {
return result;
}
if (result instanceof byte[]) { // our bridge can't handle byte[], but can do short[] - so convert to short[]
byte[] b = (byte[]) result;
short[] s = new short[b.length];
for (int i = 0; i < b.length; i++) {
s[i] = b[i];
}
return s;
}
return isKnownType(result) ? result
: HyperloopModule.getProxyFactory().newInstance(paramType, result);
}
Expand All @@ -81,14 +92,19 @@ private static boolean isKnownType(Object item) {
// Here's what TypeConverter lists:
// short, int, long, float, double, boolean, string, Date, (Object as Function?)
// Object[], boolean[], short[], int[], long[], float[], double[]
// Since we _always_ end up here due to reflection, we always get boxed types, not primitives
// so we check against the boxed types, not primitives (including arrays: for example, Integer[] instanceof Object[] == true)
return item == null || item instanceof KrollProxy || item instanceof Integer
// Since we almost always end up here due to reflection, we always boxed types, not primitives in those cases
// so we check against the boxed types, not primitives, first (including arrays: for example, Integer[] instanceof Object[] == true)
return item instanceof KrollProxy || item instanceof Integer
|| item instanceof Double || item instanceof Float
|| item instanceof Byte || item instanceof Short
|| item instanceof Long || item instanceof HashMap
|| item instanceof String || item instanceof Boolean
|| item instanceof Date || item instanceof Object[];
|| item instanceof Date || item instanceof Object[]
// When we get a field through reflection we _can_ get primitive arrays, so check for all of those too
// Note lack of byte[], since bridge doesn't handle that, we treat it specially in wrap
|| item instanceof int[] || item instanceof double[]
|| item instanceof float[] || item instanceof short[]
|| item instanceof long[] || item instanceof boolean[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Checked the java docs for primitive types. Do we need to list char here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good catch - but I'm not really sure how we should handle Character and char[].

The issue here is that:

  • the bridge doesn't handle char or char[] conversions
  • char is kind of unique in java in that it's essentially a 16-bit unsigned value meant to represent a Unicode character. So do we try and represent a char[] as String, or do we represent it as something more like short[] or int[]? (short is a 16-bit signed value, int is 32-bit signed) - to me the simple answer is turn char[] into String, but as single char into a String of length 1? Seems a little odd, but also I have seen some unique cases where people treat a char as a number more than a "character".

See http://stackoverflow.com/questions/14204907/what-is-the-difference-between-short-and-character-apart-from-processing

It looks to me like we carefully (or luckily?) avoided char[] and char altogether in terms of surfaced API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As another reference: NativeScript seems to treat char as a String: https://docs.nativescript.org/runtimes/android/marshalling/java-to-js.html

I'm going to guess char as a whole could likely use some loving here so that we:

  • Can accept a JS string for a Character/char/char[] argument in a method call (or assigning to a Character/char/char[] field)
  • Return a JS string when a field is of type Character/char/char[], or a method returns Character/char/char[]

Not sure if we also want to be even more liberal and accept JS Numbers (provided they are in a valid range) for char (or array of numbers for char[])

Copy link
Contributor

Choose a reason for hiding this comment

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

The easier to use for the developers the better it is. Pedro once supported full conversation of array, numbers, objects etc. to native types, but Jeff removed it in his implementation. So we need to see how both platforms would do it to achieve as much parity as possible, even in these points.

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'd propose we merge this for now, and open a JIRA ticket for handling char/char[]/Character separately.

}

/**
Expand Down