strip leading/trailing space from string values #619
Labels
Comments
|
Also... If anyone is writing layout code that is relying on leading/trailing spaces in translated strings, that's a bad practice. If the translator doesn't provide identical leading/trailing spaces, your layout will be hosed. Similarly if the translator accidentally adds leading/trailing space, your layout will be hosed. And Rosetta currently provides no support for either scenario. |
|
I see 3 places in string.js where |
|
One possibility would be to put an assertion like if ( parsedStrings[ key ] !== undefined ) {
var stringValue = window.phet.chipper.mapString( parsedStrings[ key ].value, stringTest );
assert && assert(stringValue.length===stringValue.trim().length,'untrimmed string: '+stringValue);
onload( stringValue );
}I tested it on several sims and haven't seen a problem yet. UPDATE: My point is that rather than gracefully handling trailing/leading whitespace we should identify it and flag it as an error so it can be corrected. |
|
Identifying leading/trailing whitespace is useful for the English (fallback) strings, which we can then fix. But we currently have no control over the strings for other locales. So stripping leading/trailing whitespace is (imo) the robust solution. |
|
We should add a |
|
Yes, we could (should?) add a trim in Rosetta, and make a pass through all existing strings. But we still need a trim at the point where the strings are read from the file. In general, anytime you read a string from an external source (user input, file,...), and that string is not supposed to have leading/trailing whitespace, one should defensively trim the string. And that's the case with the string plugin. |
|
11/9/17 dev meeting: |
|
The issue in rosetta for trimming strings provided by translators is phetsims/rosetta#163. |
|
Presumably this can be addressed by adding |
|
I found 2 calls to But with |
pixelzoom
added a commit
that referenced
this issue
Nov 9, 2017
pixelzoom
added a commit
that referenced
this issue
Nov 9, 2017
|
If Rosetta is indeed adding directional formatting, then it will also need to In the meantime, I'm going to |
|
Doing the trim in requirejs mode would be a reasonable change in string.js: 168 if ( queryParameterStringValue ) {
169 onload( queryParameterStringValue.trim() );
170 }Handling it in built mode is way ugly. It would involve changing sim.hmtl line 68, like this: 60 var stringOverrides = JSON.parse( phet.chipper.queryParameters.strings || '{}' );
68 return stringOverrides[ key ].trim() || window.phet.chipper.mapString( window.phet.chipper.strings[ window.phet.chipper.locale ][ key ], stringTest );Chatting about this with @jonathanolson on Slack, we're considering not trimming the values provided via A more general concern about this entire exercise... It's disconcerting how this responsibility is spread out, over multiple files (string.js, getStringMap.js, sim.html,.. others?), build tools, Rosetta,…Something as simple as trimming the string value should not be this difficult, or involve touching this many files. |
|
Slack developer channel:
Assigning to @jbphet to determine where directional formatting is coming from when |
|
@jbphet in Slack, re the 'strings' query parameter:
|
pixelzoom
added a commit
to phetsims/rosetta
that referenced
this issue
Nov 13, 2017
|
To maintain momentum on this, I poked around in rosetta myself. After searching for various things ('directional', 'RTL',...) I finally found 'rtl' in a couple of comments -- which I've changed to 'RTL' in the above commit. In translate-sim.js, it does look like directional formatting is being added on line 84: // add RTL embedding markers for RTL strings
if ( rtl ) {
translation = '\u202b' + translation + '\u202c';
}
else {
translation = '\u202a' + translation + '\u202c';
}Changes that need to be made in Rosetta: (1) Replace the above code with: translation = ChipperStringUtils.addDirectionalFormatting( translation, rtl );... so that (a) build process and Rosetta are using the same code to add directional formatting, and (b) it's easier to locate directional formatting in the future (as was the problem in this issue). (2) Immediately before the above code, trim whitespace by adding: // remove leading/trailing whitespace, see chipper#619. Do this before adding directional formatting.
translation = translation.trim();(3) Verify that whitespace is trimmed when English strings are displayed. If that's not happening, make it so. (4) Verify that whitespace is trimmed when translated strings are saved. If that's not happening, make it so. @jbphet Do you agree? |
pixelzoom
changed the title
|
I don't feel comfortable making Rosetta changes, since I'm not familiar with how to test and deploy. Discussed with @ariel-phet and he said to assign to @jbphet to (1) make Rosetta changes and (2) verify the changes that I made to build tools in f7be23c. Let me know if I can help. |
|
@jbphet Check with @ariel-phet on priority. Rosetta will currently show leading/trailing whitespace, while requirejs/built sims will trim it. So what the translator sees doesn't match what will be deployed. |
|
Okay, I'll set the priority and hopefully address during the upcoming chipper:2.0 effort. |
|
@samreid and I ran into this in regards to #779 and (more generally phetsims/rosetta#193). I think it would be good to work on this as part of that work. Assigning to myself for some investigation so I don't forget. |
+1 for an assertion for english strings! |
10 tasks
This was referenced Sep 23, 2019
|
I decided to do this work on the @jbphet I see phetsims/rosetta#163, so I don't think you need to be assigned here. Let me know if that changes. Index: js/common/ChipperStringUtils.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/common/ChipperStringUtils.js (date 1569546824000)
+++ js/common/ChipperStringUtils.js (date 1569547303062)
@@ -120,11 +120,15 @@
* Recurse through a string file and format each string value appropriately
* @param {StringMap} stringMap
* @param {boolean} isRTL - is right to left language
+ * @param {boolean} [assertNoWhitespace] - when true, assert that trimming each string value doesn't change the string.
* @public
*/
- formatStringValues: function( stringMap, isRTL ) {
+ formatStringValues: function( stringMap, isRTL, assertNoWhitespace ) {
ChipperStringUtils.forEachString( stringMap, ( key, stringObject ) => {
+ assert && assertNoWhitespace && assert( stringObject.value === stringObject.value.trim(),
+ `String should not have whitespace: ${stringObject.value}` );
+
// remove leading/trailing whitespace, see chipper#619. Do this before addDirectionalFormatting
stringObject.value = stringObject.value.trim();
stringObject.value = ChipperStringUtils.addDirectionalFormatting( stringObject.value, isRTL );
Index: js/requirejs-plugins/string.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/requirejs-plugins/string.js (date 1569546824000)
+++ js/requirejs-plugins/string.js (date 1569547318001)
@@ -47,8 +47,9 @@
* @param {string} url path for the string
* @param {function} callback callback when the check succeeds, returning the parsed JSON object
* @param {function} errorBack callback for when an error occurred
+ * @param {boolean} [assertNoWhitespace] - when true, assert that trimming each string value doesn't change the string.
*/
- const getWithCache = ( url, callback, errorBack ) => {
+ const getWithCache = ( url, callback, errorBack, assertNoWhitespace ) => {
// Check for cache hit, see discussion in https://github.com/phetsims/chipper/issues/730
if ( cache[ url ] ) {
@@ -69,7 +70,7 @@
const parsed = JSON.parse( loadedText );
const isRTL = localeInfo[ phet.chipper.queryParameters.locale ].direction === 'rtl';
- ChipperStringUtils.formatStringValues( parsed, isRTL );
+ ChipperStringUtils.formatStringValues( parsed, isRTL, assertNoWhitespace );
cache[ url ] = parsed;
// clear the entries added during the async loading process
@@ -179,7 +180,8 @@
const queryParameterStrings = JSON.parse( phet.chipper.queryParameters.strings || '{}' );
const locale = phet.chipper.queryParameters.locale;
const fallbackSpecificPath = `${repositoryPath}/${getFilenameForLocale( ChipperConstants.FALLBACK_LOCALE )}`;
- const localeSpecificPath = ( locale === ChipperConstants.FALLBACK_LOCALE ) ?
+ const isFallbackLocale = locale === ChipperConstants.FALLBACK_LOCALE;
+ const localeSpecificPath = isFallbackLocale ?
fallbackSpecificPath :
`${repositoryPath}/../babel/${repositoryName}/${getFilenameForLocale( locale )}`;
@@ -213,8 +215,8 @@
// Running in the browser (dynamic requirejs mode) and couldn't find the string file. Use the fallbacks.
console.log( `no string file for ${localeSpecificPath}` );
onload( getStringFromFileContents( parsedFallbackStrings, key ) );
- } );
- }, onload.error );
+ }, isFallbackLocale ); // if the main strings file is the fallback, then assert no surrounding whitespace
+ }, onload.error, true );
}
}
}, |
|
phetsims/friction#182 sorta became an issue to do this conversion in more repos than just friction (oops). And so now there don't seem to be any trailing/leading whitespace in a11y strings, and after adding the assertion in, I don't see any in the en json files either. I feel good about committing this to the branch. We will know for certain when it is merged to master. I'll keep myself assigned until then. |
|
After merging to master, I ran aqua tests and found one error (phetsims/masses-and-springs#350). I'm going to close this now that it is on master. |
|
So I just ran back into this over in phetsims/friction#237. Even though I added the assertion, it is never run, because we aren't running things with When I apply this patch, I see quite a bit of issue in our strings printed out when building just friction. I worry about what it will mean to actually limit our trailing/leading whitespace. It seems like we either need to tighten down on this, and potentially change a lot of translations, or discover another way forward. Maybe we don't care about this that much anymore? PatchIndex: js/common/ChipperStringUtils.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/ChipperStringUtils.js b/js/common/ChipperStringUtils.js
--- a/js/common/ChipperStringUtils.js (revision dbec1678d6dcab1b74728c354fbe9b62b4b1bdfe)
+++ b/js/common/ChipperStringUtils.js (date 1632324311230)
@@ -10,7 +10,13 @@
/* eslint-env node */
-const assert = require( 'assert' );
+// const assert = require( 'assert' );
+
+const assert = ( x, m ) => {
+ if ( !x ) {
+ console.log( m );
+ }
+};
const _ = require( 'lodash' ); // eslint-disable-line require-statement-match
// What divides the repo prefix from the rest of the string key, like `FRICTION/friction.title`
@@ -113,7 +119,7 @@
* @param {boolean} [assertNoWhitespace] - when true, assert that trimming each string value doesn't change the string.
* @public
*/
- formatStringValues: function( stringMap, isRTL, assertNoWhitespace ) {
+ formatStringValues: function( stringMap, isRTL, assertNoWhitespace = true ) {
ChipperStringUtils.forEachString( stringMap, ( key, stringObject ) => {
assert && assertNoWhitespace && assert( stringObject.value === stringObject.value.trim(),
Many strings with lots of whitespace trailing or leading |
|
We discussed this at today's dev meeting: SR: Should we add assertions that the english strings don't have leading/trailing strings? We agreed the pattern in phetsims/friction@b62a8d8 is what not to do. You should have a template pattern that inserts space between sentences, not relying on sentences to have their own trailing whitespace. SR: Is this a rosetta problem, or more about templating a11y strings? We decided this is at least 3 problems:
Let's touch base with @zepumph, who was not present for this discussion. |
|
We discussed this more and decided two things.
For the actual assertion mentioned in #619 (comment), I will test all sims and see how many cases there are with white space. I will note them in phetsims/rosetta#270. I will report back to a subgroup of @jbphet, and @jonathanolson. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
@pixelzoom has pointed this out. Using a string value like "____________Basics" for the title of a screen will place the string wildly out of place. (github DOES strip spaces, so I had to use underscores to show spaces 😠)
@ariel-phet please assign for someone to address.
The text was updated successfully, but these errors were encountered: