[core] Add number-format expression #14840

jmalanen · 2019-06-04T12:13:15Z

jmalanen · 2019-06-04T12:16:39Z

The number-format/currency test is still disabled because there are differences in the output between GL-JS and GL-Native:

Original
f([{},{"properties":{"locale":"ja-JP","currency":"JPY"}}])
Expected: "JP¥ 123,457"
Actual: "￥123,457"
f([{},{"properties":{"locale":"de-DE","currency":"EUR"}}])
Expected: "€ 123,456.79"
Actual: "123.456,79 €"

Roundtripped through serialize()
f([{},{"properties":{"locale":"ja-JP","currency":"JPY"}}])
Expected: "JP¥ 123,457"
Actual: "￥123,457"
f([{},{"properties":{"locale":"de-DE","currency":"EUR"}}])
Expected: "€ 123,456.79"
Actual: "123.456,79 €"

src/mbgl/style/expression/number_format.cpp

jmalanen · 2019-06-04T15:14:19Z

I disabled also the remaining tests. The CI test system doesn't have "en-US" locale installed, which is expected by the GL-JS tests. This causes tests to fail since the outputs don't match.

Serialized

123,456.789+ 123456.789Original
f([{},{}])
Expected: "123,456.789"
Actual: "123456.789"

Roundtripped through serialize()
f([{},{}])
Expected: "123,456.789"
Actual: "123456.789"

failed number-format/default

Original
f([{},{"properties":{"locale":"en-US","min":15,"max":20}}])
Expected: "987,654,321.234568000000000"
Actual: "987654321.234568000000000"
f([{},{"properties":{"locale":"en-US","min":2,"max":4}}])
Expected: "987,654,321.2346"
Actual: "987654321.2346"

Roundtripped through serialize()
f([{},{"properties":{"locale":"en-US","min":15,"max":20}}])
Expected: "987,654,321.234568000000000"
Actual: "987654321.234568000000000"
f([{},{"properties":{"locale":"en-US","min":2,"max":4}}])
Expected: "987,654,321.2346"
Actual: "987654321.2346"

failed number-format/precision

include/mbgl/style/expression/number_format.hpp

src/mbgl/style/expression/number_format.cpp

test/style/expression/expression.test.cpp

src/mbgl/style/expression/number_format.cpp

pozdnyakov · 2019-06-05T08:18:58Z

src/mbgl/style/expression/number_format.cpp

+    std::vector<mbgl::Value> serialized{{ getOperator() }};
+    serialized.emplace_back(number->serialize());
+
+    std::unordered_map<std::string, mbgl::Value> options;


options seem to have just a few items, looks like std::unordered_map is an overkill, better use std::map

include/mbgl/style/expression/number_format.hpp

src/mbgl/style/expression/number_format.cpp

include/mbgl/style/expression/number_format.hpp

src/mbgl/style/expression/number_format.cpp

pozdnyakov · 2019-06-05T08:44:37Z

There is a quite significant binary size increase with the patch, removal of unneeded optional could help to fix it.

pozdnyakov · 2019-06-05T09:19:23Z

@jmalanen pls see @kkaefer's patch removing using <locale> and <iostream>. Using of these classes must be causing the binary size increase.

1ec5 · 2019-06-05T15:46:47Z

Use of the platforms’ built-in internationalization functionality will result in much less binary size increase compared to the current implementation in this PR.

1ec5

Thanks for implementing the iOS/macOS side of things! We’re close, but we do need to figure out the right way to manage these formatter objects, as @julianrex points out.

1ec5 · 2019-06-06T15:45:22Z

platform/darwin/src/string_nsstring.mm

+                         uint8_t minFractionDigits,
+                         uint8_t maxFractionDigits) {
+    NSNumberFormatter *_numberFormatter;
+    _numberFormatter = [[NSNumberFormatter alloc] init];


Does this method always get called on the same thread? If so, we can initialize the object just once using dispatch_once, hold onto it statically, and modify the options each time the method is called. Otherwise, we’ll need to lazily create a different formatter for each combination of options.

Alternatively, we can drop down to CFNumberFormatter, which might not have the same overhead.

Previously, I had suggest _numberFormatter as the variable name with the assumption that the variable would be global rather than local. The underscore is unnecessary for a local variable.

(Also, you can combine a local variable declaration and definition on the same line.)

Thank you for the comments. Now the object is initialized just once using dispatch_once.

1ec5 · 2019-06-06T15:49:18Z

platform/darwin/src/string_nsstring.mm

+                         uint8_t maxFractionDigits) {
+    NSNumberFormatter *_numberFormatter;
+    _numberFormatter = [[NSNumberFormatter alloc] init];
+    _numberFormatter.locale = !localeId.empty() ? [NSLocale localeWithLocaleIdentifier:@(localeId.c_str())] : nil;


We should make sure the default locale matches the default locale of the comparison expression operators as well as the number formatter in GL JS. nil results in either the application’s current locale or the system preferred locale; can’t remember which off the top of my head.

I checked the NSNumberFormatter documentation, but couldn't find what's the default locale in case of nil.

tmpsantos

LGTM, just concerned about adding a dependency on QML for Qt.

platform/qt/src/format_number.cpp

pozdnyakov · 2019-06-26T13:49:30Z

src/mbgl/style/expression/number_format.cpp

+                           optional<std::unique_ptr<Expression>> locale_,
+                           optional<std::unique_ptr<Expression>> currency_,
+                           optional<std::unique_ptr<Expression>> minFractionDigits_,
+                           optional<std::unique_ptr<Expression>> maxFractionDigits_)


why not just std::unique_ptr<Expression> maxFractionDigits_ , maxFractionDigits(std::move(maxFractionDigits_)); (same for others) ?

pozdnyakov · 2019-06-26T13:52:51Z

include/mbgl/style/expression/number_format.hpp

+                 optional<std::unique_ptr<Expression>> locale_,
+                 optional<std::unique_ptr<Expression>> currency_,
+                 optional<std::unique_ptr<Expression>> minFractionDigits_,
+                 optional<std::unique_ptr<Expression>> maxFractionDigits_);


complex class needs not-inlined destructor

pozdnyakov

lgtm, thanks for following

tmpsantos · 2019-06-27T12:06:10Z

platform/qt/src/format_number.cpp

+
+    QString formatted;
+    // Qt Locale::toString() API takes only one precision argument
+    (void)minFractionDigits;


nit: you don't need to do this, you can just remove minFractionDigits from the function signature.

jmalanen requested review from pozdnyakov, alexshalamov and tmpsantos June 4, 2019 12:13

jmalanen force-pushed the jmalanen-number-format branch from e4186c8 to eaf5d02 Compare June 4, 2019 12:56

tobrun mentioned this pull request Jun 4, 2019

Add support for number format expression #14842

Closed

tmpsantos reviewed Jun 4, 2019

View reviewed changes

alexshalamov reviewed Jun 4, 2019

View reviewed changes

src/mbgl/style/expression/number_format.cpp Outdated Show resolved Hide resolved

src/mbgl/style/expression/number_format.cpp Outdated Show resolved Hide resolved

jmalanen force-pushed the jmalanen-number-format branch 5 times, most recently from 31fa02e to fbde1db Compare June 4, 2019 15:13

pozdnyakov reviewed Jun 4, 2019

View reviewed changes

include/mbgl/style/expression/number_format.hpp Outdated Show resolved Hide resolved

include/mbgl/style/expression/number_format.hpp Outdated Show resolved Hide resolved

src/mbgl/style/expression/number_format.cpp Show resolved Hide resolved

jmalanen force-pushed the jmalanen-number-format branch 3 times, most recently from a700d8c to 44bd11f Compare June 5, 2019 07:58

1ec5 reviewed Jun 5, 2019

View reviewed changes

pozdnyakov reviewed Jun 5, 2019

View reviewed changes

jmalanen force-pushed the jmalanen-number-format branch from 44bd11f to ef6b53b Compare June 5, 2019 09:07

jmalanen force-pushed the jmalanen-number-format branch from ef6b53b to 807f62a Compare June 6, 2019 12:54

jmalanen requested a review from a team June 6, 2019 12:54

1ec5 reviewed Jun 6, 2019

View reviewed changes

jmalanen force-pushed the jmalanen-number-format branch from 807f62a to 7027e47 Compare June 10, 2019 08:29

1ec5 added Core The cross-platform C++ core, aka mbgl expressions labels Jun 11, 2019

jmalanen force-pushed the jmalanen-number-format branch 11 times, most recently from f4fa5bd to 855e48f Compare June 19, 2019 12:33

jmalanen force-pushed the jmalanen-number-format branch from 855e48f to cffc1fb Compare June 24, 2019 08:47

jmalanen requested a review from 1ec5 June 24, 2019 11:04

jmalanen force-pushed the jmalanen-number-format branch 3 times, most recently from 141a46b to 252658f Compare June 25, 2019 10:55

jmalanen requested a review from tmpsantos June 25, 2019 11:27

jmalanen force-pushed the jmalanen-number-format branch from 252658f to a543cf8 Compare June 26, 2019 07:41

tmpsantos reviewed Jun 26, 2019

View reviewed changes

platform/qt/src/format_number.cpp Show resolved Hide resolved

pozdnyakov reviewed Jun 26, 2019

View reviewed changes

jmalanen force-pushed the jmalanen-number-format branch from a543cf8 to 0d6c3bd Compare June 27, 2019 11:38

[core] Add number-format expression

50bdbea

jmalanen force-pushed the jmalanen-number-format branch from 0d6c3bd to 50bdbea Compare June 27, 2019 11:44

pozdnyakov approved these changes Jun 27, 2019

View reviewed changes

tmpsantos approved these changes Jun 27, 2019

View reviewed changes

jmalanen merged commit 0ca8ea6 into master Jun 27, 2019

jmalanen deleted the jmalanen-number-format branch June 28, 2019 11:30

tobrun mentioned this pull request Aug 20, 2019

Add number format expression #15424

Merged

chloekraw modified the milestone: release-ristretto Aug 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Add number-format expression #14840

[core] Add number-format expression #14840

jmalanen commented Jun 4, 2019

jmalanen commented Jun 4, 2019

jmalanen commented Jun 4, 2019

pozdnyakov Jun 5, 2019

pozdnyakov commented Jun 5, 2019

pozdnyakov commented Jun 5, 2019

1ec5 commented Jun 5, 2019

1ec5 left a comment

1ec5 Jun 6, 2019

1ec5 Jun 6, 2019

jmalanen Jun 19, 2019 •

edited

Loading

1ec5 Jun 6, 2019

jmalanen Jun 19, 2019

tmpsantos left a comment

pozdnyakov Jun 26, 2019

jmalanen Jun 27, 2019

pozdnyakov Jun 26, 2019

jmalanen Jun 27, 2019

pozdnyakov left a comment

tmpsantos Jun 27, 2019

[core] Add number-format expression #14840

[core] Add number-format expression #14840

Conversation

jmalanen commented Jun 4, 2019

jmalanen commented Jun 4, 2019

jmalanen commented Jun 4, 2019

Choose a reason for hiding this comment

pozdnyakov commented Jun 5, 2019

pozdnyakov commented Jun 5, 2019

1ec5 commented Jun 5, 2019

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmalanen Jun 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmpsantos left a 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

pozdnyakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmalanen Jun 19, 2019 •

edited

Loading