heldentodd / xray-diffraction

This repository contains the code for one simulation, but eventually three are planned: bragg-law, Single Crystal Diffraction, and Powder Diffraction.
GNU General Public License v3.0
2 stars 1 forks source link

don't use concatenation to create strings that are visible in the user interface #7

Closed pixelzoom closed 4 years ago

pixelzoom commented 4 years ago

Related to #1 (code review).

  • [ ] Avoid using concatenation to create strings that will be visible in the user interface. Use StringUtils.fillIn and a string pattern to ensure that strings are properly localized.

In general, don't use concatenation to form strings that will appear in the user interface.

Here are some example, in XrayParameterPanel.js:

          angleText.text = incidentAngleString + ' = ' + Utils.toFixed( model.sourceAngleProperty.value * 180 / Math.PI, 1 ) + angleUnitString;
          latticeConstanstText.text = aLatticeConstantString + ' = ' + Utils.toFixed( model.lattice.latticeConstantsP.value.x, 1 ) + wavelengthUnitString +
                             '   ' + bLatticeConstantString + ' = ' + distanceString + ' = ' + Utils.toFixed( model.lattice.latticeConstantsP.value.z, 1 ) + wavelengthUnitString;
          wavelengthText.text = wavelengthString + ' = ' + Utils.toFixed( model.sourceWavelengthProperty.value, 1 ) + wavelengthUnitString;
          _2dSinText.text = '2d sin(θ) = ' + Utils.toFixed( 2 * model.lattice.latticeConstantsP.value.z * Math.sin(model.sourceAngleProperty.value), 1 ) + wavelengthUnitString;
          _2dSinLambdaText.text = '2d sin(θ)/λ = ' + Utils.toFixed( 2 * model.lattice.latticeConstantsP.value.z * Math.sin(model.sourceAngleProperty.value) / model.sourceWavelengthProperty.value, 2 );

Which looks like this in the sim:

screenshot_362

The reason you shouldn't use string concatenation has to do with translation to other languages. For example, consider the string "Incident Angle = 60.0°". English is a language that reads left-to-right, so this makes sense. But other languages read right-to-left, and we want to support that.

The general way to address this is to use what's called a "string pattern". A string pattern contains named placeholders that can be filled in with values. The placeholders are surround with double curly brackets, like {{value}}. The string pattern for incident angle would look like this in xray-diffraction-strings_en.json:

  "incidentAngle": {
    "value": "Incident Angle = {{value}}{{units}}"
  },

This allows the translator to change the order of terms in the expression.

Here's how that string pattern would be used in XrayParameterPanel.js:

import StringUtils from '../../../../phetcommon/js/StringUtils.js';

const incidentAngleString = xrayDiffractionStrings.incidentAngle;
const angleUnitString = xrayDiffractionStrings.angleUnit;

angleText.text = StringUtils.fillIn( incidentAngleString, { 
  value: Utils.toFixed( model.sourceAngleProperty.value * 180 / Math.PI, 1 ),
  units: angleUnitString
} );

If you want to address this issue, you should examine all of your Text and RichText instances to see how you're creating the strings that they use. For mathematical expressions like "2d sin(theta)= 13.5Å", you could probably get away with using string concatenation. But when in doubt, it's best to make the order translatable.

heldentodd commented 4 years ago

I think I'm going to leave this one open for now. After discussing the interface with Ariel, I may well remove a lot of the stuff from the parameter panel and show this information a different way. The one that probably most needs this is where I put up "In phase! Path Difference = xx λ". That is also somewhat likely to change (for the same reason). I'll come back to this after I have thought more on those issues. It's good to have the format for JSON though. Thanks.

pixelzoom commented 4 years ago

That sounds like an excellent plan.

heldentodd commented 4 years ago

Some of these were eliminated from the code. All those remaining (and some new ones) are now being handled this way.