google / blockly-android

Blockly for Android
Apache License 2.0
672 stars 209 forks source link

Fix missing locale on String#toLowerCase() #701

Closed ChrisCanCompute closed 6 years ago

ChrisCanCompute commented 6 years ago

Calling String#toLowerCase() or #toUpperCase() without specifying an explicit locale is a common source of bugs. The reason for that is that those methods will use the current locale on the user's device, and even though the code appears to work correctly when you are developing the app, it will fail in some locales. For example, in the Turkish locale, the uppercase replacement for i is not I.

In our situation, we need canonical names for variables, so using the user's default locale will be sufficient (there may be issues if the user's code is saved on one device and reloaded on another with a different Locale)


This change is Reviewable

AnmAtAnm commented 6 years ago

I'm confused about how your code solves any potential bugs. The docs specify that the prior zero parameter toLowerCase() is identical to passing in the default locale.

I think the concern in Blockly's case is we want the same variable or function names to map to the same variable regardless of the locale, because we are concerned about how the program runs the same program loaded on different devices.

That said, even if we did use a fixed locale such as US-EN, this might create different behavior than what we see in web Blockly in Turkish environments, for instance. The right solution in my mind would be to attach a locale to any Blockly save file (and warn when missing, using the default).

Unfortunately, JavaScript doesn't even have a equivalent Locale-based version of toLowerCase() or similar that I can find. That means the web version of Blockly, the main version, could never take advantage of such behavior. The lack of such an API hints to me about how much of an edge case this is.

Thanks for highlighting this issue, but because the provided change does not solve anything, and the right solution is not portable to other platforms, I'm going to deny this PR. Feel free to reply if you think my analysis is in error.

ChrisCanCompute commented 6 years ago

That's fair. We should probably suppress the checkstyle warning here though.

RoboErikG commented 6 years ago

Just to check my understanding: