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

standardize query parameter for "projector mode" feature #371

Closed
7 tasks done
pixelzoom opened this issue Nov 9, 2016 · 14 comments
Closed
7 tasks done

standardize query parameter for "projector mode" feature #371

pixelzoom opened this issue Nov 9, 2016 · 14 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Nov 9, 2016

Noted while working on phetsims/chipper#516.

There are currently 2 different query parameters (projector and projectorMode) used to enabled the "projector mode" feature that is available in some sims.

Q1: Should PhET standardize this?
Q2: Should it be in the QueryStringMachine schema for common code, so it's not repeated in sim-specific schemas?

Here are the clients of the 2 query parameters:

projector

  • charges-and-fields
  • molecule-shapes

projectorMode

  • atomic-interactions
  • gravity-and-orbits
  • rutherford-scattering
  • states-of-matter-basics
  • states-of-matter
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Nov 9, 2016

Since "projector mode" is a subset of the more general "color scheme" issue (see #222)... My vote is for colorScheme query parameter, defined like this in the common-code QueryStringMachine schema:

colorScheme: {
  type: 'string',
  defaultValue: 'default',
  validValues: [ 'default', 'projector' ]
}

Sample usage: ?colorScheme=projector

Molecule Shapes appears to have 3 color schemes: default, basics, projector (see MoleculeShapesColors). So we'll need to decide whether 'basics' is sufficiently common to add to the above schema, or whether Molecule Shapes should have a sim-specific schema for colorScheme.

@jonathanolson
Copy link
Contributor

My vote is for colorScheme query parameter

Sounds good

So we'll need to decide whether 'basics' is sufficiently common to add to the above schema, or whether Molecule Shapes should have a sim-specific schema for colorScheme.

It seems sim-specific (or at least, we should have ways of having sim-specific color profiles).

@samreid
Copy link
Member

samreid commented Nov 10, 2016

It seems sim-specific (or at least, we should have ways of having sim-specific color profiles).

Agreed, there should be a sim-specific way of defining color profiles.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Nov 10, 2016

Agreed, there should be a sim-specific way of defining color profiles.

There's nothing that limits a sim's ability to define a color profile. But there's currently no ability for a sim to modify the schema for a common-code query parameter -- i.e., no ability to add 'basics' to the validValues for colorScheme. Do we need to add this ability to QueryStringMachine? Or is it sufficient for a sim that needs such customization to copy-paste-modify the schema that it needs to extend?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Nov 10, 2016

... i.e., Molecule Shapes would copy the colorScheme schema and modify it as follows:

colorScheme: {
  type: 'string',
  defaultValue: 'default',
  validValues: [ 'default', 'projector', 'basics' ]
}

... then use MoleculeShapesQueryParameters.colorScheme instead of phet.chipper.queryParameters.colorScheme. That could be problematic if there's common code that that handles general color schemes, and relies on phet.chipper.queryParameters.colorScheme.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Nov 10, 2016

I asked:

But there's currently no ability for a sim to modify the schema for a common-code query parameter -- i.e., no ability to add 'basics' to the validValues for colorScheme. Do we need to add this ability to QueryStringMachine?

I don't see a way to do this. The schema is evaluated in preloads, way before the sim every exists. So there's no opportunity for a sim to (for example) add to validValues.

@samreid
Copy link
Member

samreid commented Nov 10, 2016

Regarding the proposal in #371 (comment)

If you supply ?colorScheme=basics, then the validation in phetcommon would fail.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Nov 10, 2016

Ah, yes. So the sim would also need to change the query parameter name, and that seems very undesirable.

@pixelzoom
Copy link
Contributor Author

11/10/16 dev meeting notes:
• we'll refer to these as "color profiles"
• query parameter will be colorProfile
• omit validValues from schema for colorProfile, to support sim-specific values

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Nov 10, 2016

The schema will be:

colorProfile: {
  type: 'string',
  defaultValue: 'default'
}

Sample use:

?colorProfile=projector

For sims that want to set a flag to enable projector mode:

var flag = ( phet.chipper.queryParameters.colorProfile === 'projector' )

I'll handle the changes.

@pixelzoom pixelzoom self-assigned this Nov 10, 2016
pixelzoom added a commit to phetsims/chipper that referenced this issue Nov 10, 2016
pixelzoom added a commit to phetsims/atomic-interactions that referenced this issue Nov 10, 2016
pixelzoom added a commit to phetsims/states-of-matter-basics that referenced this issue Nov 10, 2016
@pixelzoom
Copy link
Contributor Author

charges-and-fields was converted in phetsims/charges-and-fields@78353b5.

molecules-shapes was converted in phetsims/molecule-shapes@1959d3a and phetsims/molecule-shapes@7baa6c8.

pixelzoom added a commit to phetsims/states-of-matter that referenced this issue Nov 14, 2016
pixelzoom added a commit to phetsims/atomic-interactions that referenced this issue Nov 14, 2016
pixelzoom added a commit to phetsims/states-of-matter-basics that referenced this issue Nov 14, 2016
pixelzoom added a commit to phetsims/gravity-and-orbits that referenced this issue Nov 14, 2016
pixelzoom added a commit to phetsims/rutherford-scattering that referenced this issue Nov 14, 2016
@pixelzoom
Copy link
Contributor Author

All sims converted and tested. Projector mode is enabled with ?colorProfile=projector, default colors are used with anything else.

@jessegreenberg, you won the lottery - would you please verify?

@jessegreenberg
Copy link
Contributor

I reviewed the commits above and tested all of the sims listed in #371 (comment). I verified that the query parameter ?colorProfile works exactly as described in #371 (comment). A search through repos also verified that the list of sims in the initial comment included all sims that have a projector color profile.

@pixelzoom is that all for this issue?

@pixelzoom
Copy link
Contributor Author

Yep, that's it. Thanks. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants