phetsims / scenery-phet

Reusable components based on Scenery that are specific to PhET simulations.
MIT License
8 stars 6 forks source link

Un-internationalize FaceWithPointsNode #224

Closed samreid closed 8 years ago

samreid commented 8 years ago

From https://github.com/phetsims/fraction-matcher/issues/90

This is using SCENERY_PHET/FaceWithPointsNode.pattern_0sign_1points.

Here's the entry from scenery-phet:

  "FaceWithPointsNode.pattern_0sign_1points": {
    "value": "{0}{1}"
  },

@jbphet added this code to FaceWithPointsNode, can you comment why this is internationalized?

@ariel-phet should we un-internationalize FaceWithPointsNode and just show +1, -2, etc. without making it internationalized?

samreid commented 8 years ago

@ariel-phet said:

Yes un-internationalize FaceWithPointsNode (it makes no sense in our scoring if those point values are changed, and numbers themselves are international enough)

samreid commented 8 years ago

@jbphet said:

FaceWithPointsNode was internationalized because someone might want the plus sign on the other side. I'm pretty sure that there was a specific request for this from one of the other developers or a translator - it's unlikely that I would have thought of this on my own. I'm okay with un-internationalizing it as @ariel-phet is recommending above - I just checked the Arabic translation for this file and the translator did not reverse the pattern, so it must not be that important to at least one translator for an RTL language. See https://github.com/phetsims/babel/commit/7a3deb204d74a53bab86a49d49b917d5a172f2b7.

If it is un-internationalized, please remove it from all translated string files.

samreid commented 8 years ago

@ariel-phet said:

when we have checked with RTL translator, math is still written LTR (this came up with graphing lines, since allowing a flexible layout with those dynamic equations would have been brutal).

So I would still say un-internationalize . Things like writing coordinates, using commas vs decimal points, etc vary from region to region but standard arithmetic operations appear to be pretty universal.

samreid commented 8 years ago

As one more data point regarding whether it is OK to de-internationalize this, I saw 3 translations for the string for de, lv and nl locales, and they are all {0}{1} the same as the English.

samreid commented 8 years ago

De-internationalized above and tested in fraction-matcher, seems OK. Reassigning to @jbphet to remove the string references in babel, FaceWithPointsNode.pattern_0sign_1points appears in 3 files.

jbphet commented 8 years ago

In one of the comments above, I requested:

If it is un-internationalized, please remove it from all translated string files.

Looks like this wasn't done. I'll go ahead and do it.

jbphet commented 8 years ago

I did this, and in the process fixed up the German (_de) scenery-phet translation. I pulled on figaro and restarted rosetta to be safe. This should be all that is needed for this, closing.

pixelzoom commented 8 years ago

Reopening.

(1) What was the motivation for removing support for this?

(2) Removing support is wrong. The placement of a number's sign is locale specific, even when using Arabic digits (0-9). For example, in Arabic the sign is placed to the right of the number, eg. 12-.

See "Java Internationalization", top of page 118 at https://books.google.com/books?id=djuMgaEuBhQC&pg=PA117&lpg=PA117&dq=internationalization+of+signed+numbers&source=bl&ots=kJp48nH3eA&sig=f6wRM9MSnP9OXfM-WsOVaDtJ0dQ&hl=en&sa=X&ved=0ahUKEwj7ita2ycbLAhVqmIMKHWH3AJ4Q6AEIMTAD#v=onepage&q=internationalization%20of%20signed%20numbers&f=false

pixelzoom commented 8 years ago

See also "Usability and Internationalization of Information Technology", bottom of page 38 at https://books.google.com/books?id=AeYBavcZ9_UC&pg=PA38&lpg=PA38&dq=internationalization+of+signed+numbers&source=bl&ots=Mt4rEs40e3&sig=pCbtEoYcjjpH3uoSv38-lDsrsrA&hl=en&sa=X&ved=0ahUKEwj7ita2ycbLAhVqmIMKHWH3AJ4Q6AEINTAE#v=onepage&q=internationalization%20of%20signed%20numbers&f=false:

"Negative numbers may be indicated by using a minus sign before or after the number, or by enclosing the number in parenthesis or brackets."

pixelzoom commented 8 years ago

Microsoft "Globalization Step-by-Step" guide, section "Number Formatting": https://msdn.microsoft.com/en-us/goglobal/bb688127.aspx

"The negative sign can be used at the beginning of the number, but it can also be used at the end of the number."

samreid commented 8 years ago

+1 for internationalization based on the above arguments from @pixelzoom, and let's add a code comment explaining why this has been internationalized so I don't make this mistake again, and as a precedent for similar future cases.

ariel-phet commented 8 years ago

I am going to respectfully disagree with @pixelzoom here

Although I do not think there is a major downside to internationalizing this again, I also do think it would be fine to leave as is. I think there is very little possibility for confusion, but I also would not want someone messing with the scoring.

ariel-phet commented 8 years ago

I would also note I view "+1" or "+2" in this case as an arithmetic operation, not as a signed number

pixelzoom commented 8 years ago

We do not have "negative" points in games...ever. It goes against our philosophy

Then let's remove support for negative points from FaceWithPointsNode, add assert( points >= 0 ), and simply put a plus sign in from of points > 0.

ariel-phet commented 8 years ago

That sounds good to me (I did not realize there was support for negative points).

samreid commented 8 years ago

I need this for next RC of Fraction Matcher, assigning to myself with high-priority.

samreid commented 8 years ago

Assertion added above, @jbphet or @pixelzoom can you see if there is anything else to do for this issue?

jbphet commented 8 years ago

Assertion for checking that points are positive looks good, I think this can be safely closed (again).

pixelzoom commented 8 years ago

Reopening. Since points must be positive, this bit of unreachable code needs to be removed at line 94:

      else if ( points < 0 ) {
        this.pointsNode.text = points + '';
      }
samreid commented 8 years ago

Thanks @pixelzoom, back to you :)

pixelzoom commented 8 years ago

Looks good. Closing.