Why does this JLabel have 2 positions?-CodePudding

I'm making this program where a JLabel controlled by the user will move on a 500x500 frame, but for some reason it seems to have two positions and when it moves it just flickers between them. What is causing this?

This is how it currently looks like

Here's the main window:

package guipkg1;

import java.awt.event.KeyEvent;
import java.awt.event.KeyListener;

import javax.swing.*;

import guipkg1.PlayerMove;

public class KeyListenerPage extends JFrame implements KeyListener {
    public static JLabel player = new JLabel();
    public static boolean isGoingLeft = false;
    public static boolean isGoingUp = false;
    public static boolean isGoingRight = false;
    public static boolean isGoingDown = false;
    PlayerMove pmove = new PlayerMove();
    KeyListenerPage(){
        this.setDefaultCloseOperation(HIDE_ON_CLOSE);
        this.setSize(500,500);
        this.setLayout(null);
        this.setVisible(false);
        this.addKeyListener(this);
        
        player.setBounds(0,0,50,50);
        player.setText(":)");
        this.add(player);
        
        
        
        pmove.start();
    } 
        
    @Override
    public void keyTyped(KeyEvent e) {
        // TODO Auto-generated method stub
        
    }
    @Override
    public void keyPressed(KeyEvent e) {
        // TODO Auto-generated method stub
        if(e.getKeyCode()==37 || e.getKeyCode()==65) {
            isGoingLeft = true;
            System.out.println("[KeyListenerPage] going left");
        } else if(e.getKeyCode()==38 || e.getKeyCode()==87) {
            isGoingUp = true;
            System.out.println("[KeyListenerPage] going up");
        } else if(e.getKeyCode()==39 || e.getKeyCode()==68) {
            isGoingRight = true;
            System.out.println("[KeyListenerPage] going right");
        } else if(e.getKeyCode()==40 || e.getKeyCode()==83) {
            isGoingDown = true;
            System.out.println("[KeyListenerPage] going down");
        }
        
    }
    @Override
    public void keyReleased(KeyEvent e) {
        // TODO Auto-generated method stub
        if(e.getKeyCode()==37 || e.getKeyCode()==65) {
            isGoingLeft = false;
            System.out.println("[KeyListenerPage] stopped going left");
        } else if(e.getKeyCode()==38 || e.getKeyCode()==87) {
            isGoingUp = false;
            System.out.println("[KeyListenerPage] stopped going up");
        } else if(e.getKeyCode()==39 || e.getKeyCode()==68) {
            isGoingRight = false;
            System.out.println("[KeyListenerPage] stopped going right");
        } else if(e.getKeyCode()==40 || e.getKeyCode()==83) {
            isGoingDown = false;
            System.out.println("[KeyListenerPage] stopped going down");
        }
    }
}

And the thread moving the player:

package guipkg1;

import javax.swing.JLabel;

public class PlayerMove extends Thread implements Runnable {
    boolean movingLeft = KeyListenerPage.isGoingLeft;
    boolean movingUp = KeyListenerPage.isGoingUp;
    boolean movingRight = KeyListenerPage.isGoingRight;
    boolean movingDown = KeyListenerPage.isGoingDown;
    JLabel player = KeyListenerPage.player;
    boolean canMoveLeft = false;
    boolean canMoveRight = true;
    boolean canMoveUp = false;
    boolean canMoveDown = true;
    public void run() {
        System.out.println("running");
        while(true) {
            try {
                Thread.sleep(20);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
            movingLeft = KeyListenerPage.isGoingLeft;
            movingUp = KeyListenerPage.isGoingUp;
            movingRight = KeyListenerPage.isGoingRight;
            movingDown = KeyListenerPage.isGoingDown;
            if(movingLeft && canMoveLeft) {
                player.setBounds(player.getWidth(), player.getHeight(), player.getX()-10, player.getY());
                player.setVisible(true);
                if(player.getX()<=0) {
                    canMoveLeft = false;
                }
                if(player.getX()<490) {
                    canMoveRight = true;
                }
            } 
            if(movingRight && canMoveRight) {
                player.setBounds(player.getWidth(), player.getHeight(), player.getX() 10, player.getY());
                player.setVisible(true);
                if(player.getX()>0) {
                    canMoveLeft = true;
                }
                if(player.getX()>=490) {
                    canMoveRight = false;
                }
            }
            if(movingUp && canMoveUp) {
                player.setBounds(player.getWidth(), player.getHeight(), player.getX(), player.getY()-10);
                player.setVisible(true);
                if(player.getY()<=0) {
                    canMoveUp = false;
                }
                if(player.getY()<490) {
                    canMoveDown = true;
                }
            } 
            if(movingDown && canMoveDown) {
                player.setBounds(player.getWidth(), player.getHeight(), player.getX(), player.getY() 10);
                player.setVisible(true);
                if(player.getY()>0) {
                    canMoveUp = true;
                }
                if(player.getY()>=490) {
                    canMoveDown = false;
                }
            } 
            
        }
    }
}

CodePudding user response：

Why does it flicker?

Because your arguments to player.setBounds() are in the wrong order.

According to the JLabel#setBounds() JavaDoc the arguments are in the order x, y, width, height.

But your code calls the method with the arguments

player.setBounds(player.getWidth(), player.getHeight(), player.getX(), player.getY() 10);

where it should be

player.setBounds(player.getX(), player.getY() 10, player.getWidth(), player.getHeight());

Also note that Swing is not thread-safe. If you want to change anything on a displayed Swing component you must do it on the Swing EDT, for example by rewriting PlayerMove.run() as

public void run() {
    System.out.println("running");
    while(true) {
        try {
            Thread.sleep(20);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        SwingUtilities.invokeLater(this::move);
    }
}

And moving the remaining code from PlayerMove.run() into a new move() method:

private void move() {
    movingLeft = KeyListenerPage.isGoingLeft;
    movingUp = KeyListenerPage.isGoingUp;
    movingRight = KeyListenerPage.isGoingRight;
    movingDown = KeyListenerPage.isGoingDown;
    if(movingLeft && canMoveLeft) {
        player.setBounds(player.getX()-10, player.getY(), player.getWidth(), player.getHeight());
        player.setVisible(true);
        if(player.getX()<=0) {
            canMoveLeft = false;
        }
        if(player.getX()<490) {
            canMoveRight = true;
        }
    }
    if(movingRight && canMoveRight) {
        player.setBounds(player.getX() 10, player.getY(), player.getWidth(), player.getHeight());
        player.setVisible(true);
        if(player.getX()>0) {
            canMoveLeft = true;
        }
        if(player.getX()>=490) {
            canMoveRight = false;
        }
    }
    if(movingUp && canMoveUp) {
        player.setBounds(player.getX(), player.getY()-10, player.getWidth(), player.getHeight());
        player.setVisible(true);
        if(player.getY()<=0) {
            canMoveUp = false;
        }
        if(player.getY()<490) {
            canMoveDown = true;
        }
    }
    if(movingDown && canMoveDown) {
        player.setBounds(player.getX(), player.getY() 10, player.getWidth(), player.getHeight());
        player.setVisible(true);
        if(player.getY()>0) {
            canMoveUp = true;
        }
        if(player.getY()>=490) {
            canMoveDown = false;
        }
    }
}

Please note that this solution still works with a separate thread (i.e. PlayerMove still extends Thread and you still have to call pmove.start(); in the KeyListenerPage)

As MadProgrammer has already commented the better way would be to use a Swing Timer

Such a solution would change your code somewhat.

The PlayerMove class would change into

public class PlayerMove implements ActionListener {
    boolean movingLeft = false;
    boolean movingUp = false;
    boolean movingRight = false;
    boolean movingDown = false;
    JLabel player = KeyListenerPage.player;
    boolean canMoveLeft = false;
    boolean canMoveRight = true;
    boolean canMoveUp = false;
    boolean canMoveDown = true;

    @Override
    public void actionPerformed(ActionEvent e) {
        // here is all the moving code, 
        // i.e. what in your solution was in the run() method
        // but without the looping
        // The Timer will call this every 20ms for you
    }
}

And in the KeyListenerPage() constructor you would replace

    pmove.start();

with

    Timer timer = new Timer(20, pmove);
    timer.start();

There is no longer a "never ending loop" needed in your code - the Timer now takes care that the pmove.actionPerformed() is called every 20ms.

This block of code:

public void run() {
    System.out.println("running");
    while(true) {
        try {
            Thread.sleep(20);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        SwingUtilities.invokeLater(this::move);
    }
}

can be removed entirely by replacing the move() method with the actionPerformed() method (the code in the method doesn't need to change!) and by replacing pmove.start(); with two lines - IMHO a clear improvement.