phetsims / fourier-making-waves

"Fourier: Making Waves" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 3 forks source link

Replace globalKeyStateTracker with KeyboardListener #237

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

From chip away issue https://github.com/phetsims/tasks/issues/1126 ...

In WaveGameLevelNode.ts, replace this with KeyboardListener:

    // Alt+C hotkey support for 'Check Answer'. globalKeyStateTracker listeners always fire, so it's our
    // responsibility to short-circuit this listener if the checkAnswerButton is not in the PDOM, and not enabled.
    globalKeyStateTracker.keydownEmitter.addListener( event => {
      if (
        checkAnswerButton.pdomDisplayed &&
        checkAnswerButton.enabledProperty.value &&
        globalKeyStateTracker.altKeyDown &&
        KeyboardUtils.isKeyEvent( event, KeyboardUtils.KEY_C )
      ) {
        checkAnswerListener();
      }
    } );

See the example that @jessegreenberg provided in https://github.com/phetsims/tasks/issues/1126#issue-1814396968.

pixelzoom commented 1 year ago

Done in the above commit. Tested in the Wave Game screen. Closing.

jessegreenberg commented 1 year ago

Looks great! Just FYI, you can remove this now:

          checkAnswerButton.pdomDisplayed &&
          checkAnswerButton.enabledProperty.value &&

That is now handled by scenery.

pixelzoom commented 1 year ago

@jessegreenberg This listener should only fire when the game is in the state where the user's guess is ready to be checked. How does scenery handle that, or even know about it?

jessegreenberg commented 1 year ago

I see now the listener is added to the WaveGameLevelNode. If you want, you can add it to the checkAnswerButton, and then the global event will only reach the checkAnswerButton when it is displayed and enabled.

pixelzoom commented 1 year ago

OK thanks - I'll try adding to checkAnswerButton.

pixelzoom commented 1 year ago

Adding the KeyboardListener to checkAnswerButton works nicely, and simplifies the logic:

    const checkAnswerButtonKeyboardListener = new KeyboardListener( {
      keys: [ 'alt+c' ],
      callback: ( event, listener ) => checkAnswerListener(),
      global: true
    } ); 

@jessegreenberg Note that I also simplified the callback. Since there is only 1 element in the keys array, I am not checking listener.keysPressed in the callback logic. Is this safe, or is it preferrable to check?

jessegreenberg commented 1 year ago

Great, thanks! Totally fine to remote that too, the callback will only be called for 'alt+c'.

pixelzoom commented 1 year ago

Excellent. Closing.