Refactor describer listeners to avoid listener dependencies

[twant]

@zepumph and I discussed the below review item from #171 today, and decided to move away from the current pattern, which we agree could definitely cause some listener dependencies problems. I'll be refactoring all of the describers to move away from these lazyLinks to model Properties.

Items to check for lazyLinks to model:

• [ ] ConcentrationDescriber

• [ ] SoluteAmountDescriber

• [ ] VolumeDescriber

• [ ] SoluteDescriber

• [ ] The general algorithm for the lazyLinks in the constructor is to update whenever a value changes. Another way to do this would be to just update when descriptions are asked for. This would mean that the "granularity" of the descriptions would only be as good as how often describer methods are called. I don't know that this other way would be better, but one thing I notice is that state in the describer is being updated via those links, and methods all over the sim (like alerts) are calling describer methods. I'm having a hard time guaranteeing in my mind that we don't have a listener order dependency. For example. If you want to alert when the precipitateAmountProperty changes, then you have a listener from that Property calling an alert, AND a listener from that Property updating the concentrationDescriber state to provide the correct describer strings for that alert. I don't yet know enough about how the descriptions in this sim work to know if the above is a problem. Can we talk about this next meeting?

[zepumph]

After discussing with @twant, and recognizing #189, we think that we should revert this change in ConcentrationDescriber.

The main reason is because this approach is obfuscating the updating of internal ConcentrationDescriber state fields. On top of that, this doesn't even fix the central listener order dependency (which is in the model, again see #189). Because of this we will revert back to having listeners in ConcentrationDescriber. Even so we will keep the new, better naming and likely some logic from #171. @twant let me know how I can help.

[zepumph]

• [ ] From #190, we should try to name document the "increased" fields in volume/soluteAmount/concentration describers to make sure that it is clear that they should only be called after that model Property has changed (therefore either increasing or decreasing).

[twant]

@zepumph I believe I've reverted back to the previous version of this implementation, but with added documentation for the describer properties and are linked in the constructor of concentrationDescriber. Can you take a look and see if it looks right to you?

[zepumph]

From #190, we should try to name document the "increased" fields in volume/soluteAmount/concentration describers to make sure that it is clear that they should only be called after that model Property has changed (therefore either increasing or decreasing).

Part of me feels like this is weird, and that documentation isn't enough. Instead of storing concentrationIncreased directly on the describer, what if it was a function, and it can be calculated on demand. Then stored on the describer is lastConcentrationValue. What do you think about that?

concentrationIncreased() { 
  assert && assert( this.concentrationProperty.value !== this.lastConcentrationValue); // I LOVE THIS!!!
  return this.concentrationProperty.value > this.lastConcentrationValue;
}

What do you think?

Side note: (you don't need to store lastConcentrationIndex and lastConcentrationValue, and can just calculate both if you want to save a field on the describer.

[twant]

@jessegreenberg and I investigated this today. When we implemented, we got an error that we believe is caused by a listener dependency issue (wherelastConcentrationValue is being updated before concentrationIncreased() can be called). We looked into a solution for this, which involved updating lastConcentrationValue where concentrationIncreased() is called, but this did not seem better than the previous solution (since it would have to be updated at multiple call sites).

In looking at our previous solution, it also has listener dependency concerns (since the lazyLink function must be called before functions dependent on this.concentrationIncreased), but they are not causing issues right now. We agree that it's not ideal, but if we're not going to invest significant time overhauling to address the listener dependency issues so close to RC, it seems like the best solution for now.

@zepumph please feel free to review and reopen if you have concerns with this approach!