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

Why is "Region and Culture" in PDL and PSD? #324

Open
kathy-phet opened this issue May 3, 2024 · 7 comments
Open

Why is "Region and Culture" in PDL and PSD? #324

kathy-phet opened this issue May 3, 2024 · 7 comments

Comments

@kathy-phet
Copy link

@matthew-blackman @samreid @arouinfar - Since this sim doesn't have any regionAndCulture features, why do we have "supportsRegionAndCulture" true in the json, and thus bringing in the ?regionAndCulture query parameter behavior?
Shouldn't that just not be in the json, so we don't complicate this sim with those query parameters at this point?

Since RegionAndCulture is new behavior AND this is a PHET-iO sim for which we don't have RegionAndCulture, we haven't tested it fully and I don't know that this is the sim to adopt this into the API on? I hadn't realized RegionAndCulture would be active on non RegionAndCulture related sims until I ran into it with Gas Properties yesterday.

I want to make sure someone has decided that yes, we want to have this part of the tree in here??
And yes, we want the sim to say it an unsupported value if regionAndCulture=africa is added, because technically there is no special values.

Should supportsRegionsAndCultures just not be in the json for this sim?

image

@samreid
Copy link
Member

samreid commented May 3, 2024

Please note that the tree referred to above is only in the strings, the full tandem is:

projectileDataLab.general.model.strings.joist.preferences.tabs.localization.regionAndCulture.africaModestStringProperty

There are also other irrelevant strings, such as voicing (the sim does not support voicing)

projectileDataLab.general.model.strings.joist.preferences.tabs.audio.voicing.descriptionStringProperty

This problem was previously identified and discussed in https://github.com/phetsims/phet-io/issues/1877.

I also confirmed that moving the string accessors from static to the constructor does not eliminate them from the API:

Subject: [PATCH] During migration schema compatibility testing, if one has parameterValues and the other does not, they are not compatible. undefined is not compatible with the empty array[], see https://github.com/phetsims/density/issues/196
---
Index: js/preferences/RegionAndCultureComboBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/preferences/RegionAndCultureComboBox.ts b/js/preferences/RegionAndCultureComboBox.ts
--- a/js/preferences/RegionAndCultureComboBox.ts	(revision b94d701c92cab77cd4cf554f333a7e0701c5b01b)
+++ b/js/preferences/RegionAndCultureComboBox.ts	(date 1714736472872)
@@ -17,24 +17,24 @@
 import JoistStrings from '../JoistStrings.js';
 import PickOptional from '../../../phet-core/js/types/PickOptional.js';
 
-// Maps a RegionAndCulture value to a StringProperty.
-const STRING_PROPERTY_MAP: Record<RegionAndCulture, LocalizedStringProperty> = {
-  africa: JoistStrings.preferences.tabs.localization.regionAndCulture.africaStringProperty,
-  africaModest: JoistStrings.preferences.tabs.localization.regionAndCulture.africaModestStringProperty,
-  asia: JoistStrings.preferences.tabs.localization.regionAndCulture.asiaStringProperty,
-  latinAmerica: JoistStrings.preferences.tabs.localization.regionAndCulture.latinAmericaStringProperty,
-  oceania: JoistStrings.preferences.tabs.localization.regionAndCulture.oceaniaStringProperty,
-  random: JoistStrings.preferences.tabs.localization.regionAndCulture.randomStringProperty,
-  usa: JoistStrings.preferences.tabs.localization.regionAndCulture.unitedStatesOfAmericaStringProperty
-};
+type SelfOptions = EmptySelfOptions;
+type RegionAndCultureComboBoxOptions = SelfOptions & PickOptional<ComboBoxOptions, 'tandem'>;
+
+class RegionAndCultureComboBox extends ComboBox<RegionAndCulture> {
+
+  public constructor( providedOptions?: RegionAndCultureComboBoxOptions ) {
+
+    // Maps a RegionAndCulture value to a StringProperty.
+    const STRING_PROPERTY_MAP: Record<RegionAndCulture, LocalizedStringProperty> = {
+      africa: JoistStrings.preferences.tabs.localization.regionAndCulture.africaStringProperty,
+      africaModest: JoistStrings.preferences.tabs.localization.regionAndCulture.africaModestStringProperty,
+      asia: JoistStrings.preferences.tabs.localization.regionAndCulture.asiaStringProperty,
+      latinAmerica: JoistStrings.preferences.tabs.localization.regionAndCulture.latinAmericaStringProperty,
+      oceania: JoistStrings.preferences.tabs.localization.regionAndCulture.oceaniaStringProperty,
+      random: JoistStrings.preferences.tabs.localization.regionAndCulture.randomStringProperty,
+      usa: JoistStrings.preferences.tabs.localization.regionAndCulture.unitedStatesOfAmericaStringProperty
+    };
 
-type SelfOptions = EmptySelfOptions;
-type RegionAndCultureComboBoxOptions = SelfOptions & PickOptional<ComboBoxOptions, 'tandem'>;
-
-class RegionAndCultureComboBox extends ComboBox<RegionAndCulture> {
-
-  public constructor( providedOptions?: RegionAndCultureComboBoxOptions ) {
-
     const options = optionize<RegionAndCultureComboBoxOptions, SelfOptions, ComboBoxOptions>()( {
 
       // For now, do not instrument Preferences elements, see https://github.com/phetsims/joist/issues/744#issuecomment-1196028362

Also, if those strings appear in another simulation that actually does support R&C, then later undergoes change, whatever migration rules we introduce in that sim can be reused here. I'm not saying it would be 0 overhead, but it could be shared overhead.

why do we have "supportsRegionAndCulture" true in the json

I do not see the string supportsRegionAndCulture in the API json: https://phet-dev.colorado.edu/html/projectile-data-lab/1.0.0-rc.5/phet-io/projectile-data-lab-phet-io-api.json, were you referring to a different JSON? It is also not in the package.json:

"simFeatures": {
"supportsDynamicLocale": true,
"supportsInteractiveDescription": true,
"supportsSound": true
},

thus bringing in the ?regionAndCulture query parameter behavior?

Since November 2023 all new sims have this commit phetsims/chipper@dd94297 which means all sims since then support the region and culture query parameter key. But each sim only support the query parameter values that it lists in the sim package.json, or defaults to 'usa' if it doesn't mention any.

I hadn't realized RegionAndCulture would be active on non RegionAndCulture related sims until I ran into it with Gas Properties yesterday.

I'm interested to hear more about this, was it limited to strings or about more than strings?

A note for other team members as we look into this: "unbuilt" mode shows more irrelevant strings than "built" mode does, so please be aware of that.

@kathy-phet
Copy link
Author

kathy-phet commented May 3, 2024

I'm not worried about the unused Strings. I'm considering the case - similar to locale - where a client appends regionAndCulture=africa to query parameters because that is their preferred regionAndCulture for their product. This sim would pop up a dialog, even though it basically is unaffected by regionAndCulture at all and is never planned to be. Where as locale=es will just continue, even if es is not an option.

Is there such a thing as
"supportsRegionsAndCultures": false
that would be used for sims that don't have a RegionAndCulture need? if you added it to the json package.

@samreid
Copy link
Member

samreid commented May 3, 2024

Idea for discussion, populate the schema conditionally and dynamically (untested):

    /*
     * Sets the default for the Region and Culture feature. The set of valid values is determined by
     * "supportedRegionsAndCulturesValues" in package.json. If not provided in the URL, the default can
     * be set via "defaultRegionAndCulture" in package.json, which defaults to 'usa'.
     */
    regionAndCulture: ( packagePhet?.simFeatures?.supportedRegionsAndCultures?.length > 0 ) ? {
      public: true,
      type: 'string',
      defaultValue: packagePhet?.simFeatures?.defaultRegionAndCulture ?? 'usa',
      validValues: packagePhet?.simFeatures?.supportedRegionsAndCultures ?? [ 'usa' ] // default value must be in validValues
    } : {
      public: false,
      type: 'string',
      defaultValue: 'usa',
      isValidValue: () => true
    },

@samreid
Copy link
Member

samreid commented May 3, 2024

Related issue where we most recently discussed locale fallbacks and when error message popups should appear: phetsims/joist#963 (comment)

@arouinfar
Copy link

arouinfar commented May 3, 2024

@kathy-phet @samreid @matthew-blackman I don't think this is a blocking issue for PDL/PSD, as @samreid points out all sims published after November 2023 support the supportsRegionAndCulture query parameter. I'll also note that I haven't added this query parameter to phet-io-guide.md yet (see https://github.com/phetsims/phet-io-sim-specific/issues/40), so clients are unlikely to know that it is a supported query parameter in PhET-iO.

Any changes that arise from this issue would also apply to a handful of recently published phet-brand and phet-io-brand sims.

Since November 2023 all new sims have this commit phetsims/chipper@dd94297 which means all sims since then support the region and culture query parameter key. But each sim only support the query parameter values that it lists in the sim package.json, or defaults to 'usa' if it doesn't mention any.

Thanks @samreid, this was really helpful to know.

A note for other team members as we look into this: "unbuilt" mode shows more irrelevant strings than "built" mode does, so please be aware of that.

A good reminder @samreid! However, note that all Preferences strings will appear in a built PhET-iO sim, even for irrelevant Preferences. Other irrelevant common code strings (e.g. such as those under strings.sceneryPhet) should be hidden in a built version, though.

Like @kathy-phet I am not concerned with regionAndCulture strings (and Preferences strings, more generally) appearing in the PhET-iO API of all sims.

I'm considering the case - similar to locale - where a client appends regionAndCulture=africa to query parameters because that is their preferred regionAndCulture for their product. This sim would pop up a dialog, even though it basically is unaffected by regionAndCulture at all and is never planned to be. Where as locale=es will just continue, even if es is not an option.

@kathy-phet this sounds like a client training issue. All sims support locale but only select sims support regionAndCuluture. Clients should not be appending regionAndCulture to all sims. Theoretically, we could treat regionAndCuluture more like locale so that regionAndCulture=africa will fall back to usa if africa is not supported in the sim. I'm not sure that it's worth it, though. While locale and regionAndCulture are both means of localization, the latter isn't always applicable. All sims have strings to localize, but not all sims have depictions to localize. I think it's important for clients to realize this.

Is there such a thing as
"supportsRegionsAndCultures": false
that would be used for sims that don't have a RegionAndCulture need? if you added it to the json package.

We do this with lots of other features/Preferences, but you can always set them to true with a query parameter. For example, running a sim with supportsSound=true will turn on the default UI sounds and all the related infrastructure (mute button in navbar, Audio tab in Preferences). What happens if you run a sim with supportRegionsAndCultures=true? Does it add the Region and Culture control to the Localization tab with just the usa option?

I'm not worried about clients doing this, but I bring it up because I would expect it to behave like the other "supports" options in initialize-globals.js.

@samreid samreid removed their assignment May 3, 2024
@marlitas
Copy link
Contributor

marlitas commented May 3, 2024

@samreid, @matthew-blackman, @brent-phet and I met to talk through some of these questions this morning. We looked through other query parameter examples and how they behave if the sim doesn't specifically support them. For example, we tried setting projectionColor=inverted on PDL and it also popped up a warning dialog. Since the region and culture behavior is consistent with other query parameters I don't think it needs to change. And if we decide it does, it should be consistent across all query parameters that may or may not be supported by sims.

Either way I don't believe it's blocking as a change would require a maintenance release.

@marlitas marlitas removed their assignment May 3, 2024
@kathy-phet
Copy link
Author

Noting that 2 PhET-iO sims currently have ?regionAndCulture query parameter behavior:
My Solar System
Kepler's Laws

I do think that we should revisit the use case for regionAndCulture, which is more parallel with locale than with other sim-specific features (even though it is sim specific).

I was mostly concerned with bringing this behavior into PhET-iO sims that don't need have a regionAndCulture aspect, but it's already in 2 PhET-iO sims. I'll put a discussion note on PhET-iO meetings.

And I'll mark this as deferred at the moment. Not blocking publication.

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

8 participants