NumberKeypad: decimal point is not localized

[pixelzoom]

The decimal point is not '.' in all locales, and it's currently hardcoded.

[pixelzoom]

I tried adding this to scenery-phet-strings_en.json:

  "decimalPointString": {
    "value": "."
  }

And this to NumberKeypad:

  // string
  var decimalPointString = require( 'string!SCENERY_PHET/decimalPointString' );

Then replacing all '.' literals with decimalPointString. But things were horribly broken. I'm guessing it's because there are directional indicators surrounding the string.

@jonathanolson Is there a way to easily strip off the directional indicators?

[jonathanolson]

@jonathanolson Is there a way to easily strip off the directional indicators?

StringUtils.stripEmbeddingMarks.

But things were horribly broken.

Seems like we would be better off determining and fixing the break. Does the decimal count towards the number of allowed digits? Seems much cleaner to fix things without having to strip off the marks.

[pixelzoom]

Seems like we would be better off determining and fixing the break.

In the current implementation, each key's associated character gets added to a string Property (valueStringProperty), and that string is what's displayed to the user. So the decimal point character (as it is typed) is concatenated to what's in valueStringProperty (subject to constraints, like at most 1 decimal point).

yeah, I guess we could make it work with the directional indicators (aka embedding marks). But it makes the implementation of the backspace key a royal pain - it currently strips off 1 char. That's not going to cut it if the decimal point is actually multiple chars.

[pixelzoom]

Another problem with localizing the decimal point: If a translator uses more than 1 char, it's going to mess up the current backspace implementation, which removes the last char from valueStringProperty:

79 var shortenedDigitString = self.valueStringProperty.value.slice( 0, -1 );

Or We'll need to test for decimalPointString in the backspace implementation, and remove the entire string if it's at the end of valueStringProperty.

[jonathanolson]

Just use an Array with a digit enumeration (0-9, decimal, minus, etc.) internally as the model, and have a function for turning that into a number?

Also, recall that doing comparison with sim strings has caused bugs in the past, so an Array of translated strings (one for each digit) could be faulty.

[pixelzoom]

Adding @jbphet to assignees, since he implemented the decimal point feature.

[pixelzoom]

@jonathanolson suggested:

Just use an Array with a digit enumeration

Yeah, I've considered doing that. But I'm trying to get out of this without having to reimplement NumberKeypad. Hard to believe that i18n wasn't considered.

[pixelzoom]

Discussed briefly at 11/10/16 "keypad design" meeting, and it didn't sound like addressing this was a priority.

And it's certainly a can of worms. Changing '.' to some other character (e.g. ',') would most certainly make the keypad fail, since it currently relies on parseFloat, which behaves like this:

> parseFloat( '4.52' )
4.52
> parseFloat( '4,52' )
4

So... This issue is noted in the code with a TODO item. And I'm unassigning myself for now.

[pixelzoom]

Raising priority to "high" since resolving this involves some rather fundamental changes, and affects our ability to resolve other high-priority issues like #272.

[pixelzoom]

@jonathanolson has weighed in on ideas for addressing this.

@jbphet your input?

[jbphet]

For a data point, I changed the locale on a Mac OSX 10.10.5 device to be Germany and tested out a couple of things. The calculator definitely changes to use a comma as the decimal separator, here's what it looks like:

I also opened up the dev tools in Chrome and testing out Number.toLocaleString. It works as I would expect, i.e. it prints out the value with a comma as the decimal separator. Here is a screenshot of the interaction:

One thing that I found a little surprising is that while there seems to be built-in support for formatting numbers to strings that use the comma as a decimal separator, there doesn't seem to be any support to go the other way, e.g. take a string like "5,2" and parse it with the comma interpreted as a decimal separator. This can be seen in the interaction above.

I looked around a bit on line and found that a number of other people have noticed this same thing, and the general consensus is to do a replace before interpreting the number. Here's a link to a StackOverflow article: http://stackoverflow.com/questions/28181066/is-there-a-native-way-to-convert-a-string-from-any-locale-to-a-number

Based on this, I think we could probably add support for using a comma as the decimal separator with a few hours of developer time and some test time, call it a one to two days total. I don't think we should allow it to be an arbitrary value, so it would have to reject anything other than a period or comma from translators. I think we should encapsulate the internationalization in NumberKeypad so that outside of it a period is always used as the decimal separator.

As far as whether we should do this, I really don't have much insight into how important it would be our international users. I think this is mostly a continental European thing. Perhaps @ariel-phet can ask his roommate whether this would be important when used by European youth. Adding him to the list of assignees.

[jbphet]

@ariel-phet - up to you to decide whether to do this soon, later, or never.

[ariel-phet]

After discussing with @jbphet my impression is that if we proceed with the architecture suggested in #272 we will not "paint ourselves into a corner"

I asked @oliver-phet and over the past 6 years we have only received 2 requests for internationalization of the decimal point.

For the moment, it seems best for us to not include this feature, and see what sort of reaction we get from translators. If we get some requests from them, we will look into at that time and decide to either
a) give them the power to choose "." vs ","
b) automatically choose the decimal point representation based on locale.

marking deferred for the moment and removing assignments

[pixelzoom]

@ariel-phet As alluded to in #279 (comment) (or at the very least considering) this issue is essential to properly rearchitecting NumberKeypad (as we've commited to do in #272 (comment).) So I really think that it's a mistake to defer this issue.

[ariel-phet]

Originally marked priority:deferred but perhaps that was unclear

Assigning to @jbphet so that it is clear:

1. We should consider this issue in the rearchitecting of NumberKeypad. However we should not necessarily take action, but make sure that this i18n issue can be handled.
2. More and more I am leaning towards the automatic, so if a sim is launched in a locale that uses commas we just go with that convention.
3. Once the plan for NumberKeypad solidifies, it would be good to have an estimate of what it will take to include this feature.

[jbphet]

@ariel-phet - would it be possible to decide now whether we will have the keypad automatically use a comma as the decimal separator based on locale or have it be translatable? Knowing which one we intend to support will make easier to determine whether the new design adequately supports it.

[jbphet]

For reference - the Wikipedia article linked below lists the countries that use a "decimal point" and those that use a "decimal comma". The list of those that use a decimal comma is quite large. https://en.wikipedia.org/wiki/Decimal_mark.

[ariel-phet]

@jbphet I think we can automatically decide. it seems this convention is well established in various locales, and something we do not need to rely upon to be done by our translators.

[jbphet]

Assigning to @aadish to handle when the new keypad is implemented.

[jbphet]

NumberKeypad has been deprecated, but the issue of how to change the decimal separator has not been addressed in the new keypad, so this issue is being superseded by #333. Closing this one.

[samreid]

There are still TODOs marked for this issue, discovered during phetsims/chipper#946

[zepumph]

I moved the TODO to #333. Closing