phetsims / acid-base-solutions Goto Github PK

Ticket for changes that can not be considered as workflow or fixing of any other tickets or bug.

The spacing of the radio buttons at the top of the control on the first screen is a bit uneven. It may be possible to use VBox options to fix this. If not, embedding some VStruts should do it.

Screen shot:

JavaScript's toFixed, has issues, see http://stackoverflow.com/questions/10015027/javascript-tofixed-not-rounding. For consistent behavior, all usages of toFixed should be replaced with Util.toFixed from the Dot repository.

Do not use translated string values in conditional tests. For example, in AcidBaseSolutionsModel, the 'mode' constructor parameter is apparently a string. And you're doing several tests to see if the title matches the 'Custom' screen:

127 solution: (customSolutionTitleString === mode ? self.SOLUTIONS[2].type : self.SOLUTIONS[0].type), // solution's type

140 for ( var i = (customSolutionTitleString === mode ? 1 : 0), solution; i < this.SOLUTIONS.length; i++ ) {

158 if ( customSolutionTitleString === mode ) {

200 if ( this.mode === customSolutionTitleString ) {

This type of test deserves an enum. Or better yet, use inheritance to create a specialization of AcidBaseSolutionsModel that adds the things you're adding with these string comparisons.

AcidBaseSolutionsModel calls super type constructor twice, lines 126 & 159. Calling a constructor twice will get you in all kinds of trouble, don't do it.

In this case, you want to add additional properties to your PropertySet. You can do so using PropertySet.addProperty. So replace the second constructor call with 4 calls to addProperty, one for each property.

AcidBaseSolutionsModel, line 168:

        var map = [
          [self.SOLUTIONS[3].type, self.SOLUTIONS[4].type],
          [self.SOLUTIONS[1].type, self.SOLUTIONS[2].type]
        ];

        self.solution = map[+self.isAcid][+self.isWeak];

Many problems here, this is unacceptable code.

1. Hardcoded indicies to SOLUTIONS array. If that array is ever changed, things will break and be difficult to understand why.
2. Very difficult to understand what's going on here, requires referring back to SOLUTIONS array repeatedly to understand map.
3. Casting booleans to array indicies is a bad practice. Don't do it. And it really takes some work to realize how map is arranged in order to make this happen.

AcidBaseSolutionModel defines SOLUTIONS, which is an array of the solution types in the sim. Now for the problem code:

// Formula line 25
this.formulas = [
      new WaterFormula( model ),
      new AcidFormula( model, {}, false ),
      new AcidFormula( model, {}, true ),
      new StrongBaseFormula( model ),
      new WeakBaseFormula( model )
    ];

// Workspace line 51
    model.property( 'solution' ).link( function( solution ) {
      model.SOLUTIONS.forEach( function( sol, index ) {
        if ( sol.type === solution ) {
          formulas.showFormula( index );
        }
      } );
      vbox.updateLayout();
    } );

So Formula.formulas had better be in the same order as the solutions in AcidBaseModelSolutions.SOLUTIONS, because it's relying on the same index to change their visibility. This is highly fragile code.

Because of the way that the model is structured, as one monolithic thing, you can't easily pass just the bits that are needed into view components.

For example, in the Java implementation, here's the constructor for the graph node. It needs to know only about the ConcentrationGraph model element.

public ConcentrationGraphNode( final ConcentrationGraph graph )

Here's the corresponding constructor in the JavaScript implementation, where model is of type AcidBaseSolutionModel. The graph must be passed the entire model.

function EquilibriumConcentrationBarChart( model, options )

This approach decreases encapsulation, increases coupling, and increases development and maintenance cost. I don't know how to fix this, and I'm wondering why you chose to create a model with such a structure, rather than porting the Java model?

One of many examples:

// H3OMolecule
function H3OMolecule( model, coords, fromCache ) {
    var H3O_COLOR = model.MOLECULES_COLORS.H3O;
    Node.call( this, coords );

    // cache values for next execution
    this.addChild( fromCache ? (atomCache ? atomCache : atomCache = getMolecule( H3O_COLOR )) : getMolecule( H3O_COLOR ) );
  }

For this type, you've implemented caching, so there must be potential performance issues. So why define H3O_COLOR inside of the constructor? This will allocate a variable for every single instance of H3OMolecule. If it's a constant, and you expect there to be more than one or two instances, define your constants outside of the constructor. Browser memory is a precious resource.

In the specification for this simulation, it currently says:

"The original implementation of this simulation was done prior to the creation of PhET’s ‘Property’ library, and uses an older pattern of registering explicitly for various events. This should be changed to use the Axon library and the ‘Property’ pattern."

This change is significant, and it is important that it be done correctly so that the behavior of the simulation is preserved. To this end, we at PhET felt that it would be valuable to provide an example of what needs to be done. This is a single example, but this type of change will need to be made in quite a number of places as the sim is ported.

In the Java version of the simulation, in the file ABSSolution.java, there are methods for adding and removing change listeners, addModelChangeListener and removeModelChangeListener, respectively. There is a method for sending change notifications to all registered listeners called fireSolutionChanged. This should all be replaced by having an Axon Property for the solution in ABSSolution.js, and anything that needs to observe solution changes can link to this property.

Also, ABSSolution.java has a method called 'setSolution', which causes the solution to be set locally but also for a number of other model components. This method would no longer exist, and each of the other model components would be passed the solutionProperty upon construction, which each would monitor and respond to changes as needed.

Ultimately, this should lead to a simpler implementation with less 'boiler plate' code, so in some ways, it will make the port a little easier, but will take some thought and a lot of testing to make sure that the functionality is the same.

This applies to all 5 files in js/view/workspace/formulas. Example from AcidFormula:

this.addChild( new Node( {x: 40, children: [
      new Text( 'H', {font: FONT, centerX: 0, centerY: textOffset} ),
      new Text( '2', {font: FONT_SMALL, centerX: 7.5, centerY: subOffset} ),
      new Text( 'O', {font: FONT, centerX: 15, centerY: textOffset} )
    ]} ) );

(1) Implementation relies on absolute positioning. Change the size of FONT and this totally break.

(2) Subscripts and superscripts can be done with scenery.HTMLText, example:
new HTMLText( 'H₂O', { font: FONT } );

(3) In the Java implementation, there was one class (ReactionEquationNode) that handles all types of formulas for this sim. Why do you need 5 very specific formula implementations?

// Concentration Slider, line 92:
    sliderProperty.link( function( value ) {
      concentrationProperty.value = parseFloat( Math.pow( 10, value ).toPrecision( 2 ) );
    } );

Several problems here:

1. concentrationProperty is numeric. toPrecision returns a string.
2. The value displayed by the Concentration control has 3 decimal places. toPrecision(2) will truncate to 2 significant digits. For example, 1.456 will become 1.4. Why are you truncating to 1 decimal place? I suspect that there is a misunderstanding here about what toPrecision does.
3. The bigger question is: why are you limiting the precision of the model value? Explain why this is desirable or necessary.

Throughout the Java version, the symbols 'A', 'B' and 'M' are displayed in italics, to denote that they are placeholders rather than actual atoms. For example, here's the Solutions control panel from the Java sim:

In #24, PhET asked to have scenery.HTMLText used for formulas. And we're planning to eventually replace HTMLText with scenery-phet.SubSupText. Neither of these supports italics. So I will leave it up to PhET to decide what (if anything) to do about this.

As of b8ec696, this sim is failing lint.

Run 'grunt lint' to see the errors.

This applies to SolutionsControl, TestsControl and ViewsControl.

Example from ViewControls:

var menuOptions = [
      {isRadio: true, value: 'MOLECULES', text: new Text( moleculesString, {font: FONT} ), icon: new Image( magnifyingGlassImage, {scale: 0.75} )},
      {isRadio: false, text: showSolventString, icon: new H2OMolecule( model )},
      {isRadio: true, value: 'EQUILIBRIUM', text: new Node( {children: [
        new Text( equilibriumString, {font: FONT, centerX: -6} ),
        new Text( concentrationString, {font: FONT, centerX: 0, centerY: 8} )
      ]} ), icon: new Node( {children: [
        new Rectangle( 0, 0, 24.5, 18, {fill: 'white', stroke: 'black', lineWidth: 0.5} ),
        new Rectangle( 2, 6, 3, 12, {fill: model.MOLECULES_COLORS.B} ),
        new Rectangle( 7.5, 3, 3, 15, {fill: model.MOLECULES_COLORS.H2O} ),
        new Rectangle( 13, 9, 3, 9, {fill: model.MOLECULES_COLORS.A} ),
        new Rectangle( 18.5, 9, 3, 9, {fill: model.MOLECULES_COLORS.H3O} )
      ]} )},
      {isRadio: true, value: 'LIQUID', text: new Text( liquidString, {font: FONT} ), icon: new Image( beakerImage, {scale: 0.75} )}
    ];

This array is then passed to a for loop that interprets the various (undocumented) flags and literals to create the UI components and control panel. This is a mess. It makes it difficult to modify existing controls, add new controls, or new control types (the for loop will only handle check boxes or radio buttons).

Please rewrite these control panels so that they are extensible and maintainable. An approach similar to js/view/control-panel/SolutionControl/SolutionControl would be fine.

MagnifierMoleculesLayer, line 54:

    // update layer only when it visible
    property.link( this.setMolecules.bind( this ) );

The comment indicates that setMolecules will be called only when this node is visible. I don't see any code that makes that happen, and I don't see it occurring in practice (for example, see #44).

Also related to #5.

AcidBaseSolutionsModel.pHToBrightness should be either static or private. Since it's only used in AcidBaseSolutionsModel, I recommend making it private.

To make it private:

var pHToBrightness = function( pH ) {…};

To make it static:

inherit( PropertySet, AcidBaseSolutionsModel, {
   // prototype function go here
}, {
   // static functions go here
  pHToBrightness: function( pH ) {…}
} ); 

See screenshot below. Running with '?dev' added to the URL will show a red outline that indicates the 'safe' area, guaranteed to be visible in the browser window. Note that the control panel (including the 'Reset All' button) falls slightly off the bottom of the screen. This is true for the control panel in both screens.

The grunt build is failing with the error below.

Error: Error: Module loading did not complete for: acid-base-solutions-main,
ACID_BASE_SOLUTIONS/model/AcidBaseSolutionsModel, ACID_BASE_SOLUTIONS/model/Constants/Constants
The following modules share the same URL. This could be a misconfiguration if
that URL only has one anonymous module in it:
c:/git-dev/acid-base-solutions/js/model/Constants/Constants.js: ACID_BASE_SOLUTIONS/model/Constants/Constants, model/Constants/Constants
at Function.build.checkForErrors (c:\git-dev\acid-base-solutions\node_modules\requirejs\bin\r.js:27064:19)

Every subtype of AqueousSolution contains this line in its init implementation:

this.concentration = this.CONSTANTS.CONCENTRATION_RANGE.defaultValue;

Move this to the super type.

AcidBaseSolutionsModel, line 140:

for ( var i = (customSolutionTitleString === mode ? 1 : 0), solution; i < this.SOLUTIONS.length; i++ ) {

The declaration of solution is hidden in the for loop expression. And it in fact not the solution, but a string that indicates the type of solution.

Recommend renaming the variable and making its declaration more obvious by pulling it out of the for loop expression.

In AqueousSolutionModel:

212 // send signal to views for resetting
213 this.resetTrigger = !this.resetTrigger;

This is a red flag that you probably don't have your property notification set up correctly. If view components are observing the proper model properties, then they should reset automatically when the model is reset. Explain why this is additional property is necessary.

In AcidBaseSolutionsModel, there are 2 uses of notifyObserversUnsafe:

self.property( 'concentration' ).notifyObserversUnsafe();
self.property( 'strength' ).notifyObserversUnsafe();

As noted in the PropertySet JSdoc, notifyObserversUnsafe is intended to be used when a property's value is indirectly mutated, versus set. Here's an example comparing uses:

var Property = require( 'AXON/Property' );
var Vector2 = require( 'DOT/Vector2' );

// This example allocates a new Vector2 to change the property value.
var locationProperty = new Property( Vector( x, y ) );
locationProperty.set( new Vector2( newX, newY ) );

// This example changes the property value directly, and requires a call to notifyObserversUnsafe.
var locationProperty = new Property( Vector( x, y ) );
locationProperty.get().setXY( newX, newY );
locationProperty.notifyObserversUnsafe();

So the use of notifyObserversUnsafe in AcidBaseSolutionsModel doesn't appear to match the intended use. Please explain why you are using notifyObserversUnsafe, and why it is required instead of standard use of properties.

Example:

// ConcentrationSlider
98 reset: function() {
      this.property.reset();
    }

In this case, this.property was provided in the constructor, and its value is model.property('concentration'). You are already doing model.reset elsewhere (see the Reset All button) so this reset is redundant.

Similar problem in SolutionSlider, where model.property( 'strength' ) is supplied to the constructor.

AcidBaseSolutionsModel defines VIEW_MODES and TEST_MODES as string arrays. Then these strings appear throughout the code, with no connection to VIEW_MODES or TEST_MODES. For example:

``javascript
// EquilibriumConcentrationBarChart:
57 this.setVisible( this.model.viewMode === 'EQUILIBRIUM' && this.model.testMode !== 'CONDUCTIVITY' );

//ViewControl
46 {isRadio: true, value: 'EQUILIBRIUM', text: new Node( {children: [

In places where VIEW_MODES or TEST_MODES is referenced, there's a hardcoded index, making it (a) impossible to determine what value is being used without consulting the array, and (b) easy to break if the array ever changes. Examples:

```javascript
// AcidBaseSolutionsModel
128 testMode: self.TEST_MODES[0], // test mode
129 viewMode: self.VIEW_MODES[0], // view mode

// MagnifierMoleculesLayer
83 if ( numberOfMolecules !== pointer && this.model.viewMode === this.model.VIEW_MODES[0] )

Please replace these string arrays with an enum pattern.

I had 2 other PhET developers look at AcidBaseSolutionsModel, the main model type. None of us could easily figure out how this model work. And it bears no resemblance to the Java implementation. Please provide detailed documentation about what you've done here, especially the data structures and properties.

Adding a console.log to ConductivityTestLightRays.setBrightness, I see that the light bulb rays are being updated regardless of whether the conductivity tester is visible. Since setBrightness updates 60 instances of scenery.Line and makes 240 calls to the Math library (4 per line), it's worth short-circuiting this update when the tester is not visible.

This involves added a visibility test to setBrightness, and overriding setVisible so that an update occurs when the tester transitions from invisible to visible.

pH is inconsistently spelled as 'ph' and 'pH'. Choose one, preferably 'pH'.

Examples:

// AcidBaseSolutionsModel
131 ph: 0, // ph level of product
153 this.property( 'ph' ).link( function( pHValue ) {
215 pHToBrightness: function( pH ) {

The constructor is currently:

function Atom( options, radius, color )

The options parameter should be last, since that's the only way that it can actually be optional.

AcidBaseSolutionModel is a subtype of axon.PropertySet. The default implementation of PropertySet.reset automatically resets all of its properties. AcidBaseSolutionModel is overriding PropertySet.reset, is not calling chaining to the super type, and is only resetting some properties.

Is this intentional? If so, please document why in the code.

FYI, a more typical override of reset looks like this:

// @override
reset: function() {
  PropertySet.prototype.reset.call( this );
  // do subtype-specific stuff here
}

Here's the problem code:

// ConcentrationSlider
88 readoutText.text = Util.toFixed( value, 3 ) + ' ' + molesPerLiterString;

The order of value and units is locale specific, so this needs to be internationalized. Here's the procedure, which is similar to Java's MessageFormat:

var StringUtils = require( 'PHETCOMMON/util/StringUtils' );
var pattern_0value_1units = require( 'string!ACID_BASE_SOLUTIONS/pattern.0value.1units' );

readoutText.text = StringUtils.format( pattern_0value_1units, Util.toFixed( value, 3 ), molesPerLiterString );

// AqueousSolution
function AqueousSolution( CONSTANTS )

// StrongAcidSolution
function StrongAcidSolution( strength, concentration ) {
    AqueousSolution.call( this, strength, concentration );

Looking at CONSTANTS in the debugger, this is an object literal with various sim constants. It's the private variable CONSTANTS in AcidBaseSolutions. This looked very odd, because the subtypes of AqueousSolution all have strength and concentration parameters. So I examined the strength arg to StrongAcidSolution in the debugger, and it's not strength, it's the CONSTANTS literal! And concentration is undefined!!!

And here's where the subtypes of AqueousSolution are being instantiated:

// AcidBaseSolutionsModel
141 solution = new this.SOLUTIONS[i].constructor( CONSTANTS );

This is a problem with all subtypes of AqueousSolution. The constructor signatures for the basic elements of the model are bogus. This is a fail.

Preliminary comments based on review of a very early version (sha e0376ee).

(1) Use more descriptive names for types, do not rely on directory structure to describe something. For example, view/control-panel/Solution/Solution.js. At a call site in the code 'Solution' will not communicate that this is a control. And when searching for things, there will be more namespace collisions. Use a name that completely conveys the meaning of the type. In this example, 'Solution' should be called something like 'SolutionControl', and it could live in the view/ directory. The same comment applies for Views, Tests and Solutions (all currently under view/control-panel).

(2) In AcidBaseSolutionsModel (a subtype of PropertySet) the override of 'reset' is unnecessary, unless you're planning to add things to reset that are specific to this subtype.

Example below. FONT is a constant, mixed in with imports.

// SolutionControl line 12
  var Node = require( 'SCENERY/nodes/Node' ),
    inherit = require( 'PHET_CORE/inherit' ),
    AquaRadioButton = require( 'SUN/AquaRadioButton' ),
    VBox = require( 'SCENERY/nodes/VBox' ),
    HBox = require( 'SCENERY/nodes/HBox' ),
    Text = require( 'SCENERY/nodes/Text' ),
    PhetFont = require( 'SCENERY_PHET/PhetFont' ),
    FONT = new PhetFont( 12 ),
    Line = require( 'SCENERY/nodes/Line' ),
    StrengthSlider = require( 'ACID_BASE_SOLUTIONS/view/control-panel/SolutionControl/StrengthSlider' ),
    ConcentrationSlider = require( 'ACID_BASE_SOLUTIONS/view/control-panel/SolutionControl/ConcentrationSlider' ),

In the original sim when conductivity test is selected, radio buttons in "Views" control panel are disabled. I could not find disable property or function for AquaRadioButton/RadioButton. Should it be done in some other way, or this feature can be added to the library?

In the original sim radio buttons aligned with the left edge in "Views" control panel and checkbox "Show Solvent" have indent beneath the "Magnifying Glass" radio button. I've tried to do it with VBox and left aligment, but I can't add indent for checkbox. I've tried to set x-coordinate, xMargin and add transparent rectangle for checkbox, but could not outsmart VBox. Is there any way to do it for VBox? Or should make absolute positioning?

Examples:

// AcidBaseSolutionsModel
    solvent: false, // solvent visibility

// AqueousSolution
      solute: 0, // solute concentration
      product: 0, // product concentration
      H3O: 0, // H3O concentration
      OH: 0, // OH concentration
      H2O: 0, // H2O concentration

I would never guess that a property named 'solution' is in fact the solution's concentration, it should be named 'solutionConcentration'. Similarly for other properties. Their semantics should be easy to determine by looking at the property names at use sites.

// AcidBaseSolutionsModel
191 step: function() {},

The model step function is optional, there is no need to provide a function that does nothing. Here's the related code in joist:

// joist.Sim line 362
if ( screen.model.step ) {
   screen.model.step( dt );
}

Throughout this sim, constants are sometimes in uppercase, sometimes in camel-case. For example:

/// StrengthSlider, line 26
    var width = 150,
      tickLength = 10,
      sliderProperty = new Property( Math.log( range.defaultValue ) / LN10 ),
      STRENGTH_MIN = Math.log( range.min ) / LN10,
      STRENGTH_MAX = Math.log( range.max ) / LN10,
      tickOffset = 5,
      slider;

STRENGTH_MIN and STRENGTH_MAX are presumably constants, because they use the convention of uppercase names. But what about width, tickLength and tickOffset? Since they use a different convention, my first suspicion is that they are different, and not constants. But reading the code, I find that they are indeed constants. Not a big deal in this specific case, but if you don't stick with a single convention then it makes your code more difficult to read/understand than it needs to be.

In AcidBaseSolutionsModel, the SOLUTIONS array is composed of object literals. These literals need to be documented. The 'relations' property in particular is baffling.

I also recommend that you avoid using 'constructor' as a property name, since this has meaning in a prototype context and makes for some confusing code.

Finally… In general, this is a very odd approach, and I'm wondering why you deviated so drastically from the Java implementation.

1. Add a console.log to MagnifierMoleculesLayer.setMolecules.
2. Start the sim, go to 'Introduction' tab.
3. Switch between the 'Molecules' and 'Equilibrium Concentration' views.
4. Notice that each time the controls are changed, setMolecules is call 12 times.

Why? I would expect it to either not be called at all (because the solution hasn't changed) or perhaps once when 'Molecules' is selected.

The tick mark at the right (max) end of the slider is missing. See screenshot.

This became apparent after reviewing the fix for #24.

The labels in the 'Solutions' control panel are not internationalized. The order of name and formula is currently hardcoded in SolutionsControl:

    {text: new HTMLText( waterString + ChemUtils.toSubscript( ' (H2O)' ), {font: FONT, centerX: 49, centerY: 0} ), value: Solutions.WATER, icon: H2OMolecule},
    {text: new Text( strongAcidString + ' (HA)', {font: FONT} ), value: Solutions.STRONG_ACID, icon: HAMolecule},
    {text: new Text( weakAcidString + ' (HA)', {font: FONT} ), value: Solutions.WEAK_ACID, icon: HAMolecule},
    {text: new Text( strongBaseString + ' (MOH)', {font: FONT} ), value: Solutions.STRONG_BASE, icon: MOHMolecule},
    {text: new Text( weakBaseString + ' (B)', {font: FONT} ), value: Solutions.WEAK_BASE, icon: BMolecule}

The order of name and formula is locale specific. Each of the text values should be internationalized using this string pattern, which is present in the Java strings file but missing from the HTML5 strings file.

"pattern.0solution.1symbol": "{0} ({1})",

The Equilibrium Concentration graph (aka chart) shows 3 bars in the 'Introduction' screen, and 4 bars in the 'Custom' screen, for a total of 7 bars in the sim. It is implemented in EquilibriumConcentrationBarChart, and this implementation is creating a total of 33 bars. It's creating a set of bars for each solution type, then switching visibility depending on which solution is selected. This wastes memory. And since you need to dynamically resize the bars for the 'Custom' screen, I don't understand what this buys you (other than additional complexity and a bigger memory footprint.)

See EquilibriumConcentrationBarChart constructor, line 30.

MAX_MOLECULES value (view/workspace/magnifier/MagnifierMoleculesLayer.js) in the original sim is equal to 200. When I set correct value it takes long time to start. I've added some optimizations for generating of molecules but is still takes approximately 30 seconds on PC to start with correct value. Could you check if there is an error in the application assembly?

The number of digits to the right of the decimal place varies quite based on the value, should remain fixed at a resolution of 1/1000. See the Java sim if needed.

Screen shot:

Decimal number strings have a comma instead of a decimal point when, even when run in locales that use a decimal point. See screen shot.

Molecule types in view/molecules/ appear to implement some form of caching. Why is this not the case for BMolecule and MMolecule?

The need to call an init function after construction is often symptomatic of a design flaw. I can't tell if that's the case here. Please document why the init function is needed, and what its responsibilities are.

AcidBaseSolutionModel, line 44:

var AQUEOUS_SOLUTION = new Color( 193, 222, 227, 180 ), // transparent light blue

Alpha values in Java are in the range 0-255. In JavaScript, alpha is in the range 0-1.