Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it easier to switch locales during development or QA testing #1316

Closed
samreid opened this issue Aug 28, 2022 · 17 comments
Closed

Make it easier to switch locales during development or QA testing #1316

samreid opened this issue Aug 28, 2022 · 17 comments

Comments

@samreid
Copy link
Member

samreid commented Aug 28, 2022

During phetsims/circuit-construction-kit-common#960 and related to phetsims/natural-selection#319 (comment) and #1302, during development we would like a better way to switch languages to test. It is difficult to show the preferences dialog to switch, or to run with studio to find the localeProperty in the tree.

Instead, let's add a query parameter that adds a keyboard shortcut to cycle through languages. I have a nice prototype I'll commit momentarily.

Notifying @pixelzoom and @zepumph and @jonathanolson since they have been working in this area.

@samreid
Copy link
Member Author

samreid commented Aug 28, 2022

Implemented in the commits. The docs for ?stringTest say:

keyboardShortcutLocaleSwitcher: adds keyboard shortcuts. ctrl+i (forward) or ctrl+u (backward). Also,
  same physical keys on the dvorak keyboard (c=forward and g=backwards)

@pixelzoom can you please review?

  • Please test the functionality
  • Review the commits
  • Do you think this should be under ?stringTest query parameters, or a separate key?
  • If we stick with ?stringTest, do you think keyboardShortcutLocaleSwitcher is the appropriate value?

@pixelzoom
Copy link
Contributor

  • Please test the functionality

This is very nice. I especially like that I can run with a couple of locale, like ?stringTest= keyboardShortcutLocaleSwitcher&locales=en,ar,de, and cycle through them.

  • Review the commits

I don't see any problems.

  • Do you think this should be under ?stringTest query parameters, or a separate key?
  • If we stick with ?stringTest, do you think keyboardShortcutLocaleSwitcher is the appropriate value?

I think I'd rather have a flag that's separate from stringTest, and certainly a shorter name. Maybe ?localeSwitcher ?

@samreid
Copy link
Member Author

samreid commented Aug 29, 2022

I think I'd rather have a flag that's separate from stringTest, and certainly a shorter name. Maybe ?localeSwitcher ?

I moved it out of ?stringTest, but after prototyping with ?localeSwitcher I changed it to ?localeKeyboardSwitcher in case we want localeSwitcher to mean something else (like automatic locale switching?) in the future.

Back to @pixelzoom, please close if all is well.

Also, do we need a PSA or phetmarks checkbox for this?

@samreid samreid assigned pixelzoom and unassigned samreid Aug 29, 2022
@pixelzoom
Copy link
Contributor

pixelzoom commented Aug 30, 2022

Looking good.

Yes, ...

  • PSA to dev team
  • add localeKeyboardSwitcher to phetmarks

I also keep forgetting to run with locales=* or some subset of locales. I guess I'll get used to adding that, and when I see "en" printed to the console repeatedly it reminds me :)

@pixelzoom pixelzoom assigned samreid and unassigned pixelzoom Aug 30, 2022
@samreid
Copy link
Member Author

samreid commented Aug 30, 2022

I added it to phetmarks and it looks like so:

image

I'll label for Status meeting PSA, since it seems good for QA and other team members to be aware of as well.

@samreid samreid removed their assignment Aug 30, 2022
@pixelzoom
Copy link
Contributor

How about also adding this to phetmarks for 'studio' mode? I'm doing most of my work in Studio these days, and it would be nice not to have to manually add the query parameters, or resort to changing localeProperty in Studio.

@pixelzoom
Copy link
Contributor

pixelzoom commented Aug 30, 2022

Also want to point out that I keep typing keyboardLocaleSwitcher instead of localeKeyboardSwitcher. The adjectives feel reversed -- the locale is switching, not the keyboard.

@samreid
Copy link
Member Author

samreid commented Aug 30, 2022

In the commits, we renamed the query string and pass it through to all phet-io wrappers.

However, not all phet-io wrappers forward their key events to sims (including studio), so this doesn't work at the moment. I tried:

Index: js/studio-main.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/studio-main.ts b/js/studio-main.ts
--- a/js/studio-main.ts	(revision 4bcb311e0d032d6be28a6b3ba30e6b49a3918c4c)
+++ b/js/studio-main.ts	(date 1661878825971)
@@ -89,6 +89,7 @@
    */
   phetioClient.launchSim( {
     passThroughQueryParameters: true,
+    forwardZoomKeyboardEventsToSim: true,
     queryString: 'phetioEmitAPIBaseline&phetioCreateArchetypes&phetioEmitHighFrequencyEvents=false&phetioEmitStates=true&phetioEmitStatesInterval=60',
 
     onPhetioInitialized: function( phetioEngineElements: PhetioElementCreatedData[] ) {

But then you cannot use ctrl +/- to zoom in and out on the studio frame. @zepumph and/or @jessegreenberg can you please advise?

@samreid samreid assigned zepumph and unassigned samreid Aug 30, 2022
@samreid
Copy link
Member Author

samreid commented Aug 30, 2022

@zepumph said we can forward the key commands if the query parameter is on. See keyboardEventForwarding.

@samreid samreid assigned samreid and unassigned jessegreenberg and zepumph Aug 30, 2022
@zepumph
Copy link
Member

zepumph commented Aug 30, 2022

@zepumph
Copy link
Member

zepumph commented Aug 31, 2022

Assigning to myself because I see some trouble in global event listeners.

@zepumph
Copy link
Member

zepumph commented Aug 31, 2022

Ok. Committed above. In general I think it is nicer to have a single place where all global keyboard listeners are added (globalKeyStateTracker). This feels a bit more prudent to me.

@samreid
Copy link
Member Author

samreid commented Aug 31, 2022

Thanks, it seems like it is working well. I also added an adapter that initializes it in phet-io wrappers when you have keyboardLocaleSwitcher set. It works in studio and screenshot wrapper, but not the state wrapper because it doesn't use launchSim. @zepumph can you please review?

@samreid samreid assigned zepumph and unassigned samreid Aug 31, 2022
@zepumph
Copy link
Member

zepumph commented Sep 1, 2022

I believe I got everything set. It works in the state wrapper now, and could work in multi, or the unsupported mirror inputs and input-record-and-playback if we just added that function. Doesn't matter too much to me. Please review and fee free to close.

@zepumph
Copy link
Member

zepumph commented Sep 1, 2022

Again, I need to prevent default so that ctrl + u doesn't open the source of the page in chrome.

@zepumph zepumph assigned samreid and unassigned zepumph Sep 1, 2022
zepumph added a commit that referenced this issue Sep 1, 2022
zepumph added a commit to phetsims/joist that referenced this issue Sep 1, 2022
@zepumph
Copy link
Member

zepumph commented Sep 1, 2022

ctrl + shift + i is a very important shortcut for me (to open the dev tools) in chrome. So I made sure that we didn't have any other modifier keys presetn in addition to having ctrl.

@samreid
Copy link
Member Author

samreid commented Sep 1, 2022

The changes look great, thanks! Closing.

@samreid samreid closed this as completed Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants