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

Options dialog + query params #154

Closed
pixelzoom opened this issue Sep 3, 2014 · 23 comments
Closed

Options dialog + query params #154

pixelzoom opened this issue Sep 3, 2014 · 23 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

This issue came up in phetsims/molecule-polarity#5, where we need to be able to choose from 2 conventions for bond dipoles.

Here's one idea that I proposed:

As a general solution for setting global options, we might consider putting an (optional) 'Options' menu item in the PhET menu at the bottom-right of the navigation bar. Selecting 'Options' would open an Options dialog (similar to the About dialog) where global options could be set. In addition, we could have a query parameter for each option, so that teachers would have a choice of whether to provide a pre-configured URL, or let students set the options themselves.

Other ideas?

We should definitely ask what the requirements are, because changing global settings while the sim is running will be more expensive to implement than configuring the sim once at startup based on query parameters.

@samreid
Copy link
Member

samreid commented Sep 3, 2014

I would personally like to make some of the options changeable at runtime (such as whether to show sims with a black or white background).

Perhaps some (simple) options should just appear in the PhET Popup menu (between separators), instead of in a separate Options dialog? Or a pull-out menu like this:
http://www.archidigm.com/classroom/adt_4_development_guide/layer_management_pull-down_menu.gif

@pixelzoom
Copy link
Contributor Author

At 9/4/14 developer meeting, we decided to move forward with Options menu item + query parameters. Assigned to me, since I need this functionality in Molecule Polarity.

@pixelzoom
Copy link
Contributor Author

Based on #56, I could not generalize AboutDialog into a general dialog type, it's implementation is very questionable, and responsibilities are not well-encapsulated. We should start over.

Starting over means major changes in joist. About which @jonathanolson said:

CM, let me know once those changes are done, sounds like it will be a more complicated merge into Scenery 0.2

And @samreid pointed out:

keep in mind that JO is maintaining scenery 0.2 and we also have the "arch" joist branch that will need to be merged in to master at some point.

So my question is... Should I wait on this until Scenery 0.2? And what is the ETA for that?

The most expedient way of creating an Options dialog would be to make a copy of what AboutDialog is doing, but that seems like a bad idea given that we plan to replace AboutDialog because it's not general.

@pixelzoom pixelzoom changed the title interface for specifying global options interface for specifying global options (Options dialog + query params) Sep 24, 2014
@pixelzoom pixelzoom changed the title interface for specifying global options (Options dialog + query params) Options dialog + query params Sep 24, 2014
@pixelzoom
Copy link
Contributor Author

I'm looking for a good pattern to use with global options, here's some brainstorming. Would like to hear other ideas and opinions.

The initial value of global options can be set via query parameters, and they can be changed from PhetMenu->Options. They are probably best encapsulated as a PropertySet. Here's an example from Molecule Polarity, where the initial values can be set via query parameters:

var globalOptions = new PropertySet( {
   // direction of dipoles, 'negativeToPositive' or 'positiveToNegative'
   dipoleDirection: window.phetcommon.getQueryParameter( 'dipoleDirection' ) || 'negativeToPositive',
   // color scheme for electrostatic potential surface, 'RWB' or 'ROYGB' 
   surfaceColor: window.phetcommon.getQueryParameter( 'surfaceColor' ) || 'RWB'
} );

How to best pass these to where they need to be observed? Some options I can think of:

(1) Define globalOptions in main.js, and pass it to each Screen that is created, then continue to pass down to each model/view component that needs to know about it. In the above example, this means passing dipoleDirection all the way down to Bond model elements. This makes for some ugly constructor overhead, as well as needing a means of unlinking Bonds when they are no longer needed.

(2) Define globalOptions as a singleton in its own .js file, and include that .js file where needed. No need to propagate it via constructors. But it has the smell associated with global variables. And some means of unlinking things from global properties is still needed.

(3) Attach globalOptions to window.phet (or something similar). This smells a little worse than (2), and doesn't have any advantages other than no need to create a singleton, and can be attached in main.js.

Opinions? Other suggestions?

@samreid
Copy link
Member

samreid commented Oct 1, 2014

I recommend (2) from above. Also, wasn't sure what this comment meant:

And some means of unlinking things from global properties is still needed.

Don't you just unlink?

@pixelzoom
Copy link
Contributor Author

Don't you just unlink?

yes. but if something is opaquely linking to a global property, then you need some sort of 'cleanup' function call to tell it to opaquely unlink when whatever is keeping a reference to it decides that the reference is no longer needed.

@jonathanolson
Copy link
Contributor

Slightly prefer (2), but recognize that this could cause havoc when trying to unit test isolated things.

@pixelzoom
Copy link
Contributor Author

@jbphet prefers (2).

@jonathanolson
Copy link
Contributor

Discussed, this will be a required feature for Molecule Shapes. Let me know if you would like to continue development on this, or if I should help out.

@pixelzoom
Copy link
Contributor Author

Let me know when you need this for Molecule Shapes. Molecule Polarity is on hold, with no deadline. I don't mind bumping up the priority of this, but it's currently medium-low.

@jonathanolson
Copy link
Contributor

Would the next few days work?

@pixelzoom
Copy link
Contributor Author

If you want Option dialog by the end of the week, you’ll unfortunately have to tackle it. I’m out Wed & Fri, and Thu is consumed with meetings. Sorry… Let me know if you want me to consult.

@pixelzoom
Copy link
Contributor Author

Note that #166 (Dialog base type) is a prerequisite to creating an Options dialog.

@jonathanolson
Copy link
Contributor

I've made an initial implementation which (at this point) only supports boolean options (using a checkbox).

In Molecule Shapes' main:

var simOptions = {
  credits: {
    // ...
  },
  globalOptions: [
    {
      label: showOuterLonePairsString,
      type: 'boolean',
      property: MoleculeShapesGlobals.showOuterLonePairsProperty
    },
    {
      label: projectorColorsString,
      type: 'boolean',
      property: MoleculeShapesGlobals.projectorColorsProperty
    }
  ]
};

where MoleculeShapesGlobals uses (among other items):

var MoleculeShapesGlobals = new PropertySet( {
  showOuterLonePairs: !!window.phetcommon.getQueryParameter( 'showOuterLonePairs' ) || false,
  projectorColors: !!window.phetcommon.getQueryParameter( 'projector' ) || false
} );

For Molecule Polarity it seems you would want things other than checkboxes (multiple choice combo-box or radio buttons), and this implementation can be added fairly easily in OptionsDialog.

@pixelzoom
Copy link
Contributor Author

Woah... Let's talk about this. The view should have a PropertySet of global options. And the dialog should take an arbitrary {Node} content, which is a collection of controls. The dialog shouldn't be creating the components.

@jonathanolson
Copy link
Contributor

We'll be passing in a node to the sim for the global options, and make a font available for consistency.

jonathanolson added a commit that referenced this issue Jan 29, 2015
…tioned correctly in the corner, and doesn't push content over), see #154, phetsims/molecule-shapes#96, #166
@jonathanolson
Copy link
Contributor

Added back the close button for the Options dialog, but with better positioning (doesn't push content over, and is fitted along the border with an adjustable constant). Will wait to hear feedback on appearance, as it is likely to change.

@jonathanolson
Copy link
Contributor

This feature is being used in molecule-shapes, molecule-polarity, charges-and-fields and states-of-matter-basics. I don't see any uncompleted requests here, so I'm closing the issue.

I'd encourage any requests for changes to be made in a new issue that is specific to the change.

@pixelzoom
Copy link
Contributor Author

Reopening. Does anyone else feel that the 'Options' title + close button occupies way too much vertical space? See screenshot below from Molecule Polarity. I think it makes more sense to position the 'Options' title to the left of the close button, not below and left.

screenshot_197

@jonathanolson
Copy link
Contributor

It does bug me a bit, however the vertical spacing should be the same for the top/bottom. Recommended way to fix this?

@jonathanolson jonathanolson reopened this Jan 29, 2015
@pixelzoom
Copy link
Contributor Author

Looks like this is a general issue with joist.Dialog, not just Options.dialog.

Recommended to make 'Options' look more like a dialog title, with close button to the right. This would be fairly easy to do if we didn't have the titleAlign option. Imo, title should always be centered, so that all dialogs look similar, I don't see a reason to make it configurable.

Here's a sample iOS options dialog. Not saying we should copy this, but just for comparison of how the title and buttons are placed. It don't think the title and button needs to be in a bar.

screenshot_443

@pixelzoom
Copy link
Contributor Author

Perhaps this discussion (title + close button placement) should be moved to #166 (create a Dialog base type)?

@pixelzoom
Copy link
Contributor Author

Moved the discussion of title + close button to #166. Closing.

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

3 participants