Remove nunicode from android binding #12497

tobrun · 2018-07-27T12:30:49Z

This PR is the first step to remove nunicode from the Mapbox Maps SDK for Android. Removing this c++ library would result in decreasing the overal binary size of the SDK.

In this PR, we are replacing the nunicode uppercase and lowercase invocations by wiring through jni to leverage the android platform specific java/lang/String#toUpperCase() and jave/lang/String#toLowerCase() instead.

Note though, we can't remove nunicode yet as it's used for collator expressions (#12268).
@ChrisLoer would it be possible to move that implementaton to Collator.java in the same way as this PR does for upperCasing/lowerCasing?

ChrisLoer · 2018-07-27T15:48:59Z

@ChrisLoer would it be possible to move that implementaton to Collator.java in the same way as this PR does for upperCasing/lowerCasing?

Our Android Collator implementation currently uses nunicode for "diacritic folding", not for case shifting, so I don't know a straightforward way to apply the same change there. I'm definitely not saying it's not possible -- just that I haven't figured it out.

kkaefer · 2018-07-31T10:20:23Z

Did you run the tests in https://github.com/mapbox/mapbox-gl-native/blob/master/test/util/text_conversions.test.cpp?

kkaefer · 2018-08-20T16:25:33Z

platform/android/src/string_stdlib.cpp

@@ -0,0 +1,21 @@
+#include <mbgl/util/platform.hpp>


Let's rename this file, since it's no longer "stdlib"

kkaefer · 2018-08-20T16:26:35Z

platform/android/src/java/lang.cpp

+jni::Object<String> String::New(JNIEnv& env, jni::String value) {
+    static auto constructor = String::javaClass.GetConstructor<jni::String>(env);
+    return String::javaClass.New(env, constructor, value);
+}


Why do we need this function?

jni::String is a Object, to be able to hook into toUpperCase/toLowerCase I'm creating mbgl::android::java::lang::String. Would prefer not having to create a new String for each uppercase/lowercase. Any pointers on how to get this working?

If you remove static constexpr auto Name() { return "java/lang/String"; }; and change jni::Class<String> to jni::Class<jni::StringTag>, you will be able to use value.Call(env, method) directly.

Thanks @jfirebaugh! this makes total sense

tobrun · 2018-08-20T16:31:38Z

Did you run the tests in https://github.com/mapbox/mapbox-gl-native/blob/master/test/util/text_conversions.test.cpp?

Was having issues running them, I'm making running our gtest suite on android on CI a priority.

kkaefer · 2018-08-28T16:21:37Z

Is there anything that's blocking this PR?

lilykaiser · 2018-08-28T20:26:48Z

iirc it may be something related to #12688 but I will let @tobrun weigh in.

tobrun · 2018-08-29T05:25:24Z

@kkaefer :

Running core tests on this change:
- have issues running them on android, ticketed Running core tests results in segmentation fault #12761
- implemented the core test as a java test to be sure this PR works as expected see UpperLowerCaseTest.java.
Is there anything that's blocking this PR?
- to completely remove nunicode on android and close Android Unicode handling #2355, I need to look into supporting the correlator support that was added in [android] "collator" and "resolved-locale" expressions support #12688, this can also be tackled in a follow up PR.

I addressed #12497 (comment) and would love some input on #12497 (comment) if possible.

tobrun · 2018-09-03T10:03:27Z

@ChrisLoer been looking what it would take to remove nunicode completely, I was able to get it compile with providing a platform specific implementation of unaccent.cpp. We can leverage Normalizer.java to provide that implementation on Android. Branch withr removed nunicde + a no-op unaccent.cpp here.

Thoughts?

ChrisLoer · 2018-09-04T21:47:42Z

We can leverage Normalizer.java to provide that implementation on Android

The idea sounds attractive to me, but don't you need something that can denormalize instead of normalize? My understanding of the nunicode implementation is that it essentially denormalizes the text and then strips out all the combining characters that are in one of the "accent" blocks:

		{ 0x0300, 0x036F },  /* Combining Diacritical Marks */
		{ 0x1AB0, 0x1AFF },  /* Combining Diacritical Marks Extended */
		{ 0x20D0, 0x20FF },  /* Combining Diacritical Marks for Symbols */
		{ 0x1DC0, 0x1DFF },  /* Combining Diacritical Marks Supplement */

If you're going to work on normalized text then you'd have to maintain a much larger map of character transformations.

kkaefer · 2018-09-05T07:02:33Z

Looks like it supports passing Normalizer.Form.NFD to get the canonical decomposition, then strip diacritics with a regular expression.

tobrun · 2018-09-05T12:34:15Z

Thanks for the input! was able to get it working and the render tests confirm this:

Actual

Expected

Diff

I took the approach to remove diatrics with a regex as shown in this blogpost.

Will clean up the code and merge to this PR.

kkaefer · 2018-09-05T13:22:15Z

platform/android/src/unaccent.cpp

+    jni::String unaccented = android::java::lang::String::replaceAll(*env, normalized,
+                                                                     jni::Make<jni::String>(*env, "\\p{InCombiningDiacriticalMarks}+"),
+                                                                     jni::Make<jni::String>(*env, ""));
+    return jni::Make<std::string>(*env, unaccented);


Could we implement an unaccent(String str) function in com.mapbox.mapboxsdk.utils.StringUtils that does all of that instead of going back and forth through the JNI? That could potentially lower the binary footprint a bit.

yes, this is way simpler solution, addressed with 5b326ef

kkaefer · 2018-09-05T15:17:50Z

platform/android/src/unaccent.cpp

+
+std::string unaccent(const std::string& str) {
+    android::UniqueEnv env = android::AttachEnv();
+    jni::String unaccented = android::StringUtils::unaccent(*env,jni::Make<jni::String>(*env, str));


Do we need to delete refs here to avoid table overflows in tight loops?

yes, looking forward using jni::SeizeLocal to avoid these issues!

It's going to be even better than that -- mapbox/jni.hpp#40.

ChrisLoer · 2018-09-05T15:47:33Z

platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/utils/StringUtils.java

+  @NonNull
+  static String unaccent(@NonNull String value) {
+    return Normalizer.normalize(value, Normalizer.Form.NFD)
+      .replaceAll("\\p{InCombiningDiacriticalMarks}+", "");


Awesome! From https://docs.oracle.com/javase/8/docs/api/java/util/regex/Pattern.html, it looks like you can use match any Unicode block, in which case we can get a little closer to the nunicode implementation by also matching against Combining Diacritical Marks Extended/for Symbols/Supplement.

@ChrisLoer having a hard time figuring out the correct configuration for above 😅Typography is clearly not my cup of tea. Based on above, I should allow any unicode character? Thus something as"[^\\p{ASCII}]" or do I need to work on allowing multiple \\p{} items (For example: InCombiningDiacriticalMarks in combination with configurations on symbols/supplement from https://www.regular-expressions.info/unicode.html).

Err, I think it'll be something like:

(\\p{InCombiningDiacriticalMarks}|\\p{InCombiningDiacriticalMarksExtended}|\\p{InCombiningDiacriticalMarksForSymbols}|\\p{InCombiningDiacriticalMarksSupplement})+

?

Wow, I wouldn't have figured that out on my own. Thank you, I now went with:

(\p{InCombiningDiacriticalMarks}|\p{InCombiningDiacriticalMarksForSymbols}|\p{InCombiningDiacriticalMarksSupplement})+

as InCombiningDiacriticalMarksExtended didn't compile for me:

Huh, I wonder why "Extended" wasn't included (although the documentation is pretty vague on the naming -- it sounds like you just strip the spaces from the Unicode block name). That will make for a small difference from the nunicode implementation, but we already tolerate differences in collator behavior from device to device and locale to locale so I think it should be OK.

tobrun · 2018-09-10T21:52:27Z

21fc53f updates this PR to be jni 0.4.0 compatible.

kkaefer · 2018-09-11T09:08:03Z

platform/android/src/string_util.cpp

+std::string uppercase(const std::string& str) {
+    auto env{ android::AttachEnv() };
+    jni::Local<jni::String> value = jni::Make<jni::String>(*env, str.c_str());
+    static auto& javaClass = jni::Class<jni::StringTag>::Singleton(*env);


Let's fold this line into the next to avoid the need for another static variable. Same for lowercase

…r uppercasing an lowercasing with an Android specific String.java equivalent

tobrun added the Android Mapbox Maps SDK for Android label Jul 27, 2018

tobrun added this to the android-v6.4.0 milestone Jul 27, 2018

tobrun self-assigned this Jul 27, 2018

tobrun requested a review from ivovandongen July 27, 2018 12:30

tobrun mentioned this pull request Jul 27, 2018

Android Unicode handling #2355

Closed

lilykaiser added the high priority label Jul 30, 2018

LukasPaczos removed this from the android-v6.4.0 milestone Aug 15, 2018

kkaefer reviewed Aug 20, 2018

View reviewed changes

tobrun force-pushed the tvn-remove-nunicode branch from afc8838 to cef6472 Compare August 29, 2018 05:08

tobrun force-pushed the tvn-remove-nunicode branch from cef6472 to 98a3c0a Compare August 29, 2018 18:34

kkaefer reviewed Sep 5, 2018

View reviewed changes

tobrun force-pushed the tvn-remove-nunicode branch from d52559c to 5b326ef Compare September 5, 2018 14:12

kkaefer reviewed Sep 5, 2018

View reviewed changes

tobrun force-pushed the tvn-remove-nunicode branch from 5b326ef to f13eee1 Compare September 5, 2018 15:27

ChrisLoer reviewed Sep 5, 2018

View reviewed changes

tobrun force-pushed the tvn-remove-nunicode branch from f13eee1 to 64bd64c Compare September 5, 2018 15:58

tobrun changed the title ~~Replace nunicode upperCase/lowerCase with Android String~~ Remove nunicode from android binding Sep 10, 2018

tobrun force-pushed the tvn-remove-nunicode branch 2 times, most recently from a11979e to 21fc53f Compare September 10, 2018 21:20

tobrun force-pushed the tvn-remove-nunicode branch from 21fc53f to ff68253 Compare September 11, 2018 08:13

kkaefer approved these changes Sep 11, 2018

View reviewed changes

tobrun force-pushed the tvn-remove-nunicode branch from ff68253 to 67f605f Compare September 11, 2018 10:18

[android] - replace platform default implementation using nunicode fo…

4e49c7b

…r uppercasing an lowercasing with an Android specific String.java equivalent

tobrun force-pushed the tvn-remove-nunicode branch from 67f605f to 4e49c7b Compare September 11, 2018 10:18

tobrun merged commit 6e171a6 into master Sep 11, 2018

tobrun deleted the tvn-remove-nunicode branch September 11, 2018 10:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove nunicode from android binding #12497

Remove nunicode from android binding #12497

tobrun commented Jul 27, 2018

ChrisLoer commented Jul 27, 2018

kkaefer commented Jul 31, 2018

kkaefer Aug 20, 2018

kkaefer Aug 20, 2018

tobrun Aug 29, 2018

jfirebaugh Aug 29, 2018

tobrun Aug 29, 2018

tobrun commented Aug 20, 2018

kkaefer commented Aug 28, 2018

lilykaiser commented Aug 28, 2018

tobrun commented Aug 29, 2018

tobrun commented Sep 3, 2018 •

edited

Loading

ChrisLoer commented Sep 4, 2018

kkaefer commented Sep 5, 2018

tobrun commented Sep 5, 2018

kkaefer Sep 5, 2018

tobrun Sep 5, 2018

kkaefer Sep 5, 2018

tobrun Sep 5, 2018

jfirebaugh Sep 5, 2018

ChrisLoer Sep 5, 2018

tobrun Sep 10, 2018

ChrisLoer Sep 10, 2018

tobrun Sep 11, 2018

ChrisLoer Sep 11, 2018

tobrun commented Sep 10, 2018 •

edited

Loading

kkaefer Sep 11, 2018

Remove nunicode from android binding #12497

Remove nunicode from android binding #12497

Conversation

tobrun commented Jul 27, 2018

ChrisLoer commented Jul 27, 2018

kkaefer commented Jul 31, 2018

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

tobrun commented Aug 20, 2018

kkaefer commented Aug 28, 2018

lilykaiser commented Aug 28, 2018

tobrun commented Aug 29, 2018

tobrun commented Sep 3, 2018 • edited Loading

ChrisLoer commented Sep 4, 2018

kkaefer commented Sep 5, 2018

tobrun commented Sep 5, 2018

Actual

Expected

Diff

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

tobrun commented Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

tobrun commented Sep 3, 2018 •

edited

Loading

tobrun commented Sep 10, 2018 •

edited

Loading