Is it time to generalize "KeypadLayer"

[brandonLi8]

In both ProjectileMotion and UnitRates there is a Keypad layer which allows the user to modify some numeric Property. CollisionLab is now the third sim to need this functionality.

• Both ProjectileMotion and UnitRates have a KeypadLayer class which
  • creates a Keypad
  • creates a Enter Button
  • creates a text box to display the value
  • creates a text Node to display the range
  • changes the value text and the range text to red if the user enters in something out of the range
  • cancels the edit if the user hits outside the keypad panel

In my opinion, this is a lot of code functionality to essentially copy and paste into collision lab. Should there be a KeypadLayer class in common code?

@jonathanolson @pixelzoom thoughts?

Edit: The KeypadLayer looks like

Edit 2: forgot to mention this issue is related to #27.

[pixelzoom]

From https://github.com/phetsims/unit-rates/issues/153:

KeypadLayer and KeypadPanel are a workaround. They are a workaround for the lack of a robust Dialog (phetsims/joist#359) and the lack of closure on the next-gen keypad (phetsims/scenery-phet#283, phetsims/scenery-phet#271). Rather than factor out "workaround" code, KeypadLayer and KeypadPanel should be replaced when the above-mentioned issues are addressed.

So no, I don't think that KeypadLayer should be generalized. The time would be better spent finishing Keypad, so that we don't have to copy code and continue to put workarounds in new code.

The redesign of Keypad started in 2016 and stalled out. The main GitHub issue is phetsims/scenery-phet#283. And there are quite a few related issues that still need to be addressed, see phetsims/scenery-phet#271, phetsims/scenery-phet#272, phetsims/scenery-phet#279, phetsims/scenery-phet#333, phetsims/scenery-phet#541.

So my recommendation is to ask @ariel-phet whether he would like you to make another copy of the KeypadLayer workaround, or whether the Keypad work should be prioritized so that the workaround is unnecessary.

[pixelzoom]

See also https://github.com/phetsims/projectile-motion/issues/127 for the status of keypad workarounds (including KeypadLayer) in Projectile Motion. This has also been seen no progress since August 2017 (coming up on 3 years!)

[brandonLi8]

@pixelzoom I'm not sure I'm quite understanding. Quickly looking at all the issues you mentioned in https://github.com/phetsims/collision-lab/issues/60#issuecomment-632420740, it seems like they were all related to NumberKeypad except for https://github.com/phetsims/scenery-phet/issues/541 and https://github.com/phetsims/scenery-phet/issues/333, when both the KeypadLayer class in projectile-motion and now in collision-lab use the "next-gen" Keypad. For collision lab, there are no problems with Keypad itself.

Unit-rates is still using the "old" NumberKeypad while projectile-motion is using the "next-gen" keypad. The part I'm concerned about is how I essentially copied and pasted KeypadLayer from projectile-motion.

But thanks for pointing me to phetsims/projectile-motion#127 where you said to use the new Dialog when it has been revamped. It looks like it was revamped in 2018 so I'm going to go ahead and try it. But still, I would need to copy over the enter button, value number display, etc.

[pixelzoom]

But still, I would need to copy over the enter button, value number display, etc.

Now I'm confused. None of the stuff you mentioned is part of KeypadLayer and (based on the title of this issue) I thought that's what we were discussing. But I see that you also said:

Should there be a KeypadPlane or KeypadPanel class in common code?

Yes, something that provides these features should be in common code, part of the "keypad" feature. My understanding is that's part of the "unfinished work" that I pointed to in https://github.com/phetsims/collision-lab/issues/60#issuecomment-632420740. I have no idea how the developers who are working on that (@jbphet and @Denz1994) intend to address this. So the way forward is to continue/finish the work that was started, not start a new/separate effort to generalize the unit-rates versions of KeypadPanel and KeypadLayer. If that work can't be given some attention, then I would (regrettably) recommend making yet-another copy.

[brandonLi8]

Now I'm confused. None of the stuff you mentioned is part of KeypadLayer and (based on the title of this issue) I thought that's what we were discussing. But I see that you also said:

In projectile-motion, the KeypadLayer (not the Keypad itself) creates the enter button and the value Panel and the range message. I had to copy and paste this over because Keypad itself doesn't do this (which I agree with you, it should be apart of the Keypad).

I was able to remove the canceling functionality when I switched to Dialog, which was nice. Dialog also provides a way for me to position the Keypad however I want.

For now, I'm happy with my version of KeypadDialog. If new Keypad features are added, I'll update KeypadDialog.

[brandonLi8]

I think this issue can be closed. The new Dialog works really well and the usage is elegant in Collision Lab. Although there are still issues with Keypad, there are already plenty of open issues for that. So I'm closing.