KeyBinding multiple keys pressed not working

Question

So I have a painted rectangle that I want to move with the arrow keys that includes diagonal movement, or rather allowance of multiple keys being pressed at the same time (In other words, movement that is similar to player movement in a 2D game). I have attempted to do this with a KeyListener, which was not working. So I decided to move to KeyBindings and I found an example from this website: https://coderanch.com/t/606742/java/key-bindings (Rob Camick's post)

I directly copied the code and it works as intended, except it is moving an icon and not a painted rectangle like I want to do. I have attempted to modify the code so that it would move a painted rectangle, but was only failing. Here is my latest attempt, which is also a minimal reproducible:

import java.awt.event.*;
import java.awt.*;
import javax.swing.*;
 
public class Test extends JPanel implements ActionListener
{
    public JPanel component = this;
    public int x = 100;
    public int y = 100;
    private Timer timer;
    private int keysPressed;
    private InputMap inputMap;
 
    public void addAction(String name, int keyDeltaX, int keyDeltaY, int keyCode)
    {
        new NavigationAction(name, keyDeltaX, keyDeltaY, keyCode);
    }
 
    //  Move the component to its new location
 
    private void handleKeyEvent(boolean pressed, int deltaX, int deltaY)
    {
        keysPressed += pressed ? 1 : -1;

        x += deltaX;
        y += deltaY;
 
        //  Start the Timer when the first key is pressed
        
        if (keysPressed == 1)
            timer.start();
 
        //  Stop the Timer when all keys have been released
 
        if (keysPressed == 0)
            timer.stop();
    }
 
    //  Invoked when the Timer fires
 
    public void actionPerformed(ActionEvent e)
    {
        repaint();
    }
 
    class NavigationAction extends AbstractAction implements ActionListener
    {
        private int keyDeltaX;
        private int keyDeltaY;
 
        private KeyStroke pressedKeyStroke;
        private boolean listeningForKeyPressed;
 
        public NavigationAction(String name, int keyDeltaX, int keyDeltaY, int keyCode)
        {
            super(name);
 
            this.keyDeltaX = keyDeltaX;
            this.keyDeltaY = keyDeltaY;
            
            pressedKeyStroke = KeyStroke.getKeyStroke(keyCode, 0, false);
            KeyStroke releasedKeyStroke = KeyStroke.getKeyStroke(keyCode, 0, true);
 
            inputMap.put(pressedKeyStroke, getValue(Action.NAME));
            inputMap.put(releasedKeyStroke, getValue(Action.NAME));
            component.getActionMap().put(getValue(Action.NAME), this);
            listeningForKeyPressed = true;
        }
 
        public void actionPerformed(ActionEvent e)
        {
            //  While the key is held down multiple keyPress events are generated,
            //  we only care about the first event generated
            if (listeningForKeyPressed)
            {
                handleKeyEvent(true, keyDeltaX, keyDeltaY);
                inputMap.remove(pressedKeyStroke);
                listeningForKeyPressed = false;
            }
            else // listening for key released
            {
                handleKeyEvent(false, -keyDeltaX, -keyDeltaY);
                inputMap.put(pressedKeyStroke, getValue(Action.NAME));
                listeningForKeyPressed = true;
            }
        }
    }
    
    public void paintComponent(Graphics tool) {
        super.paintComponent(tool);
        System.out.println(x + ", " + y);
        tool.drawRect(x, y, 50, 50);
    }
    
    public Test() {}
 
    public static void main(String[] args)
    {
        new Test().create();
    }
    
    public void create() {
        JFrame frame = new JFrame();
        frame.setDefaultCloseOperation( JFrame.EXIT_ON_CLOSE );
        frame.setSize(600, 600);
        frame.setLocationRelativeTo( null );
        
        frame.getContentPane().add(component);
        inputMap = component.getInputMap(JComponent.WHEN_IN_FOCUSED_WINDOW);
        
        timer = new Timer(24, this);
        timer.setInitialDelay( 0 );
        
        addAction("Left", -3,  0, KeyEvent.VK_LEFT);
        addAction("Right", 3,  0, KeyEvent.VK_RIGHT);
        addAction("Up",    0, -3, KeyEvent.VK_UP);
        addAction("Down",  0,  3, KeyEvent.VK_DOWN);
        
        frame.setVisible(true);
    }
 
}

Answer 1

Okay, so one of the issues I have with the code you've presented is the fact that you're leak responsibility. It's not the responsibility of the NavigationAction to register key bindings or modify the state the of UI. It should only be responsible for generating a notification that the state has changed back to a responsible handler.

This could be achieved through the use of some kind of model, which maintains the some kind of state, which is updated by the NavigationAction and read by the UI or, as I've chosen to do, via a delegation callback.

Another issue is, what happens when the user releases the key? Another issue is, how do you deal with the delay in repeated key press (ie when you hold the key down, there is a small delay between the first key event and the repeating key events)?

We can deal with these things through a simple change in mind set. Instead of using the NavigationAction to determine "what" should happen, it should be used to tell our UI "when" something has happened. In this a directional key has been pressed or released.

Instead of reacting to key stroke directly, we either "add" or "remove" the direction from a state model and allow the Swing Timer to update the state of the UI accordingly, this will generally be faster and smoother.

It also has the side effect of decoupling logic, as you no longer need to care about "how" the state was changed, only that it was and you can then decide how you want to react to it.

This approach is generally commonly used (in some form or another) in gaming engines.

import java.awt.Dimension;
import java.awt.EventQueue;
import java.awt.Graphics;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.KeyEvent;
import java.util.HashSet;
import java.util.Set;
import javax.swing.AbstractAction;
import javax.swing.ActionMap;
import javax.swing.InputMap;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.KeyStroke;
import javax.swing.Timer;

public class Test {

    public static void main(String[] args) {
        new Test();
    }

    public Test() {
        EventQueue.invokeLater(new Runnable() {
            @Override
            public void run() {
                JFrame frame = new JFrame();
                frame.add(new TestPane());
                frame.pack();
                frame.setLocationRelativeTo(null);
                frame.setVisible(true);
            }
        });
    }

    public interface KeyActionHandler {

        public void keyActionPerformed(Direction direction, boolean pressed);
    }

    public enum Direction {
        LEFT, RIGHT, DOWN, UP;
    }

    class NavigationAction extends AbstractAction implements ActionListener {

        private KeyActionHandler keyActionHandler;
        private Direction direction;
        private Boolean pressed;

        public NavigationAction(Direction direction, boolean pressed, KeyActionHandler keyActionHandler) {
            this.direction = direction;
            this.pressed = pressed;
            this.keyActionHandler = keyActionHandler;
        }

        public void actionPerformed(ActionEvent e) {
            keyActionHandler.keyActionPerformed(direction, pressed);
        }
    }

    public class TestPane extends JPanel implements ActionListener, KeyActionHandler {

        private int xDelta = 0;
        private int yDelta = 0;

        public int x = 100;
        public int y = 100;
        private Timer timer;

        public TestPane() {
            timer = new Timer(24, this);

            InputMap inputMap = getInputMap(WHEN_IN_FOCUSED_WINDOW);
            ActionMap actionMap = getActionMap();

            inputMap.put(KeyStroke.getKeyStroke(KeyEvent.VK_LEFT, 0, false), "Pressed-Left");
            inputMap.put(KeyStroke.getKeyStroke(KeyEvent.VK_RIGHT, 0, false), "Pressed-Right");
            inputMap.put(KeyStroke.getKeyStroke(KeyEvent.VK_UP, 0, false), "Pressed-Up");
            inputMap.put(KeyStroke.getKeyStroke(KeyEvent.VK_DOWN, 0, false), "Pressed-Down");

            actionMap.put("Pressed-Left", new NavigationAction(Direction.LEFT, true, this));
            actionMap.put("Pressed-Right", new NavigationAction(Direction.RIGHT, true, this));
            actionMap.put("Pressed-Up", new NavigationAction(Direction.UP, true, this));
            actionMap.put("Pressed-Down", new NavigationAction(Direction.DOWN, true, this));

            inputMap.put(KeyStroke.getKeyStroke(KeyEvent.VK_LEFT, 0, true), "Released-Left");
            inputMap.put(KeyStroke.getKeyStroke(KeyEvent.VK_RIGHT, 0, true), "Released-Right");
            inputMap.put(KeyStroke.getKeyStroke(KeyEvent.VK_UP, 0, true), "Released-Up");
            inputMap.put(KeyStroke.getKeyStroke(KeyEvent.VK_DOWN, 0, true), "Released-Down");

            actionMap.put("Released-Left", new NavigationAction(Direction.LEFT, false, this));
            actionMap.put("Released-Right", new NavigationAction(Direction.RIGHT, false, this));
            actionMap.put("Released-Up", new NavigationAction(Direction.UP, false, this));
            actionMap.put("Released-Down", new NavigationAction(Direction.DOWN, false, this));
        }

        @Override
        public Dimension getPreferredSize() {
            return new Dimension(400, 400);
        }

        @Override
        public void addNotify() {
            super.addNotify();
            timer.start();

        }

        @Override
        public void removeNotify() {
            super.removeNotify();
            timer.stop();
        }

        private Set<Direction> directions = new HashSet<>();

        //  Invoked when the Timer fires
        @Override
        public void actionPerformed(ActionEvent e) {

            if (directions.contains(Direction.LEFT)) {
                x -= 3;
            } else if (directions.contains(Direction.RIGHT)) {
                x += 3;
            }

            if (directions.contains(Direction.UP)) {
                y -= 3;
            } else if (directions.contains(Direction.DOWN)) {
                y += 3;
            }

            repaint();
        }

        @Override
        protected void paintComponent(Graphics g) {
            super.paintComponent(g);
            g.drawRect(x, y, 50, 50);
        }

        @Override
        public void keyActionPerformed(Direction direction, boolean pressed) {            
            if (pressed) {
                directions.add(direction);
            } else {
                directions.remove(direction);
            }
        }

    }
}

This is just one more approach to solve the same problem ;)

However, I am encountering a problem: it seems that removeNotify() is never called. I wanted to verify this and copied this post and pasted it into a new java file. I then added print statements in the add and remove notify methods, and only the print statement in the addNotify() method was printed. Also, if you add a print statement in the actionPerformed method, it prints repeatedly never ending

removeNotify is only called when the component, or a parent container, is removed.

import java.awt.Container;
import java.awt.Dimension;
import java.awt.GridBagLayout;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.SwingUtilities;

public class Test {

    public static void main(String[] args) {
        new Test();
    }

    public Test() {
        SwingUtilities.invokeLater(new Runnable() {
            @Override
            public void run() {
                JFrame frame = new JFrame();
                frame.add(new TestPane());
                frame.pack();
                frame.setVisible(true);
            }
        });
    }

    public class TestPane extends JPanel {

        public TestPane() {
            setLayout(new GridBagLayout());
            JButton btn = new JButton("Remove");
            btn.addActionListener(new ActionListener() {
                @Override
                public void actionPerformed(ActionEvent evt) {
                    Container parent = getParent();
                    parent.remove(TestPane.this);
                    parent.revalidate();
                    parent.repaint();
                }
            });
            add(btn);
        }

        @Override
        public Dimension getPreferredSize() {
            return new Dimension(200, 200);
        }

        @Override
        public void addNotify() {
            super.addNotify();
            System.out.println("Add");
        }

        @Override
        public void removeNotify() {
            super.removeNotify();
            System.out.println("Remove");
        }


    }
}

I verified that removeNotify will be called through the hierarchy when a parent container is removed.