Closed pixelzoom closed 2 months ago
From https://github.com/phetsims/gas-properties/issues/213#issuecomment-2014974052, here are @terracoda's thoughts on alt input BicyclePumpNode:
For the bicycle pump we could look at mapping that to a slider, going up/left is loading with air and going down/right is pumping out particles and increasing pressure. We could explore the standard types of slider shortcuts to pump a little or a lot.
Proof of concept in a patch, but I am not sure about it. I used KeyboardDragListener instead of AccessibleSlider. That was a bit easier because of the Property the BicyclePumpNode controls - the number of particles instead of the height of the handle.
Notes from discussion with @arouinfar @terracoda @pixelzoom :
1) Using KeyboardDragListener instead of AccessibleSlider makes sense for this component. 2) It should use RichKeyboardDragListener and RichDragListener to get default sounds. 3) No specific keyboard help content necessary, sim specific content using 'handle' terminology will be used in gas-properties.
Also during the meeting @pixelzoom offered to take a look at the patch from https://github.com/phetsims/scenery-phet/issues/848#issuecomment-2062120401. The patch proposes one way to factor out the drag code for DragListener and KeyboardDragListener. Assigning, and thanks!
We also discussed the Grab/Drag sounds.
There was some discussion about when the the grab/release sounds play for ALT input. For mouse they play on mouse down and mouse-up. @jessegreenberg, could you point me to the general issue about the sounds.
@terracoda Default grab/release sounds are being added to drag listeners in https://github.com/phetsims/scenery-phet/issues/849. @zepumph and @AgustinVallejo are the leads.
I reviewed the patch in https://github.com/phetsims/scenery-phet/issues/848#issuecomment-2062120401. InputListenerContainer
doesn't seem like an unreasonable way to factor out the code shared by the DragListener and the KeyboardDragListener. But I wonder if a class is really necessary. And it's going to make PhET-iO instrumentation of those listeners a bit more complicated -- we will need to instrument both listeners, and PumpHandleDragListenerOptions isn't going to do the job. It will also complicate future features, like providing sim-specific sounds for dragging.
Stand by while I experiment with another approach.
A more common pattern for factoring out shared code is the Delegate pattern, which uses composition instead of inheritance. In https://github.com/phetsims/scenery-phet/commit/1b677a72bf2cdf17983f586ffc86db5ea9943ade, I've used the Delegate pattern to factor out code that is shared by both drag listeners -- see class DragDelegate
. @jessegreenberg I hope you don't mind that I took the liberty of committing and pushing this. I felt pretty confident about it. And it involved a minor API change that affected a couple of sims - see https://github.com/phetsims/gas-properties/commit/011850e8640e5069a8de0df4ac63f59c3450092a and https://github.com/phetsims/states-of-matter/commit/3c207b60f6a59bed4071ed552dddf7aefc158deb.
Also in https://github.com/phetsims/scenery-phet/commit/1b677a72bf2cdf17983f586ffc86db5ea9943ade, I addressed default UI sound by using RichDragListener and RichKeyboardDragListener. And I addressed PhET-iO instrumentation of both RichDragListener and RichKeyboardDragListener.
@jessegreenberg please review. Let me know if you see any problems, have concerns/questions, etc.
Btw... The patch in https://github.com/phetsims/scenery-phet/issues/848#issuecomment-2062120401 was very helpful, and was easily adapted to the Delegate pattern. So thanks for that @jessegreenberg!
Reminder that we still have an issue with the pump handle's focus highlight:
//TODO https://github.com/phetsims/scenery-phet/issues/848 scale causes the focus highlight to also be scaled.
this.pumpHandleNode.scale( pumpHandleHeight / this.pumpHandleNode.height );
Now that I've been using BicyclePumpNode with the keyboard for a few days, I have to say that it feels kind of jerky. @jessegreenberg did I recall you saying that there was an option to make it move smoothly, instead of incrementally? How would I test-drive that?
Yes, dragSpeed
/shiftDragSpeed
instead of dragDelta
/shiftDragDelta
.
Thanks @jessegreenberg. I added this TODO in BicyclePumpNode.ts:
//TODO https://github.com/phetsims/scenery-phet/issues/848 This feels jerky. Would dragSpeed/shiftDragSpeed be better?
dragDelta: viewRange / 10,
shiftDragDelta: viewRange / 20,
I reviewed the behavior with @arouinfar, and she agreed that using dragSpeed
and shiftDragSpeed
feels a lot better - "we want it to feel like a phyiscal pump". So in https://github.com/phetsims/scenery-phet/commit/f8b34296a6b733b759c7db50340f5a7939c1549b, I made this change to the KeyboardDragListener's options, with values that were tuned with @arouinfar:
- moveOnHoldDelay: 750,
- moveOnHoldInterval: 200,
- dragDelta: viewRange / 10,
- shiftDragDelta: viewRange / 20,
+ dragSpeed: 200,
+ shiftDragSpeed: 50,
@jessegreenberg Let me know if you see any problems with this.
In Slack#dev-public, @jessegreenberg said:
There has been a change to the way focus highlight line widths are set. Let me know something seems wrong in that area.
I confirmed that the focus highlight looks good for the pump handle. So I removed the TODO in https://github.com/phetsims/scenery-phet/commit/ae9ceb62de414fa03cce05dd0f496f2e5a6ecbb4.
So... Next step for this issue is to complete the review requested in https://github.com/phetsims/scenery-phet/issues/848#issuecomment-2067416856.
The delegate pattern looks great, thanks! Commits look good. And I have no problems with using dragSpeed
for this. I tried it out in states-of-matter, and it works nicely. I think we are done with this one, closing.
Needed for Gas Properties suite PhET-iO release, https://github.com/phetsims/gas-properties/issues/191.
From https://github.com/phetsims/gas-properties/issues/213:
From https://github.com/phetsims/gas-properties/issues/214: