How to test dynamic locale in sims that have no translations?

[pixelzoom]

I've been adding support to various sims for dynamic locale. Those sims have been published, so translations exist. How do we test sims that have not been translated, and have no translations? What support is needed?

geometric-optics-basics is an immediate example, included in https://github.com/orgs/phetsims/projects/44. Luckily, only its title is specific to that sim, and everything else comes from geometric-optics, which does have translations.

[jonathanolson]

phet.chipper.setAllStrings( '...' ) and studio have been the most useful for me, although I've also manually adjusted strings phet-brand with phet.someSim.someSimStrings.someStringProperty.value = '...'.

[zepumph]

I trust @jonathanolson on whatever he would like to implement. My main hopes are that it is easy to get back to a test case where you witnessed a bug. If there is a "random strings" mode, then perhaps being able to copy that into a query string to relaunch would be ideal.

I also like the idea of using the same string test values/modes we already have, but cycling through them dynamically.

[pixelzoom]

I also like the idea of using the same string test values/modes we already have, but cycling through them dynamically.

+1

[pixelzoom]

This issue blocks further work on phetsims/models-of-the-hydrogen-atom#39. MOTHA is a new sim with no translations, so I can't identify layout problems, or prevent new problems.

[pixelzoom]

This is impacting more than sims that have no translations. In many math sims, symbols are translatable, but are seldom (or possibly never) translated. So after adding support for dynamic locale, I'm spending a lot of time pressing Ctrl-i, trying to cycle through a translation that may have translated a specific math symbol, and often finding none -- which means not testing adequately. That also means that QA can't test adequately.

So I'm writing new sims that I can't test at all. I'm converting existing sims that that I can't test adequately. QA can't really be certain that they are testing any sim adequately for dynamic locale. And we're supposed to do this for all sims.

This has been high prioirty for almost 2 month, and there has been no progress. That's no fault of @jonathanolson -- it's a project management problem. So unfortunately, I'm going to move this back to the "To be discussed" column of the Developer Meeting project board, so that we can discuss how this should be prioritized and managed.

[samreid]

Patch from dev meeting:

Index: js/getStringModule.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/getStringModule.ts b/js/getStringModule.ts
--- a/js/getStringModule.ts	(revision 05c66c8da184a50129996bb093d5444f50eb06bc)
+++ b/js/getStringModule.ts	(date 1668105694695)
@@ -26,10 +26,34 @@
 // Holds all of our localizedStrings, so that we can save our phet-io string change state
 export const localizedStrings: LocalizedString[] = [];
 
+if ( phet.chipper.queryParameters.dev ) {
+  window.addEventListener( 'keydown', ( event ) => {
+    if ( event.key === 'p' ) {
+      localizedStrings.forEach( localizedString => {
+        // const length = Math.round( Math.random() * 20 ) + 2;
+        localizedString.property.value = localizedString.property.value + localizedString.property.value;
+      } );
+    }
+    if ( event.key === 'm' ) {
+      localizedStrings.forEach( localizedString => {
+        // const length = Math.round( Math.random() * 20 ) + 2;
+        localizedString.property.value = localizedString.property.value.substring( 0, localizedString.property.value.length / 2 );
+      } );
+    }
+    if ( event.key === 'r' ) {
+      localizedStrings.forEach( localizedString => {
+        const length = Math.round( Math.random() * 20 ) + 2;
+        localizedString.property.value = ( '' + Math.random() + '' + Math.random() + '' ).substring( 0, length );
+      } );
+    }
+  } );
+}
+
 // For developer internal use, similar to the stringTest query parameter
 window.phet.chipper.setAllStrings = ( str: string ) => {
   localizedStrings.forEach( localizedString => {
-    localizedString.property.value = str;
+    // const length = Math.round( Math.random() * 20 ) + 2;
+    localizedString.property.value = localizedString.property.value + localizedString.property.value;
   } );
 };
 

@pixelzoom said that design is reasonable and better than we have now. @marlitas and @samreid volunteered to bring it to production and add it to the QA handbook.

Also we want reproducible seeds, perhaps? But nobody described a reasonable UX for that.

Maybe a different query parameter too.

Should we use the new scenery key input globals?

[jbphet]

We discussed this during the 11/10/2022 dev meeting, and @samreid created a quick demo that seemed like a reasonable next step. He and @marlitas will implement it. One of the requirements will be to make it reproducible when using random values to create variations in the string lengths.

Once this is implemented, it will be assigned to @pixelzoom for testing and review.

[samreid]

Today @marlitas and I wrote a working draft for this and it seems to be working well. The key commands are described in initialize-globals:

stringTest=dynamic:

adds global hotkey listeners to change the strings.
right arrow - doubles a string, like string = string+string
left arrow - halves a string
up arrow - cycles to next stride in random word list
down arrow - cycles to previous stride in random word list
spacebar - reset to initial english strings, and resets the stride

@pixelzoom this is ready for use in development.

@marlitas will additionally look into using KeyboardListener and adding it to the display via addInputListener. Thanks! After that, it will be ready for review.

[pixelzoom]

Reminder to document the final solution in the QA Book, and notify @KatieWoe.

[pixelzoom]

Some problems with commit phetsims/joist@2663d3d

(1) function setStride is defined inside the event listener. So every time we get an event, we're reallocating this function. It should be moved outside the listener.

(2) The are 5 if statements, and no else. So all 5 if expressions are evaluated for every event. Use if {} else if {}... logic.

[pixelzoom]

Some things that I reported in Slack#dev-public:

• If you press left arrow enough times, it results in an empty string, which it probably should not do — there should be a minimum length of 1. And then pressing right arrow has no affect, because doubling an empty string results in an empty string.

• If I press right arrow a few times in Fourier, Chrome hangs for a long time. Sometimes it eventually comes back, sometimes I have to Force Quit. @samreid reproduced.

• This approach is apparently chopping up pattern strings, with some very strange (and not very useful) results. Ideally it shouldn't touch pattern strings. See for example Fourier. Here's a piece of the control panel in the Discrete screen before pressing any keys:

Here it is after pressing left arrow once:

So this results in invalid pattern string, which could not possibly be entered via Rosetta. Also note that “space & time ({{spaceS” is now longer than the original string, not shorter.

• There seems to be something wrong with the left-arrow "cut the length in half" algorithm. For example, in Fourier I see "Discrete" -> "Disc" -> "D". That last string should be "Di".

• We quickly lose any resemblance to the original string. For example in Fourier's Discrete screen:

original string: Harmonics
left arrow: Harm
left arrow: H
right arrow: HH
right arrow: HHHH
right arrow: HHHHHHHH

More useful behavior would preserve the relationship to the original string:

original string: Harmonics
left arrow: Harm
left arrow: Ha
right arrow: Harm
right arrow: Harmonics
right arrow: HarmonicsHarmonics

[marlitas]

@pixelzoom I think we should plan on living with the current behavior. I'm not sure the effort to address that is a worthy return on investment. Right now we're stripping out the string pattern and doubling it at the end, rather than addressing the logic to remember it's original placement in the string. This logic gets especially tricky when we begin halving strings.

If there is something that is really prohibitive about the behavior in using the tool then that would be good to know, and then perhaps the effort invested is worth it. Let me know your thoughts.

EDIT: I also want to let you know that I have read through your review comments and they are greatly appreciated. Work on this issue is just on hold until other priorities clear up.

[pixelzoom]

Right now we're stripping out the string pattern and doubling it at the end, rather than addressing the logic to remember it's original placement in the string. This logic gets especially tricky when we begin halving strings.

I can see how that would be easier (necessary?) when making strings shorter than their original length. But it does not seem necessary when making strings longer than their initial length. For example, if the initial string pattern is "x = {{x}}", it's preferrable that the doubled string would be "x = {{x}}x = {{x}}", not "x = x = {{x}}{{x}}". @marlitas would that be difficult to change?

[pixelzoom]

Lots of work in the above commits. Summary:

• Implemented the behavior that I suggested in How to test dynamic locale in sims that have no translations? #1319 (comment). If the string is getting longer than its initial length, it is simply doubled. If it's getting shorter than its initial length, then pattern placeholders are moved to the end of the shortened string. See applyToString.

• Fixed a bug with spacebar. It was not resetting stringFactor to 1.

• Improved readability by factoring out keyCode constants, and factoring out functions for the actions associated with keyCodes.

• Factored out private functions instead of relying on confusing closures. These functions appear after (outside) the class definition.

I went over most of this with @marlitas on Zoom, but I'll assign to her for a review. If this looks OK to you, I'm OK with closing this issue.

[pixelzoom]

One last change (I promise :)

Something that has bugged me from the get-go is the overall design of DynamicStringTest.ts. It's a class, but it has no constructor, no fields, and consists of only one static init method that reilies on a bunch of closures to keep state. This is an improper use of a class.

In the above commit, I reworked DynamicStringTest to be a proper class, with fields for state, and public/private methods that operate on that state. Here's how its use changed in SimDisplay.ts:

    if ( phet.chipper.queryParameters.stringTest === 'dynamic' ) {
-     DynamicStringTest.init();
+     const dynamicStringTest = new DynamicStringTest();
+     window.addEventListener( 'keydown', event => dynamicStringTest.handleEvent( event ) );
    }

[pixelzoom]

One more thing :)

In #1319 (comment), I reported:

(1) It's still very easy to get the sim to freeze / lockup.

Start any sim (e.g. Beer's Law Lab). Press the RIGHT arrow key ~10x. The sim will freeze, the browser (Chrome) will become totally non-responsize, and a "Force Quit" is required.

I'm not sure if this is a performance problem, and the sim is working hard to compute some really long strings. Or if the sim has crashed. (EDIT: I suspect the former, because the sim came back to life after waiting ~5 minutes.)

Perhaps there should be a limit on the number of times that strings can be doubled?

This is still a serious problem -- I locked up Chrome several times while working on this, and had to force quit each time. So in the above commit, I put a limit on the number of times that you can double strings, see MAX_STRING_FACTOR = 8.

[marlitas]

@pixelzoom I looked through the commits and your comments and this all seems reasonable. I also ran through a couple of DynamicStringTest usages and it is working well and has definitely addressed concerns you have brought up in the past.
I think these are great improvements, and I am ready to close this issue as completed.

I also updated the dynamic string layout quickstart guide to include information and references to DynamicStringTest

[pixelzoom]

While working on phetsims/calculus-grapher#193, I noticed that @veillette had used KeyboardEvent.code instead of the deprecated KeyboardEvent.keyCode. So I made a similar change to DynamicStringTest.ts in the above commit. @marlitas FYI.