Use NumberSpinner - Githubissues

jonathanolson commented 7 years ago

See issue https://github.com/phetsims/scenery-phet/issues/305, notably @ariel-phet approving it (https://github.com/phetsims/scenery-phet/issues/305#issuecomment-301870227), and my comments (https://github.com/phetsims/scenery-phet/issues/305#issuecomment-306882193, https://github.com/phetsims/scenery-phet/issues/305#issuecomment-306894672).

It seems the model should have number properties for the values I described, each of which the spinners can change (and other things can change them also).

jonathanolson commented 7 years ago

Additionally, if anything else is needed to use NumberSpinner (questions, or if there is disagreement), let me know here. If there are parts of NumberSpinner that can be changed in common code, I may be able to make those changes.

veillette commented 7 years ago

@jonathanolson: The denominator and max spinners can easily be changed to use NumberSpinner.

The numerator spinner cannot use the number spinner as currently designed. The problems lies in the fact the the numerator is an output of the simulation rather than an input. However, the spinners are wired to changed the value of the numerator. So we have something like this


    // present for the lifetime of the simulation
    // responsible for creating pieces emanating or returning to the bucket
    this.numeratorProperty.link( function( numerator, oldNumerator ) {

      var difference = numerator - oldNumerator;

      // adding a piece that will be animated from the bucket to a cell for the difference
      if ( difference > 0 && self.containerSet.getEmptyCellsCount() > 0 ) {

        for ( var i = difference; i > 0; i-- ) {
          self.addAnimatingPieceInBucket();
        }
      }

      // a piece will fly from a cell to the bucket each time for the difference
      if ( difference < 0 && self.containerSet.getFilledCellsCount() > 0 ) {

        for ( var j = difference; j < 0; j++ ) {
          self.addAnimatingPieceAtCell();
        }
      }
    } );

But there are many ways that can affect the number of filled cells in the simulations for which we don't want to add additional pieces. Ideally the numerator would be simply a way to keep tab on their numbers and all the logic of the cells would be in the ContainerSet.

Right now, numeratorProperty acts as a input and output at the same time and this lead to situation such as : https://github.com/phetsims/fractions-intro/issues/58. Ideally the increment and decrement buttons would have listeners that we could override. The increment and decrement buttons could then be wired to our addAnimatingPieceInBucket and addAnimatingPieceAtCell methods.

jonathanolson commented 7 years ago

The numerator spinner cannot use the number spinner as currently designed.

If you need to be able to specifically tag changes that come from the spinner, it's possible to pass it a property that is handled separately:

var spinnerNumeratorProperty = new NumberProperty( 0 );
var isChangeFromSpinner = true;
numeratorProperty.link( function( numerator ) {
  isChangeFromSpinner = false;
  spinnerNumeratorProperty.value = numerator;
  isChangeFromSpinner = true;
} );
spinnerNumeratorProperty.lazyLink( function( numerator, oldNumerator ) {
  if ( isChangeFromSpinner ) {
    // put the logic here that checks difference and handles
  }
} );
new NumberSpinner( spinnerNumeratorProperty, ... );

however the similar "don't do X when Property Y changes" flag could also presumably be used to toggle whether animation happens when the numerator changes.

Would it be possible to directly pass in the numeratorProperty, but in cases where no animation is desired, set a flag first that will prevent the animation from happening?

coledu commented 7 years ago

As I start to implement the number spinner, I found that the spinner needs to be on the left side of the spinner and there is not an option for this. These are the only options for number spinner:

var ARROWS_POSITION_VALUES = [
    'leftRight', // arrow buttons on left and right of value
    'topBottom', // arrow buttons on top and bottom of value
    'bothRight', // both arrow buttons to the right of the value
    'bothBottom' // both arrow buttons below the value
  ];

Would it be possible to add a 'bothleft' options to number spinner? My current solution is to put the fontsize to zero and put a separate text to the right of the spinner. This is what I have so far:

jonathanolson commented 7 years ago

@pixelzoom, I hadn't evaluated this previously, but is NumberSpinner the correct thing here? I didn't realize the readout was basically required. That makes it complicated for the right-hand side, where the NumberSpinner is editing a value that is not directly displayed (instead, it affects a multiplier for the entire fraction displayed to the left of the spinner).

If it is the thing to use, can the position value above be added? (And how would the nice centered layout be done nicely if the readout number is included as part of the spinner)?

pixelzoom commented 7 years ago

... is NumberSpinner the correct thing here? ...

Not as NumberSpinner is currently implemented. Up to you and the design team where the change the design, change NumberSpinner, or implement something new (e.g. decouple the arrows and the display). I would probably go with decoupling.

jonathanolson commented 7 years ago

Sorry @coledu and @veillette, I had assumed that this was the ideal component to use based on the previous content in the issue.

Can it be switched back to the more custom spinner implementation that was used before? Sorry!

coledu commented 7 years ago

Ok, I have reverted back to the UpDownSpinner like before. We would like to add the following options to UpDownSpinner. An options to allow fire on hold, and another options to pass in a callback that would allow us to pass in our animate button function. And a final option to change the radius. Adding these feature would be relatively simple and only require a few lines. The following is the code with two out of the three suggested features.

function UpDownSpinner( valueProperty, upEnabledProperty, downEnabledProperty, options ) { Tandem.indicateUninstrumentedCode();

options = _.extend( {
  upButtonListener: function() {
    valueProperty.set( valueProperty.get() + 1 );
  },
  downButtonListener: function() {
    valueProperty.set( valueProperty.get() - 1 );
  },
  fireOnHold: false
}, options );

var shapeWidth = 26;
var upShape = new Shape().moveTo( 0, 0 ).lineTo( shapeWidth / 2, -10 ).lineTo( shapeWidth, 0 );
var downShape = new Shape().moveTo( 0, 0 ).lineTo( shapeWidth / 2, 10 ).lineTo( shapeWidth, 0 );

var upIcon = new Path( upShape, { lineWidth: 5, stroke: 'black', lineCap: 'round' } );
var downIcon = new Path( downShape, { lineWidth: 5, stroke: 'black', lineCap: 'round' } );

var radius = 20;
var upButton = new RoundPushButton( {
  content: upIcon,
  listener: options.upButtonListener,
  radius: radius,
  touchAreaDilation: 5,
  baseColor: '#fefd53',
  yContentOffset: -3,

  // options related to fire-on-hold feature
  fireOnHold: options.fireOnHold,
  fireOnHoldDelay: 400, // start to fire continuously after pressing for this long (milliseconds)
  fireOnHoldInterval: 100 // fire continuously at this interval (milliseconds)
} );
upEnabledProperty.linkAttribute( upButton, 'enabled' );

var downButton = new RoundPushButton( {
  content: downIcon,
  listener: options.downButtonListener,
  radius: radius,
  touchAreaDilation: 5,
  baseColor: '#fefd53',
  yContentOffset: +3,

  // options related to fire-on-hold feature
  fireOnHold: options.fireOnHold,
  fireOnHoldDelay: 400, // start to fire continuously after pressing for this long (milliseconds)
  fireOnHoldInterval: 100 // fire continuously at this interval (milliseconds)
} );
downEnabledProperty.linkAttribute( downButton, 'enabled' );

VBox.call( this, { spacing: 6, children: [ upButton, downButton ] } );

this.mutate( options );

}

sceneryPhet.register( 'UpDownSpinner', UpDownSpinner );

return inherit( VBox, UpDownSpinner ); } );

We would appreciate any feed back that you might have @jonathanolson

jonathanolson commented 7 years ago

I may handle more cleanup of UpDownSpinner/LeftRightSpinner as part of https://github.com/phetsims/scenery-phet/issues/311.

Also notably, I'd probably make the relevant changes that are different from the proposal above:

Seems like the spinners should just take callbacks instead of a valueProperty.
The horizontal and vertical variants would ideally be combined into one file.
Ideally you'd be able to just directly pass in options for the button, e.g. new DirectionalSpinner( ..., { ..., buttonOptions: { fireOnHold: true } } ), so that clients have the ability to customize things like fireOnHold or other properties (like colors), or the radius.

Assigning myself to handle.

jonathanolson commented 7 years ago

Discussed with @samreid, and I think it seems simplest to just copy the required spinner code into fractions-intro directly, modify it (ideally as I've noted above, I'll elaborate more below), and then when finished have an issue created in fraction-comparison to look at generalizing the fractions-intro one. It will probably be faster if there's something you can modify to suit the desired appearance and behavior easily.

I'd recommend:

One spinner type/file, uses orientation ('horizontal' or 'vertical' like LayoutBox) or just rotation to determine the orientation. (You can swap HBox/VBox by passing in { orientation: 'horizontal' } or { orientation: 'vertical' }, and probably would want to do that for the implementation.
Callbacks should be passed instead of a valueProperty.
Hard-code whatever fireOnHold or other button options are desired.
Use whatever touch areas work for this sim
When possible, just rotate things (like potentially the full button, so you don't have to handle what type of offset the shape needs in four different orientations)

I'll handle review for it as necessary

samreid commented 7 years ago

@jonathanolson and I discussed it and it seems like sun.NumberSpinner has the following problems:

it does not support adding callbacks as listeners (must be through Property interface)
it does not support hiding the readout

Hence @jonathanolson will recommend we use a custom implementation so that the Fractions Intro team can have full control over this new component. Once it is ready we can evaluate whether it should also be commonized and used in Fraction Comparison.

jonathanolson commented 7 years ago

Also, sorry about the confusion and changes of direction! I haven't dealt with NumberSpinner/UpDownSpinner/LeftRightSpinner before.

coledu commented 7 years ago

I have added the following options: fireOnHold, radius (which scales the rest of the button), callback, and a two orientation. We are still working on the rotation and the four different orientations.

coledu commented 7 years ago

Ok, we believe that we have added everything to our new spinner that we need for Fractions-Intro. We have added a dispose function along with the following options:

      upButtonListener: function() {valueProperty.set( valueProperty.get() + 1 );},
      downButtonListener: function() {valueProperty.set( valueProperty.get() - 1 );},
      baseColor: '#fefd53',  // color of the button
      radius: 20, // radius of the push button
      iconScale: 0.65, // ratio of the width of the arrow over the diameter of the button
      spacing: 6, // separation of the two buttons
      orientation: 'vertical', // orientation of the spinner with respect to one another
      arrowOrientation: 'vertical', // direction of the arrow within a spinner
      fireOnHold: true

We also added the four different orientations that we believe you wanted that are customize by the orientation and arrow Orientation. Below are picture of what value to use for each orientations:

{orientation: 'vertical', arrowOrientation: 'vertical'}

{orientation: 'horizontal', arrowOrientation: 'vertical'}

{orientation: 'horizontal', arrowOrientation: 'horizontal'}

{orientation: 'vertical', arrowOrientation: 'horizontal'}

If you have any suggestions for improvement to the spinner, we will make the necessary changes. Assigning to @jonathanolson for review.

jonathanolson commented 7 years ago

It looks like RoundSpinner is using valueProperty. I was anticipating it just using callbacks, which in some cases may be able to adjust a property, as I believe you noted direct access to a property was a problem?

Also, is it planned to have the orientation and arrowOrientation be different?

coledu commented 7 years ago

I have made the changing to not use value property but pass two callback into Round Spinner instead. And yes we are planing are using different orientation for the equality lab.

coledu commented 7 years ago

I believe this issue might be ready to close. We have deciding to use round spinner it our sim. Assigning to @jonathanolson for review.

phetsims / fractions-intro

Use NumberSpinner #62